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
