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.
;)
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.
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.
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