This adds an option to the xl domain configuration file syntax for specifying
a kernel command line for device-model stubdomains. It is intended for use with
Linux-based stubdomains.

Signed-off-by: Scott Davis <scott.davis@starlab.io>
---
 docs/man/xl.cfg.5.pod.in             | 4 ++++
 tools/golang/xenlight/helpers.gen.go | 3 +++
 tools/golang/xenlight/types.gen.go   | 1 +
 tools/libs/light/libxl_dm.c          | 1 +
 tools/libs/light/libxl_types.idl     | 1 +
 tools/xl/xl_parse.c                  | 2 ++
 6 files changed, 12 insertions(+)

diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 56370a37db..6c777cad5c 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -2742,6 +2742,10 @@ In case of B<qemu-xen-traditional> it is expected to be MiniOS-based stubdomain
 image, in case of B<qemu-xen> it is expected to be Linux-based stubdomain
 kernel.
 
+=item B<stubdomain_cmdline="STRING">
+
+Append B<STRING> to the device-model stubdomain kernel command line.
+
 =item B<stubdomain_ramdisk="PATH">
 
 Override the path to the ramdisk image used as device-model stubdomain.
diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
index db82537b42..bfc1e7f312 100644
--- a/tools/golang/xenlight/helpers.gen.go
+++ b/tools/golang/xenlight/helpers.gen.go
@@ -1018,6 +1018,7 @@ return fmt.Errorf("converting field DeviceModelStubdomain: %v", err)
 }
 x.StubdomainMemkb = uint64(xc.stubdomain_memkb)
 x.StubdomainKernel = C.GoString(xc.stubdomain_kernel)
+x.StubdomainCmdline = C.GoString(xc.stubdomain_cmdline)
 x.StubdomainRamdisk = C.GoString(xc.stubdomain_ramdisk)
 x.DeviceModel = C.GoString(xc.device_model)
 x.DeviceModelSsidref = uint32(xc.device_model_ssidref)
@@ -1344,6 +1345,8 @@ return fmt.Errorf("converting field DeviceModelStubdomain: %v", err)
 xc.stubdomain_memkb = C.uint64_t(x.StubdomainMemkb)
 if x.StubdomainKernel != "" {
 xc.stubdomain_kernel = C.CString(x.StubdomainKernel)}
+if x.StubdomainCmdline != "" {
+xc.stubdomain_cmdline = C.CString(x.StubdomainCmdline)}
 if x.StubdomainRamdisk != "" {
 xc.stubdomain_ramdisk = C.CString(x.StubdomainRamdisk)}
 if x.DeviceModel != "" {
diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go
index a214dd9df6..09a3bb67e2 100644
--- a/tools/golang/xenlight/types.gen.go
+++ b/tools/golang/xenlight/types.gen.go
@@ -483,6 +483,7 @@ DeviceModelVersion DeviceModelVersion
 DeviceModelStubdomain Defbool
 StubdomainMemkb uint64
 StubdomainKernel string
+StubdomainCmdline string
 StubdomainRamdisk string
 DeviceModel string
 DeviceModelSsidref uint32
diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
index dbd3c7f278..2d54596834 100644
--- a/tools/libs/light/libxl_dm.c
+++ b/tools/libs/light/libxl_dm.c
@@ -2373,6 +2373,7 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
     }
 
     stubdom_state->pv_kernel.path = guest_config->b_info.stubdomain_kernel;
+    stubdom_state->pv_cmdline = guest_config->b_info.stubdomain_cmdline;
     stubdom_state->pv_ramdisk.path = guest_config->b_info.stubdomain_ramdisk;
 
     /* fixme: this function can leak the stubdom if it fails */
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index f45adddab0..e782e15cf2 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -523,6 +523,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
     ("device_model_stubdomain", libxl_defbool),
     ("stubdomain_memkb",   MemKB),
     ("stubdomain_kernel",  string),
+    ("stubdomain_cmdline", string),
     ("stubdomain_ramdisk", string),
     # if you set device_model you must set device_model_version too
     ("device_model",     string),
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 9fb0791429..17dddb4cd5 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -2533,6 +2533,8 @@ skip_usbdev:
 
     xlu_cfg_replace_string (config, "stubdomain_kernel",
                             &b_info->stubdomain_kernel, 0);
+    xlu_cfg_replace_string (config, "stubdomain_cmdline",
+                            &b_info->stubdomain_cmdline, 0);
     xlu_cfg_replace_string (config, "stubdomain_ramdisk",
                             &b_info->stubdomain_ramdisk, 0);
     if (!xlu_cfg_get_long (config, "stubdomain_memory", &l, 0))
-- 
2.25.1

On Tue, Jul 20, 2021 at 1:57 PM Scott Davis <scottwd@gmail.com> wrote:
>
> This adds an option to the xl domain configuration file syntax for specifying
> a kernel command line for device-model stubdomains. It is intended for use with
> Linux-based stubdomains.
>
> Signed-off-by: Scott Davis <scott.davis@starlab.io>
> ---
>  docs/man/xl.cfg.5.pod.in             | 4 ++++
>  tools/golang/xenlight/helpers.gen.go | 3 +++
>  tools/golang/xenlight/types.gen.go   | 1 +
>  tools/libs/light/libxl_dm.c          | 1 +
>  tools/libs/light/libxl_types.idl     | 1 +
>  tools/xl/xl_parse.c                  | 2 ++
>  6 files changed, 12 insertions(+)
>
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index 56370a37db..6c777cad5c 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -2742,6 +2742,10 @@ In case of B<qemu-xen-traditional> it is expected to be MiniOS-based stubdomain
>  image, in case of B<qemu-xen> it is expected to be Linux-based stubdomain
>  kernel.
>
> +=item B<stubdomain_cmdline="STRING">
> +
> +Append B<STRING> to the device-model stubdomain kernel command line.

I think this option actually sets the string, so you want "Set
B<STRING> as the device-model stubdomain kernel command line." or
something equivalent?

With a suitable change,
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>

Jason Andryuk writes ("Re: [PATCH] tools/xl: Add stubdomain_cmdline option to xl.cfg"):
> I think this option actually sets the string, so you want "Set
> B<STRING> as the device-model stubdomain kernel command line." or
> something equivalent?
> 
> With a suitable change,
> Reviewed-by: Jason Andryuk <jandryuk@gmail.com>

Does it then override an existing commandline calculated by libxl ?

Often people want to just add an option, so a config setting to append
things is useful (but one to override it completely is useful too).

Ian.

On Wed, Jul 21, 2021 at 8:50 AM Ian Jackson <iwj@xenproject.org> wrote:
>
> Jason Andryuk writes ("Re: [PATCH] tools/xl: Add stubdomain_cmdline option to xl.cfg"):
> > I think this option actually sets the string, so you want "Set
> > B<STRING> as the device-model stubdomain kernel command line." or
> > something equivalent?
> >
> > With a suitable change,
> > Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
>
> Does it then override an existing commandline calculated by libxl ?

Today, libxl doesn't handle a command line string for the stubdom
kernel, so it defaults to an empty string.

> Often people want to just add an option, so a config setting to append
> things is useful (but one to override it completely is useful too).

Yes, they can both be useful.  Append is sufficient until you want to
override or remove an option that is already included.  Set can be
tedious since you have to copy the existing options before appending
your new one.

Anyway, I just wanted the documentation to match the implementation.
Looks like xl.cfg.5.pod.in says Append for cmdline/root/extra, so
Scott was repeating that.  Looking around, aside from concatenating
root and extra in xl_parse.c:parse_cmdline(), libxl doesn't seem to
calculate command lines.  If libxl is reserving the right to calculate
cmdline in the future, then keeping Append is fine by me.

Regards,
Jason

Jason Andryuk writes ("Re: [PATCH] tools/xl: Add stubdomain_cmdline option to xl.cfg"):
> Yes, they can both be useful.  Append is sufficient until you want to
> override or remove an option that is already included.  Set can be
> tedious since you have to copy the existing options before appending
> your new one.
> 
> Anyway, I just wanted the documentation to match the implementation.

Yes.  I am happy with either approach.  Given the name I think
override is probably better; then we can do append with _extra later
if we like.

So in summary I agree with your suggested change to this patch.

> Looks like xl.cfg.5.pod.in says Append for cmdline/root/extra, so
> Scott was repeating that.  Looking around, aside from concatenating
> root and extra in xl_parse.c:parse_cmdline(), libxl doesn't seem to
> calculate command lines.  If libxl is reserving the right to calculate
> cmdline in the future, then keeping Append is fine by me.

Thanks for the investigation.

Ian.

Hi Scott,

On 20/07/2021 18:56, Scott Davis wrote:
> This adds an option to the xl domain configuration file syntax for specifying
> a kernel command line for device-model stubdomains. It is intended for use with
> Linux-based stubdomains.

May I ask why embedding the command line in the kernel would not be a 
solution? Do you expect it to change from stubdom to stubdom?

> Signed-off-by: Scott Davis <scott.davis@starlab.io>
> ---
>   docs/man/xl.cfg.5.pod.in             | 4 ++++
>   tools/golang/xenlight/helpers.gen.go | 3 +++
>   tools/golang/xenlight/types.gen.go   | 1 +
>   tools/libs/light/libxl_dm.c          | 1 +
>   tools/libs/light/libxl_types.idl     | 1 +
>   tools/xl/xl_parse.c                  | 2 ++
>   6 files changed, 12 insertions(+)
> 
> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
> index 56370a37db..6c777cad5c 100644
> --- a/docs/man/xl.cfg.5.pod.in
> +++ b/docs/man/xl.cfg.5.pod.in
> @@ -2742,6 +2742,10 @@ In case of B<qemu-xen-traditional> it is expected to be MiniOS-based stubdomain
>   image, in case of B<qemu-xen> it is expected to be Linux-based stubdomain
>   kernel.
>   
> +=item B<stubdomain_cmdline="STRING">
> +
> +Append B<STRING> to the device-model stubdomain kernel command line.
> +
>   =item B<stubdomain_ramdisk="PATH">
>   
>   Override the path to the ramdisk image used as device-model stubdomain.
> diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
> index db82537b42..bfc1e7f312 100644
> --- a/tools/golang/xenlight/helpers.gen.go
> +++ b/tools/golang/xenlight/helpers.gen.go
> @@ -1018,6 +1018,7 @@ return fmt.Errorf("converting field DeviceModelStubdomain: %v", err)
>   }
>   x.StubdomainMemkb = uint64(xc.stubdomain_memkb)
>   x.StubdomainKernel = C.GoString(xc.stubdomain_kernel)
> +x.StubdomainCmdline = C.GoString(xc.stubdomain_cmdline)
>   x.StubdomainRamdisk = C.GoString(xc.stubdomain_ramdisk)
>   x.DeviceModel = C.GoString(xc.device_model)
>   x.DeviceModelSsidref = uint32(xc.device_model_ssidref)
> @@ -1344,6 +1345,8 @@ return fmt.Errorf("converting field DeviceModelStubdomain: %v", err)
>   xc.stubdomain_memkb = C.uint64_t(x.StubdomainMemkb)
>   if x.StubdomainKernel != "" {
>   xc.stubdomain_kernel = C.CString(x.StubdomainKernel)}
> +if x.StubdomainCmdline != "" {
> +xc.stubdomain_cmdline = C.CString(x.StubdomainCmdline)}
>   if x.StubdomainRamdisk != "" {
>   xc.stubdomain_ramdisk = C.CString(x.StubdomainRamdisk)}
>   if x.DeviceModel != "" {
> diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go
> index a214dd9df6..09a3bb67e2 100644
> --- a/tools/golang/xenlight/types.gen.go
> +++ b/tools/golang/xenlight/types.gen.go
> @@ -483,6 +483,7 @@ DeviceModelVersion DeviceModelVersion
>   DeviceModelStubdomain Defbool
>   StubdomainMemkb uint64
>   StubdomainKernel string
> +StubdomainCmdline string
>   StubdomainRamdisk string
>   DeviceModel string
>   DeviceModelSsidref uint32
> diff --git a/tools/libs/light/libxl_dm.c b/tools/libs/light/libxl_dm.c
> index dbd3c7f278..2d54596834 100644
> --- a/tools/libs/light/libxl_dm.c
> +++ b/tools/libs/light/libxl_dm.c
> @@ -2373,6 +2373,7 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
>       }
>   
>       stubdom_state->pv_kernel.path = guest_config->b_info.stubdomain_kernel;
> +    stubdom_state->pv_cmdline = guest_config->b_info.stubdomain_cmdline;
>       stubdom_state->pv_ramdisk.path = guest_config->b_info.stubdomain_ramdisk;
>   
>       /* fixme: this function can leak the stubdom if it fails */
> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> index f45adddab0..e782e15cf2 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -523,6 +523,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>       ("device_model_stubdomain", libxl_defbool),
>       ("stubdomain_memkb",   MemKB),
>       ("stubdomain_kernel",  string),
> +    ("stubdomain_cmdline", string),

Please add a LIBXL_HAVE_... in include/libxl.h. This is used by external 
toolstack (e.g. libvirt) to know whether a given version of libxl 
provide the field.

>       ("stubdomain_ramdisk", string),
>       # if you set device_model you must set device_model_version too
>       ("device_model",     string),
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 9fb0791429..17dddb4cd5 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -2533,6 +2533,8 @@ skip_usbdev:
>   
>       xlu_cfg_replace_string (config, "stubdomain_kernel",
>                               &b_info->stubdomain_kernel, 0);
> +    xlu_cfg_replace_string (config, "stubdomain_cmdline",
> +                            &b_info->stubdomain_cmdline, 0);
>       xlu_cfg_replace_string (config, "stubdomain_ramdisk",
>                               &b_info->stubdomain_ramdisk, 0);
>       if (!xlu_cfg_get_long (config, "stubdomain_memory", &l, 0))
> 

Cheers,

-- 
Julien Grall

Thanks for the feedback, all.

On 7/21/21, 4:21 AM, Julien Grall wrote:
> May I ask why embedding the command line in the kernel would not be a
> solution? Do you expect it to change from stubdom to stubdom?

Of course. For Crucible, we're using a common kernel and initramfs for 
stubdomains and driver domains. The command line lets us tell each domain 
type how to configure itself on boot via the tool stack.

> Please add a LIBXL_HAVE_... in include/libxl.h. This is used by external
> toolstack (e.g. libvirt) to know whether a given version of libxl
> provide the field.

Ack. It appears there was not an entry added there for the other 
Linux-based "stubdomain_*" fields when they were inserted last year, so I 
will add a single entry in v2 to cover those three (memkb, kernel, and 
ramdisk) plus cmdline, unless there is an objection.

On 7/21/21, 8:42 AM, Jason Andryuk wrote:
> I think this option actually sets the string, so you want "Set
> B<STRING> as the device-model stubdomain kernel command line." or
> something equivalent?

Ack. As you later noted, I copied the "Append" wording from the cmdline 
option on the assumption that xl was leaving room for itself to add other 
items to the command line in the future. Will use "Set" in v2 for clarity, 
though.

Good day,

-Scott Davis 

On 21/07/2021 09:21, Julien Grall wrote:
> Hi Scott,
>
> On 20/07/2021 18:56, Scott Davis wrote:
>> This adds an option to the xl domain configuration file syntax for
>> specifying
>> a kernel command line for device-model stubdomains. It is intended
>> for use with
>> Linux-based stubdomains.
>
> May I ask why embedding the command line in the kernel would not be a
> solution? Do you expect it to change from stubdom to stubdom?

Why should users of stubdoms be forced to embed command line options? 
Especially when its not the normal way of working?

They shouldn't, and this alone is enough justification for the change.

~Andrew

Hi,

On 21/07/2021 10:06, Andrew Cooper wrote:
> On 21/07/2021 09:21, Julien Grall wrote:
>> Hi Scott,
>>
>> On 20/07/2021 18:56, Scott Davis wrote:
>>> This adds an option to the xl domain configuration file syntax for
>>> specifying
>>> a kernel command line for device-model stubdomains. It is intended
>>> for use with
>>> Linux-based stubdomains.
>>
>> May I ask why embedding the command line in the kernel would not be a
>> solution? Do you expect it to change from stubdom to stubdom?
> 
> Why should users of stubdoms be forced to embed command line options?
> Especially when its not the normal way of working?

I didn't suggest they should be forced. I was more interested to know 
the setup because I was expecting stubdomain to use a very tailored kernel.

> 
> They shouldn't, and this alone is enough justification for the change.

Everyone has a different perspective. I don't see the problem of asking 
the question... Maybe I should have add "OOI" to make clear with wasn't 
a complain.

Cheers,

-- 
Julien Grall

Julien Grall writes ("Re: [PATCH] tools/xl: Add stubdomain_cmdline option to xl.cfg"):
> Everyone has a different perspective. I don't see the problem of asking 
> the question... Maybe I should have add "OOI" to make clear with wasn't 
> a complain.

Yes, I think asking questions is fine, but we need to be conscious of
our status as maintainers and therefore gatekeepers.  When someone in
a gatekeeper position asks a question, the possibility of it being a
blocker is always present.  Indeed, I think it is even usual.

Adding "OOI" helps but it can help to be even more explicit.

Particularly, if someone proposes to add a feature, and a maintainer
asks "why can't you do X instead", there is a strong sense that the
maintainer thinks the feature is not (or may not be) necessary and
wants a stronger justification.  That can be quite discouraging.

If that disccouragement is not what's intended, then it can help for
the maintaier to be more explicit.  For example:

  "I don't oppose this feature.  But I am curious:..."

As for the original patch, I am in support of it and have reviewed it.
I have have only one question:

> +    stubdom_state->pv_cmdline = guest_config->b_info.stubdomain_cmdline;

It's been a while since I looked at this code.  I think that this is
the effective line, which takes the end result of the plumbing in the
rest of the patch and delivers it to this field of stubdom_state,
which is otherwise always null ?

Ian.

On 21.07.21 10:21, Julien Grall wrote:
> Hi Scott,
> 
> On 20/07/2021 18:56, Scott Davis wrote:
>> This adds an option to the xl domain configuration file syntax for 
>> specifying
>> a kernel command line for device-model stubdomains. It is intended for 
>> use with
>> Linux-based stubdomains.
> 
> May I ask why embedding the command line in the kernel would not be a 
> solution? Do you expect it to change from stubdom to stubdom?

This would preclude the possibility to use a standard distro kernel.


Juergen