Re: [Xen-devel] [PATCH v9 12/28] ARM: vGIC: advertise LPI support

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi,

On 23/05/17 18:47, Stefano Stabellini wrote:
> On Tue, 23 May 2017, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 22/05/17 23:19, Stefano Stabellini wrote:
>>> On Tue, 16 May 2017, Julien Grall wrote:
>>>>> @@ -436,8 +473,26 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct
>>>>> vcpu
>>>>> *v, mmio_info_t *info,
>>>>>      switch ( gicr_reg )
>>>>>      {
>>>>>      case VREG32(GICR_CTLR):
>>>>> -        /* LPI's not implemented */
>>>>> -        goto write_ignore_32;
>>>>> +    {
>>>>> +        unsigned long flags;
>>>>> +
>>>>> +        if ( !v->domain->arch.vgic.has_its )
>>>>> +            goto write_ignore_32;
>>>>> +        if ( dabt.size != DABT_WORD ) goto bad_width;
>>>>> +
>>>>> +        vgic_lock(v);                   /* protects rdists_enabled */
>>>>
>>>> Getting back to the locking. I don't see any place where we get the domain
>>>> vgic lock before vCPU vgic lock. So this raises the question why this
>>>> ordering
>>>> and not moving this lock into vgic_vcpu_enable_lpis.
>>>>
>>>> At least this require documentation in the code and explanation in the
>>>> commit
>>>> message.
>>>
>>> It doesn't look like we need to take the v->arch.vgic.lock here. What is
>>> it protecting?
>>
>> The name of the function is a bit confusion. It does not take the vCPU vgic
>> lock but the domain vgic lock.
>>
>> I believe the vcpu is passed to avoid have v->domain in most of the callers.
>> But we should probably rename the function.
>>
>> In this case it protects vgic_vcpu_enable_lpis because you can configure the
>> number of LPIs per re-distributor but this is a domain wide value. I know the
>> spec is confusing on this.
> 
> The quoting here is very unhelpful. In Andre's patch:
> 
> @@ -436,8 +473,26 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu 
> *v, mmio_info_t *info,
>      switch ( gicr_reg )
>      {
>      case VREG32(GICR_CTLR):
> -        /* LPI's not implemented */
> -        goto write_ignore_32;
> +    {
> +        unsigned long flags;
> +
> +        if ( !v->domain->arch.vgic.has_its )
> +            goto write_ignore_32;
> +        if ( dabt.size != DABT_WORD ) goto bad_width;
> +
> +        vgic_lock(v);                   /* protects rdists_enabled */
> +        spin_lock_irqsave(&v->arch.vgic.lock, flags);
> +
> +        /* LPIs can only be enabled once, but never disabled again. */
> +        if ( (r & GICR_CTLR_ENABLE_LPIS) &&
> +             !(v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED) )
> +            vgic_vcpu_enable_lpis(v);
> +
> +        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> +        vgic_unlock(v);
> +
> +        return 1;
> +    }
> 
> My question is: do we need to take both vgic_lock and v->arch.vgic.lock?

The domain lock (taken by vgic_lock()) protects rdists_enabled. This
variable stores whether at least one redistributor has LPIs enabled. In
this case the property table gets into use and since the table is shared
across all redistributors, we must not change it anymore, even on
another redistributor which has its LPIs still disabled.
So while this looks like this is a per-redistributor (=per-VCPU)
property, it is actually per domain, hence this lock.
The VGIC VCPU lock is then used to naturally protect the enable bit
against multiple VCPUs accessing this register simultaneously - the
redists are MMIO mapped, but not banked, so this is possible.

Does that make sense?

Cheers,
Andre

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel