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

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

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)
> > - 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.
Sure. Just have to ask for clarification what to return:
-EOPNOTSUPP (my choise) vs -EINVAL.

?

> > > 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.
--
Best regards,
-grygorii