All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] tools/xl: Add stubdomain_cmdline option to xl.cfg
@ 2021-07-20 17:56 Scott Davis
  2021-07-21  8:21 ` Julien Grall
  2021-07-21 12:42 ` Jason Andryuk
  0 siblings, 2 replies; 11+ messages in thread
From: Scott Davis @ 2021-07-20 17:56 UTC (permalink / raw)
  To: xen-devel
  Cc: Scott Davis, Ian Jackson, Wei Liu, George Dunlap, Nick Rosbrook,
	Anthony PERARD, Juergen Gross

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



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] tools/xl: Add stubdomain_cmdline option to xl.cfg
  2021-07-20 17:56 [PATCH] tools/xl: Add stubdomain_cmdline option to xl.cfg Scott Davis
@ 2021-07-21  8:21 ` Julien Grall
  2021-07-21  9:00   ` Juergen Gross
                     ` (2 more replies)
  2021-07-21 12:42 ` Jason Andryuk
  1 sibling, 3 replies; 11+ messages in thread
From: Julien Grall @ 2021-07-21  8:21 UTC (permalink / raw)
  To: Scott Davis, xen-devel
  Cc: Scott Davis, Ian Jackson, Wei Liu, George Dunlap, Nick Rosbrook,
	Anthony PERARD, Juergen Gross

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] tools/xl: Add stubdomain_cmdline option to xl.cfg
  2021-07-21  8:21 ` Julien Grall
@ 2021-07-21  9:00   ` Juergen Gross
  2021-07-21  9:06   ` Andrew Cooper
  2021-07-21 22:30   ` Scott Davis
  2 siblings, 0 replies; 11+ messages in thread
From: Juergen Gross @ 2021-07-21  9:00 UTC (permalink / raw)
  To: Julien Grall, Scott Davis, xen-devel
  Cc: Scott Davis, Ian Jackson, Wei Liu, George Dunlap, Nick Rosbrook,
	Anthony PERARD


[-- Attachment #1.1.1: Type: text/plain, Size: 533 bytes --]

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

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] tools/xl: Add stubdomain_cmdline option to xl.cfg
  2021-07-21  8:21 ` Julien Grall
  2021-07-21  9:00   ` Juergen Gross
@ 2021-07-21  9:06   ` Andrew Cooper
  2021-07-21  9:11     ` Julien Grall
  2021-07-21 22:30   ` Scott Davis
  2 siblings, 1 reply; 11+ messages in thread
From: Andrew Cooper @ 2021-07-21  9:06 UTC (permalink / raw)
  To: Julien Grall, Scott Davis, xen-devel
  Cc: Scott Davis, Ian Jackson, Wei Liu, George Dunlap, Nick Rosbrook,
	Anthony PERARD, Juergen Gross

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] tools/xl: Add stubdomain_cmdline option to xl.cfg
  2021-07-21  9:06   ` Andrew Cooper
@ 2021-07-21  9:11     ` Julien Grall
  2021-07-21  9:57       ` Ian Jackson
  0 siblings, 1 reply; 11+ messages in thread
From: Julien Grall @ 2021-07-21  9:11 UTC (permalink / raw)
  To: Andrew Cooper, Scott Davis, xen-devel
  Cc: Scott Davis, Ian Jackson, Wei Liu, George Dunlap, Nick Rosbrook,
	Anthony PERARD, Juergen Gross

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] tools/xl: Add stubdomain_cmdline option to xl.cfg
  2021-07-21  9:11     ` Julien Grall
@ 2021-07-21  9:57       ` Ian Jackson
  0 siblings, 0 replies; 11+ messages in thread
From: Ian Jackson @ 2021-07-21  9:57 UTC (permalink / raw)
  To: Scott Davis, Julien Grall
  Cc: Andrew Cooper, xen-devel, Scott Davis, Wei Liu, George Dunlap,
	Nick Rosbrook, Anthony PERARD, Juergen Gross

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.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] tools/xl: Add stubdomain_cmdline option to xl.cfg
  2021-07-20 17:56 [PATCH] tools/xl: Add stubdomain_cmdline option to xl.cfg Scott Davis
  2021-07-21  8:21 ` Julien Grall
@ 2021-07-21 12:42 ` Jason Andryuk
  2021-07-21 12:50   ` Ian Jackson
  1 sibling, 1 reply; 11+ messages in thread
From: Jason Andryuk @ 2021-07-21 12:42 UTC (permalink / raw)
  To: Scott Davis
  Cc: xen-devel, Scott Davis, Ian Jackson, Wei Liu, George Dunlap,
	Nick Rosbrook, Anthony PERARD, Juergen Gross

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>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] tools/xl: Add stubdomain_cmdline option to xl.cfg
  2021-07-21 12:42 ` Jason Andryuk
@ 2021-07-21 12:50   ` Ian Jackson
  2021-07-21 13:26     ` Jason Andryuk
  0 siblings, 1 reply; 11+ messages in thread
From: Ian Jackson @ 2021-07-21 12:50 UTC (permalink / raw)
  To: Jason Andryuk
  Cc: Scott Davis, xen-devel, Scott Davis, Wei Liu, George Dunlap,
	Nick Rosbrook, Anthony PERARD, Juergen Gross

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.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] tools/xl: Add stubdomain_cmdline option to xl.cfg
  2021-07-21 12:50   ` Ian Jackson
@ 2021-07-21 13:26     ` Jason Andryuk
  2021-07-21 14:03       ` Ian Jackson
  0 siblings, 1 reply; 11+ messages in thread
From: Jason Andryuk @ 2021-07-21 13:26 UTC (permalink / raw)
  To: Ian Jackson
  Cc: Scott Davis, xen-devel, Scott Davis, Wei Liu, George Dunlap,
	Nick Rosbrook, Anthony PERARD, Juergen Gross

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] tools/xl: Add stubdomain_cmdline option to xl.cfg
  2021-07-21 13:26     ` Jason Andryuk
@ 2021-07-21 14:03       ` Ian Jackson
  0 siblings, 0 replies; 11+ messages in thread
From: Ian Jackson @ 2021-07-21 14:03 UTC (permalink / raw)
  To: Jason Andryuk
  Cc: Scott Davis, xen-devel, Scott Davis, Wei Liu, George Dunlap,
	Nick Rosbrook, Anthony PERARD, Juergen Gross

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.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] tools/xl: Add stubdomain_cmdline option to xl.cfg
  2021-07-21  8:21 ` Julien Grall
  2021-07-21  9:00   ` Juergen Gross
  2021-07-21  9:06   ` Andrew Cooper
@ 2021-07-21 22:30   ` Scott Davis
  2 siblings, 0 replies; 11+ messages in thread
From: Scott Davis @ 2021-07-21 22:30 UTC (permalink / raw)
  To: Julien Grall, Jason Andryuk, xen-devel
  Cc: Ian Jackson, Wei Liu, George Dunlap, Nick Rosbrook,
	Anthony PERARD, Juergen Gross

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 


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-07-21 22:31 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-20 17:56 [PATCH] tools/xl: Add stubdomain_cmdline option to xl.cfg Scott Davis
2021-07-21  8:21 ` Julien Grall
2021-07-21  9:00   ` Juergen Gross
2021-07-21  9:06   ` Andrew Cooper
2021-07-21  9:11     ` Julien Grall
2021-07-21  9:57       ` Ian Jackson
2021-07-21 22:30   ` Scott Davis
2021-07-21 12:42 ` Jason Andryuk
2021-07-21 12:50   ` Ian Jackson
2021-07-21 13:26     ` Jason Andryuk
2021-07-21 14:03       ` Ian Jackson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.