Re: [Xen-devel] [PATCH 4/4] x86/boot: Size the boot/directmap mappings dynamically

2020-01-15 Thread Jan Beulich 
On 15.01.2020 10:40, Jan Beulich wrote:
> On 14.01.2020 18:27, Andrew Cooper wrote:
>> On 14/01/2020 17:02, Jan Beulich wrote:
>>> Even when that remaining issue got addressed, I think it would be better
>>> to keep it, altering the bound to GB(1).
>>
>> A 1G check wouldn't be correct.
>>
>> We've already got a more suitable one, which is the check that Xen
>> doesn't encroach into the stubs range.
> 
> Oh, right. If only that check was correct. I guess it ought to be
> using &, not |, and perhaps also __image_base__ == XEN_VIRT_START.
> I'll give this a try and send a patch unless in the course of
> doing so I realize there's a reason for it being the way it is.

So the | is correct (to deal with the case of the intermediate
file getting linked at a different base address when producing
xen.efi), but probably misleading. I guess I'll submit a patch
anyway, despite the construct not being broken.

Jan

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/4] x86/boot: Size the boot/directmap mappings dynamically

2020-01-15 Thread Jan Beulich 
On 14.01.2020 18:27, Andrew Cooper wrote:
> On 14/01/2020 17:02, Jan Beulich wrote:
>> On 13.01.2020 18:50, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/boot/head.S
>>> +++ b/xen/arch/x86/boot/head.S
>>> @@ -687,14 +687,19 @@ trampoline_setup:
>>>   * handling/walking), and identity map Xen into bootmap (needed for
>>>   * the transition into long mode), using 2M superpages.
>>>   */
>>> -lea sym_esi(start),%ebx
>>> -lea 
>>> (1<>> -shr $(L2_PAGETABLE_SHIFT-3),%ebx
>>> -mov $8,%ecx
>>> -1:  mov %eax,sym_fs(l2_bootmap)-8(%ebx,%ecx,8)
>>> -mov %eax,sym_fs(l2_directmap)-8(%ebx,%ecx,8)
>>> -sub $(1<>> -loop1b
>>> +lea sym_esi(_start), %ecx
>>> +lea -1 + sym_esi(_end), %edx
>> This looks pretty odd - does
>>
>> lea sym_esi(_end) - 1, %edx
>>
>> not work?
> 
> No:
> 
> head.S: Assembler messages:
> head.S:521: Error: junk `(%esi)-1' after expression
> 
> but it is not at all surprising when you expand the macro:
> 
> lea (_end - start)(%esi) - 1, %edx
> 
> The expression for the displacement ends up split across both sides of
> the SIB.

Hmm, seems I've mis-remembered that stuff ahead of ( and after
) gets concatenated.

>>> +lea _PAGE_PSE + PAGE_HYPERVISOR_RWX(%ecx), %eax /* PTE to 
>>> write. */
>>> +shr $L2_PAGETABLE_SHIFT, %ecx   /* First slot 
>>> to write. */
>>> +shr $L2_PAGETABLE_SHIFT, %edx   /* Final slot 
>>> to write. */
>>> +
>>> +1:  mov %eax, sym_offs(l2_bootmap)  (%esi, %ecx, 8)
>>> +mov %eax, sym_offs(l2_directmap)(%esi, %ecx, 8)
>> I guess I could have noticed this on the previous patch already:
>> This would look better as
>>
>> 1:  mov %eax, sym_esi(l2_bootmap,   %ecx, 8)
>> mov %eax, sym_esi(l2_directmap, %ecx, 8)
>>
>> Can sym_esi() perhaps be made
>>
>> #define sym_esi(sym, extra...)  sym_offs(sym)(%esi, ## extra)
>>
>> ?
> 
> I considered and dismissed this approach.  Yes, the code is slightly
> shorter, but at the expense of readability.
> 
> The advantage of the longhand version is that it is obvious which half
> is the displacement expression, and which half is the SIB.
> 
> The reduced version leaves a distinct possibility of %ecx being mistaken
> as the base register, rather than the index.

With it being sym_esi() that gets used, I don't see any such risk.
But anyway, if you're convinced of the longer form being better,
so be it then.

>>> --- a/xen/arch/x86/xen.lds.S
>>> +++ b/xen/arch/x86/xen.lds.S
>>> @@ -384,6 +384,3 @@ ASSERT((trampoline_end - trampoline_start) < 
>>> TRAMPOLINE_SPACE - MBI_SPACE_MIN,
>>>  "not enough room for trampoline and mbi data")
>>>  ASSERT((wakeup_stack - wakeup_stack_start) >= WAKEUP_STACK_MIN,
>>>  "wakeup stack too small")
>>> -
>>> -/* Plenty of boot code assumes that Xen isn't larger than 16M. */
>>> -ASSERT(_end - _start <= MB(16), "Xen too large for early-boot assumptions")
>> Following your reply to the cover letter, this can't be dropped just yet.
> 
> Correct.
> 
>> Even when that remaining issue got addressed, I think it would be better
>> to keep it, altering the bound to GB(1).
> 
> A 1G check wouldn't be correct.
> 
> We've already got a more suitable one, which is the check that Xen
> doesn't encroach into the stubs range.

Oh, right. If only that check was correct. I guess it ought to be
using &, not |, and perhaps also __image_base__ == XEN_VIRT_START.
I'll give this a try and send a patch unless in the course of
doing so I realize there's a reason for it being the way it is.

Jan

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/4] x86/boot: Size the boot/directmap mappings dynamically

2020-01-14 Thread Andrew Cooper 
On 14/01/2020 17:02, Jan Beulich wrote:
> On 13.01.2020 18:50, Andrew Cooper wrote:
>> --- a/xen/arch/x86/boot/head.S
>> +++ b/xen/arch/x86/boot/head.S
>> @@ -687,14 +687,19 @@ trampoline_setup:
>>   * handling/walking), and identity map Xen into bootmap (needed for
>>   * the transition into long mode), using 2M superpages.
>>   */
>> -lea sym_esi(start),%ebx
>> -lea 
>> (1<> -shr $(L2_PAGETABLE_SHIFT-3),%ebx
>> -mov $8,%ecx
>> -1:  mov %eax,sym_fs(l2_bootmap)-8(%ebx,%ecx,8)
>> -mov %eax,sym_fs(l2_directmap)-8(%ebx,%ecx,8)
>> -sub $(1<> -loop1b
>> +lea sym_esi(_start), %ecx
>> +lea -1 + sym_esi(_end), %edx
> This looks pretty odd - does
>
> lea sym_esi(_end) - 1, %edx
>
> not work?

No:

head.S: Assembler messages:
head.S:521: Error: junk `(%esi)-1' after expression

but it is not at all surprising when you expand the macro:

lea (_end - start)(%esi) - 1, %edx

The expression for the displacement ends up split across both sides of
the SIB.

>
>> +lea _PAGE_PSE + PAGE_HYPERVISOR_RWX(%ecx), %eax /* PTE to 
>> write. */
>> +shr $L2_PAGETABLE_SHIFT, %ecx   /* First slot 
>> to write. */
>> +shr $L2_PAGETABLE_SHIFT, %edx   /* Final slot 
>> to write. */
>> +
>> +1:  mov %eax, sym_offs(l2_bootmap)  (%esi, %ecx, 8)
>> +mov %eax, sym_offs(l2_directmap)(%esi, %ecx, 8)
> I guess I could have noticed this on the previous patch already:
> This would look better as
>
> 1:  mov %eax, sym_esi(l2_bootmap,   %ecx, 8)
> mov %eax, sym_esi(l2_directmap, %ecx, 8)
>
> Can sym_esi() perhaps be made
>
> #define sym_esi(sym, extra...)  sym_offs(sym)(%esi, ## extra)
>
> ?

I considered and dismissed this approach.  Yes, the code is slightly
shorter, but at the expense of readability.

The advantage of the longhand version is that it is obvious which half
is the displacement expression, and which half is the SIB.

The reduced version leaves a distinct possibility of %ecx being mistaken
as the base register, rather than the index.

>
>> --- a/xen/arch/x86/xen.lds.S
>> +++ b/xen/arch/x86/xen.lds.S
>> @@ -384,6 +384,3 @@ ASSERT((trampoline_end - trampoline_start) < 
>> TRAMPOLINE_SPACE - MBI_SPACE_MIN,
>>  "not enough room for trampoline and mbi data")
>>  ASSERT((wakeup_stack - wakeup_stack_start) >= WAKEUP_STACK_MIN,
>>  "wakeup stack too small")
>> -
>> -/* Plenty of boot code assumes that Xen isn't larger than 16M. */
>> -ASSERT(_end - _start <= MB(16), "Xen too large for early-boot assumptions")
> Following your reply to the cover letter, this can't be dropped just yet.

Correct.

> Even when that remaining issue got addressed, I think it would be better
> to keep it, altering the bound to GB(1).

A 1G check wouldn't be correct.

We've already got a more suitable one, which is the check that Xen
doesn't encroach into the stubs range.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/4] x86/boot: Size the boot/directmap mappings dynamically

2020-01-14 Thread Jan Beulich 
On 13.01.2020 18:50, Andrew Cooper wrote:
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -687,14 +687,19 @@ trampoline_setup:
>   * handling/walking), and identity map Xen into bootmap (needed for
>   * the transition into long mode), using 2M superpages.
>   */
> -lea sym_esi(start),%ebx
> -lea 
> (1< -shr $(L2_PAGETABLE_SHIFT-3),%ebx
> -mov $8,%ecx
> -1:  mov %eax,sym_fs(l2_bootmap)-8(%ebx,%ecx,8)
> -mov %eax,sym_fs(l2_directmap)-8(%ebx,%ecx,8)
> -sub $(1< -loop1b
> +lea sym_esi(_start), %ecx
> +lea -1 + sym_esi(_end), %edx

This looks pretty odd - does

lea sym_esi(_end) - 1, %edx

not work?

> +lea _PAGE_PSE + PAGE_HYPERVISOR_RWX(%ecx), %eax /* PTE to write. 
> */
> +shr $L2_PAGETABLE_SHIFT, %ecx   /* First slot to 
> write. */
> +shr $L2_PAGETABLE_SHIFT, %edx   /* Final slot to 
> write. */
> +
> +1:  mov %eax, sym_offs(l2_bootmap)  (%esi, %ecx, 8)
> +mov %eax, sym_offs(l2_directmap)(%esi, %ecx, 8)

I guess I could have noticed this on the previous patch already:
This would look better as

1:  mov %eax, sym_esi(l2_bootmap,   %ecx, 8)
mov %eax, sym_esi(l2_directmap, %ecx, 8)

Can sym_esi() perhaps be made

#define sym_esi(sym, extra...)  sym_offs(sym)(%esi, ## extra)

?

> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -384,6 +384,3 @@ ASSERT((trampoline_end - trampoline_start) < 
> TRAMPOLINE_SPACE - MBI_SPACE_MIN,
>  "not enough room for trampoline and mbi data")
>  ASSERT((wakeup_stack - wakeup_stack_start) >= WAKEUP_STACK_MIN,
>  "wakeup stack too small")
> -
> -/* Plenty of boot code assumes that Xen isn't larger than 16M. */
> -ASSERT(_end - _start <= MB(16), "Xen too large for early-boot assumptions")

Following your reply to the cover letter, this can't be dropped just yet.
Even when that remaining issue got addressed, I think it would be better
to keep it, altering the bound to GB(1).

Jan

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 4/4] x86/boot: Size the boot/directmap mappings dynamically

2020-01-13 Thread Andrew Cooper 
... rather than presuming that 16M will do.  With this fixed, Xen is now
capable of booting even when its compiled size is larger than 16M.

On the EFI side, use l2e_add_flags() to reduce the code-generation overhead of
using l2e_from_paddr() twice.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Wei Liu 
CC: Roger Pau Monné 

Can be tested by trying to boot with:

  diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
  index 759827a19a..fa83a9a28f 100644
  --- a/xen/arch/x86/setup.c
  +++ b/xen/arch/x86/setup.c
  @@ -52,6 +52,8 @@
   #include 
   #include 

  +static uint8_t __used big_data[MB(16)] = { 42, };
  +
   /* opt_nosmp: If true, secondary processors are ignored. */
   static bool __initdata opt_nosmp;
   boolean_param("nosmp", opt_nosmp);

Before this series, Xen will triple fault in one of two places (both on the
transition to Xen's high mappings), both ultimately because of cpu0_stack[]
getting shifted off the top of the mappings.
---
 xen/arch/x86/boot/head.S| 21 +
 xen/arch/x86/efi/efi-boot.h | 23 ++-
 xen/arch/x86/xen.lds.S  |  3 ---
 3 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 94bed4a2d3..eda3161fb1 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -687,14 +687,19 @@ trampoline_setup:
  * handling/walking), and identity map Xen into bootmap (needed for
  * the transition into long mode), using 2M superpages.
  */
-lea sym_esi(start),%ebx
-lea 
(1<> L2_PAGETABLE_SHIFT) & (4 * L2_PAGETABLE_ENTRIES - 1))
+
+for ( i  = l2_4G_offset(_start);
+  i <= l2_4G_offset(_end - 1); ++i )
 {
-unsigned int slot = (xen_phys_start >> L2_PAGETABLE_SHIFT) + i;
-paddr_t addr = slot << L2_PAGETABLE_SHIFT;
+l2_pgentry_t pte = l2e_from_paddr(i << L2_PAGETABLE_SHIFT,
+  __PAGE_HYPERVISOR | _PAGE_PSE);
+
+l2_bootmap[i] = pte;
+
+/* Bootmap RWX/Non-global.  Directmap RW/Global. */
+l2e_add_flags(pte, PAGE_HYPERVISOR);
 
-l2_directmap[slot] = l2e_from_paddr(addr, PAGE_HYPERVISOR|_PAGE_PSE);
-l2_bootmap[slot] = l2e_from_paddr(addr, __PAGE_HYPERVISOR|_PAGE_PSE);
+l2_directmap[i] = pte;
 }
+#undef l2_4G_offset
 }
 
 static void __init efi_arch_handle_module(struct file *file, const CHAR16 
*name,
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 7c351b9df3..a71853a856 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -384,6 +384,3 @@ ASSERT((trampoline_end - trampoline_start) < 
TRAMPOLINE_SPACE - MBI_SPACE_MIN,
 "not enough room for trampoline and mbi data")
 ASSERT((wakeup_stack - wakeup_stack_start) >= WAKEUP_STACK_MIN,
 "wakeup stack too small")
-
-/* Plenty of boot code assumes that Xen isn't larger than 16M. */
-ASSERT(_end - _start <= MB(16), "Xen too large for early-boot assumptions")
-- 
2.11.0


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel