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

Roger Pau Monne posted 1 patch 3 years, 1 month ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20210129090551.15608-1-roger.pau@citrix.com
xen/arch/x86/hvm/dom0_build.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
[PATCH] x86/pvh: pass module command line to dom0
Posted by Roger Pau Monne 3 years, 1 month ago
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(-)

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


Re: [PATCH] x86/pvh: pass module command line to dom0
Posted by Jan Beulich 3 years, 1 month ago
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

Re: [PATCH] x86/pvh: pass module command line to dom0
Posted by Roger Pau Monné 3 years, 1 month ago
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.

Re: [PATCH] x86/pvh: pass module command line to dom0
Posted by Ian Jackson 3 years, 1 month ago
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>