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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

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


> 
>>>> --- a/xen/common/kernel.c
>>>> +++ b/xen/common/kernel.c
>>>> @@ -324,11 +324,14 @@ int __init parse_signed_integer(const char *name, 
>>>> const char *s, const char *e,
>>>>    slen = e ? ({ ASSERT(e >= s); e - s; }) : strlen(s);
>>>>    nlen = strlen(name);
>>>> 
>>>> +    if ( !e )
>>>> +        e = s + slen;
>>>> +
>>>>    /* Check that this is the name we're looking for and a value was 
>>>> provided */
>>>> -    if ( (slen <= nlen) || strncmp(s, name, nlen) || (s[nlen] != '=') )
>>>> +    if ( slen <= nlen || strncmp(s, name, nlen) || s[nlen] != '=' )
>>>>        return -1;
>>>> 
>>>> -    pval = simple_strtoll(&s[nlen + 1], &str, 0);
>>>> +    pval = simple_strtoll(&s[nlen + 1], &str, 10);
>>>> 
>>>>    /* Number not recognised */
>>>>    if ( str != e )
>>>> 
>>>> 
>>>> Please note that I’ve also included your comment about the base, which I 
>>>> forgot to add, apologies for that.
>>>> 
>>>> slen <= nlen doesn’t seems redundant to me, I have that because I’m 
>>>> accessing s[nlen] and I would like
>>>> the string s to be at least > nlen
>>> 
>>> Right, but doesn't strncmp() guarantee that already?
>> 
>> I thought strncmp() guarantees s contains at least nlen chars, meaning from 
>> 0 to nlen-1, is my understanding wrong?
> 
> That's my understanding too. Translated to C this means "slen >= nlen",
> i.e. the "slen < nlen" case is covered. The "slen == nlen" case is then
> covered by "s[nlen] != '='", which - due to the earlier guarantee - is
> going to be in bounds. That's because even when e is non-NULL and points
> at non-nul, it still points into a valid nul-terminated string. (But yes,
> I see now that the "slen == nlen" case is a little hairy, so perhaps
> indeed best to keep the check as you have it.)
> 
> Jan