Re: [PATCH v5] x86: make Viridian support optional

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

On 17.10.25 10:38, Roger Pau Monné wrote:
> On Fri, Oct 17, 2025 at 12:40:33AM +0300, Grygorii Strashko wrote:
> > On 15.10.25 11:00, Roger Pau Monné wrote:
> > > On Tue, Oct 14, 2025 at 06:48:23PM +0300, Grygorii Strashko wrote:
> > > > On 14.10.25 17:38, Roger Pau Monné wrote:
> > > > > On Tue, Oct 14, 2025 at 04:24:53PM +0300, Grygorii Strashko wrote:
> > > > > > On 13.10.25 15:17, Roger Pau Monné wrote:
> > > > > > > On Tue, Sep 30, 2025 at 12:52:16PM +0000, Grygorii Strashko wrote:
> > > > > > > > From: Sergiy Kibrik <Sergiy_Kibrik@xxxxxxxx>
> > > > > > > > +
> > > > > > > > +         If unsure, say Y.
> > > > > > > > +
> > > > > > > >      config MEM_PAGING
> > > > > > > >         bool "Xen memory paging support (UNSUPPORTED)" if UNSUPPORTED
> > > > > > > >         depends on VM_EVENT
> > > > > > > > diff --git a/xen/arch/x86/hvm/Makefile b/xen/arch/x86/hvm/Makefile
> > > > > > > > index 6ec2c8f2db56..736eb3f966e9 100644
> > > > > > > > --- a/xen/arch/x86/hvm/Makefile
> > > > > > > > +++ b/xen/arch/x86/hvm/Makefile
> > > > > > > > @@ -1,6 +1,6 @@
> > > > > > > >      obj-$(CONFIG_AMD_SVM) += svm/
> > > > > > > >      obj-$(CONFIG_INTEL_VMX) += vmx/
> > > > > > > > -obj-y += viridian/
> > > > > > > > +obj-$(CONFIG_VIRIDIAN) += viridian/
> > > > > > > >      obj-y += asid.o
> > > > > > > >      obj-y += dm.o
> > > > > > > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > > > > > > > index 23bd7f078a1d..95a80369b9b8 100644
> > > > > > > > --- a/xen/arch/x86/hvm/hvm.c
> > > > > > > > +++ b/xen/arch/x86/hvm/hvm.c
> > > > > > > > @@ -701,9 +701,12 @@ int hvm_domain_initialise(struct domain *d,
> > > > > > > >          if ( hvm_tsc_scaling_supported )
> > > > > > > >              d->arch.hvm.tsc_scaling_ratio = hvm_default_tsc_scaling_ratio;
> > > > > > > > -    rc = viridian_domain_init(d);
> > > > > > > > -    if ( rc )
> > > > > > > > -        goto fail2;
> > > > > > > > +    if ( is_viridian_domain(d) )
> > > > > > > > +    {
> > > > > > > > +        rc = viridian_domain_init(d);
> > > > > > > > +        if ( rc )
> > > > > > > > +            goto fail2;
> > > > > > > > +    }
> > > > > > > Are you sure this works as expected?
> > > > > > >
> > > > > > > The viridian_feature_mask() check is implemented using an HVM param,
> > > > > > > and hence can only be possibly set after the domain object is created.
> > > > > > > AFAICT is_viridian_domain(d) will unconditionally return false when
> > > > > > > called from domain_create() context, because the HVM params cannot
> > > > > > > possibly be set ahead of the domain being created.
> > > > > > You are right. Thanks for the this catch.
> > > > > >
> > > > > > Taking above into account above, it seems Jan's proposal to convert below
> > > > > > viridian APIs into wrappers for VIRIDIAN=n case is right way to move forward:
> > > > > >
> > > > > > int viridian_vcpu_init(struct vcpu *v);
> > > > > > int viridian_domain_init(struct domain *d);
> > > > > > void viridian_vcpu_deinit(struct vcpu *v);
> > > > > > void viridian_domain_deinit(struct domain *d);
> > > > > >
> > > > > > Right?
> > > > > Possibly. If you don't want to introduce a XEN_DOMCTL_createdomain
> > > > > flag you need to exclusively use the Kconfig option to decide whether
> > > > > the Viridian related structs must be allocated.  IOW: you could also
> > > > > solve it by using IS_ENABLED(CONFIG_VIRIDIAN) instead of
> > > > > is_viridian_domain() for most of the calls here.
> > > > >
> > > > > The wrapper option might be better IMO, rather than adding
> > > > > IS_ENABLED(CONFIG_VIRIDIAN) around.
> > > > I'll do wrappers - less if(s) in common HVM code.
> > > >
> > > > > > [1] https://patchwork.kernel.org/comment/26595213/
> > > > > >
> > > > > > > If you want to do anything like this you will possibly need to
> > > > > > > introduce a new flag to XEN_DOMCTL_createdomain to signal whether the
> > > > > > > domain has Viridian extensions are enabled or not, so that it's know
> > > > > > > in the context where domain_create() gets called.
> > > > > > In my opinion, it might be good not to go so far within this submission.
> > > > > > - It's not intended  to change existing behavior of neither Xen nor toolstack
> > > > > >      for VIRIDIAN=y (default)
> > [1]
> >
> > > > > > - just optout Viridian support when not needed.
> > > > > OK, that's fine.
> > > > >
> > > > > On further request though: if Viridian is build-time disabled in
> > > > > Kconfig, setting or fetching HVM_PARAM_VIRIDIAN should return -ENODEV
> > > > > or similar error.  I don't think this is done as part of this patch.
> > > Another bit I've noticed, you will need to adjust write_hvm_params()
> > > so it can tolerate xc_hvm_param_get() returning an error when
> > > HVM_PARAM_VIRIDIAN is not implemented by the hypervisor.
> > >
> > > Implementing the Viridian features using an HVM parameter was a bad
> > > approach probably.
> > I've just realized how toolstack need to be modified and all consequences...
> > Have to try to push back a little bit:
> >
> > VIRIDIAN=n: Now HVM_PARAM_VIRIDIAN will be R/W with functionality NOP.
> >
> > I'd prefer avoid modifying toolstack if possible.
> >
> > How about sanitizing HVM_PARAM_VIRIDIAN to be RAZ/WI for VIRIDIAN=n?
> I've been thinking more into it, and we must still allow writes/reads
> to it, otherwise migration would fail when restoring a stream that
> contains a Viridian record, even if Viridian is not enabled for the
> domain.  Right now the HVM param is unconditionally part of the
> migration stream, even when Viridian is not enabled.
>
> I think Xen could return an error when VIRIDIAN = n and a non-zero
> value is passed to HVM_PARAM_VIRIDIAN?
Right...
> That shouldn't require any changes to the toolstack, as when Viridian
> is not enabled the toolstack will just set HVM_PARAM_VIRIDIAN = 0.  It
> would however still fail if toolstack attempts HVM_PARAM_VIRIDIAN != 0
> and Viridian has been build time disabled.
I was thinking about the same, in general:
Get HVM_PARAM_VIRIDIAN (VIRIDIAN = n):
 - no fail, expected to return 0

   [toolstack] write_hvm_params()
    ...
    for ( i = 0; i < ARRAY_SIZE(params); i++ )
    {
        ...
        rc = xc_hvm_param_get(xch, ctx->domid, index, &value);
        if ( rc )
        {
            PERROR("Failed to get HVMPARAM at index %u", index);
            return rc;
        }

        if ( value != 0 )
        {
            entries[hdr.count].index = index;
            entries[hdr.count].value = value;
            hdr.count++;
        }

        [gs] will skip HVM_PARAM_VIRIDIAN entry as it's 0 for VIRIDIAN = n

Set HVM_PARAM_VIRIDIAN (VIRIDIAN = n):
 - fail if value != 0 with -ENODEV

   [toolstack]  hvm_set_viridian_features() will fail and reject domain if
   Viridian is configured for domain, but VIRIDIAN = n

   [toolstack] xg_sr_restore_x86_hvm.c : handle_hvm_params() will fail on 
attempt to load
   domain with Viridian enabled (value != 0) under Xen with VIRIDIAN = n.
   The value == 0 will pass

Will send new version.


--
Best regards,
-grygorii