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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.10.2025 10: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)
>>>> - 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.

ENODEV was suggested here; it's not clear to me why ...

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

... other values were suggested here.

> Let me add Jan also to the To: field so we get consensus in one round.
> 
> I won't use EINVAL, because that's returned for deprecated parameters
> also, and when the passed Viridian feature mask is invalid.
> 
> EOPNOTSUPP is also returned for non-implemented hypercalls, so I'm not
> sure whether it could cause confusion here, as the hypercall is
> implemented, it's just the param that's not supported if
> build-disabled.  Maybe ENODEV or ENXIO?

I'd be okay with either of these two, with a slight preference to ENODEV.

Jan