Re: [Xen-devel] [PATCH v3 5/5] fix: add multiboot2 protocol support for EFI platforms

[Doug Goldstein]

On 1/18/17 8:41 AM, Jan Beulich wrote:
 On 18.01.17 at 14:33,  wrote:
>> On 1/18/17 4:52 AM, Jan Beulich wrote:
>> On 17.01.17 at 21:07,  wrote:
 @@ -686,6 +683,10 @@ paddr_t __init efi_multiboot2(EFI_HANDLE ImageHandle, 
 EFI_SYSTEM_TABLE *SystemTa
  setup_efi_pci();
  efi_variables();
  
 +/* This is the maximum size of our trampoline + our low memory stack 
 */
 +cfg.size = max_t(UINTN, 64 << 10,
 +(trampoline_end - trampoline_start) + 4096);
>>>
>>> Considering the consuming code further up, I don't understand the
>>> reason for the addition of 4096 here. And if there is a reason, I'm
>>> pretty sure you actually mean PAGE_SIZE.
>>
>> You are correct. Given that the assembly is hardcoded at 64k there is no
>> reason for this. I had kicked around doing a similar max() call in
>> assembly instead of hardcoding the value but didn't do it. So I should
>> just remove this.
> 
> Well, I don't mind the max() (albeit it's not very useful, especially
> if trampoline size would ever get pretty close to 64k). And as said
> in reply to the earlier version - I think this would better be checked
> at build time (see the various ASSERT()s at the end of xen.lds.S).
> 
> Jan
> 
Thank you. I had looked for BUILD_ASSERT() and didn't see it so I didn't
know what mechanism we had which is why I didn't add it.

-- 
Doug Goldstein



signature.asc
Description: OpenPGP digital signature
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 5/5] fix: add multiboot2 protocol support for EFI platforms

[Jan Beulich]

>>> On 18.01.17 at 14:33,  wrote:
> On 1/18/17 4:52 AM, Jan Beulich wrote:
> On 17.01.17 at 21:07,  wrote:
>>> --- a/xen/arch/x86/boot/head.S
>>> +++ b/xen/arch/x86/boot/head.S
>>> @@ -519,6 +519,7 @@ trampoline_setup:
>>>  1:
>>>  /* Switch to low-memory stack.  */
>>>  mov sym_phys(trampoline_phys),%edi
>>> +/* The stack base is 64kb after the location of trampoline_phys */
>>>  lea 0x1(%edi),%esp
>> 
>> I'm sorry for being picky, but the stack _base_ isn't where the stack
>> pointer should point initially, or else there would be no room on the
>> stack at all. Perhaps you had a reason for not using the wording I
>> did suggest, but whatever wording is chosen should not risk to cause
>> confusion (otherwise I think we'd be better off not adding any
>> comment here).
> 
> So this was my perception. Maybe I'm totally wrong here.
> 
> |--|-|--|
> a  b c  d
> 
> example values
> a = 0x0 (0)
> b = 0x8c000 (trampoline_phys)
> c = 0x9c000 (base of stack as it grows towards 0x8c000)
> d = 0x10 (1mb)
> 
> movsym_phys(trampoline_phys),%edi
> lea0x1(%edi),%esp
> 
> I would think that 0x1 + %edi would be the base or start of the
> stack since its growing downwards towards b.

Well, to me "start" and "base" are the lowest address and "end" the
highest (or one byte beyond).

>>> @@ -686,6 +683,10 @@ paddr_t __init efi_multiboot2(EFI_HANDLE ImageHandle, 
>>> EFI_SYSTEM_TABLE *SystemTa
>>>  setup_efi_pci();
>>>  efi_variables();
>>>  
>>> +/* This is the maximum size of our trampoline + our low memory stack */
>>> +cfg.size = max_t(UINTN, 64 << 10,
>>> +(trampoline_end - trampoline_start) + 4096);
>> 
>> Considering the consuming code further up, I don't understand the
>> reason for the addition of 4096 here. And if there is a reason, I'm
>> pretty sure you actually mean PAGE_SIZE.
> 
> You are correct. Given that the assembly is hardcoded at 64k there is no
> reason for this. I had kicked around doing a similar max() call in
> assembly instead of hardcoding the value but didn't do it. So I should
> just remove this.

Well, I don't mind the max() (albeit it's not very useful, especially
if trampoline size would ever get pretty close to 64k). And as said
in reply to the earlier version - I think this would better be checked
at build time (see the various ASSERT()s at the end of xen.lds.S).

Jan


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

Re: [Xen-devel] [PATCH v3 5/5] fix: add multiboot2 protocol support for EFI platforms

[Doug Goldstein]

On 1/18/17 4:52 AM, Jan Beulich wrote:
 On 17.01.17 at 21:07,  wrote:
>> --- a/xen/arch/x86/boot/head.S
>> +++ b/xen/arch/x86/boot/head.S
>> @@ -519,6 +519,7 @@ trampoline_setup:
>>  1:
>>  /* Switch to low-memory stack.  */
>>  mov sym_phys(trampoline_phys),%edi
>> +/* The stack base is 64kb after the location of trampoline_phys */
>>  lea 0x1(%edi),%esp
> 
> I'm sorry for being picky, but the stack _base_ isn't where the stack
> pointer should point initially, or else there would be no room on the
> stack at all. Perhaps you had a reason for not using the wording I
> did suggest, but whatever wording is chosen should not risk to cause
> confusion (otherwise I think we'd be better off not adding any
> comment here).

So this was my perception. Maybe I'm totally wrong here.

|--|-|--|
a  b c  d

example values
a = 0x0 (0)
b = 0x8c000 (trampoline_phys)
c = 0x9c000 (base of stack as it grows towards 0x8c000)
d = 0x10 (1mb)

movsym_phys(trampoline_phys),%edi
lea0x1(%edi),%esp

I would think that 0x1 + %edi would be the base or start of the
stack since its growing downwards towards b.

I'll use "ends" however and resubmit.

> 
>> --- a/xen/arch/x86/efi/efi-boot.h
>> +++ b/xen/arch/x86/efi/efi-boot.h
>> @@ -146,8 +146,6 @@ static void __init 
>> efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
>>  {
>>  struct e820entry *e;
>>  unsigned int i;
>> -/* Check for extra mem for mbi data if Xen is loaded via multiboot2 
>> protocol. */
>> -UINTN extra_mem = efi_enabled(EFI_LOADER) ? 0 : (64 << 10);
>>  
>>  /* Populate E820 table and check trampoline area availability. */
>>  e = e820map - 1;
>> @@ -170,8 +168,7 @@ static void __init 
>> efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
>>  /* fall through */
>>  case EfiConventionalMemory:
>>  if ( !trampoline_phys && desc->PhysicalStart + len <= 0x10 
>> &&
>> - len >= cfg.size + extra_mem &&
>> - desc->PhysicalStart + len > cfg.addr )
>> + len >= cfg.size && desc->PhysicalStart + len > cfg.addr )
>>  cfg.addr = (desc->PhysicalStart + len - cfg.size) & 
>> PAGE_MASK;
>>  /* fall through */
>>  case EfiLoaderCode:
>> @@ -686,6 +683,10 @@ paddr_t __init efi_multiboot2(EFI_HANDLE ImageHandle, 
>> EFI_SYSTEM_TABLE *SystemTa
>>  setup_efi_pci();
>>  efi_variables();
>>  
>> +/* This is the maximum size of our trampoline + our low memory stack */
>> +cfg.size = max_t(UINTN, 64 << 10,
>> +(trampoline_end - trampoline_start) + 4096);
> 
> Considering the consuming code further up, I don't understand the
> reason for the addition of 4096 here. And if there is a reason, I'm
> pretty sure you actually mean PAGE_SIZE.
> 
> Jan
> 

You are correct. Given that the assembly is hardcoded at 64k there is no
reason for this. I had kicked around doing a similar max() call in
assembly instead of hardcoding the value but didn't do it. So I should
just remove this.

-- 
Doug Goldstein



signature.asc
Description: OpenPGP digital signature
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 5/5] fix: add multiboot2 protocol support for EFI platforms

[Jan Beulich]

>>> On 17.01.17 at 21:07,  wrote:
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -519,6 +519,7 @@ trampoline_setup:
>  1:
>  /* Switch to low-memory stack.  */
>  mov sym_phys(trampoline_phys),%edi
> +/* The stack base is 64kb after the location of trampoline_phys */
>  lea 0x1(%edi),%esp

I'm sorry for being picky, but the stack _base_ isn't where the stack
pointer should point initially, or else there would be no room on the
stack at all. Perhaps you had a reason for not using the wording I
did suggest, but whatever wording is chosen should not risk to cause
confusion (otherwise I think we'd be better off not adding any
comment here).

> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -146,8 +146,6 @@ static void __init 
> efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
>  {
>  struct e820entry *e;
>  unsigned int i;
> -/* Check for extra mem for mbi data if Xen is loaded via multiboot2 
> protocol. */
> -UINTN extra_mem = efi_enabled(EFI_LOADER) ? 0 : (64 << 10);
>  
>  /* Populate E820 table and check trampoline area availability. */
>  e = e820map - 1;
> @@ -170,8 +168,7 @@ static void __init 
> efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
>  /* fall through */
>  case EfiConventionalMemory:
>  if ( !trampoline_phys && desc->PhysicalStart + len <= 0x10 &&
> - len >= cfg.size + extra_mem &&
> - desc->PhysicalStart + len > cfg.addr )
> + len >= cfg.size && desc->PhysicalStart + len > cfg.addr )
>  cfg.addr = (desc->PhysicalStart + len - cfg.size) & 
> PAGE_MASK;
>  /* fall through */
>  case EfiLoaderCode:
> @@ -686,6 +683,10 @@ paddr_t __init efi_multiboot2(EFI_HANDLE ImageHandle, 
> EFI_SYSTEM_TABLE *SystemTa
>  setup_efi_pci();
>  efi_variables();
>  
> +/* This is the maximum size of our trampoline + our low memory stack */
> +cfg.size = max_t(UINTN, 64 << 10,
> +(trampoline_end - trampoline_start) + 4096);

Considering the consuming code further up, I don't understand the
reason for the addition of 4096 here. And if there is a reason, I'm
pretty sure you actually mean PAGE_SIZE.

Jan


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

[Xen-devel] [PATCH v3 5/5] fix: add multiboot2 protocol support for EFI platforms

[Doug Goldstein]

This should be squashed into the 4/4 patch 'x86: add multiboot2 protocol
support for EFI platforms'.

- fix incorrect assembly (identified by Andrew Cooper)
- fix issue where the trampoline size was left as 0 and the
  way the memory is allocated for the trampolines we would go to
  the end of an available section and then subtract off the size
  to decide where to place it. The end result was that we would
  always copy the trampolines and the 32-bit stack into some
  form of reserved memory after the conventional region we
  wanted to put things into. On some systems this did not
  manifest as a crash while on others it did. Reworked the
  changes to always reserve 64kb for both the stack and the size
  of the trampolines.

Signed-off-by: Doug Goldstein 
Reviewed-by: Doug Goldstein 
---
Doug v3 - drop ASSERTs since they are runtime only without any output.
  This should be completely mitigated by using max() and
  ensuring we have a sane value.
  (found by Jan Beulich)
- removed extra_mem variable that was incorrectly left behind.
  (found by Jan Beulich)
- fix comment around the "start of stack"
  (found by Jan Beulich)
Doug v2 - new in this version to help show what's changed
---
---
 xen/arch/x86/boot/head.S|  1 +
 xen/arch/x86/efi/efi-boot.h |  9 +
 xen/arch/x86/efi/stub.c |  2 +-
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index ac93df0..d288959 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -519,6 +519,7 @@ trampoline_setup:
 1:
 /* Switch to low-memory stack.  */
 mov sym_phys(trampoline_phys),%edi
+/* The stack base is 64kb after the location of trampoline_phys */
 lea 0x1(%edi),%esp
 lea trampoline_boot_cpu_entry-trampoline_start(%edi),%eax
 pushl   $BOOT_CS32
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index dc857d8..a73134c 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -146,8 +146,6 @@ static void __init 
efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
 {
 struct e820entry *e;
 unsigned int i;
-/* Check for extra mem for mbi data if Xen is loaded via multiboot2 
protocol. */
-UINTN extra_mem = efi_enabled(EFI_LOADER) ? 0 : (64 << 10);
 
 /* Populate E820 table and check trampoline area availability. */
 e = e820map - 1;
@@ -170,8 +168,7 @@ static void __init 
efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
 /* fall through */
 case EfiConventionalMemory:
 if ( !trampoline_phys && desc->PhysicalStart + len <= 0x10 &&
- len >= cfg.size + extra_mem &&
- desc->PhysicalStart + len > cfg.addr )
+ len >= cfg.size && desc->PhysicalStart + len > cfg.addr )
 cfg.addr = (desc->PhysicalStart + len - cfg.size) & PAGE_MASK;
 /* fall through */
 case EfiLoaderCode:
@@ -686,6 +683,10 @@ paddr_t __init efi_multiboot2(EFI_HANDLE ImageHandle, 
EFI_SYSTEM_TABLE *SystemTa
 setup_efi_pci();
 efi_variables();
 
+/* This is the maximum size of our trampoline + our low memory stack */
+cfg.size = max_t(UINTN, 64 << 10,
+(trampoline_end - trampoline_start) + 4096);
+
 if ( gop )
 efi_set_gop_mode(gop, gop_mode);
 
diff --git a/xen/arch/x86/efi/stub.c b/xen/arch/x86/efi/stub.c
index 6ea6aa1..b81adc0 100644
--- a/xen/arch/x86/efi/stub.c
+++ b/xen/arch/x86/efi/stub.c
@@ -33,7 +33,7 @@ paddr_t __init noreturn efi_multiboot2(EFI_HANDLE ImageHandle,
  * not be directly supported by C compiler.
  */
 asm volatile(
-"call %2  \n"
+"call *%2 \n"
 "0:  hlt  \n"
 "jmp  0b  \n"
: "+c" (StdErr), "+d" (err) : "g" (StdErr->OutputString)
-- 
git-series 0.9.1

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