Re: [PATCH][XEN] construct_dom0: Initialize variable before use

Re: [PATCH][XEN] construct_dom0: Initialize variable before use

[Christoph Egger]

Great, thanks! I just can't test the removal of the 166GB RAM limit :-)

Christoph


On Thursday 06 December 2007 14:39:51 Keir Fraser wrote:
> Changeset 16548.
>
>  -- Keir
>
> On 29/11/07 13:02, "Christoph Egger" <Christoph.Egger@amd.com> wrote:
> > Hi!
> >
> > Attached patch initializes d->arch.physaddr_bitsize properly for a
> > 64bit Dom0/64bit hypervisor.
> > This bug has been found when booting a NetBSD/Xen Dom0 with more
> > than 4GB RAM.
> >
> > Without this fix, d->arch.physaddr_bitsize is 0 in
> > domain_clamp_alloc_bitsize(). This causes all attempts to
> > XENMEM_increase_reservation with bits > 0 to fail. More precisely,
> > __alloc_domheap_pages() returns NULL.
> > This impacts Xen heap allocation in general.
> > Question: How did that work on Linux Dom0?
> >
> > I think, initializing physaddr_bitsize to 64 unconditionally is ok
> > because in construct_dom0() it is overridden with the proper value when
> > you boot 64bit hypervisor with a 32bit Dom0. If the hypervisor is not
> > 64bit, physaddr_bitsize isn't used at all anyway.
> >
> > Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>



-- 
AMD Saxony, Dresden, Germany
Operating System Research Center

Legal Information:
AMD Saxony Limited Liability Company & Co. KG
Sitz (Geschäftsanschrift):
   Wilschdorfer Landstr. 101, 01109 Dresden, Deutschland
Registergericht Dresden: HRA 4896
vertretungsberechtigter Komplementär:
   AMD Saxony LLC (Sitz Wilmington, Delaware, USA)
Geschäftsführer der AMD Saxony LLC:
   Dr. Hans-R. Deppe, Thomas McCoy

[PATCH][XEN] construct_dom0: Initialize variable before use

[Christoph Egger]

[-- Attachment #1: Type: text/plain, Size: 1252 bytes --]


Hi!

Attached patch initializes d->arch.physaddr_bitsize properly for a
64bit Dom0/64bit hypervisor.
This bug has been found when booting a NetBSD/Xen Dom0 with more
than 4GB RAM.

Without this fix, d->arch.physaddr_bitsize is 0 in 
domain_clamp_alloc_bitsize(). This causes all attempts to 
XENMEM_increase_reservation with bits > 0 to fail. More precisely,
__alloc_domheap_pages() returns NULL.
This impacts Xen heap allocation in general.
Question: How did that work on Linux Dom0?

I think, initializing physaddr_bitsize to 64 unconditionally is ok because in
construct_dom0() it is overridden with the proper value when you boot
64bit hypervisor with a 32bit Dom0. If the hypervisor is not 64bit,
physaddr_bitsize isn't used at all anyway.

Signed-off-by: Christoph Egger <Christoph.Egger@amd.com>


-- 
AMD Saxony, Dresden, Germany
Operating System Research Center

Legal Information:
AMD Saxony Limited Liability Company & Co. KG
Sitz (Geschäftsanschrift):
   Wilschdorfer Landstr. 101, 01109 Dresden, Deutschland
Registergericht Dresden: HRA 4896
vertretungsberechtigter Komplementär:
   AMD Saxony LLC (Sitz Wilmington, Delaware, USA)
Geschäftsführer der AMD Saxony LLC:
   Dr. Hans-R. Deppe, Thomas McCoy

[-- Attachment #2: xen_dom0.diff --]
[-- Type: text/plain, Size: 629 bytes --]

diff -r 6fd17d0dcbcd xen/arch/x86/domain_build.c
--- a/xen/arch/x86/domain_build.c	Tue Nov 27 12:49:16 2007 +0000
+++ b/xen/arch/x86/domain_build.c	Wed Nov 28 17:37:53 2007 +0100
@@ -353,6 +353,11 @@ int __init construct_dom0(
     if ( parms.pae == PAEKERN_extended_cr3 )
             set_bit(VMASST_TYPE_pae_extended_cr3, &d->vm_assist);
 
+    /* Unconditionally initialize this member here for 64bit Dom0s.
+     * In the below if (), this is overridden for 32bit Dom0s.
+     */ 
+    d->arch.physaddr_bitsize = 64;
+
     if ( UNSET_ADDR != parms.virt_hv_start_low && elf_32bit(&elf) )
     {
 #if CONFIG_PAGING_LEVELS < 4

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH][XEN] construct_dom0: Initialize variable before use

[Keir Fraser]

On 29/11/07 13:02, "Christoph Egger" <Christoph.Egger@amd.com> wrote:

> Without this fix, d->arch.physaddr_bitsize is 0 in
> domain_clamp_alloc_bitsize(). This causes all attempts to
> XENMEM_increase_reservation with bits > 0 to fail. More precisely,
> __alloc_domheap_pages() returns NULL.
> This impacts Xen heap allocation in general.
> Question: How did that work on Linux Dom0?

Yes, that's pretty broken. It works for Linux because Linux allocates its
lowmem I/o pages (e.g., swiotlb) using the XENMEM_exchange command, and that
allocates the new memory anonymously in the first instance. This defeats the
bitsize clamp check (which is okay just now because our truncation of the
phsyical memory map to 166GB is sufficient to ensure that compat domUs can
address all memory).

 -- Keir

Re: [PATCH][XEN] construct_dom0: Initialize variable before use

[Christoph Egger]

On Thursday 29 November 2007 14:28:00 Keir Fraser wrote:
> On 29/11/07 13:02, "Christoph Egger" <Christoph.Egger@amd.com> wrote:
> > Without this fix, d->arch.physaddr_bitsize is 0 in
> > domain_clamp_alloc_bitsize(). This causes all attempts to
> > XENMEM_increase_reservation with bits > 0 to fail. More precisely,
> > __alloc_domheap_pages() returns NULL.
> > This impacts Xen heap allocation in general.
> > Question: How did that work on Linux Dom0?
>
> Yes, that's pretty broken. It works for Linux because Linux allocates its
> lowmem I/o pages (e.g., swiotlb) using the XENMEM_exchange command, and
> that allocates the new memory anonymously in the first instance. This
> defeats the bitsize clamp check (which is okay just now because our
> truncation of the phsyical memory map to 166GB is sufficient to ensure that
> compat domUs can address all memory).

Thanks for clarification.
NetBSD Dom0 failed when allocating DMA-safe memory above 4GB.

Christoph


-- 
AMD Saxony, Dresden, Germany
Operating System Research Center

Legal Information:
AMD Saxony Limited Liability Company & Co. KG
Sitz (Geschäftsanschrift):
   Wilschdorfer Landstr. 101, 01109 Dresden, Deutschland
Registergericht Dresden: HRA 4896
vertretungsberechtigter Komplementär:
   AMD Saxony LLC (Sitz Wilmington, Delaware, USA)
Geschäftsführer der AMD Saxony LLC:
   Dr. Hans-R. Deppe, Thomas McCoy

Re: [PATCH][XEN] construct_dom0: Initialize variable before use

[Christoph Egger]

On Thursday 29 November 2007 14:37:24 Christoph Egger wrote:
> On Thursday 29 November 2007 14:28:00 Keir Fraser wrote:
> > On 29/11/07 13:02, "Christoph Egger" <Christoph.Egger@amd.com> wrote:
> > > Without this fix, d->arch.physaddr_bitsize is 0 in
> > > domain_clamp_alloc_bitsize(). This causes all attempts to
> > > XENMEM_increase_reservation with bits > 0 to fail. More precisely,
> > > __alloc_domheap_pages() returns NULL.
> > > This impacts Xen heap allocation in general.
> > > Question: How did that work on Linux Dom0?
> >
> > Yes, that's pretty broken. It works for Linux because Linux allocates its
> > lowmem I/o pages (e.g., swiotlb) using the XENMEM_exchange command, and
> > that allocates the new memory anonymously in the first instance. This
> > defeats the bitsize clamp check (which is okay just now because our
> > truncation of the phsyical memory map to 166GB is sufficient to ensure
> > that compat domUs can address all memory).
>
> Thanks for clarification.
> NetBSD Dom0 failed when allocating DMA-safe memory above 4GB.

Keir: Can you also put this fix onto your patch-queue for Xen 3.1.3, please?

Thanks,
Christoph


-- 
AMD Saxony, Dresden, Germany
Operating System Research Center

Legal Information:
AMD Saxony Limited Liability Company & Co. KG
Sitz (Geschäftsanschrift):
   Wilschdorfer Landstr. 101, 01109 Dresden, Deutschland
Registergericht Dresden: HRA 4896
vertretungsberechtigter Komplementär:
   AMD Saxony LLC (Sitz Wilmington, Delaware, USA)
Geschäftsführer der AMD Saxony LLC:
   Dr. Hans-R. Deppe, Thomas McCoy

Re: [PATCH][XEN] construct_dom0: Initialize variable before use

[Keir Fraser]

On 29/11/07 14:32, "Christoph Egger" <Christoph.Egger@amd.com> wrote:

>> Thanks for clarification.
>> NetBSD Dom0 failed when allocating DMA-safe memory above 4GB.
> 
> Keir: Can you also put this fix onto your patch-queue for Xen 3.1.3, please?

Definitely.

 -- Keir