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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

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

Otherwise I will add the barrier.


> 
> [...]
> 
>> @@ -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?

> 
>> -    WRITE_SYSREG((HCPTR_CP_MASK & ~(HCPTR_CP(10) | HCPTR_CP(11))) |
>> -                 HCPTR_TTA | HCPTR_TAM,
>> -                 CPTR_EL2);
>> +    cptr_bits &= ~HCPTR_CP(8);
>> +#endif
>> +
>> +    WRITE_SYSREG(cptr_bits, CPTR_EL2);
>>        /*
>>       * Configure HCR_EL2 with the bare minimum to run Xen until a guest
> 
> Cheers,
> -- 
> Julien Grall