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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan