Re: [PATCH v6 07/12] xen: enable Dom0 to use SVE feature

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 24 Apr 2023, at 16:25, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 24.04.2023 17:18, Luca Fancellu wrote:
>>> On 24 Apr 2023, at 16:06, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>> On 24.04.2023 16:57, Luca Fancellu wrote:
>>>>> On 24 Apr 2023, at 15:05, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>>> On 24.04.2023 16:00, Luca Fancellu wrote:
>>>>>>> On 24 Apr 2023, at 12:34, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>>>>> On 24.04.2023 08:02, Luca Fancellu wrote:
>>>>>>>> @@ -30,9 +37,11 @@ int sve_context_init(struct vcpu *v);
>>>>>>>> void sve_context_free(struct vcpu *v);
>>>>>>>> void sve_save_state(struct vcpu *v);
>>>>>>>> void sve_restore_state(struct vcpu *v);
>>>>>>>> +bool sve_domctl_vl_param(int val, unsigned int *out);
>>>>>>>> 
>>>>>>>> #else /* !CONFIG_ARM64_SVE */
>>>>>>>> 
>>>>>>>> +#define opt_dom0_sve     (0)
>>>>>>>> #define is_sve_domain(d) (0)
>>>>>>>> 
>>>>>>>> static inline register_t compute_max_zcr(void)
>>>>>>>> @@ -59,6 +68,11 @@ static inline void sve_context_free(struct vcpu *v) 
>>>>>>>> {}
>>>>>>>> static inline void sve_save_state(struct vcpu *v) {}
>>>>>>>> static inline void sve_restore_state(struct vcpu *v) {}
>>>>>>>> 
>>>>>>>> +static inline bool sve_domctl_vl_param(int val, unsigned int *out)
>>>>>>>> +{
>>>>>>>> +    return false;
>>>>>>>> +}
>>>>>>> 
>>>>>>> Once again I don't see the need for this stub: opt_dom0_sve is #define-d
>>>>>>> to plain zero when !ARM64_SVE, so the only call site merely requires a
>>>>>>> visible declaration, and DCE will take care of eliminating the actual 
>>>>>>> call.
>>>>>> 
>>>>>> I’ve tried to do that, I’ve put the declaration outside the ifdef so 
>>>>>> that it was always included
>>>>>> and I removed the stub, but I got errors on compilation because of 
>>>>>> undefined function.
>>>>>> For that reason  I left that change out.
>>>>> 
>>>>> Interesting. I don't see where the reference would be coming from.
>>>> 
>>>> Could it be because the declaration is visible, outside the ifdef, but the 
>>>> definition is not compiled in? 
>>> 
>>> Well, yes, likely. But the question isn't that but "Why did the reference
>>> not get removed, when it's inside an if(0) block?"
>> 
>> Oh ok, I don’t know, here what I get if for example I build arm32:
>> 
>> arm-linux-gnueabihf-ld -EL -T arch/arm/xen.lds -N prelink.o \
>> ./common/symbols-dummy.o -o ./.xen-syms.0
>> arm-linux-gnueabihf-ld: prelink.o: in function `create_domUs':
>> (.init.text+0x13464): undefined reference to `sve_domctl_vl_param'
> 
> In particular with seeing this: What you copied here is a build with the
> series applied only up to this patch? I ask because the patch here adds a
> call only out of create_dom0().

No I’ve do the changes on top of the serie, I’ve tried it now, only to this 
patch and it builds correctly,
It was my mistake to don’t read carefully the error output.

Anyway I guess this change is not applicable because we don’t have a symbol 
that is plain 0 for domUs
to be placed inside create_domUs.

> 
> Jan
> 
>> arm-linux-gnueabihf-ld: (.init.text+0x136b4): undefined reference to 
>> `sve_domctl_vl_param'
>> arm-linux-gnueabihf-ld: ./.xen-syms.0: hidden symbol `sve_domctl_vl_param' 
>> isn't defined
>> arm-linux-gnueabihf-ld: final link failed: bad value
>> make[3]: *** [/data_sdc/lucfan01/kirkstone_xen/xen/xen/arch/arm/Makefile:95: 
>> xen-syms] Error 1
>> make[2]: *** [/data_sdc/lucfan01/kirkstone_xen/xen/xen/./build.mk:90: xen] 
>> Error 2
>> make[1]: *** [/data_sdc/lucfan01/kirkstone_xen/xen/xen/Makefile:590: xen] 
>> Error 2
>> make[1]: Leaving directory 
>> '/data_sdc/lucfan01/kirkstone_xen/build/xen-qemu-arm32'
>> make: *** [Makefile:181: __sub-make] Error 2
>> make: Leaving directory '/data_sdc/lucfan01/kirkstone_xen/xen/xen’
>> 
>> These are the modification I’ve done:
>> 
>> diff --git a/xen/arch/arm/include/asm/arm64/sve.h 
>> b/xen/arch/arm/include/asm/arm64/sve.h
>> index 71bddb41f19c..330c47ea8864 100644
>> --- a/xen/arch/arm/include/asm/arm64/sve.h
>> +++ b/xen/arch/arm/include/asm/arm64/sve.h
>> @@ -24,6 +24,8 @@ static inline unsigned int sve_encode_vl(unsigned int 
>> sve_vl_bits)
>>     return sve_vl_bits / SVE_VL_MULTIPLE_VAL;
>> }
>> 
>> +bool sve_domctl_vl_param(int val, unsigned int *out);
>> +
>> #ifdef CONFIG_ARM64_SVE
>> 
>> extern int opt_dom0_sve;
>> @@ -37,7 +39,6 @@ int sve_context_init(struct vcpu *v);
>> void sve_context_free(struct vcpu *v);
>> void sve_save_state(struct vcpu *v);
>> void sve_restore_state(struct vcpu *v);
>> -bool sve_domctl_vl_param(int val, unsigned int *out);
>> 
>> #else /* !CONFIG_ARM64_SVE */
>> 
>> @@ -68,11 +69,6 @@ static inline void sve_context_free(struct vcpu *v) {}
>> static inline void sve_save_state(struct vcpu *v) {}
>> static inline void sve_restore_state(struct vcpu *v) {}
>> 
>> -static inline bool sve_domctl_vl_param(int val, unsigned int *out)
>> -{
>> -    return false;
>> -}
>> -
>> #endif /* CONFIG_ARM64_SVE */
>> 
>> #endif /* _ARM_ARM64_SVE_H */