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