Re: [PATCH v6 01/12] xen/arm: enable SVE extension for Xen

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 22 May 2023, at 10:30, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi,
> 
> On 22/05/2023 09:43, Luca Fancellu wrote:
>>> On 22 May 2023, at 08:50, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>> 
>>> On 19.05.2023 16:46, Julien Grall wrote:
>>>> On 19/05/2023 15:26, Luca Fancellu wrote:
>>>>>> On 18 May 2023, at 10:35, Julien Grall <julien@xxxxxxx> wrote:
>>>>>>> +/*
>>>>>>> + * Arm SVE feature code
>>>>>>> + *
>>>>>>> + * Copyright (C) 2022 ARM Ltd.
>>>>>>> + */
>>>>>>> +
>>>>>>> +#include <xen/types.h>
>>>>>>> +#include <asm/arm64/sve.h>
>>>>>>> +#include <asm/arm64/sysregs.h>
>>>>>>> +#include <asm/processor.h>
>>>>>>> +#include <asm/system.h>
>>>>>>> +
>>>>>>> +extern unsigned int sve_get_hw_vl(void);
>>>>>>> +
>>>>>>> +register_t compute_max_zcr(void)
>>>>>>> +{
>>>>>>> +    register_t cptr_bits = get_default_cptr_flags();
>>>>>>> +    register_t zcr = vl_to_zcr(SVE_VL_MAX_BITS);
>>>>>>> +    unsigned int hw_vl;
>>>>>>> +
>>>>>>> +    /* Remove trap for SVE resources */
>>>>>>> +    WRITE_SYSREG(cptr_bits & ~HCPTR_CP(8), CPTR_EL2);
>>>>>>> +    isb();
>>>>>>> +
>>>>>>> +    /*
>>>>>>> +     * Set the maximum SVE vector length, doing that we will know the 
>>>>>>> VL
>>>>>>> +     * supported by the platform, calling sve_get_hw_vl()
>>>>>>> +     */
>>>>>>> +    WRITE_SYSREG(zcr, ZCR_EL2);
>>>>>> 
>>>>>> From my reading of the Arm (D19-6331, ARM DDI 0487J.a), a direct write 
>>>>>> to a system register would need to be followed by an context 
>>>>>> synchronization event (e.g. isb()) before the software can rely on the 
>>>>>> value.
>>>>>> 
>>>>>> In this situation, AFAICT, the instruciton in sve_get_hw_vl() will use 
>>>>>> the content of ZCR_EL2. So don't we need an ISB() here?
>>>>> 
>>>>> From what I’ve read in the manual for ZCR_ELx:
>>>>> 
>>>>> An indirect read of ZCR_EL2.LEN appears to occur in program order 
>>>>> relative to a direct write of
>>>>> the same register, without the need for explicit synchronization
>>>>> 
>>>>> I’ve interpreted it as “there is no need to sync before write” and I’ve 
>>>>> looked into Linux and it does not
>>>>> Appear any synchronisation mechanism after a write to that register, but 
>>>>> if I am wrong I can for sure
>>>>> add an isb if you prefer.
>>>> 
>>>> Ah, I was reading the generic section about synchronization and didn't
>>>> realize there was a paragraph in the ZCR_EL2 section as well.
>>>> 
>>>> Reading the new section, I agree with your understanding. The isb() is
>>>> not necessary.
>>> 
>>> And RDVL counts as an "indirect read"? I'm pretty sure "normal" SVE insn
>>> use is falling in that category, but RDVL might also be viewed as more
>>> similar to MRS in this regard? While the construct CurrentVL is used in
>>> either case, I'm still not sure this goes without saying.
>> Hi Jan,
>> Looking into the Linux code, in function vec_probe_vqs(...) in 
>> arch/arm64/kernel/fpsimd.c,
>> ZCR_EL1 is written, without synchronisation, and afterwards RDVL is used.
> 
> You are making the assumption that the Linux code is correct. It is mostly 
> likely the case, but in general it is best to justify barriers based on the 
> Arm Arm because it is authoritative.
> 
> In this case, the Arm Arm is pretty clear on the difference between indirect 
> read and direct read (see D19-63333 ARM DDI 0487J.A). The latter only refers 
> to use of the instruction of MRS. RDVL is its own instruction and therefore 
> this is an indirect read.

Yes you are right

> 
> Cheers,
> 
> -- 
> Julien Grall