| Message ID | 20210129090551.15608-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/pvh: pass module command line to dom0 | expand |
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. > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Acked-by: Jan Beulich <jbeulich@suse.com> 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. Jan
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@citrix.com> > > Acked-by: Jan Beulich <jbeulich@suse.com> > 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.
Roger Pau Monné writes ("Re: [PATCH] x86/pvh: pass module command line to dom0"):
> 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.
Thanks for the explanation. I'm not sure from reading this, whether
this is a bugfix, but your analysis is convincing, so:
Release-Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
diff --git a/xen/arch/x86/hvm/dom0_build.c b/xen/arch/x86/hvm/dom0_build.c index 12a82c9d7c..5f9281ce30 100644 --- 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; + } + last_addr = ROUNDUP(last_addr, PAGE_SIZE); } /* Free temporary buffers. */
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') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- NB: str cannot be made const because __hvm_copy buf parameter (that maps to str in the added code) is bi-directional depending on the flags passed to the function. --- xen/arch/x86/hvm/dom0_build.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)