diff mbox series

x86/pvh: pass module command line to dom0

Message ID 20210129090551.15608-1-roger.pau@citrix.com (mailing list archive)
State New
Headers show
Series x86/pvh: pass module command line to dom0 | expand

Commit Message

Roger Pau Monné Jan. 29, 2021, 9:05 a.m. UTC
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(-)

Comments

Jan Beulich Jan. 29, 2021, 9:26 a.m. UTC | #1
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
Roger Pau Monné Jan. 29, 2021, 9:48 a.m. UTC | #2
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.
Ian Jackson Jan. 29, 2021, 2:57 p.m. UTC | #3
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 mbox series

Patch

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. */