Re: [PATCH] xen/argo: Command line handling improvements
On Wed, May 21, 2025 at 9:43 PM Andrew Cooper
wrote:
> On 21/05/2025 2:01 am, Christopher Clark wrote:
> > On Tue, May 20, 2025 at 3:10 PM Andrew Cooper
> > wrote:
> >
> > Treat "argo" on the command line as a positive boolean, rather
> > than requiring
> > the user to pass "argo=1/on/enable/true".
>
I have tested that this patch does change the command line behaviour as
described, and doesn't prevent the existing valid options from parsing and
acting as they currently do, to enable and disable the subsystem, so that
is positive. I do have significant reservations stated below, however.
> >
> > Move both opt_argo* variables into __ro_after_init. They're set
> > during
> > command line parsing and never modified thereafter.
>
I haven't directly tested this aspect of the patch.
> >
> >
> > Instead of binding to static values set at boot, would
> > boolean_runtime_param be acceptable?
>
> No, for several reasons.
>
Thanks for the reply, your perspective is helpful.
>
> Sure, you could dynamically turn it on, but existing domains can't use
> it because argo_init() wasn't called on them. Now consider what happens
> when dynamically turning it off behind the back of a domain which was
> using it.
>
> All the existing runtime controls are for properties that are not
> visible to guests. Adding opt_argo to this list would create a lot of
> carnage.
>
OK, your aversion to it is clear and I'm not pushing to make that change.
>
> (Like almost everything else in Xen), Argo is entirely broken with
> respect to enumeration and controls. And while this isn't your fault
> for having copied the status quo, it is still a problem needing fixing.
>
> Argo's availability needs advertising to the toolstack like all other
> features, and the toolstack needs to be able to choose the Argo settings
> on a per-VM basis. Like everything else about VMs, the Argo-ness must
> be invariant for the lifetime of the domain.
> This means changes to sysctls/domctls, which IMO are prerequisites for
> Argo to move from Tech Preview to Supported. In a world where such
> controls existed, the argo cmdline option would really only be for
> someone trying to disable Argo globally.
>
The above is why I'd prefer not to apply this patch: at the moment, the
population of Argo developers and system integrators do not create or use
bootloader configuration files with the single "argo" keyword on the Xen
command line. (They use "argo=1" or similar instead.)
Once a change such as this is merged, then there is a new behaviour that is
made available, and a new expectation created not to change the behaviour
of the standalone command line option (ie. "argo").
I'd like to retain using the standalone argo keyword for when the only boot
option that is necessary is just a simple on or off. At the moment, that's
not the case: the suboption ("mac-permissive=1") is valid to either include
or omit, and there is work to do in order to enable retiring it - and
hopefully it will enable behaviour similar to the wider connectivity of
that option by default, which will not be the case for a system with "argo"
on the command line if just this current patch is applied.
>
> This particular patch comes simply from me trying to experiment with
> Argo to sort out the XTF test, and deciding that the behaviour was
> objectionable enough that I'd improve it.
>
I agree with the end goal; but don't think this is the right next step to
get there, and I don't think the existing situation is sufficiently
objectionable to make this change this way.
Christopher
>
> ~Andrew
>
Re: [PATCH] xen/argo: Command line handling improvements
On 21/05/2025 2:01 am, Christopher Clark wrote: > On Tue, May 20, 2025 at 3:10 PM Andrew Cooper > wrote: > > Treat "argo" on the command line as a positive boolean, rather > than requiring > the user to pass "argo=1/on/enable/true". > > Move both opt_argo* variables into __ro_after_init. They're set > during > command line parsing and never modified thereafter. > > > Instead of binding to static values set at boot, would > boolean_runtime_param be acceptable? No, for several reasons. Sure, you could dynamically turn it on, but existing domains can't use it because argo_init() wasn't called on them. Now consider what happens when dynamically turning it off behind the back of a domain which was using it. All the existing runtime controls are for properties that are not visible to guests. Adding opt_argo to this list would create a lot of carnage. (Like almost everything else in Xen), Argo is entirely broken with respect to enumeration and controls. And while this isn't your fault for having copied the status quo, it is still a problem needing fixing. Argo's availability needs advertising to the toolstack like all other features, and the toolstack needs to be able to choose the Argo settings on a per-VM basis. Like everything else about VMs, the Argo-ness must be invariant for the lifetime of the domain. This means changes to sysctls/domctls, which IMO are prerequisites for Argo to move from Tech Preview to Supported. In a world where such controls existed, the argo cmdline option would really only be for someone trying to disable Argo globally. This particular patch comes simply from me trying to experiment with Argo to sort out the XTF test, and deciding that the behaviour was objectionable enough that I'd improve it. ~Andrew
Re: [PATCH] xen/argo: Command line handling improvements
On 20.05.2025 20:45, Daniel P. Smith wrote:
> On 5/20/25 10:10, Andrew Cooper wrote:
>> Treat "argo" on the command line as a positive boolean, rather than requiring
>> the user to pass "argo=1/on/enable/true".
>>
>> Move both opt_argo* variables into __ro_after_init. They're set during
>> command line parsing and never modified thereafter.
>>
>> Signed-off-by: Andrew Cooper
>> ---
>> CC: Christopher Clark
>> CC: Daniel P. Smith
>> CC: Denis Mukhin
>>
>> Found while
>> ---
>> xen/common/argo.c | 9 ++---
>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/common/argo.c b/xen/common/argo.c
>> index cbe8911a4364..027b37b18a6c 100644
>> --- a/xen/common/argo.c
>> +++ b/xen/common/argo.c
>> @@ -79,8 +79,8 @@ DEFINE_XEN_GUEST_HANDLE(xen_argo_unregister_ring_t);
>> DEFINE_COMPAT_HANDLE(compat_argo_iov_t);
>> #endif
>>
>> -static bool __read_mostly opt_argo;
>> -static bool __read_mostly opt_argo_mac_permissive;
>> +static bool __ro_after_init opt_argo;
>> +static bool __ro_after_init opt_argo_mac_permissive;
>>
>> static int __init cf_check parse_argo(const char *s)
>> {
>> @@ -92,7 +92,10 @@ static int __init cf_check parse_argo(const char *s)
>> if ( !ss )
>> ss = strchr(s, '\0');
>>
>> -if ( (val = parse_bool(s, ss)) >= 0 )
>> +/* Intepret "argo" as a positive boolean. */
>> +if ( *s == '\0' )
>> +opt_argo = true;
>> +else if ( (val = parse_bool(s, ss)) >= 0 )
>> opt_argo = val;
>> else if ( (val = parse_boolean("mac-permissive", s, ss)) >= 0 )
>> opt_argo_mac_permissive = val;
>>
>> base-commit: 293abb9e0c5e8df96cc5dfe457c62dfcb7542b19
>
> While it is logical, this does technically change the behavior of the
> command line flag. Should there be an update to xen-command-line.pandoc
> to clarify that the list is optional?
I'd view it the other way around: If you look at the neighboring doc
entries, we uniformly say
> `= `
when the options can appear all by themselves. This would therefore be
the expected behavior for "argo" alone, too.
Jan
Re: [PATCH] xen/argo: Command line handling improvements
On Tue, May 20, 2025 at 3:10 PM Andrew Cooper
wrote:
> Treat "argo" on the command line as a positive boolean, rather than
> requiring
> the user to pass "argo=1/on/enable/true".
>
> Move both opt_argo* variables into __ro_after_init. They're set during
> command line parsing and never modified thereafter.
>
Instead of binding to static values set at boot, would
boolean_runtime_param be acceptable? This allows the system administrator
to adjust the Argo settings if the hypervisor has been compiled with it
enabled and booted with the default settings, and avoid having to reboot
and modify the bootloader configuration.
I agree with Daniel's request for a doc update where affected.
thanks,
Christopher
>
> Signed-off-by: Andrew Cooper
> ---
> CC: Christopher Clark
> CC: Daniel P. Smith
> CC: Denis Mukhin
>
> Found while
> ---
> xen/common/argo.c | 9 ++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/xen/common/argo.c b/xen/common/argo.c
> index cbe8911a4364..027b37b18a6c 100644
> --- a/xen/common/argo.c
> +++ b/xen/common/argo.c
> @@ -79,8 +79,8 @@ DEFINE_XEN_GUEST_HANDLE(xen_argo_unregister_ring_t);
> DEFINE_COMPAT_HANDLE(compat_argo_iov_t);
> #endif
>
> -static bool __read_mostly opt_argo;
> -static bool __read_mostly opt_argo_mac_permissive;
> +static bool __ro_after_init opt_argo;
> +static bool __ro_after_init opt_argo_mac_permissive;
>
> static int __init cf_check parse_argo(const char *s)
> {
> @@ -92,7 +92,10 @@ static int __init cf_check parse_argo(const char *s)
> if ( !ss )
> ss = strchr(s, '\0');
>
> -if ( (val = parse_bool(s, ss)) >= 0 )
> +/* Intepret "argo" as a positive boolean. */
> +if ( *s == '\0' )
> +opt_argo = true;
> +else if ( (val = parse_bool(s, ss)) >= 0 )
> opt_argo = val;
> else if ( (val = parse_boolean("mac-permissive", s, ss)) >= 0 )
> opt_argo_mac_permissive = val;
>
> base-commit: 293abb9e0c5e8df96cc5dfe457c62dfcb7542b19
> --
> 2.39.5
>
>
Re: [PATCH] xen/argo: Command line handling improvements
On Tue, May 20, 2025 at 03:10:27PM +0100, Andrew Cooper wrote:
> Treat "argo" on the command line as a positive boolean, rather than requiring
> the user to pass "argo=1/on/enable/true".
>
> Move both opt_argo* variables into __ro_after_init. They're set during
> command line parsing and never modified thereafter.
>
> Signed-off-by: Andrew Cooper
Reviewed-by: Denis Mukhin
> ---
> CC: Christopher Clark
> CC: Daniel P. Smith
> CC: Denis Mukhin
>
> Found while
> ---
> xen/common/argo.c | 9 ++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/xen/common/argo.c b/xen/common/argo.c
> index cbe8911a4364..027b37b18a6c 100644
> --- a/xen/common/argo.c
> +++ b/xen/common/argo.c
> @@ -79,8 +79,8 @@ DEFINE_XEN_GUEST_HANDLE(xen_argo_unregister_ring_t);
> DEFINE_COMPAT_HANDLE(compat_argo_iov_t);
> #endif
>
> -static bool __read_mostly opt_argo;
> -static bool __read_mostly opt_argo_mac_permissive;
> +static bool __ro_after_init opt_argo;
> +static bool __ro_after_init opt_argo_mac_permissive;
>
> static int __init cf_check parse_argo(const char *s)
> {
> @@ -92,7 +92,10 @@ static int __init cf_check parse_argo(const char *s)
> if ( !ss )
> ss = strchr(s, '\0');
>
> -if ( (val = parse_bool(s, ss)) >= 0 )
> +/* Intepret "argo" as a positive boolean. */
> +if ( *s == '\0' )
> +opt_argo = true;
> +else if ( (val = parse_bool(s, ss)) >= 0 )
> opt_argo = val;
> else if ( (val = parse_boolean("mac-permissive", s, ss)) >= 0 )
> opt_argo_mac_permissive = val;
>
> base-commit: 293abb9e0c5e8df96cc5dfe457c62dfcb7542b19
> --
> 2.39.5
>
Re: [PATCH] xen/argo: Command line handling improvements
On 5/20/25 10:10, Andrew Cooper wrote:
Treat "argo" on the command line as a positive boolean, rather than requiring
the user to pass "argo=1/on/enable/true".
Move both opt_argo* variables into __ro_after_init. They're set during
command line parsing and never modified thereafter.
Signed-off-by: Andrew Cooper
---
CC: Christopher Clark
CC: Daniel P. Smith
CC: Denis Mukhin
Found while
---
xen/common/argo.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/xen/common/argo.c b/xen/common/argo.c
index cbe8911a4364..027b37b18a6c 100644
--- a/xen/common/argo.c
+++ b/xen/common/argo.c
@@ -79,8 +79,8 @@ DEFINE_XEN_GUEST_HANDLE(xen_argo_unregister_ring_t);
DEFINE_COMPAT_HANDLE(compat_argo_iov_t);
#endif
-static bool __read_mostly opt_argo;
-static bool __read_mostly opt_argo_mac_permissive;
+static bool __ro_after_init opt_argo;
+static bool __ro_after_init opt_argo_mac_permissive;
static int __init cf_check parse_argo(const char *s)
{
@@ -92,7 +92,10 @@ static int __init cf_check parse_argo(const char *s)
if ( !ss )
ss = strchr(s, '\0');
-if ( (val = parse_bool(s, ss)) >= 0 )
+/* Intepret "argo" as a positive boolean. */
+if ( *s == '\0' )
+opt_argo = true;
+else if ( (val = parse_bool(s, ss)) >= 0 )
opt_argo = val;
else if ( (val = parse_boolean("mac-permissive", s, ss)) >= 0 )
opt_argo_mac_permissive = val;
base-commit: 293abb9e0c5e8df96cc5dfe457c62dfcb7542b19
While it is logical, this does technically change the behavior of the
command line flag. Should there be an update to xen-command-line.pandoc
to clarify that the list is optional?
Other than the doc concern,
Reviewed-by: Daniel P. Smith
