Re: [Xen-devel] [PATCH] libxc/arm: align to page size the base address of the device tree

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-11-20 at 13:17 +0000, Julien Grall wrote:
> 
> On 11/20/2013 01:13 PM, Ian Campbell wrote:
> > On Wed, 2013-11-20 at 13:04 +0000, Julien Grall wrote:
> >>
> >> On 11/20/2013 09:48 AM, Ian Campbell wrote:
> >>> On Tue, 2013-11-19 at 18:52 +0000, Julien Grall wrote:
> >>>> xc_dom_alloc_segment requires start address to be page align.
> >>>
> >>> I wonder why I didn't see this? It seems very unlikely that my dtb would
> >>> be exactly page sized, yet it works... Ah, I have >128M of RAM in my
> >>> guest so the base would be 128M. Well spotted.
> >>>
> >>> I think it would be slightly preferable to round dtbsize up to a page.
> >>> e.g. the following. What do you think?
> >>>
> >>
> >> Sounds good to me. I wrote my patch in the other way because I though
> >> RAM size can be non-page aligned.
> >
> > That would be concern except the sizes are defined immediately above as
> > shifts based on page numbers.
> >
> >>
> >>> 8>------------------
> >>>
> >>>   From 079a2815d86567de1cbb541541d08a0b9ac3d18d Mon Sep 17 00:00:00 2001
> >>> From: Ian Campbell <ian.campbell@xxxxxxxxxx>
> >>> Date: Wed, 20 Nov 2013 09:45:32 +0000
> >>> Subject: [PATCH] libxl: arm: ensure DTB is page aligned
> >>>
> >>> xc_dom_alloc_segment requires this. Since rambase and ramend are both page
> >>> aligned, rounding up the DTB is sufficient.
> >>>
> >>> Reported-by: Julien Grall <julien.grall@xxxxxxxxxx>
> >>> Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> >>> ---
> >>>    tools/libxc/xc_dom_arm.c |    2 +-
> >>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> >>> index ffe575b..a40e04d 100644
> >>> --- a/tools/libxc/xc_dom_arm.c
> >>> +++ b/tools/libxc/xc_dom_arm.c
> >>> @@ -282,7 +282,7 @@ int arch_setup_meminit(struct xc_dom_image *dom)
> >>>        {
> >>>            const uint64_t rambase = dom->rambase_pfn << XC_PAGE_SHIFT;
> >>>            const uint64_t ramend = rambase + ( dom->total_pages << 
> >>> XC_PAGE_SHIFT );
> >>> -        const uint64_t dtbsize = ( dom->devicetree_size + 3 ) & ~0x3;
> >>> +        const uint64_t dtbsize = ROUNDUP(dom->devicetree_size, 
> >>> XC_PAGE_SHIFT);
> >>
> >> XC_PAGE_SHIFT only contains the shift. You should use XC_PAGE_SIZE.
> >
> > That's not how ROUNDUP is defined in libxc:
> > #define ROUNDUP(_x,_w) (((unsigned long)(_x)+(1UL<<(_w))-1) & 
> > ~((1UL<<(_w))-1))
> >
> > so it expects _w (width?) to be the shift.
> 
> Oh right, I though it was defined as in xen. In any case, I think you 
> should use XC_DOM_PAGE_SHIFT(dom).

Those macros are almost unused in libxc. I suspect they are leftovers
from ia64 support, that being the only platform which we've supported
which had the possibility of non-4k pages.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel