new domain builder breaks compatibility

new domain builder breaks compatibility

[John Levon]

To quote xen.h:

495  *  9. There is guaranteed to be at least 512kB padding after the final
496  *     bootstrap element. If necessary, the bootstrap virtual region is
497  *     extended by an extra 4MB to ensure this.

The new domain builder forgot this:

763     if (dom->alloc_bootstack)
764         dom->bootstack_pfn = xc_dom_alloc_page(dom, "boot stack");
765     xc_dom_printf("%-20s: virt_alloc_end : 0x%" PRIx64 "\n",
766                   __FUNCTION__, dom->virt_alloc_end);

which, predictably enough, breaks Solaris, which tries to use this area as
ecratch memory during early boot.

regards
john

Re: new domain builder breaks compatibility

[Gerd Hoffmann]

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

John Levon wrote:
> To quote xen.h:
> 
> 495  *  9. There is guaranteed to be at least 512kB padding after the final
> 496  *     bootstrap element. If necessary, the bootstrap virtual region is
> 497  *     extended by an extra 4MB to ensure this.

> which, predictably enough, breaks Solaris, which tries to use this area as
> ecratch memory during early boot.

The attached patch should fix this.

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>

[-- Attachment #2: padding-fix --]
[-- Type: text/plain, Size: 330 bytes --]

--- tools/libxc/xc_dom_x86.c~	2007-01-31 18:07:56.000000000 +0100
+++ tools/libxc/xc_dom_x86.c	2007-02-01 10:02:08.000000000 +0100
@@ -66,6 +66,7 @@
 
     extra_pages = dom->alloc_bootstack ? 1 : 0;
     extra_pages += dom->extra_pages;
+    extra_pages += 128; /* 512kB padding */
     pages = extra_pages;
     for (;;)
     {

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

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

Re: new domain builder breaks compatibility

[John Levon]

On Thu, Feb 01, 2007 at 10:04:43AM +0100, Gerd Hoffmann wrote:

> John Levon wrote:
> > To quote xen.h:
> > 
> > 495  *  9. There is guaranteed to be at least 512kB padding after the final
> > 496  *     bootstrap element. If necessary, the bootstrap virtual region is
> > 497  *     extended by an extra 4MB to ensure this.
> 
> > which, predictably enough, breaks Solaris, which tries to use this area as
> > ecratch memory during early boot.
> 
> The attached patch should fix this.

Does that ensure the 4Mb alignment too? We're expecting:

scratch_end = RNDUP((paddr_t)scratch_start + 512 * 1024, FOUR_MEG);

BTW what is dom->extra_pages intended for?

thanks,
john

Re: new domain builder breaks compatibility

[Gerd Hoffmann]

John Levon wrote:
> On Thu, Feb 01, 2007 at 10:04:43AM +0100, Gerd Hoffmann wrote:
> 
>> John Levon wrote:
>>> To quote xen.h:
>>>
>>> 495  *  9. There is guaranteed to be at least 512kB padding after the final
>>> 496  *     bootstrap element. If necessary, the bootstrap virtual region is
>>> 497  *     extended by an extra 4MB to ensure this.
>>> which, predictably enough, breaks Solaris, which tries to use this area as
>>> ecratch memory during early boot.
>> The attached patch should fix this.
> 
> Does that ensure the 4Mb alignment too?

Yes.  Uhm, no, depends.  It completely fills the topmost L1 pagetable
with entries, so you end up with an 4MB alignment for 32bit and 2MB
alignment for PAE and 64bit.  I think the old builder had the same
behaviour, so the 4MB are actually a documentation bug ;)

> BTW what is dom->extra_pages intended for?

Allows allocation of some extra space.  Used by the kexec bits I have in
the queue, it places some code which finishes the memory layout before
jumping to the new kernel entry point above the boot stack, so it needs
some way to reserve memory there.

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>

Re: new domain builder breaks compatibility

[John Levon]

On Thu, Feb 01, 2007 at 04:15:16PM +0100, Gerd Hoffmann wrote:

> > Does that ensure the 4Mb alignment too?
> 
> Yes.  Uhm, no, depends.  It completely fills the topmost L1 pagetable
> with entries, so you end up with an 4MB alignment for 32bit and 2MB
> alignment for PAE and 64bit.  I think the old builder had the same
> behaviour, so the 4MB are actually a documentation bug ;)

Hmm... am I misunderstanding this code:

 816         /* v_end = (vstack_end + (1UL<<22)-1) & ~((1UL<<22)-1); */
 817         v_end = vstack_end;
 818         if ( !increment_ulong(&v_end, (1UL<<22)-1) )
 819             goto error_out;
 820         v_end &= ~((1UL<<22)-1);
 821
 822         if ( (v_end - vstack_end) < (512UL << 10) )
 823         {
 824             /* Add extra 4MB to get >= 512kB padding. */
 825             if ( !increment_ulong(&v_end, 1UL << 22) )
 826                 goto error_out;
 827         }

Surely that guarantees we get at least 512Kb and it ends on a 4Mb
boundary?

regards
john

Re: new domain builder breaks compatibility

[Gerd Hoffmann]

John Levon wrote:
> On Thu, Feb 01, 2007 at 04:15:16PM +0100, Gerd Hoffmann wrote:
> Hmm... am I misunderstanding this code:

>  822         if ( (v_end - vstack_end) < (512UL << 10) )
>  823         {
>  824             /* Add extra 4MB to get >= 512kB padding. */
>  825             if ( !increment_ulong(&v_end, 1UL << 22) )
>  826                 goto error_out;
>  827         }

No, you are right, it's 4MB unconditionally.  Seems to have changed
though since I checked last time.

> Surely that guarantees we get at least 512Kb and it ends on a 4Mb
> boundary?

Yep.  Is the 2MB alignment ok for Solaris?  You can depend on the 512kb
only anyway, so the alignment shouldn't matter much ...

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>

Re: new domain builder breaks compatibility

[John Levon]

On Thu, Feb 01, 2007 at 05:33:20PM +0100, Gerd Hoffmann wrote:

> >  822         if ( (v_end - vstack_end) < (512UL << 10) )
> >  823         {
> >  824             /* Add extra 4MB to get >= 512kB padding. */
> >  825             if ( !increment_ulong(&v_end, 1UL << 22) )
> >  826                 goto error_out;
> >  827         }
> 
> No, you are right, it's 4MB unconditionally.  Seems to have changed
> though since I checked last time.
> 
> > Surely that guarantees we get at least 512Kb and it ends on a 4Mb
> > boundary?
> 
> Yep.  Is the 2MB alignment ok for Solaris?  You can depend on the 512kb
> only anyway, so the alignment shouldn't matter much ...

I think we could probably make it 2Mb. I'll test it out with your patch.
I don't see a reason why it would fail on 3.0.3 or 3.0.4 anyway.

regards
john

Re: new domain builder breaks compatibility

[John Levon]

On Thu, Feb 01, 2007 at 10:04:43AM +0100, Gerd Hoffmann wrote:

> --- tools/libxc/xc_dom_x86.c~	2007-01-31 18:07:56.000000000 +0100
> +++ tools/libxc/xc_dom_x86.c	2007-02-01 10:02:08.000000000 +0100
> @@ -66,6 +66,7 @@
>  
>      extra_pages = dom->alloc_bootstack ? 1 : 0;
>      extra_pages += dom->extra_pages;
> +    extra_pages += 128; /* 512kB padding */

I've tested this along with a kernel change to only assume 2Mb
alignment, and it works on PAE and 64-bit now. Could you send a patch
upstream please? And update xen.h to clarify things?

thanks,
john

Re: new domain builder breaks compatibility

[Keir Fraser]

On 1/2/07 19:17, "John Levon" <levon@movementarian.org> wrote:

>> --- tools/libxc/xc_dom_x86.c~ 2007-01-31 18:07:56.000000000 +0100
>> +++ tools/libxc/xc_dom_x86.c 2007-02-01 10:02:08.000000000 +0100
>> @@ -66,6 +66,7 @@
>>  
>>      extra_pages = dom->alloc_bootstack ? 1 : 0;
>>      extra_pages += dom->extra_pages;
>> +    extra_pages += 128; /* 512kB padding */
> 
> I've tested this along with a kernel change to only assume 2Mb
> alignment, and it works on PAE and 64-bit now. Could you send a patch
> upstream please? And update xen.h to clarify things?

I'll handle this and update xen.h.

 Thanks,
 Keir

Re: new domain builder breaks compatibility

[Gerd Hoffmann]

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

Keir Fraser wrote:
> On 1/2/07 19:17, "John Levon" <levon@movementarian.org> wrote:
> 
>>> --- tools/libxc/xc_dom_x86.c~ 2007-01-31 18:07:56.000000000 +0100
>>> +++ tools/libxc/xc_dom_x86.c 2007-02-01 10:02:08.000000000 +0100
>>> @@ -66,6 +66,7 @@
>>>  
>>>      extra_pages = dom->alloc_bootstack ? 1 : 0;
>>>      extra_pages += dom->extra_pages;
>>> +    extra_pages += 128; /* 512kB padding */
>> I've tested this along with a kernel change to only assume 2Mb
>> alignment, and it works on PAE and 64-bit now. Could you send a patch
>> upstream please? And update xen.h to clarify things?
> 
> I'll handle this and update xen.h.

Restoring the 4MB alignment is easy too, see attached patch (which also
fixes the 4MB alignment for virt_base).

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>

[-- Attachment #2: fix-builder-4mb-align.diff --]
[-- Type: text/x-patch, Size: 1118 bytes --]

---
 tools/libxc/xc_dom_x86.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Index: build-64-unstable-13762/tools/libxc/xc_dom_x86.c
===================================================================
--- build-64-unstable-13762.orig/tools/libxc/xc_dom_x86.c
+++ build-64-unstable-13762/tools/libxc/xc_dom_x86.c
@@ -66,11 +66,12 @@ static int count_pgtables(struct xc_dom_
 
     extra_pages = dom->alloc_bootstack ? 1 : 0;
     extra_pages += dom->extra_pages;
+    extra_pages += 128; /* 512kB padding */
     pages = extra_pages;
     for (;;)
     {
 	try_virt_end = round_up(dom->virt_alloc_end + pages * PAGE_SIZE_X86,
-				bits_to_mask(l1_bits));
+				bits_to_mask(22)); /* 4MB alignment */
 	dom->pg_l4 =
 	    nr_page_tables(dom->parms.virt_base, try_virt_end, l4_bits);
 	dom->pg_l3 =
@@ -313,6 +314,9 @@ static int alloc_magic_pages(struct xc_d
     if (xc_dom_feature_translated(dom))
 	dom->shared_info_pfn = xc_dom_alloc_page(dom, "shared info");
     dom->alloc_bootstack = 1;
+
+    /* 4MB align start address */
+    dom->parms.virt_base &= ~(((uint64_t)1<<22)-1);
     return 0;
 }
 

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

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

Re: new domain builder breaks compatibility

[Gerd Hoffmann]

John Levon wrote:
> I've tested this along with a kernel change to only assume 2Mb
> alignment, and it works on PAE and 64-bit now.

Oh, you change the kernel for that?  Hmm, why don't you simply check how
many L2 entries are filled?  Then you don't have to hard-code that
domain builder implementation detail into the solaris kernel ...

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>

Re: new domain builder breaks compatibility

[Keir Fraser]

On 2/2/07 07:59, "Gerd Hoffmann" <kraxel@suse.de> wrote:

>> I'll handle this and update xen.h.
> 
> Restoring the 4MB alignment is easy too, see attached patch (which also
> fixes the 4MB alignment for virt_base).

I suppose it is nice to stick with the guarantees described in xen.h, since
it is so easy to do so.

 -- Keir

Re: new domain builder breaks compatibility

[Gerd Hoffmann]

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

Keir Fraser wrote:
> On 2/2/07 07:59, "Gerd Hoffmann" <kraxel@suse.de> wrote:
> 
>>> I'll handle this and update xen.h.
>> Restoring the 4MB alignment is easy too, see attached patch (which also
>> fixes the 4MB alignment for virt_base).
> 
> I suppose it is nice to stick with the guarantees described in xen.h, since
> it is so easy to do so.

Slightly updated version of the patch, the previous one did fixup
virt_base too late.  This one should work in theory.  It's not really
tested though, the VIRT_BASE specified by linux kernels is at a 4MB
border anyway, so the fixup is a no-op ...

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>

[-- Attachment #2: fix-builder-4mb-align.diff --]
[-- Type: text/x-patch, Size: 1635 bytes --]

---
 tools/libxc/xc_dom_core.c |    3 +++
 tools/libxc/xc_dom_x86.c  |    4 +++-
 2 files changed, 6 insertions(+), 1 deletion(-)

Index: build-32-unstable-13788/tools/libxc/xc_dom_x86.c
===================================================================
--- build-32-unstable-13788.orig/tools/libxc/xc_dom_x86.c
+++ build-32-unstable-13788/tools/libxc/xc_dom_x86.c
@@ -66,11 +66,12 @@ static int count_pgtables(struct xc_dom_
 
     extra_pages = dom->alloc_bootstack ? 1 : 0;
     extra_pages += dom->extra_pages;
+    extra_pages += 128; /* 512kB padding */
     pages = extra_pages;
     for (;;)
     {
 	try_virt_end = round_up(dom->virt_alloc_end + pages * PAGE_SIZE_X86,
-				bits_to_mask(l1_bits));
+				bits_to_mask(22)); /* 4MB alignment */
 	dom->pg_l4 =
 	    nr_page_tables(dom->parms.virt_base, try_virt_end, l4_bits);
 	dom->pg_l3 =
@@ -313,6 +314,7 @@ static int alloc_magic_pages(struct xc_d
     if (xc_dom_feature_translated(dom))
 	dom->shared_info_pfn = xc_dom_alloc_page(dom, "shared info");
     dom->alloc_bootstack = 1;
+
     return 0;
 }
 
Index: build-32-unstable-13788/tools/libxc/xc_dom_core.c
===================================================================
--- build-32-unstable-13788.orig/tools/libxc/xc_dom_core.c
+++ build-32-unstable-13788/tools/libxc/xc_dom_core.c
@@ -717,6 +717,9 @@ int xc_dom_build_image(struct xc_dom_ima
     }
     page_size = XC_DOM_PAGE_SIZE(dom);
 
+    /* 4MB align virtual base address */
+    dom->parms.virt_base &= ~(((uint64_t)1<<22)-1);
+
     /* load kernel */
     if (0 != xc_dom_alloc_segment(dom, &dom->kernel_seg, "kernel",
 				  dom->kernel_seg.vstart,

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

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

Re: new domain builder breaks compatibility

[John Levon]

On Fri, Feb 02, 2007 at 04:13:54PM +0100, Gerd Hoffmann wrote:

> Slightly updated version of the patch, the previous one did fixup
> virt_base too late.  This one should work in theory.  It's not really
> tested though, the VIRT_BASE specified by linux kernels is at a 4MB
> border anyway, so the fixup is a no-op ...

Well, we're only expecting 2Mb now anyway...

regards
john