Re: [Xen-devel] [PATCH v2 04/10] ARM: VGIC: streamline gic_restore_pending_irqs()

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 7 Dec 2017, Andre Przywara wrote:
> In gic_restore_pending_irqs() we push our pending virtual IRQs into the
> list registers. This function is called once from a GIC context and once
> from a VGIC context. Refactor the calls so that we have only one callsite
> from the VGIC context. This will help separating the two worlds later.
> 
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
> ---
>  xen/arch/arm/domain.c     |  1 +
>  xen/arch/arm/gic.c        | 11 +++++------
>  xen/arch/arm/traps.c      |  2 +-
>  xen/include/asm-arm/gic.h |  2 +-
>  4 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index a74ff1c07c..73f4d4b2b2 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -185,6 +185,7 @@ static void ctxt_switch_to(struct vcpu *n)
>  
>      /* VGIC */
>      gic_restore_state(n);
> +    gic_inject(n);
>  
>      /* VFP */
>      vfp_restore_state(n);
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index bac8ada2bb..1f00654ef5 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -36,8 +36,6 @@
>  #include <asm/vgic.h>
>  #include <asm/acpi.h>
>  
> -static void gic_restore_pending_irqs(struct vcpu *v);
> -
>  static DEFINE_PER_CPU(uint64_t, lr_mask);
>  
>  #define lr_all_full() (this_cpu(lr_mask) == ((1 << gic_hw_ops->info->nr_lrs) 
> - 1))
> @@ -91,8 +89,6 @@ void gic_restore_state(struct vcpu *v)
>      gic_hw_ops->restore_state(v);
>  
>      isb();
> -
> -    gic_restore_pending_irqs(v);
>  }
>  
>  /* desc->irq needs to be disabled before calling this function */
> @@ -715,11 +711,14 @@ out:
>      return rc;
>  }
>  
> -void gic_inject(void)
> +void gic_inject(struct vcpu *v)
>  {
>      ASSERT(!local_irq_is_enabled());
>  
> -    gic_restore_pending_irqs(current);
> +    gic_restore_pending_irqs(v);

This looks suspicious to me: gic_restore_pending_irqs calls gic_set_lr.
It doesn't look like gic_restore_pending_irqs can actually be called for
v != current.

However, I think that we could remove the call to
gic_restore_pending_irqs from gic_restore_state safely, because we can
still rely on gic_inject being called before entering the guest.

There is no need to add a call to gic_inject in ctxt_switch_to, I think.


> +    if ( v != current )
> +        return;
>  
>      if ( !list_empty(&current->arch.vgic.lr_pending) && lr_all_full() )
>          gic_hw_ops->update_hcr_status(GICH_HCR_UIE, true);
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index ff3d6ff2aa..7fd676ed9d 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2298,7 +2298,7 @@ void leave_hypervisor_tail(void)
>      {
>          local_irq_disable();
>          if (!softirq_pending(smp_processor_id())) {
> -            gic_inject();
> +            gic_inject(current);
>  
>              /*
>               * If the SErrors handle option is "DIVERSE", we have to prevent
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 587a14f8b9..28cf16654a 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -235,7 +235,7 @@ extern int gic_route_irq_to_guest(struct domain *, 
> unsigned int virq,
>  int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
>                                struct irq_desc *desc);
>  
> -extern void gic_inject(void);
> +extern void gic_inject(struct vcpu *v);
>  extern void gic_clear_pending_irqs(struct vcpu *v);
>  extern int gic_events_need_delivery(void);

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel