Re: [RFC 22/22] xen/arm: gic-v3: Add support of vGICv2 when available

[Julien Grall <julien.grall@citrix.com>]

Hi Ian,

On 08/06/2015 12:01, Ian Campbell wrote:
> On Fri, 2015-06-05 at 17:35 +0100, Julien Grall wrote:
>
>>>>    */
>>>>   static void gicv3_enable_sre(void)
>>>>   {
>>>>       uint32_t val;
>>>>
>>>>       val = READ_SYSREG32(ICC_SRE_EL2);
>>>> -    val |= GICC_SRE_EL2_SRE | GICC_SRE_EL2_ENEL1;
>>>> +    val |= GICC_SRE_EL2_SRE;
>>>>
>>>>       WRITE_SYSREG32(val, ICC_SRE_EL2);
>>>>       isb();
>>>> @@ -373,6 +372,20 @@ static void gicv3_save_state(struct vcpu *v)
>>>>
>>>>   static void gicv3_restore_state(const struct vcpu *v)
>>>>   {
>>>> +    uint32_t val;
>>>> +
>>>> +    val = READ_SYSREG32(ICC_SRE_EL2);
>>>> +    /*
>>>> +     * Don't give access to system registers when the guest is using
>>>> +     * GICv2
>>>> +     */
>>>> +    if ( v->domain->arch.vgic.version == GIC_V2 )
>>>> +        val &= ~GICC_SRE_EL2_ENEL1;
>>>> +    else
>>>> +        val |= GICC_SRE_EL2_ENEL1;
>>>> +    WRITE_SYSREG32(val, ICC_SRE_EL2);
>>>
>>> Perhaps save/restore v->arch.gic.v3.sre_el2 rather than reading and
>>> recalculating each time? Then you just need to set sre_el2 appropriately
>>> during domain init.
>>
>> Hmmm that would mean to reintroduce gicv_setup for setting sre_el2.
>>
>> What is your concern here?
>
> I don't really like read/modify/write idiom in the restore path, I'd
> prefer a straight save/restore model (I know we have some R/M/W
> already).

I though about it. Most of the R/M/W idioms are to modify field in EL2 
registers. Thoses registers can be modified at any time by Xen, it's not 
dependent to the current domain running. If we don't use the R/M/W we 
would have to propagate the change.

SRE_EL2 falls in the same category. And it would be preferable to stay 
with the same model as today i.e:
	*_EL1: straight save/restore
	*_EL2: R/M/W to necessary field

Regards,

-- 
Julien Grall