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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

> [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)
> - 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.

> > 
> > Given that HyperV is available on arm64 also it should be a global
> > flag, as opposed to a per-arch one in xen_arch_domainconfig IMO.
> > >       rc = alternative_call(hvm_funcs.domain_initialise, d);
> > >       if ( rc != 0 )
> > > @@ -739,7 +742,8 @@ void hvm_domain_relinquish_resources(struct domain *d)
> > >       if ( hvm_funcs.nhvm_domain_relinquish_resources )
> > >           alternative_vcall(hvm_funcs.nhvm_domain_relinquish_resources, 
> > > d);
> > > -    viridian_domain_deinit(d);
> > > +    if ( is_viridian_domain(d) )
> > > +        viridian_domain_deinit(d);
> > >       ioreq_server_destroy_all(d);
> > > @@ -1643,9 +1647,12 @@ int hvm_vcpu_initialise(struct vcpu *v)
> > >            && (rc = nestedhvm_vcpu_initialise(v)) < 0 ) /* teardown: 
> > > nestedhvm_vcpu_destroy */
> > >           goto fail5;
> > > -    rc = viridian_vcpu_init(v);
> > > -    if ( rc )
> > > -        goto fail6;
> > > +    if ( is_viridian_domain(d) )
> > > +    {
> > > +        rc = viridian_vcpu_init(v);
> > > +        if ( rc )
> > > +            goto fail6;
> > > +    }
> > >       rc = ioreq_server_add_vcpu_all(d, v);
> > >       if ( rc != 0 )
> > > @@ -1675,13 +1682,15 @@ int hvm_vcpu_initialise(struct vcpu *v)
> > >    fail2:
> > >       hvm_vcpu_cacheattr_destroy(v);
> > >    fail1:
> > > -    viridian_vcpu_deinit(v);
> > > +    if ( is_viridian_domain(d) )
> > > +        viridian_vcpu_deinit(v);
> > >       return rc;
> > >   }
> > >   void hvm_vcpu_destroy(struct vcpu *v)
> > >   {
> > > -    viridian_vcpu_deinit(v);
> > > +    if ( is_viridian_domain(v->domain) )
> > > +        viridian_vcpu_deinit(v);
> > >       ioreq_server_remove_vcpu_all(v->domain, v);
> > > diff --git a/xen/arch/x86/hvm/viridian/viridian.c 
> > > b/xen/arch/x86/hvm/viridian/viridian.c
> > > index c0be24bd2210..1212cc418728 100644
> > > --- a/xen/arch/x86/hvm/viridian/viridian.c
> > > +++ b/xen/arch/x86/hvm/viridian/viridian.c
> > > @@ -1116,14 +1116,14 @@ static int cf_check viridian_save_domain_ctxt(
> > >   {
> > >       const struct domain *d = v->domain;
> > >       const struct viridian_domain *vd = d->arch.hvm.viridian;
> > > -    struct hvm_viridian_domain_context ctxt = {
> > > -        .hypercall_gpa = vd->hypercall_gpa.raw,
> > > -        .guest_os_id = vd->guest_os_id.raw,
> > > -    };
> > > +    struct hvm_viridian_domain_context ctxt = {};
> > >       if ( !is_viridian_domain(d) )
> > >           return 0;
> > > +    ctxt.hypercall_gpa = vd->hypercall_gpa.raw;
> > > +    ctxt.guest_os_id = vd->guest_os_id.raw,
> > > +
> > >       viridian_time_save_domain_ctxt(d, &ctxt);
> > >       viridian_synic_save_domain_ctxt(d, &ctxt);
> > > @@ -1136,6 +1136,9 @@ static int cf_check viridian_load_domain_ctxt(
> > >       struct viridian_domain *vd = d->arch.hvm.viridian;
> > >       struct hvm_viridian_domain_context ctxt;
> > > +    if ( !is_viridian_domain(d) )
> > > +        return -EILSEQ;
> > > +
> > >       if ( hvm_load_entry_zeroextend(VIRIDIAN_DOMAIN, h, &ctxt) != 0 )
> > >           return -EINVAL;
> > > @@ -1172,6 +1175,9 @@ static int cf_check viridian_load_vcpu_ctxt(
> > >       struct vcpu *v;
> > >       struct hvm_viridian_vcpu_context ctxt;
> > > +    if ( !is_viridian_domain(d) )
> > > +        return -EILSEQ;
> > 
> > Nit: we usually use EILSEQ for unreachable conditions, but here it's a
> > toolstack controlled input.  Maybe we could instead use ENODEV here?
> > 
> > As it's not really an illegal restore stream, but the selected guest
> > configuration doesn't match what's then loaded.
> 
> I'm a "working bee" here and can change it once again her to -ENODEV here.
> But It will be really cool if it will be agreed on Maintainers level somehow.
> 
> EILSEQ was used as per [2]

Didn't know it was explicitly requested, then leave it like that and
ignore this comment.

Thanks, Roger.