| Message ID | 1384940907.28441.11.camel@kazak.uk.xensource.com |
|---|---|
| State | Accepted |
| Commit | d239c0a68fb32716da8d2585cc2470ea4e8df3bd |
| Headers | show |
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. > 8>------------------ > > From 079a2815d86567de1cbb541541d08a0b9ac3d18d Mon Sep 17 00:00:00 2001 > From: Ian Campbell <ian.campbell@citrix.com> > 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@linaro.org> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > --- > 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. Even better, XC_DOM_PAGE_SIZE(dom), so we don't hardcode the size.
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@citrix.com> > > 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@linaro.org> > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > --- > > 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. Ian.
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@citrix.com> >>> 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@linaro.org> >>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com> >>> --- >>> 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).
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@citrix.com> > >>> 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@linaro.org> > >>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > >>> --- > >>> 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.
On 11/20/2013 01:30 PM, Ian Campbell wrote: > 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@citrix.com> >>>>> 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@linaro.org> >>>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com> >>>>> --- >>>>> 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. Sorry, I have only checked xc_dom_core.c For your patch: Acked-by: Julien Grall <julien.grall@linaro.org>
On Wed, 2013-11-20 at 16:16 +0000, Julien Grall wrote: > For your patch: > > Acked-by: Julien Grall <julien.grall@linaro.org> Applied, thanks.
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); /* Place at 128MB if there is sufficient RAM */ if ( ramend >= rambase + 128*1024*1024 + dtbsize )