[PATCH v2] libxc: fix the types used in xc_dom_image to build HVM guests

[PATCH v2] libxc: fix the types used in xc_dom_image to build HVM guests

[Roger Pau Monne]

Fix the types used to store the memory parameters of an HVM guest,
previously they defaulted to unsigned long on 32bit toolstack builds, which
is wrong because a 32bit value cannot hold a 64bit memory address that
crosses the 4GB boundary.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
I don't have a 32bit Dom0 at hand, so if someone can try to create a HVM
guests using a 32bit toolstack with more than 4GB of RAM it would be
helpful.
---
Changes since v1:
 - Use xen_paddr_t for all the physical address related fields.
 - Use xen_pfn_t for target_pages.
---
 tools/libxc/include/xc_dom.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
index e52b023..7cb6b0c 100644
--- a/tools/libxc/include/xc_dom.h
+++ b/tools/libxc/include/xc_dom.h
@@ -187,10 +187,10 @@ struct xc_dom_image {
 
     /* HVM specific fields. */
     xen_pfn_t target_pages;
-    xen_pfn_t mmio_start;
-    xen_pfn_t mmio_size;
-    xen_pfn_t lowmem_end;
-    xen_pfn_t highmem_end;
+    xen_paddr_t mmio_start;
+    xen_paddr_t mmio_size;
+    xen_paddr_t lowmem_end;
+    xen_paddr_t highmem_end;
 
     /* Extra ACPI tables passed to HVMLOADER */
     struct xc_hvm_firmware_module acpi_module;
-- 
1.9.5 (Apple Git-50.3)


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

Re: [PATCH v2] libxc: fix the types used in xc_dom_image to build HVM guests

[Ian Campbell]

On Thu, 2015-10-15 at 19:23 +0200, Roger Pau Monne wrote:
> Fix the types used to store the memory parameters of an HVM guest,
> previously they defaulted to unsigned long on 32bit toolstack builds,
> which
> is wrong because a 32bit value cannot hold a 64bit memory address that
> crosses the 4GB boundary.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
> I don't have a 32bit Dom0 at hand, so if someone can try to create a HVM
> guests using a 32bit toolstack with more than 4GB of RAM it would be
> helpful.

I am happy after eyeballing that this is at least an improvement and am
content to let osstest answer this question.

I'm still having some trouble accessing the machines where I typically do
all my semi-automated pre-push build tests but given that these issues have
been blocking the push gate for a while now I am going to push this change
without the full, i.e. just a quick local adhoc build which I am doing now.

Ian.

> ---
> Changes since v1:
>  - Use xen_paddr_t for all the physical address related fields.
>  - Use xen_pfn_t for target_pages.
> ---
>  tools/libxc/include/xc_dom.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> index e52b023..7cb6b0c 100644
> --- a/tools/libxc/include/xc_dom.h
> +++ b/tools/libxc/include/xc_dom.h
> @@ -187,10 +187,10 @@ struct xc_dom_image {
>  
>      /* HVM specific fields. */
>      xen_pfn_t target_pages;
> -    xen_pfn_t mmio_start;
> -    xen_pfn_t mmio_size;
> -    xen_pfn_t lowmem_end;
> -    xen_pfn_t highmem_end;
> +    xen_paddr_t mmio_start;
> +    xen_paddr_t mmio_size;
> +    xen_paddr_t lowmem_end;
> +    xen_paddr_t highmem_end;
>  
>      /* Extra ACPI tables passed to HVMLOADER */
>      struct xc_hvm_firmware_module acpi_module;

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

Re: [PATCH v2] libxc: fix the types used in xc_dom_image to build HVM guests

[Wei Liu]

On Fri, Oct 16, 2015 at 09:48:34AM +0100, Ian Campbell wrote:
> On Thu, 2015-10-15 at 19:23 +0200, Roger Pau Monne wrote:
> > Fix the types used to store the memory parameters of an HVM guest,
> > previously they defaulted to unsigned long on 32bit toolstack builds,
> > which
> > is wrong because a 32bit value cannot hold a 64bit memory address that
> > crosses the 4GB boundary.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

> 
> > ---
> > I don't have a 32bit Dom0 at hand, so if someone can try to create a HVM
> > guests using a 32bit toolstack with more than 4GB of RAM it would be
> > helpful.
> 
> I am happy after eyeballing that this is at least an improvement and am
> content to let osstest answer this question.
> 

Agreed.

> I'm still having some trouble accessing the machines where I typically do
> all my semi-automated pre-push build tests but given that these issues have
> been blocking the push gate for a while now I am going to push this change
> without the full, i.e. just a quick local adhoc build which I am doing now.
> 
> Ian.
> 
> > ---
> > Changes since v1:
> >  - Use xen_paddr_t for all the physical address related fields.
> >  - Use xen_pfn_t for target_pages.
> > ---
> >  tools/libxc/include/xc_dom.h | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> > index e52b023..7cb6b0c 100644
> > --- a/tools/libxc/include/xc_dom.h
> > +++ b/tools/libxc/include/xc_dom.h
> > @@ -187,10 +187,10 @@ struct xc_dom_image {
> >  
> >      /* HVM specific fields. */
> >      xen_pfn_t target_pages;
> > -    xen_pfn_t mmio_start;
> > -    xen_pfn_t mmio_size;
> > -    xen_pfn_t lowmem_end;
> > -    xen_pfn_t highmem_end;
> > +    xen_paddr_t mmio_start;
> > +    xen_paddr_t mmio_size;
> > +    xen_paddr_t lowmem_end;
> > +    xen_paddr_t highmem_end;
> >  
> >      /* Extra ACPI tables passed to HVMLOADER */
> >      struct xc_hvm_firmware_module acpi_module;

Re: [PATCH v2] libxc: fix the types used in xc_dom_image to build HVM guests

[Ian Campbell]

On Fri, 2015-10-16 at 09:50 +0100, Wei Liu wrote:
> On Fri, Oct 16, 2015 at 09:48:34AM +0100, Ian Campbell wrote:
> > On Thu, 2015-10-15 at 19:23 +0200, Roger Pau Monne wrote:
> > > Fix the types used to store the memory parameters of an HVM guest,
> > > previously they defaulted to unsigned long on 32bit toolstack builds,
> > > which
> > > is wrong because a 32bit value cannot hold a 64bit memory address
> > > that
> > > crosses the 4GB boundary.
> > > 
> > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > > Cc: Ian Campbell <ian.campbell@citrix.com>
> > > Cc: Wei Liu <wei.liu2@citrix.com>
> > 
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>

Thanks.

> > > ---
> > > I don't have a 32bit Dom0 at hand, so if someone can try to create a
> > > HVM
> > > guests using a 32bit toolstack with more than 4GB of RAM it would be
> > > helpful.
> > 
> > I am happy after eyeballing that this is at least an improvement and am
> > content to let osstest answer this question.
> > 
> 
> Agreed.

Done.

> > I'm still having some trouble accessing the machines where I typically do
> > all my semi-automated pre-push build tests but given that these issues have
> > been blocking the push gate for a while now I am going to push this change
> > without the full, i.e. just a quick local adhoc build which I am doing now.

Roger, please keep an eye out for build issues in the smoke test etc.

Ian.

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

Re: [PATCH v2] libxc: fix the types used in xc_dom_image to build HVM guests

[Ian Campbell]

On Thu, 2015-10-15 at 19:23 +0200, Roger Pau Monne wrote:
> Fix the types used to store the memory parameters of an HVM guest,
> previously they defaulted to unsigned long on 32bit toolstack builds, which
> is wrong because a 32bit value cannot hold a 64bit memory address that
> crosses the 4GB boundary.

Just for my own interest, I suppose this was a latent issue even before the
dom builder rework, but what was the change which exposed it now?

Ian.

> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
> I don't have a 32bit Dom0 at hand, so if someone can try to create a HVM
> guests using a 32bit toolstack with more than 4GB of RAM it would be
> helpful.
> ---
> Changes since v1:
>  - Use xen_paddr_t for all the physical address related fields.
>  - Use xen_pfn_t for target_pages.
> ---
>  tools/libxc/include/xc_dom.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> index e52b023..7cb6b0c 100644
> --- a/tools/libxc/include/xc_dom.h
> +++ b/tools/libxc/include/xc_dom.h
> @@ -187,10 +187,10 @@ struct xc_dom_image {
>  
>      /* HVM specific fields. */
>      xen_pfn_t target_pages;
> -    xen_pfn_t mmio_start;
> -    xen_pfn_t mmio_size;
> -    xen_pfn_t lowmem_end;
> -    xen_pfn_t highmem_end;
> +    xen_paddr_t mmio_start;
> +    xen_paddr_t mmio_size;
> +    xen_paddr_t lowmem_end;
> +    xen_paddr_t highmem_end;
>  
>      /* Extra ACPI tables passed to HVMLOADER */
>      struct xc_hvm_firmware_module acpi_module;

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

Re: [PATCH v2] libxc: fix the types used in xc_dom_image to build HVM guests

[Wei Liu]

On Fri, Oct 16, 2015 at 09:57:02AM +0100, Ian Campbell wrote:
> On Thu, 2015-10-15 at 19:23 +0200, Roger Pau Monne wrote:
> > Fix the types used to store the memory parameters of an HVM guest,
> > previously they defaulted to unsigned long on 32bit toolstack builds, which
> > is wrong because a 32bit value cannot hold a 64bit memory address that
> > crosses the 4GB boundary.
> 
> Just for my own interest, I suppose this was a latent issue even before the
> dom builder rework, but what was the change which exposed it now?
> 

I don't think this is a latent issue.

During the refactoring a bunch of types were changed to unsigned long.
The original types used were uint64_t, which is of the same width on 32
bit and 64 bit.

Wei.

Re: [PATCH v2] libxc: fix the types used in xc_dom_image to build HVM guests

[Roger Pau Monné]

El 16/10/15 a les 11.00, Wei Liu ha escrit:
> On Fri, Oct 16, 2015 at 09:57:02AM +0100, Ian Campbell wrote:
>> On Thu, 2015-10-15 at 19:23 +0200, Roger Pau Monne wrote:
>>> Fix the types used to store the memory parameters of an HVM guest,
>>> previously they defaulted to unsigned long on 32bit toolstack builds, which
>>> is wrong because a 32bit value cannot hold a 64bit memory address that
>>> crosses the 4GB boundary.
>>
>> Just for my own interest, I suppose this was a latent issue even before the
>> dom builder rework, but what was the change which exposed it now?
>>
> 
> I don't think this is a latent issue.
> 
> During the refactoring a bunch of types were changed to unsigned long.
> The original types used were uint64_t, which is of the same width on 32
> bit and 64 bit.

Yes, those fields were in xc_hvm_build_args as uint64_t, and during the
rework of the HVM builder I had changed them to xen_pfn_t, so it's
completely my fault, previous code should be fine.

I will keep an eye on the smoke OSSTest tests.

Roger.

Re: [PATCH v2] libxc: fix the types used in xc_dom_image to build HVM guests

[Ian Campbell]

On Fri, 2015-10-16 at 11:03 +0200, Roger Pau Monné wrote:
> El 16/10/15 a les 11.00, Wei Liu ha escrit:
> > On Fri, Oct 16, 2015 at 09:57:02AM +0100, Ian Campbell wrote:
> > > On Thu, 2015-10-15 at 19:23 +0200, Roger Pau Monne wrote:
> > > > Fix the types used to store the memory parameters of an HVM guest,
> > > > previously they defaulted to unsigned long on 32bit toolstack
> > > > builds, which
> > > > is wrong because a 32bit value cannot hold a 64bit memory address
> > > > that
> > > > crosses the 4GB boundary.
> > > 
> > > Just for my own interest, I suppose this was a latent issue even
> > > before the
> > > dom builder rework, but what was the change which exposed it now?
> > > 
> > 
> > I don't think this is a latent issue.
> > 
> > During the refactoring a bunch of types were changed to unsigned long.
> > The original types used were uint64_t, which is of the same width on 32
> > bit and 64 bit.
> 
> Yes, those fields were in xc_hvm_build_args as uint64_t, and during the
> rework of the HVM builder I had changed them to xen_pfn_t, so it's
> completely my fault, previous code should be fine.

Ah ok, thanks.

> I will keep an eye on the smoke OSSTest tests.

Thank you.

Ian.

> 
> Roger.
> 

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