diff mbox series

[v6,11/44] x86/boot: add start and size fields to struct boot_module

Message ID 20241017170325.3842-12-dpsmith@apertussolutions.com (mailing list archive)
State Superseded
Headers show
Series Boot modules for Hyperlaunch | expand

Commit Message

Daniel P. Smith Oct. 17, 2024, 5:02 p.m. UTC
This commit introduces the start and size fields to struct boot_module and
assigns their value during boot_info construction.

The EFI entry point special cased itself, and pre-converted mod_start and
mod_end to mfn and size respectively. This required an additional test
in the coversion loop to not convert mod_start and mod_end when the conversion
was done for both the multiboot and PVH boot entry points. To simplify the
logic populating the start and size fields, the EFI module population logic
was coverted to using start and end addresses.

Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
---
Changes since v5:
- switched EFI population of mod_start/mod_end to addresses
---
 xen/arch/x86/efi/efi-boot.h         | 4 ++--
 xen/arch/x86/include/asm/bootinfo.h | 3 +++
 xen/arch/x86/setup.c                | 7 ++++++-
 3 files changed, 11 insertions(+), 3 deletions(-)

Comments

Jason Andryuk Oct. 17, 2024, 11:12 p.m. UTC | #1
On 2024-10-17 13:02, Daniel P. Smith wrote:
> This commit introduces the start and size fields to struct boot_module and
> assigns their value during boot_info construction.
> 
> The EFI entry point special cased itself, and pre-converted mod_start and
> mod_end to mfn and size respectively. This required an additional test
> in the coversion loop to not convert mod_start and mod_end when the conversion

conversion

> was done for both the multiboot and PVH boot entry points. To simplify the
> logic populating the start and size fields, the EFI module population logic
> was coverted to using start and end addresses.

converted

> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>

with that
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

EFI now gains the alignment check, but beforehand it was just silently 
truncating.  Seems better to check :)

Regards,
Jason
Andrew Cooper Oct. 18, 2024, 12:23 a.m. UTC | #2
On 17/10/2024 6:02 pm, Daniel P. Smith wrote:
> This commit introduces the start and size fields to struct boot_module and
> assigns their value during boot_info construction.
>
> The EFI entry point special cased itself, and pre-converted mod_start and
> mod_end to mfn and size respectively. This required an additional test
> in the coversion loop to not convert mod_start and mod_end when the conversion
> was done for both the multiboot and PVH boot entry points. To simplify the
> logic populating the start and size fields, the EFI module population logic
> was coverted to using start and end addresses.
>
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> ---
> Changes since v5:
> - switched EFI population of mod_start/mod_end to addresses
> ---
>  xen/arch/x86/efi/efi-boot.h         | 4 ++--
>  xen/arch/x86/include/asm/bootinfo.h | 3 +++
>  xen/arch/x86/setup.c                | 7 ++++++-
>  3 files changed, 11 insertions(+), 3 deletions(-)

Despite being small already, I'd still prefer to split this into two
patches.

One changing the EFI path, and a separate one adding in the new (real)
start/size fields.

Sods law would says that if we don't, bisection is going to end up here,
except it doesn't need to...

> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index 94f34433640f..779f101c3de7 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -732,8 +732,8 @@ static void __init efi_arch_handle_module(const struct file *file,
>      if ( options )
>          place_string(&mb_modules[mbi.mods_count].string, options);
>      place_string(&mb_modules[mbi.mods_count].string, local_name.s);
> -    mb_modules[mbi.mods_count].mod_start = file->addr >> PAGE_SHIFT;
> -    mb_modules[mbi.mods_count].mod_end = file->size;
> +    mb_modules[mbi.mods_count].mod_start = file->addr;
> +    mb_modules[mbi.mods_count].mod_end = file->addr + file->size;

... because you can't drop this shift until you have a full-width start
field to use.   file->addr might exceed 4G, and truncate with this patch
in place.

You're going to need to keep the old semantics here, fill in all 4
fields, and keep the EFI special case in the final hunk until you can
remove the ->mod_{start,end} narrow fields.

> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index f87faa31a2cf..6ee352fc0cde 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -309,8 +309,13 @@ static struct boot_info *__init multiboot_fill_boot_info(unsigned long mbi_p)
>       * reserved for Xen itself
>       */
>      for ( i = 0; i <= bi->nr_modules; i++ )
> +    {
>          bi->mods[i].mod = &mods[i];
>  
> +        bi->mods[i].start = (paddr_t)mods[i].mod_start;

This cast isn't needed, but don't we need a shift in here?

~Andrew
Daniel P. Smith Oct. 19, 2024, 11:32 a.m. UTC | #3
On 10/17/24 19:12, Jason Andryuk wrote:
> On 2024-10-17 13:02, Daniel P. Smith wrote:
>> This commit introduces the start and size fields to struct boot_module 
>> and
>> assigns their value during boot_info construction.
>>
>> The EFI entry point special cased itself, and pre-converted mod_start and
>> mod_end to mfn and size respectively. This required an additional test
>> in the coversion loop to not convert mod_start and mod_end when the 
>> conversion
> 
> conversion

Ack.

>> was done for both the multiboot and PVH boot entry points. To simplify 
>> the
>> logic populating the start and size fields, the EFI module population 
>> logic
>> was coverted to using start and end addresses.
> 
> converted

Ack.

>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> 
> with that
> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>

Thank you but will hold off since Andy has asked this be split and changed.

> EFI now gains the alignment check, but beforehand it was just silently 
> truncating.  Seems better to check :)

My thinking as well.

v/r,
dps
Daniel P. Smith Oct. 19, 2024, 11:53 a.m. UTC | #4
On 10/17/24 20:23, Andrew Cooper wrote:
> On 17/10/2024 6:02 pm, Daniel P. Smith wrote:
>> This commit introduces the start and size fields to struct boot_module and
>> assigns their value during boot_info construction.
>>
>> The EFI entry point special cased itself, and pre-converted mod_start and
>> mod_end to mfn and size respectively. This required an additional test
>> in the coversion loop to not convert mod_start and mod_end when the conversion
>> was done for both the multiboot and PVH boot entry points. To simplify the
>> logic populating the start and size fields, the EFI module population logic
>> was coverted to using start and end addresses.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
>> ---
>> Changes since v5:
>> - switched EFI population of mod_start/mod_end to addresses
>> ---
>>   xen/arch/x86/efi/efi-boot.h         | 4 ++--
>>   xen/arch/x86/include/asm/bootinfo.h | 3 +++
>>   xen/arch/x86/setup.c                | 7 ++++++-
>>   3 files changed, 11 insertions(+), 3 deletions(-)
> 
> Despite being small already, I'd still prefer to split this into two
> patches.

I was going to agree with you but thinking about your point that in EFI 
the addr could legitimately be >4G and thus the conversion to pfn and 
size will need to stay. Therefore this will cause the efi changes to 
drop back out and require the introduction of EFI test and alternative 
path for setting start/size by converting back. If I were to break up it 
up, what will result is there will be a patch that introduces straight 
assignment for MB and PVH boot. The second commit would introduce the 
EFI test and alternate branch to assign start using the shift 
conversion. I think this all probably should stay in a single patch, but 
if you still would like to see it separate than I will do so.

> One changing the EFI path, and a separate one adding in the new (real)
> start/size fields.
> 
> Sods law would says that if we don't, bisection is going to end up here,
> except it doesn't need to...
> 
>> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
>> index 94f34433640f..779f101c3de7 100644
>> --- a/xen/arch/x86/efi/efi-boot.h
>> +++ b/xen/arch/x86/efi/efi-boot.h
>> @@ -732,8 +732,8 @@ static void __init efi_arch_handle_module(const struct file *file,
>>       if ( options )
>>           place_string(&mb_modules[mbi.mods_count].string, options);
>>       place_string(&mb_modules[mbi.mods_count].string, local_name.s);
>> -    mb_modules[mbi.mods_count].mod_start = file->addr >> PAGE_SHIFT;
>> -    mb_modules[mbi.mods_count].mod_end = file->size;
>> +    mb_modules[mbi.mods_count].mod_start = file->addr;
>> +    mb_modules[mbi.mods_count].mod_end = file->addr + file->size;
> 
> ... because you can't drop this shift until you have a full-width start
> field to use.   file->addr might exceed 4G, and truncate with this patch
> in place.
> 
> You're going to need to keep the old semantics here, fill in all 4
> fields, and keep the EFI special case in the final hunk until you can
> remove the ->mod_{start,end} narrow fields.

While in testing I did not see it occur, you are correct that it is 
possible that the EFI allocator may choose to do so. I will drop the 
change in the efi code and handle with a condition in the 
multiboot_fill_boot_info() funciton for the time being.

>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>> index f87faa31a2cf..6ee352fc0cde 100644
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -309,8 +309,13 @@ static struct boot_info *__init multiboot_fill_boot_info(unsigned long mbi_p)
>>        * reserved for Xen itself
>>        */
>>       for ( i = 0; i <= bi->nr_modules; i++ )
>> +    {
>>           bi->mods[i].mod = &mods[i];
>>   
>> +        bi->mods[i].start = (paddr_t)mods[i].mod_start;
> 
> This cast isn't needed, but don't we need a shift in here?

Only for EFI.

v/r,
dps
diff mbox series

Patch

diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 94f34433640f..779f101c3de7 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -732,8 +732,8 @@  static void __init efi_arch_handle_module(const struct file *file,
     if ( options )
         place_string(&mb_modules[mbi.mods_count].string, options);
     place_string(&mb_modules[mbi.mods_count].string, local_name.s);
-    mb_modules[mbi.mods_count].mod_start = file->addr >> PAGE_SHIFT;
-    mb_modules[mbi.mods_count].mod_end = file->size;
+    mb_modules[mbi.mods_count].mod_start = file->addr;
+    mb_modules[mbi.mods_count].mod_end = file->addr + file->size;
     ++mbi.mods_count;
     efi_bs->FreePool(ptr);
 }
diff --git a/xen/arch/x86/include/asm/bootinfo.h b/xen/arch/x86/include/asm/bootinfo.h
index e8ba9390a51f..5862054b8cef 100644
--- a/xen/arch/x86/include/asm/bootinfo.h
+++ b/xen/arch/x86/include/asm/bootinfo.h
@@ -42,6 +42,9 @@  struct boot_module {
 
     uint32_t flags;
 #define BOOTMOD_FLAG_X86_RELOCATED     (1U << 0)
+
+    paddr_t start;
+    size_t size;
 };
 
 /*
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index f87faa31a2cf..6ee352fc0cde 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -309,8 +309,13 @@  static struct boot_info *__init multiboot_fill_boot_info(unsigned long mbi_p)
      * reserved for Xen itself
      */
     for ( i = 0; i <= bi->nr_modules; i++ )
+    {
         bi->mods[i].mod = &mods[i];
 
+        bi->mods[i].start = (paddr_t)mods[i].mod_start;
+        bi->mods[i].size = mods[i].mod_end - mods[i].mod_start;
+    }
+
     /* map the last mb module for xen entry */
     bi->mods[bi->nr_modules].type = BOOTMOD_XEN;
 
@@ -1351,7 +1356,7 @@  void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
      */
     initial_images = bi->mods[0].mod;
 
-    for ( i = 0; !efi_enabled(EFI_LOADER) && i < bi->nr_modules; i++ )
+    for ( i = 0; i < bi->nr_modules; i++ )
     {
         if ( bi->mods[i].mod->mod_start & (PAGE_SIZE - 1) )
             panic("Bootloader didn't honor module alignment request\n");