Re: [PATCH] x86/pvh: pass module command line to dom0

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Jan 29, 2021 at 10:26:31AM +0100, Jan Beulich wrote:
> On 29.01.2021 10:05, Roger Pau Monne wrote:
> > Both the multiboot and the HVM start info structures allow passing a
> > string together with a module. Implement the missing support in
> > pvh_load_kernel so that module strings found in the multiboot
> > structure are forwarded to dom0.
> > 
> > Fixes: 62ba982424 ('x86: parse Dom0 kernel for PVHv2')
> 
> This really is relevant only when it's not an initrd which gets
> passed as module, I suppose? I'm not fully convinced I'd call
> this a bug then, but perhaps more a missing feature. Which in
> turn means I'm not sure about the change's disposition wrt 4.15.
> Cc-ing Ian.

Right, the whole kernel loading stuff is IMO not the best one, because
all the multiboot modules apart from Xen and the dom0 kernel (the
first 2) should be passed to the dom0 IMO using the HVM start_info
structure.

The module command line is just a red herring, as none of the OSes
that currently have PVH dom0 support need this, but still would be
nice to get it fixed so that if new OSes are added it works properly.
It's unexpected that a loader can append a string to a module, but
then the dom0 kernel finds none in the start_info module list.

Regarding 4.15 acceptance: I think this is very low risk, as it
exclusively touches PVH dom0 code which is experimental anyway, but
I'm not going to insist on it getting committed.

> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> Albeit ...
> 
> > --- a/xen/arch/x86/hvm/dom0_build.c
> > +++ b/xen/arch/x86/hvm/dom0_build.c
> > @@ -617,7 +617,21 @@ static int __init pvh_load_kernel(struct domain *d, 
> > const module_t *image,
> >  
> >          mod.paddr = last_addr;
> >          mod.size = initrd->mod_end;
> > -        last_addr += ROUNDUP(initrd->mod_end, PAGE_SIZE);
> > +        last_addr += ROUNDUP(initrd->mod_end, elf_64bit(&elf) ? 8 : 4);
> > +        if ( initrd->string )
> > +        {
> > +            char *str = __va(initrd->string);
> > +
> > +            rc = hvm_copy_to_guest_phys(last_addr, str, strlen(str) + 1, 
> > v);
> > +            if ( rc )
> > +            {
> > +                printk("Unable to copy module command line\n");
> > +                return rc;
> > +            }
> > +            mod.cmdline_paddr = last_addr;
> > +            last_addr += strlen(str) + 1;
> 
> ... it would have been nice if this length was calculated just
> once, into a local variable. I'd be fine making the adjustment
> while committing, so long as you agree.

Sure, feel free to add a len variable to the scope of the if.

Thanks, Roger.