Julien Grall
2013-Nov-19 18:52 UTC
[PATCH] libxc/arm: align to page size the base address of the device tree
xc_dom_alloc_segment requires start address to be page align.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
tools/libxc/xc_dom_arm.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index ffe575b..366061d 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -290,6 +290,8 @@ int arch_setup_meminit(struct xc_dom_image *dom)
else /* otherwise at top of RAM */
dom->devicetree_seg.vstart = ramend - dtbsize;
+ dom->devicetree_seg.vstart &= XC_PAGE_MASK;
+
dom->devicetree_seg.vend dom->devicetree_seg.vstart
+ dom->devicetree_size;
DOMPRINTF("%s: devicetree: 0x%" PRIx64 " -> 0x%"
PRIx64 "",
--
1.7.10.4
Ian Campbell
2013-Nov-20 09:48 UTC
Re: [PATCH] libxc/arm: align to page size the base address of the device tree
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? 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); /* Place at 128MB if there is sufficient RAM */ if ( ramend >= rambase + 128*1024*1024 + dtbsize ) -- 1.7.10.4
Julien Grall
2013-Nov-20 13:04 UTC
Re: [PATCH] libxc/arm: align to page size the base address of the device tree
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. -- Julien Grall
Ian Campbell
2013-Nov-20 13:13 UTC
Re: [PATCH] libxc/arm: align to page size the base address of the device tree
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.
Julien Grall
2013-Nov-20 13:17 UTC
Re: [PATCH] libxc/arm: align to page size the base address of the device tree
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). -- Julien Grall
Ian Campbell
2013-Nov-20 13:30 UTC
Re: [PATCH] libxc/arm: align to page size the base address of the device tree
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.
Julien Grall
2013-Nov-20 16:16 UTC
Re: [PATCH] libxc/arm: align to page size the base address of the device tree
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> -- Julien Grall
Possibly Parallel Threads
- [PATCH] libxc/arm: Correctly handle the difference between virtual and physical address
- [PATCH v6 00/16] xen: arm: 64-bit guest support and domU FDT autogeneration
- [PATCH v2 00/14] xen: arm: 64-bit guest support and domU FDT autogeneration
- Bare-metal Xen on ARM boot
- frequently ballooning results in qemu exit