[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Fri, 29 Jan 2021 10:48:38 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=hTmy30oIwGm3CM9kBqi/4iHWD/EryOnwkFe1Y2snvXs=; b=iPlfhCYo8EPIxWGbI1a9GB2HZg16LELgqVDOKrY85nxzQNzdpv+v5wc/IYNneVE03647A91gXuIqPUjyNQTagmnJUCdty2nGDeBhDK4LBq9qNxNDJp4gYsoUdm+ASU/vaL1Kd2cwi5mS6GRnlaOjrGPaIWg1lTt7jl9fhHGx6TvpsmnAG9bCTHUANPF/n/Y4agvLTJFF+AfbqavcMUP6cYhC0/caCW3DUGrTYVr1EEJy+2CiZnp27BGhq8EvgYuZZQo6S3nBmafnZA8ARK9Z/l+8JpKLfT493DaWDVYXDhihTA0521FLla6Ll24w4yac6E7juAmVtPUi2uQzMmk52Q==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Ot9Uf9d+Mg9q0Fg0woC3RsfE5bnhgaz+u2ShHxCbXsRtpx17w9DYjaFIDLK7JKXP+r+MvN2+KxhTpbXq71Js8jCm80JanuI7xhQtzBzWGzwAFBi4YPrDQyNJNXn61QpTlIBu/q0ovDtTe5jLIkl6LYouaUhiehSE2a4u5S13576YtgVP1A6K8CDiHEtqr5yEUTW7IZfLLkdi2Ja9Ueqh2M975zx3Yp78KrXUtEzWRLCJSDPoGum8mFFxhvCzzLETnQ6/FBqHWlzgm0iyUZmWbrV9QfKkJJEfwcTRAJ3TsqqRmTU7BVFnDPlBI5/invgZUVd7KK4PEDRhA3LnSLyHog==
  • Authentication-results: esa2.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Ian Jackson <iwj@xxxxxxxxxxxxxx>
  • Delivery-date: Fri, 29 Jan 2021 09:48:50 +0000
  • Ironport-sdr: cw4qTC5Yqi/nLjla1fBUIUhGcIxmZ8JHp4i66H7V/Ie19tP+KMA86iqHWDgUA4bJTHJ8EbqPL3 xpfNbj5lAVfkjtIrAPR9CnZ/l27ZsxOJzQAFtKBZ0yxQRlojru5dJObKbSMnPM2rFe5j1rsAWB ggUJBMIVARQRp3bJv1aa9bpEFv2YtJgZ3NkVdgmlnrFnwiDPBaYeiioSmHxjIgA7pCgAFX8jyF tm/5+f0naQM5iFyUp6OY8QCPV6C/NjNAvqigRjS+8WcC8pM3uCkDSgsYq9G8P7MyVeaDtgRgZg SX4=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.