On 01/23/10 09:13, Christoph Egger wrote:
On 23.01.10 02:53, Jean-Yves Migeon wrote:On 01/23/10 02:01, Christoph Egger wrote:So we have address overflow in xennet and in xengnt. Some more variables storing physical addresses must be of type paddr_t.[snip]<side_note> On a more general perspective, MD code is messy with Xen and PAE, because there is an assumption that obtaining the page address is as easy as PAGE_SHIFTing the PFN, which is wrong. Xen handles PFNs as "unsigned long" and not "unsigned long long" in its API; if we are not careful enough, we may lose bits in the shift process. Bha. </side_note>Thanks for the comments, Jean. Writing a patch at 1am is probably not a good idea.
;)
It is something that is worth considering, although not as easy to fix as it seems. (FWIW, the UVM/pmap code of GENERIC i386 is quite resilient in this regard, thanks to the paddr_t/PRIxPADDR abstractions.) The Xen part is messy, because of the API. Xen uses xen_pfn_t everywhere (API is designed that way), which is an "unsigned long". But promoting PFNs bluntly to "unsigned long long" may have bad results, like [1], as the "xen_pfn_t" convention is also used by xentools, and NetBSD sits between the two. There could be a tiny abstraction to get from/to PFN and addresses. But we should be extra careful not to break Xen <=> xentools ABI. I guess that the ptoa/atop macros where expected to be used that way, but the casts they use make them improper in this case. And I am not to keen to change them, as there is !x86 MD code that depend on them.Attached patch should now work. Mark: Please apply attached patch additionally to Manuel's patch, recompile both dom0 and domu kernels and retry. Jean, Manuel: We should introduce helper macros which hide the needed casts completely. This makes new code less error prone.
Questions arise:- fix ptoa/atop to avoid masking high order bits when sizeof paddr_t > sizeof vaddr_t?
- have a Xen specific ptoa routine?- bluntly consider we only use paddr_t for PFNs, and carefully work out the interfaces?
- something else?[1] http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/arch/xen/x86/hypervisor_machdep.c?rev=1.11.8.2&content-type=text/x-cvsweb-markup
-- Jean-Yves Migeon jeanyves.migeon%free.fr@localhost