Re: [Xen-devel] [RFC PATCH v2 09/22] ARM: vITS: protect LPI priority update with pending_irq lock

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

On 21/07/17 20:59, Andre Przywara wrote:
> As the priority value is now officially a member of struct pending_irq,
> we need to take its lock when manipulating it via ITS commands.
> Make sure we take the IRQ lock after the VCPU lock when we need both.
>
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> ---
>  xen/arch/arm/vgic-v3-its.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index 66095d4..705708a 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -402,6 +402,7 @@ static int update_lpi_property(struct domain *d, struct 
> pending_irq *p)
>      uint8_t property;
>      int ret;
>
> +    ASSERT(spin_is_locked(&p->lock));
>      /*
>       * If no redistributor has its LPIs enabled yet, we can't access the
>       * property table. In this case we just can't update the properties,
> @@ -419,7 +420,7 @@ static int update_lpi_property(struct domain *d, struct 
> pending_irq *p)
>      if ( ret )
>          return ret;
>
> -    write_atomic(&p->priority, property & LPI_PROP_PRIO_MASK);
> +    p->priority = property & LPI_PROP_PRIO_MASK;
>
>      if ( property & LPI_PROP_ENABLED )
>          set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> @@ -457,7 +458,7 @@ static int its_handle_inv(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;
> -    unsigned long flags;
> +    unsigned long flags, vcpu_flags;
Same remark as patch #8 for the vcpu_flags and the locking.

>      struct vcpu *vcpu;
>      uint32_t vlpi;
>      int ret = -1;
> @@ -485,7 +486,8 @@ static int its_handle_inv(struct virt_its *its, uint64_t 
> *cmdptr)
>      if ( unlikely(!p) )
>          goto out_unlock_its;
>
> -    spin_lock_irqsave(&vcpu->arch.vgic.lock, flags);
> +    spin_lock_irqsave(&vcpu->arch.vgic.lock, vcpu_flags);
> +    vgic_irq_lock(p, flags);
>
>      /* Read the property table and update our cached status. */
>      if ( update_lpi_property(d, p) )
> @@ -497,7 +499,8 @@ static int its_handle_inv(struct virt_its *its, uint64_t 
> *cmdptr)
>      ret = 0;
>
>  out_unlock:
> -    spin_unlock_irqrestore(&vcpu->arch.vgic.lock, flags);
> +    vgic_irq_unlock(p, flags);
> +    spin_unlock_irqrestore(&vcpu->arch.vgic.lock, vcpu_flags);
>
>  out_unlock_its:
>      spin_unlock(&its->its_lock);
> @@ -517,7 +520,7 @@ static int its_handle_invall(struct virt_its *its, uint64_t 
> *cmdptr)
>      struct pending_irq *pirqs[16];
>      uint64_t vlpi = 0;          /* 64-bit to catch overflows */
>      unsigned int nr_lpis, i;
> -    unsigned long flags;
> +    unsigned long flags, vcpu_flags;
>      int ret = 0;
>
>      /*
> @@ -542,7 +545,7 @@ static int its_handle_invall(struct virt_its *its, uint64_t 
> *cmdptr)
>      vcpu = get_vcpu_from_collection(its, collid);
>      spin_unlock(&its->its_lock);
>
> -    spin_lock_irqsave(&vcpu->arch.vgic.lock, flags);
> +    spin_lock_irqsave(&vcpu->arch.vgic.lock, vcpu_flags);
>      read_lock(&its->d->arch.vgic.pend_lpi_tree_lock);
>
>      do
> @@ -555,9 +558,13 @@ static int its_handle_invall(struct virt_its *its, 
> uint64_t *cmdptr)
>
>          for ( i = 0; i < nr_lpis; i++ )
>          {
> +            vgic_irq_lock(pirqs[i], flags);
>              /* We only care about LPIs on our VCPU. */
>              if ( pirqs[i]->lpi_vcpu_id != vcpu->vcpu_id )
> +            {
> +                vgic_irq_unlock(pirqs[i], flags);
This locking does not seem to be related to the priority.

>                  continue;
> +            }
>
>              vlpi = pirqs[i]->irq;
>              /* If that fails for a single LPI, carry on to handle the rest. */
> @@ -566,6 +573,8 @@ static int its_handle_invall(struct virt_its *its, uint64_t 
> *cmdptr)
>                  update_lpi_vgic_status(vcpu, pirqs[i]);
>              else
>                  ret = err;
> +
> +            vgic_irq_unlock(pirqs[i], flags);
>          }
>      /*
>       * Loop over the next gang of pending_irqs until we reached the end of
> @@ -576,7 +585,7 @@ static int its_handle_invall(struct virt_its *its, uint64_t 
> *cmdptr)
>                (nr_lpis == ARRAY_SIZE(pirqs)) );
>
>      read_unlock(&its->d->arch.vgic.pend_lpi_tree_lock);
> -    spin_unlock_irqrestore(&vcpu->arch.vgic.lock, flags);
> +    spin_unlock_irqrestore(&vcpu->arch.vgic.lock, vcpu_flags);
>
>      return ret;
>  }
> @@ -712,6 +721,7 @@ static int its_handle_mapti(struct virt_its *its, uint64_t 
> *cmdptr)
>      uint32_t intid = its_cmd_get_physical_id(cmdptr), _intid;
>      uint16_t collid = its_cmd_get_collection(cmdptr);
>      struct pending_irq *pirq;
> +    unsigned long flags;
>      struct vcpu *vcpu = NULL;
>      int ret = -1;
>
> @@ -765,7 +775,9 @@ static int its_handle_mapti(struct virt_its *its, uint64_t 
> *cmdptr)
>       * We don't need the VGIC VCPU lock here, because the pending_irq isn't
>       * in the radix tree yet.
>       */
> +    vgic_irq_lock(pirq, flags);
>      ret = update_lpi_property(its->d, pirq);
> +    vgic_irq_unlock(pirq, flags);
>      if ( ret )
>          goto out_remove_host_entry;
Cheers,

--
Julien Grall

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