Re: [Xen-devel] [PATCH v8 20/27] ARM: GICv3: handle unmapped LPIs

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

TBH, I would have expected this patch to be split in two:
        - Introduction of the flag before patch #19
        - Set flag in patch #19

This would have make easier to review the implementation of MAPTI.

On 12/04/17 01:44, Andre Przywara wrote:
> When LPIs get unmapped by a guest, they might still be in some LR of
> some VCPU. Nevertheless we remove the corresponding pending_irq
> (possibly freeing it), and detect this case (irq_to_pending() returns
> NULL) when the LR gets cleaned up later.
> However a *new* LPI may get mapped with the same number while the old
> LPI is *still* in some LR. To avoid getting the wrong state, we mark
> every newly mapped LPI as PRISTINE, which means: has never been in an
> LR before. If we detect the LPI in an LR anyway, it must have been an
> older one, which we can simply retire.
From the GICv3 spec (8.4.6 in ARM IHI 0069C):

"Behavior is UNPREDICTABLE if two or more List Registers specify the 
same vINTID when
        - ICH_LR<n>_EL2.State == 01.
        - ICH_LR<n>_EL2.State == 10.
        - ICH_LR<n>_EL2.State == 11.
"

The LPI does not have active bit and we remap the vLPI to another pLPI. 
So I think we might end up to have the same vLPI twice in the LR if the 
new and old vLPI was routed to the same vCPU which is UNPREDICABLE.
Another case if both pending_irq are targeting a different vCPU. In this 
case, the interrupt may be injected simultaneously to different vCPU. I 
am not sure if it is valid...
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> ---
>  xen/arch/arm/gic.c         | 17 ++++++++++++++++-
>  xen/arch/arm/vgic-v3-its.c |  5 +++++
>  xen/include/asm-arm/vgic.h |  1 +
>  3 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index d752352..e8c3202 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -373,6 +373,8 @@ static inline void gic_set_lr(int lr, struct pending_irq *p,
>  {
>      ASSERT(!local_irq_is_enabled());
>
> +    clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status);
> +
This placement looks wrong, gic_set_lr is called by vgic_vcpu_inject_irq 
but you don't know if the LRs have been cleared yet so you could end up 
with the vIRQ in 2 different LRs which is UNPREDICTABLE.
>      gic_hw_ops->update_lr(lr, p, state);
>
>      set_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
> @@ -510,7 +512,17 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>      }
>      else if ( lr_val.state & GICH_LR_PENDING )
>      {
> -        int q __attribute__ ((unused)) = test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, 
> &p->status);
> +        int q __attribute__ ((unused));
> +
> +        if ( test_and_clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status) )
> +        {
> +            gic_hw_ops->clear_lr(i);
> +            clear_bit(i, &this_cpu(lr_mask));
> +
> +            return;
> +        }
> +
> +        q = test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
>  #ifdef GIC_DEBUG
>          if ( q )
>              gdprintk(XENLOG_DEBUG, "trying to inject irq=%d into d%dv%d, when it is 
> already pending in LR%d\n",
> @@ -522,6 +534,9 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>          gic_hw_ops->clear_lr(i);
>          clear_bit(i, &this_cpu(lr_mask));
>
> +        if ( test_and_clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status) )
> +            return;
> +
>          if ( p->desc != NULL )
>              clear_bit(_IRQ_INPROGRESS, &p->desc->status);
>          clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> index b7e61b2..0765810 100644
> --- a/xen/arch/arm/vgic-v3-its.c
> +++ b/xen/arch/arm/vgic-v3-its.c
> @@ -618,6 +618,11 @@ static int its_handle_mapti(struct virt_its *its, uint64_t 
> *cmdptr)
>          goto out_remove_host_entry;
>
>      pirq->lpi_vcpu_id = vcpu->vcpu_id;
> +    /*
> +     * Mark this LPI as new, so any older (now unmapped) LPI in any LR
> +     * can be easily recognised as such.
> +     */
> +    set_bit(GIC_IRQ_GUEST_PRISTINE_LPI, pirq->status);
>
>      /*
>       * Now insert the pending_irq into the domain's LPI tree, so that
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 02732db..b1a7525 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -66,6 +66,7 @@ struct pending_irq
>  #define GIC_IRQ_GUEST_VISIBLE  2
>  #define GIC_IRQ_GUEST_ENABLED  3
>  #define GIC_IRQ_GUEST_MIGRATING   4
> +#define GIC_IRQ_GUEST_PRISTINE_LPI  5
Please document this new flag...

>      unsigned long status;
>      struct irq_desc *desc; /* only set it the irq corresponds to a physical 
> irq */
>      unsigned int irq;
Cheers,

--
Julien Grall

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