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

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi,

On 26/05/17 11:19, Julien Grall wrote:
> Hi Stefano,
> 
> On 25/05/17 22:05, Stefano Stabellini wrote:
>> On Thu, 25 May 2017, Julien Grall wrote:
>>> Hi Stefano,
>>>
>>> On 25/05/2017 19:49, Stefano Stabellini wrote:
>>>> On Thu, 25 May 2017, Andre Przywara wrote:
>>>>> 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?
>>>>
>>>> If the VGIC VCPU lock is only used to protect VGIC_V3_LPIS_ENABLED,
>>>> couldn't we just read/write the bit atomically? It's just a bit after
>>>> all, it doesn't need a lock.
>>>
>>> The vGIC vCPU lock is also here to serialize access to the
>>> re-distributor
>>> state when necessary.
>>>
>>> For instance you don't want to allow write in PENDBASER after LPIs
>>> have been
>>> enabled.
>>>
>>> If you don't take the lock here, you would have a small race where
>>> PENDBASER
>>> might be written whilst the LPIs are getting enabled.
>>>
>>> The code in PENDBASER today does not strictly require the locking,
>>> but I think
>>> we should keep the lock around. Moving to the atomic will not really
>>> benefit
>>> here as write to those registers will be very unlikely so we don't
>>> need very
>>> good performance.
>>
>> I suggested the atomic as a way to replace the lock, to reduce the
>> number of lock order dependencies, rather than for performance (who
>> cares about performance for this case). If all accesses to
>> VGIC_V3_LPIS_ENABLED are atomic, then we wouldn't need the lock.
>>
>> Another maybe simpler way to keep the vgic vcpu lock but avoid
>> introducing the vgic domain lock -> vgic vcpu lock dependency (the less
>> the better) would be to take the vgic vcpu lock first, release it, then
>> take the vgic domain lock and call vgic_vcpu_enable_lpis after.  In
>> pseudo-code:
>>
>>     vgic vcpu lock
>>     read old value of VGIC_V3_LPIS_ENABLED
>>     write new value of VGIC_V3_LPIS_ENABLED
>>     vgic vcpu unlock
>>
>>     vgic domain lock
>>     vgic_vcpu_enable_lpis (minus the setting of arch.vgic.flags)
>>     vgic domain unlock
>>
>> It doesn't look like we need to set VGIC_V3_LPIS_ENABLED within
>> vgic_vcpu_enable_lpis, so this seems to be working. What do you think?
> 
> From a point of view of the vGIC you want to enable VGIC_V3_LPIS_ENABLED
> after all the sanity checks have been done.
> 
> I would have expected the ITS to check if the redistributor has been
> enabled before enabling it (see vgic_v3_verify_its_status). This is
> because the ITS is using the priority table and also the number of LPIs.
> 
> So you effectively want to have VGIC_V3_LPIS_ENABLED set after in
> vgic_vcpu_enable_lpis to avoid potential race condition. You may also
> want to have a mb() before writing to it so you can using
> VGIC_V3_LPIS_ENABLED without any lock safely.

Right, I added an smp_mb() after the rdists_enabled write to make sure
this is in sync.

> Andre, can you explain why the ITS does not check whether the
> rdists_enabled are enabled?

So architecturally it's not required to have LPIs enabled, and from a
spec point of view the ITS does not care about the LPI's properties.
We check for LPIs being enabled on that redistributor before injecting LPI.

But I think you are right that our implementation is a bit sloppy with
the separation between LPIs and the ITS, and reads the property table
while handling commands - because we only keep track of mapped LPIs.
So I added a check now in update_lpi_properties() to bail out (without
an error) if no redistributor has LPIs enabled yet. That should solve
that corner case.

Cheers,
Andre.

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