Re: [PATCH v9 1/6] x86/boot: convert domain construction to use boot info

["Daniel P. Smith" <dpsmith@apertussolutions.com>]

On 11/15/24 09:25, Andrew Cooper wrote:
> On 15/11/2024 1:11 pm, Daniel P. Smith wrote:
>> With all the components used to construct dom0 encapsulated in struct boot_info
>> and struct boot_module, it is no longer necessary to pass all them as
>> parameters down the domain construction call chain. Change the parameter list
>> to pass the struct boot_info instance and the struct domain reference.
>>
>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> 
> There are two minor things needing noting in the commit message.
> 
> 1) dom0_construct() turns i from being signed to unsigned.  This is
> necessary for it's new use, and compatible with all pre-existing uses.
> 
> 2) dom0_construct() also splits some 3-way assignments to placate MISRA,
> on lines which are modified.

Ack.

>> diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c
>> index 3dd913bdb029..d1bdf1b14601 100644
>> --- a/xen/arch/x86/hvm/dom0_build.c
>> +++ b/xen/arch/x86/hvm/dom0_build.c
>> @@ -642,15 +643,15 @@ static bool __init check_and_adjust_load_address(
>>       return true;
>>   }
>>   
>> -static int __init pvh_load_kernel(struct domain *d, const module_t *image,
>> -                                  unsigned long image_headroom,
>> -                                  module_t *initrd, void *image_base,
>> -                                  const char *cmdline, paddr_t *entry,
>> -                                  paddr_t *start_info_addr)
>> +static int __init pvh_load_kernel(
>> +    struct domain *d, struct boot_module *image, struct boot_module *initrd,
>> +    paddr_t *entry, paddr_t *start_info_addr)
>>   {
>> -    void *image_start = image_base + image_headroom;
>> -    unsigned long image_len = image->mod_end;
>> -    unsigned long initrd_len = initrd ? initrd->mod_end : 0;
>> +    void *image_base = bootstrap_map_bm(image);
>> +    void *image_start = image_base + image->headroom;
>> +    unsigned long image_len = image->mod->mod_end;
>> +    unsigned long initrd_len = initrd ? initrd->mod->mod_end : 0;
>> +    const char *cmdline = __va(image->cmdline_pa);
> 
> This isn't safe.  __va(0) != NULL, so later between ...

Yah, that was careless to assume.

>>       struct elf_binary elf;
>>       struct elf_dom_parms parms;
>>       paddr_t last_addr;
>> @@ -725,8 +726,8 @@ static int __init pvh_load_kernel(struct domain *d, const module_t *image,
> 
> ... these two hunks in the calculation for last_addr, we have:
> 
>      ... cmdline ? ROUNDUP(strlen(cmdline) + 1, ...
> 
> which does the wrong thing.  (And includes the 16bit IVT onto the
> guest's cmdline.)
> 
> 
> I'd suggest doing the same as we do with initrd_len/etc, and having:
> 
>      const char *cmdline = image->cmdline_pa ? __va(image->cmdline_pa) :
> NULL;
> 
> to maintain the prior semantics.

Agreed.

>>   
>>       if ( initrd != NULL )
>>       {
>> -        rc = hvm_copy_to_guest_phys(last_addr, mfn_to_virt(initrd->mod_start),
>> -                                    initrd_len, v);
>> +        rc = hvm_copy_to_guest_phys(
>> +            last_addr, mfn_to_virt(initrd->mod->mod_start), initrd_len, v);
> 
> This is a temporary adjustment, ending up shorter than it starts by
> patch 3.  I've tweaked it to reduce the churn overall.  I can live with
> 83 chars width for a commit or two...

Just trying to ensure I don't get dinged, so no objection on my part.

>> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
>> index cc882bee61c3..6be3d7745fab 100644
>> --- a/xen/arch/x86/pv/dom0_build.c
>> +++ b/xen/arch/x86/pv/dom0_build.c
>> @@ -354,13 +355,10 @@ static struct page_info * __init alloc_chunk(struct domain *d,
>>       return page;
>>   }
>>   
>> -static int __init dom0_construct(struct domain *d,
>> -                                 const module_t *image,
>> -                                 unsigned long image_headroom,
>> -                                 module_t *initrd,
>> -                                 const char *cmdline)
>> +static int __init dom0_construct(struct boot_info *bi, struct domain *d)
>>   {
>> -    int i, rc, order, machine;
>> +    unsigned int i;
>> +    int rc, order, machine;
>>       bool compatible, compat;
>>       struct cpu_user_regs *regs;
>>       unsigned long pfn, mfn;
>> @@ -374,10 +372,13 @@ static int __init dom0_construct(struct domain *d,
>>       unsigned int flush_flags = 0;
>>       start_info_t *si;
>>       struct vcpu *v = d->vcpu[0];
>> -    void *image_base = bootstrap_map(image);
>> -    unsigned long image_len = image->mod_end;
>> -    void *image_start = image_base + image_headroom;
>> -    unsigned long initrd_len = initrd ? initrd->mod_end : 0;
>> +    struct boot_module *image;
>> +    struct boot_module *initrd = NULL;
>> +    void *image_base;
>> +    unsigned long image_len;
>> +    void *image_start;
>> +    unsigned long initrd_len = 0;
>> +    const char *cmdline;
> 
> I'm tempted to put in some newlines here, just to break up the giant
> block of variables.

Yes, this is a very long block of declarations.

> This use of cmdline in principle needs a similar adjustment to the pvh
> case, but it's only used once, so I suggest this instead:
> 
> @@ -984,8 +982,8 @@ static int __init dom0_construct(struct boot_info
> *bi, struct domain *d)
>       }
>   
>       memset(si->cmd_line, 0, sizeof(si->cmd_line));
> -    if ( cmdline != NULL )
> -        strlcpy((char *)si->cmd_line, cmdline, sizeof(si->cmd_line));
> +    if ( image->cmdline_pa )
> +        strlcpy((char *)si->cmd_line, __va(image->cmdline_pa),
> sizeof(si->cmd_line));
>   
>   #ifdef CONFIG_VIDEO
>       if ( !pv_shim && fill_console_start_info((void *)(si + 1)) )
> 
> 
> [edit] Turns out you do this in patch 6 anyway, so this way around will
> reduce churn.

Ack.

> Happy to fix on commit.

No objection.

v/r,
dps