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

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

[Julien Grall]

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

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

[Ian Campbell]

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

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

[Julien Grall]

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

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

[Ian Campbell]

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.

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

[Julien Grall]

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

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

[Ian Campbell]

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.

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

[Julien Grall]

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

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

[Ian Campbell]

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.