Re: [PATCH v2 5/8] xen: arm: add interfaces to save/restore the state of a PPI.

[Julien Grall <julien.grall@citrix.com>]

Hi Ian,

On 10/11/15 16:21, Ian Campbell wrote:
> Make use of the GICD I[SC]ACTIVER registers to save and
> restore the active state of the interrupt.
> 
> For edge triggered interrupts we also need to context switch the
> pending bit via I[SC]PENDR. Note that for level triggered interrupts
> SPENDR sets a latch which is only cleared by ICPENDR (and not by h/w
> state changes), therefore we do not want to context switch the pending
> state for level PPIs -- instead we rely on the context switch of the
> peripheral to restore the correct level.
> 
> Tested on GIC v2 only.

I will give a try on the foundation model with GICv3.

> Unused as yet, will be used by the vtimer code shortly.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/gic-v2.c        | 67 ++++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/gic-v3.c        | 67 ++++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/gic.c           | 54 +++++++++++++++++++++++++++++++++++
>  xen/arch/arm/irq.c           |  7 +++++
>  xen/include/asm-arm/domain.h | 11 ++++++++
>  xen/include/asm-arm/gic.h    | 16 +++++++++++
>  xen/include/asm-arm/irq.h    |  3 ++
>  7 files changed, 225 insertions(+)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 01e36b5..5308c35 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -81,6 +81,9 @@ static DEFINE_PER_CPU(u8, gic_cpu_id);
>  /* Maximum cpu interface per GIC */
>  #define NR_GIC_CPU_IF 8
>  
> +static void gicv2_irq_enable(struct irq_desc *desc);
> +static void gicv2_irq_disable(struct irq_desc *desc);
> +
>  static inline void writeb_gicd(uint8_t val, unsigned int offset)
>  {
>      writeb_relaxed(val, gicv2.map_dbase + offset);
> @@ -149,6 +152,37 @@ static void gicv2_save_state(struct vcpu *v)
>      writel_gich(0, GICH_HCR);
>  }
>  
> +static void gicv2_save_and_mask_hwppi(struct irq_desc *desc,
> +                                      struct hwppi_state *s)
> +{
> +    const unsigned int mask = (1u << desc->irq);

I would add a comment to explain that PPI are always below 31. Otherwise
this construction would be invalid.

> +    const unsigned int pendingr = readl_gicd(GICD_ISPENDR);
> +    const unsigned int activer = readl_gicd(GICD_ISACTIVER);
> +    const unsigned int enabler = readl_gicd(GICD_ISENABLER);

pendingr, activer, enabler are 32-bit register. Please use uint32_t here
to make it clear.

> +    const bool is_edge = !!(desc->arch.type & DT_IRQ_TYPE_EDGE_BOTH);
> +
> +    s->active = !!(activer & mask);
> +    s->enabled = !!(enabler & mask);
> +    s->pending = !!(pendingr & mask);
> +
> +    /* Write a 1 to IC...R to clear the corresponding bit of state */
> +    if ( s->active )
> +        writel_gicd(mask, GICD_ICACTIVER);

NIT: Newline here for clarity.

> +    /*
> +     * For an edge interrupt clear the pending state, for a level interrupt
> +     * this clears the latch there is no need since saving the peripheral state
> +     * (and/or restoring the next VCPU) will cause the correct action.
> +     */
> +    if ( is_edge && s->pending )
> +        writel_gicd(mask, GICD_ICPENDR);
> +
> +    if ( s->enabled )
> +        gicv2_irq_disable(desc);

Don't we want to disable the IRQ first and then saving the state?

> +
> +    ASSERT(!(readl_gicd(GICD_ISACTIVER) & mask));
> +    ASSERT(!(readl_gicd(GICD_ISENABLER) & mask));
> +}
> +
>  static void gicv2_restore_state(const struct vcpu *v)
>  {
>      int i;
> @@ -161,6 +195,37 @@ static void gicv2_restore_state(const struct vcpu *v)
>      writel_gich(GICH_HCR_EN, GICH_HCR);
>  }
>  
> +static void gicv2_restore_hwppi(struct irq_desc *desc,
> +                                const struct hwppi_state *s)
> +{
> +    const unsigned int mask = (1u << desc->irq);
> +    const bool is_edge = !!(desc->arch.type & DT_IRQ_TYPE_EDGE_BOTH);
> +
> +    /*
> +     * The IRQ must always have been set inactive and masked etc by
> +     * the saving of the previous state via save_and_mask_hwppi.
> +     */
> +    ASSERT(!(readl_gicd(GICD_ISACTIVER) & mask));
> +    ASSERT(!(readl_gicd(GICD_ISENABLER) & mask));
> +
> +    if ( s->active )
> +        writel_gicd(mask, GICD_ICACTIVER);
> +
> +    /*
> +     * Restore pending state for edge triggered interrupts only. For
> +     * level triggered interrupts the level will be restored as
> +     * necessary by restoring the state of the relevant peripheral.
> +     *
> +     * For a level triggered interrupt ISPENDR acts as a *latch* which
> +     * is only cleared by ICPENDR (i.e. the input level is no longer
> +     * relevant). We certainly do not want that here.
> +     */
> +    if ( is_edge && s->pending )
> +        writel_gicd(mask, GICD_ISPENDR);

NIT: New line for clarity.

> +    if ( s->enabled )
> +        gicv2_irq_enable(desc);
> +}
> +
>  static void gicv2_dump_state(const struct vcpu *v)
>  {
>      int i;
> @@ -744,7 +809,9 @@ const static struct gic_hw_operations gicv2_ops = {
>      .init                = gicv2_init,
>      .secondary_init      = gicv2_secondary_cpu_init,
>      .save_state          = gicv2_save_state,
> +    .save_and_mask_hwppi = gicv2_save_and_mask_hwppi,
>      .restore_state       = gicv2_restore_state,
> +    .restore_hwppi       = gicv2_restore_hwppi,
>      .dump_state          = gicv2_dump_state,
>      .gic_host_irq_type   = &gicv2_host_irq_type,
>      .gic_guest_irq_type  = &gicv2_guest_irq_type,
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 4fe0c37..cfe705a 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -61,6 +61,9 @@ static DEFINE_PER_CPU(void __iomem*, rbase);
>  #define GICD_RDIST_BASE        (this_cpu(rbase))
>  #define GICD_RDIST_SGI_BASE    (GICD_RDIST_BASE + SZ_64K)
>  
> +void gicv3_irq_enable(struct irq_desc *desc);
> +void gicv3_irq_disable(struct irq_desc *desc);
> +
>  /*
>   * Saves all 16(Max) LR registers. Though number of LRs implemented
>   * is implementation specific.
> @@ -373,6 +376,37 @@ static void gicv3_save_state(struct vcpu *v)
>      v->arch.gic.v3.sre_el1 = READ_SYSREG32(ICC_SRE_EL1);
>  }
>  
> +static void gicv3_save_and_mask_hwppi(struct irq_desc *desc,
> +                                      struct hwppi_state *s)
> +{
> +    const unsigned int mask = (1u << desc->irq);
> +    const unsigned int pendingr = readl_relaxed(GICD_RDIST_SGI_BASE + GICR_ISPENDR);
> +    const unsigned int activer = readl_gicd(GICD_RDIST_SGI_BASE + GICR_ISACTIVER);
> +    const unsigned int enabler = readl_gicd(GICD_RDIST_SGI_BASE + GICR_ISENABLER);

Those registers don't exists. Did you try to build the GICv3 code?

[...]

>  void gic_restore_state(struct vcpu *v)
>  {
>      ASSERT(!local_irq_is_enabled());
> @@ -94,6 +124,30 @@ void gic_restore_state(struct vcpu *v)
>      gic_restore_pending_irqs(v);
>  }
>  
> +void gic_restore_hwppi(struct vcpu *v,
> +                       const unsigned virq,
> +                       const struct hwppi_state *s)
> +{
> +    struct pending_irq *p = irq_to_pending(v, virq);
> +    struct irq_desc *desc = irq_to_desc(s->irq);
> +
> +    spin_lock(&desc->lock);
> +
> +    ASSERT(virq >= 16 && virq < 32);
> +    ASSERT(!is_idle_vcpu(v));
> +
> +    p->desc = desc; /* Migrate to new physical processor */

Is it safe?

For GICv3, the re-distributor of a VCPU A could be access by VCPU B
which means that all the operations (disable/enable...) could be done
while we restore the PPI.

> +
> +    irq_set_virq(desc, virq);
> +
> +    gic_hw_ops->restore_hwppi(desc, s);
> +
> +    if ( s->inprogress )
> +        set_bit(_IRQ_INPROGRESS, &desc->status);
> +
> +    spin_unlock(&desc->lock);
> +}
> +
>  /*
>   * needs to be called with a valid cpu_mask, ie each cpu in the mask has
>   * already called gic_cpu_init
> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 5b073d1..95be10e 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -138,6 +138,13 @@ static inline struct irq_guest *irq_get_guest_info(struct irq_desc *desc)
>      return desc->action->dev_id;
>  }
>  
> +void irq_set_virq(struct irq_desc *desc, unsigned virq)
> +{
> +    struct irq_guest *info = irq_get_guest_info(desc);
> +    ASSERT(test_bit(_IRQ_PER_CPU, &desc->status));
> +    info->virq = virq;
> +}
> +
>  static inline struct domain *irq_get_domain(struct irq_desc *desc)
>  {
>      return irq_get_guest_info(desc)->d;
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index c56f06e..550e28b 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -34,6 +34,17 @@ enum domain_type {
>  extern int dom0_11_mapping;
>  #define is_domain_direct_mapped(d) ((d) == hardware_domain && dom0_11_mapping)
>  
> +struct hwppi_state {
> +    /* h/w state */
> +    unsigned irq;
> +    unsigned long enabled:1;
> +    unsigned long pending:1;
> +    unsigned long active:1;
> +
> +    /* Xen s/w state */
> +    unsigned long inprogress:1;
> +};
> +
>  struct vtimer {
>      struct vcpu *v;
>      int irq;
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 0116481..fe9af3e 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -258,6 +258,20 @@ extern int gicv_setup(struct domain *d);
>  extern void gic_save_state(struct vcpu *v);
>  extern void gic_restore_state(struct vcpu *v);
>  
> +/*
> + * Save/restore the state of a single PPI which must be routed to
> + * <current-vcpu> (that is, is defined to be injected to the current
> + * vcpu).

I would add a comment here to explain that we expect the device which
use this PPI to be quiet while we save/restore.

For instance we want to disable the timer before saving the state.
Otherwise we will mess up the state.

> + */
> +struct hwppi_state;
> +extern void gic_hwppi_state_init(struct hwppi_state *s, unsigned irq);
> +extern void gic_hwppi_set_pending(struct hwppi_state *s);
> +extern void gic_save_and_mask_hwppi(struct vcpu *v, unsigned irq,
> +                                    struct hwppi_state *s);
> +extern void gic_restore_hwppi(struct vcpu *v,
> +                              const unsigned virq,
> +                              const struct hwppi_state *s);
> +
>  /* SGI (AKA IPIs) */
>  enum gic_sgi {
>      GIC_SGI_EVENT_CHECK = 0,
> @@ -308,8 +322,10 @@ struct gic_hw_operations {
>      int (*init)(void);
>      /* Save GIC registers */
>      void (*save_state)(struct vcpu *);
> +    void (*save_and_mask_hwppi)(struct irq_desc *desc, struct hwppi_state *s);
>      /* Restore GIC registers */
>      void (*restore_state)(const struct vcpu *);
> +    void (*restore_hwppi)(struct irq_desc *desc, const struct hwppi_state *s);
>      /* Dump GIC LR register information */
>      void (*dump_state)(const struct vcpu *);
>  
> diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
> index f33c331..d354e3b 100644
> --- a/xen/include/asm-arm/irq.h
> +++ b/xen/include/asm-arm/irq.h
> @@ -43,6 +43,7 @@ void init_secondary_IRQ(void);
>  
>  int route_irq_to_guest(struct domain *d, unsigned int virq,
>                         unsigned int irq, const char *devname);
> +int route_irq_to_current_vcpu(unsigned int irq, const char *devname);

The prototype belongs to patch #6.

Regards,

-- 
Julien Grall