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

[Julien Grall <julien.grall@xxxxxxx>]

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

Cheers,

--
Julien Grall

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