Re: [RFC PATCH v3 1/7] xen/arm: add generic SCI subsystem

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.03.2025 14:11, Oleksii Moisieiev wrote:
> Hi Jan,
> 
> On 24/03/2025 12:05, Jan Beulich wrote:
>> On 24.03.2025 10:00, Oleksii Moisieiev wrote:
>>> Hi Jan,
>>>
>>> Let me answer one of your comment. Please see below:
>>>
>>> On 11/03/2025 13:43, Jan Beulich wrote:
>>>> On 11.03.2025 12:16, Grygorii Strashko wrote:
>>>>> --- a/MAINTAINERS
>>>>> +++ b/MAINTAINERS
>>>>> @@ -526,6 +526,12 @@ S:   Supported
>>>>>    F:     xen/arch/arm/include/asm/tee/
>>> [snip]
>>>>> --- a/xen/include/public/arch-arm.h
>>>>> +++ b/xen/include/public/arch-arm.h
>>>>> @@ -327,6 +327,8 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
>>>>>    #define XEN_DOMCTL_CONFIG_TEE_OPTEE     1
>>>>>    #define XEN_DOMCTL_CONFIG_TEE_FFA       2
>>>>>    
>>>>> +#define XEN_DOMCTL_CONFIG_ARM_SCI_NONE      0
>>>>> +
>>>>>    struct xen_arch_domainconfig {
>>>>>        /* IN/OUT */
>>>>>        uint8_t gic_version;
>>>>> @@ -350,6 +352,8 @@ struct xen_arch_domainconfig {
>>>>>         *
>>>>>         */
>>>>>        uint32_t clock_frequency;
>>>>> +    /* IN */
>>>>> +    uint8_t arm_sci_type;
>>>>>    };
>>>> You're not re-using a pre-existing padding field, so I don't see how you
>>>> can get away without bumping XEN_DOMCTL_INTERFACE_VERSION.
>>> We are reusing an existing padding field in xen_domctl, which is defined
>>> as pad[128].
>>>
>>> The xen_arch_domainconfig structure is a part of the following domctl
>>> structures:
>>>
>>> - xen_domctl_createdomain
>>>
>>> - xen_domctl_getdomaininfo
>>>
>>> These structures are included in the union within xen_domctl, which
>>> defines pad[128] for padding.
>> Except that "an existing padding field" means a field which isn't just
>> there in space, but is also checked to be zero right now. That is, new
>> code setting the field to non-zero needs to properly get an error
>> indicator back from an older hypervisor.
> 
> I completely agree with you that XEN_DOMCTL_INTERFACE_VERSION should be 
> incremented
> 
> before the changes are merged. I just wanted to point out that we do not 
> exceed the size of the xen_domctl padding. If you are okay with the 
> fields we have added, then XEN_DOMCTL_INTERFACE_VERSION will be updated 
> in the next patch series.
> 
>> but is also checked to be zero right now.
> 
> Just out of curiosity, I have one more question: I couldn't find the 
> check you've mentioned. Could you point me to where Xen or
> the toolstack checks the domctl structure for 0? I would greatly 
> appreciate it if you could show me.

There's no such check here (as far as I'm aware), hence why the interface
version bump is necessary. There are _other_ padding fields which do have
appropriate checks, and hence could be assigned purpose without bumping
the interface version.

Jan