Re: [RFC PATCH 1/8] xen/arm: enable SVE extension for Xen

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 13 Jan 2023, at 08:53, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Luca,
> 
> On 12/01/2023 10:46, Luca Fancellu wrote:
>>> On 11 Jan 2023, at 17:16, Julien Grall <julien@xxxxxxx> wrote:
>>> 
>>> Hi Luca,
>>> 
>>> As this is an RFC, I will be mostly making general comments.
>> Hi Julien,
>> Thank you.
>>> 
>>> On 11/01/2023 14:38, Luca Fancellu wrote:
>>>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>>>> index 99577adb6c69..8ea3843ea8e8 100644
>>>> --- a/xen/arch/arm/domain.c
>>>> +++ b/xen/arch/arm/domain.c
>>>> @@ -181,6 +181,8 @@ static void ctxt_switch_to(struct vcpu *n)
>>>>      /* VGIC */
>>>>      gic_restore_state(n);
>>>>  +    WRITE_SYSREG(n->arch.cptr_el2, CPTR_EL2);
>>> 
>>> Shouldn't this need an isb() afterwards do ensure that any previously 
>>> trapped will be accessible?
>> Yes you are right, would it be ok for you if I move this before 
>> gic_restore_state because it inside
>> has an isb()? This to limit the isb() usage. I could put also a comment to 
>> don’t forget it.
> 
> I would rather prefer if we don't rely on gic_restore_state() to have an 
> isb() because this could change in the future (although unlikely).
> 
> Looking at the context switch code, I think we can move the call to restore 
> the floating register towards the end of the helper and use one of the 
> existing isb() for our purpose.

Sounds good to me

> 
> 
>>>> @@ -122,6 +137,7 @@ __initcall(update_serrors_cpu_caps);
>>>>    void init_traps(void)
>>>>  {
>>>> +    register_t cptr_bits = get_default_cptr_flags();
>>>>      /*
>>>>       * Setup Hyp vector base. Note they might get updated with the
>>>>       * branch predictor hardening.
>>>> @@ -135,17 +151,15 @@ void init_traps(void)
>>>>      /* Trap CP15 c15 used for implementation defined registers */
>>>>      WRITE_SYSREG(HSTR_T(15), HSTR_EL2);
>>>>  -    /* Trap all coprocessor registers (0-13) except cp10 and
>>>> -     * cp11 for VFP.
>>>> -     *
>>>> -     * /!\ All coprocessors except cp10 and cp11 cannot be used in Xen.
>>>> -     *
>>>> -     * On ARM64 the TCPx bits which we set here (0..9,12,13) are all
>>>> -     * RES1, i.e. they would trap whether we did this write or not.
>>>> +#ifdef CONFIG_ARM64_SVE
>>>> +    /*
>>>> +     * Don't trap SVE now, Xen might need to access ZCR reg in cpufeature 
>>>> code,
>>>> +     * trapping again or not will be handled on vcpu creation/scheduling 
>>>> later
>>>>       */
>>> 
>>> Instead of enable by default at boot, can we try to enable/disable only 
>>> when this is strictly needed?
>> Yes we could un-trap inside compute_max_zcr() just before accessing SVE 
>> resources and trap it
>> again when finished. Would it be ok for you this approach?
> 
> Yes.
> 
> Cheers,
> 
> -- 
> Julien Grall