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

[Jan Beulich <jbeulich@xxxxxxxx>]

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().

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 */