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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

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

I think ZCR_EL2 has the same behaviour.

Cheers,
Luca

> 
> Jan