Re: [PATCH v4 08/12] xen/physinfo: encode Arm SVE vector length in arch_capabilities

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.03.2023 12:41, Luca Fancellu wrote:
> 
> 
>> On 28 Mar 2023, at 11:14, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 27.03.2023 12:59, Luca Fancellu wrote:
>>> --- a/xen/arch/arm/arm64/sve.c
>>> +++ b/xen/arch/arm/arm64/sve.c
>>> @@ -124,3 +124,15 @@ int __init sve_parse_dom0_param(const char *str_begin, 
>>> const char *str_end)
>>> {
>>>     return parse_integer("sve", str_begin, str_end, (int*)&opt_dom0_sve);
>>> }
>>> +
>>> +void sve_arch_cap_physinfo(uint32_t *arch_capabilities)
>>> +{
>>> +    if ( cpu_has_sve )
>>> +    {
>>> +        /* Vector length is divided by 128 to save some space */
>>> +        uint32_t sve_vl = MASK_INSR(sve_encode_vl(get_sys_vl_len()),
>>> +                                    XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK);
>>> +
>>> +        *arch_capabilities |= sve_vl;
>>> +    }
>>> +}
>>
>> I'm again wondering why a separate function is needed, when everything
>> that's needed is ...
>>
>>> --- a/xen/arch/arm/sysctl.c
>>> +++ b/xen/arch/arm/sysctl.c
>>> @@ -11,11 +11,14 @@
>>> #include <xen/lib.h>
>>> #include <xen/errno.h>
>>> #include <xen/hypercall.h>
>>> +#include <asm/arm64/sve.h>
>>
>> ... becoming available here for use ...
>>
>>> #include <public/sysctl.h>
>>>
>>> void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
>>> {
>>>     pi->capabilities |= XEN_SYSCTL_PHYSCAP_hvm | XEN_SYSCTL_PHYSCAP_hap;
>>> +
>>> +    sve_arch_cap_physinfo(&pi->arch_capabilities);
>>
>> ... here. That would be even more so if, like suggested before,
>> get_sys_vl_len() returned 0 when !cpu_has_sve.
> 
> I’ve had a look on this, I can do everything In arch_do_physinfo if in 
> xen/include/public/sysctl.h
> the XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK is protected by __aarch64__ or __arm__ .
> 
> Do you agree on that?

I don't see the connection, but guarding the #define in the public header
looks not only okay, but even desirable to me.

Jan