Re: [Xen-devel] [PATCH v9 18/28] ARM: vITS: handle CLEAR command

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi,

On 17/05/17 18:45, Julien Grall wrote:
> Hi Andre,
> 
> On 11/05/17 18:53, Andre Przywara wrote:
>> This introduces the ITS command handler for the CLEAR command, which
>> clears the pending state of an LPI.
>> This removes a not-yet injected, but already queued IRQ from a VCPU.
>> As read_itte() is now eventually used, we add the static keyword.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>> ---
>>  xen/arch/arm/vgic-v3-its.c | 59
>> ++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 57 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
>> index 8f1c217..8a200e9 100644
>> --- a/xen/arch/arm/vgic-v3-its.c
>> +++ b/xen/arch/arm/vgic-v3-its.c
>> @@ -52,6 +52,7 @@
>>   */
>>  struct virt_its {
>>      struct domain *d;
>> +    paddr_t doorbell_address;
>>      unsigned int devid_bits;
>>      unsigned int evid_bits;
>>      spinlock_t vcmd_lock;       /* Protects the virtual command
>> buffer, which */
>> @@ -251,8 +252,8 @@ static bool read_itte_locked(struct virt_its *its,
>> uint32_t devid,
>>   * This function takes care of the locking by taking the its_lock
>> itself, so
>>   * a caller shall not hold this. Before returning, the lock is
>> dropped again.
>>   */
>> -bool read_itte(struct virt_its *its, uint32_t devid, uint32_t evid,
>> -               struct vcpu **vcpu_ptr, uint32_t *vlpi_ptr)
>> +static bool read_itte(struct virt_its *its, uint32_t devid, uint32_t
>> evid,
>> +                      struct vcpu **vcpu_ptr, uint32_t *vlpi_ptr)
>>  {
>>      bool ret;
>>
>> @@ -362,6 +363,57 @@ static int its_handle_mapc(struct virt_its *its,
>> uint64_t *cmdptr)
>>      return 0;
>>  }
>>
>> +/*
>> + * CLEAR removes the pending state from an LPI. */
>> +static int its_handle_clear(struct virt_its *its, uint64_t *cmdptr)
>> +{
>> +    uint32_t devid = its_cmd_get_deviceid(cmdptr);
>> +    uint32_t eventid = its_cmd_get_id(cmdptr);
>> +    struct pending_irq *p;
>> +    struct vcpu *vcpu;
>> +    uint32_t vlpi;
>> +    unsigned long flags;
>> +    int ret = -1;
>> +
>> +    spin_lock(&its->its_lock);
>> +
>> +    /* Translate the DevID/EvID pair into a vCPU/vLPI pair. */
>> +    if ( !read_itte_locked(its, devid, eventid, &vcpu, &vlpi) )
>> +        goto out_unlock;
>> +
>> +    p = gicv3_its_get_event_pending_irq(its->d, its->doorbell_address,
>> +                                        devid, eventid);
>> +    /* Protect against an invalid LPI number. */
>> +    if ( unlikely(!p) )
>> +        goto out_unlock;
>> +
>> +    spin_lock_irqsave(&vcpu->arch.vgic.lock, flags);
> 
> My comment in patch #9 about crafting the memory handed over to the ITS
> applies here too.
> 
>> +
>> +    /*
>> +     * If the LPI is already visible on the guest, it is too late to
>> +     * clear the pending state. However this is a benign race that can
>> +     * happen on real hardware, too: If the LPI has already been
>> forwarded
>> +     * to a CPU interface, a CLEAR request reaching the redistributor
>> has
>> +     * no effect on that LPI anymore. Since LPIs are edge triggered and
>> +     * have no active state, we don't need to care about this here.
>> +     */
>> +    if ( !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
>> +    {
>> +        /* Remove a pending, but not yet injected guest IRQ. */
>> +        clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
>> +        list_del_init(&p->inflight);
>> +        list_del_init(&p->lr_queue);
> 
> On the previous version I was against this open-coding of
> gic_remove_from_queues and instead rework the function.

Well, I consider gic_remove_from_queues() somewhat broken:
- It should be called vgic_remove... and live in vgic.c, because it
deals with the virtual side only.
- The plural in the name is wrong, since it only removes the IRQ from
lr_pending, not inflight.
- vgic_migrate_irq removes an IRQ from both queues as well, and doesn't
use the function (for the same reason).

So to make it usable in our case, I'd need to either open code the
inflight removal here (which would make calling this function a bit
pointless) or add that to the function, but remove the existing caller.
Looks like a can of worms to me and a distraction from the actual goal
of getting the ITS in place.
So I will surely address this with the VGIC rework (possibly removing
this function altogether), but would like to avoid doing this rework
*now*. To catch all users of the list I would need to grep for inflight
and lr_pending anyway, so one more "open-coded" place is not a big deal.

> It still does not make any sense to me because if one day someone
> decides to update gic_remove_from_queues (such as you because you are
> going to rework the vGIC), he will have to remember that you open-coded
> in MOVE because you didn't want to touch the common code.

As I mentioned above this is the same situation for vgic_migrate_irq()
already.

> Common code is not set in stone. The goal is to abstract all the issues
> to make easier to propagate change. So please address this comment.

I clearly understand this and am all for fixing this, but I don't
believe the ITS series is the place to do this. In fact I don't want to
add more code to this series.
If gic_remove_from_queues would keep up the promise its name gives, I
would love to use it, but it doesn't, so ...

Cheers,
Andre.

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