Re: [Xen-devel] [PATCH 6/6] ARM: GIC: extend LR read/write functions to cover EOI and source

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi,

On 09/03/18 16:35, julien.grall@xxxxxxx wrote:
> From: Andre Przywara <andre.przywara@xxxxxxxxxx>

I think this is quite different from what I ever wrote, so please drop
my authorship here.

> So far our LR read/write functions do not handle the EOI bit and the
> source CPUID bits in an LR, because the current VGIC implementation does
> not use them.
> Extend the gic_lr data structure to hold these bits of information by
> using a union to differentiate field used depending on whether the vIRQ
> has a corresponding pIRQ.

As mentioned before, I am not sure this is really necessary. The idea of
struct gic_lr is to provide a hardware agnostic view of an LR. So the
actual read_lr/write_lr function take care of reading/populating pINTID,
iff the HW bit is set (as done in your patch 5/6).
Given that, I don't think we need to change the current code in this
respect, as this is just a small internal interface.

But then again I don't care enough, so if that makes you happy ....

> Note that source is not covered by GICv3 LR.

I was thinking the same, but Marc pointed me to that inconspicuous note
on page 8-299 in ARM IHI 0069D (at the end of "vINTID", chapter 8.4.6).

> This allows the new VGIC to use this information.
> 
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> ---
>  xen/arch/arm/gic-v2.c     | 22 +++++++++++++++++++---
>  xen/arch/arm/gic-v3.c     | 11 +++++++++--
>  xen/include/asm-arm/gic.h | 16 ++++++++++++++--
>  3 files changed, 42 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index daf8c61258..69f8d6044a 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -474,8 +474,17 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
>  
>      if ( lr_reg->hw_status )
>      {
> -        lr_reg->pirq = lrv >> GICH_V2_LR_PHYSICAL_SHIFT;
> -        lr_reg->pirq &= GICH_V2_LR_PHYSICAL_MASK;
> +        lr_reg->h.pirq = lrv >> GICH_V2_LR_PHYSICAL_SHIFT;
> +        lr_reg->h.pirq &= GICH_V2_LR_PHYSICAL_MASK;
> +    }
> +    else
> +    {
> +        lr_reg->v.eoi = (lrv & GICH_V2_LR_MAINTENANCE_IRQ) == 
> GICH_V2_LR_MAINTENANCE_IRQ;

I think you can drop the " == GICH_V2_LR_MAINTENANCE_IRQ", as .eoi is a
bool. Avoids the linebreak.

> +        /*
> +         * This is only valid for SGI, but it does not matter to always
> +         * read it as it should be 0 by default.
> +         */
> +        lr_reg->v.source = (lrv >> GICH_V2_LR_CPUID_SHIFT) & 
> GICH_V2_LR_CPUID_MASK;
>      }
>  }
>  
> @@ -496,7 +505,14 @@ static void gicv2_write_lr(int lr, const struct gic_lr 
> *lr_reg)
>      if ( lr_reg->hw_status )
>      {
>          lrv |= GICH_V2_LR_HW;
> -        lrv |= lr_reg->pirq << GICH_V2_LR_PHYSICAL_SHIFT;
> +        lrv |= lr_reg->h.pirq << GICH_V2_LR_PHYSICAL_SHIFT;
> +    }
> +    else
> +    {
> +        if ( lr_reg->v.eoi )
> +            lrv |= GICH_V2_LR_MAINTENANCE_IRQ;
> +        if ( lr_reg->virq < NR_GIC_SGI )
> +            lrv |= (uint32_t)lr_reg->v.source << GICH_V2_LR_CPUID_SHIFT;
>      }
>  
>      writel_gich(lrv, GICH_LR + lr * 4);
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index f73d386df1..a855069111 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1014,7 +1014,9 @@ static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
>      lr_reg->hw_status = (lrv & ICH_LR_HW) == ICH_LR_HW;
>  
>      if ( lr_reg->hw_status )
> -        lr_reg->pirq = (lrv >> ICH_LR_PHYSICAL_SHIFT) & ICH_LR_PHYSICAL_MASK;
> +        lr_reg->h.pirq = (lrv >> ICH_LR_PHYSICAL_SHIFT) & 
> ICH_LR_PHYSICAL_MASK;
> +    else
> +        lr_reg->v.eoi = (lrv & ICH_LR_MAINTENANCE_IRQ) == 
> ICH_LR_MAINTENANCE_IRQ;

Same here.

>  }
>  
>  static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
> @@ -1033,7 +1035,12 @@ static void gicv3_write_lr(int lr_reg, const struct 
> gic_lr *lr)
>      if ( lr->hw_status )
>      {
>          lrv |= ICH_LR_HW;
> -        lrv |= (uint64_t)lr->pirq << ICH_LR_PHYSICAL_SHIFT;
> +        lrv |= (uint64_t)lr->h.pirq << ICH_LR_PHYSICAL_SHIFT;
> +    }
> +    else
> +    {
> +        if ( lr->v.eoi )
> +            lrv |= ICH_LR_MAINTENANCE_IRQ;
>      }
>  
>      /*
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 545901b120..4cf5bb385d 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -204,14 +204,26 @@ union gic_state_data {
>   * The LR register format is different for GIC HW version
>   */
>  struct gic_lr {
> -   /* Physical IRQ -> Only set when hw_status is set. */
> -   uint32_t pirq;
>     /* Virtual IRQ */
>     uint32_t virq;
>     uint8_t priority;
>     bool active;
>     bool pending;
>     bool hw_status;
> +   union
> +   {
> +       /* Only filled when there are a corresponding pIRQ (hw_state = true) 
> */
> +       struct
> +       {
> +           uint32_t pirq;
> +       } h;
> +       /* Only filled when there are no corresponding pIRQ (hw_state = 
> false) */
> +       struct
> +       {
> +           bool eoi;
> +           uint8_t source;      /* GICv2 only */
> +       } v;

That looks somewhat confusing to me. So either you use "hw" and "virt"
for the struct identifiers, or, preferably, just drop them altogether:

union {
        uint32_t pirq;  /* Contains the associated hardware IRQ. */
        struct          /* Only used for virtual interrupts. */
        {
                bool eoi;
                uint8_t source;
        };
};

Cheers,
Andre.

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