Re: [PATCH] xen/argo: Command line handling improvements

[Christopher Clark <christopher.w.clark@xxxxxxxxx>]

On Wed, May 21, 2025 at 9:43 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:

On 21/05/2025 2:01 am, Christopher Clark wrote:
> On Tue, May 20, 2025 at 3:10 PM Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx> 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