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

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

On 05/03/18 16:03, Andre Przywara wrote:
> 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 as
> well, packing it on the way to avoid it to grow.
Not sure if it matter that much as you will always allocate gic_lr on 
the stack.
> Then extract and assemble those bits from/to an LR.
>
> This allows the new VGIC to use this information.
>
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
> ---
> Changelog RFC ... v1:
> - new patch
>
>   xen/arch/arm/gic-v2.c     | 7 +++++++
>   xen/include/asm-arm/gic.h | 8 +++++---
>   2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 031be920cc..c5ec0d4d35 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -470,6 +470,9 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
>       lr_reg->state     = (lrv >> GICH_V2_LR_STATE_SHIFT) & 
> GICH_V2_LR_STATE_MASK;
>       lr_reg->hw_status = (lrv >> GICH_V2_LR_HW_SHIFT) & GICH_V2_LR_HW_MASK;
>       lr_reg->grp       = (lrv >> GICH_V2_LR_GRP_SHIFT) & GICH_V2_LR_GRP_MASK;
> +    lr_reg->eoi       = !!(lrv & GICH_V2_LR_MAINTENANCE_IRQ);
If you turn eoi to bool eoi:1, you can drop !!.

> +    if ( lr_reg->virq < NR_GIC_SGI )
> +        lr_reg->source = (lrv >> GICH_V2_LR_CPUID_SHIFT) & 
> GICH_V2_LR_CPUID_MASK;
>   }
>   
>   static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
> @@ -485,6 +488,10 @@ static void gicv2_write_lr(int lr, const struct gic_lr 
> *lr_reg)
>             ((uint32_t)(lr_reg->hw_status & GICH_V2_LR_HW_MASK)
>                                          << GICH_V2_LR_HW_SHIFT)  |
>             ((uint32_t)(lr_reg->grp & GICH_V2_LR_GRP_MASK) << 
> GICH_V2_LR_GRP_SHIFT) );
> +    if ( lr_reg->eoi )
> +        lrv |= GICH_V2_LR_MAINTENANCE_IRQ;
> +    if ( lr_reg->virq < NR_GIC_SGI )
> +        lrv |= (uint32_t)lr_reg->source << GICH_V2_LR_CPUID_SHIFT;
>   
>       writel_gich(lrv, GICH_LR + lr * 4);
>   }
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 8fab458d7f..89a07ae6b4 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -223,9 +223,11 @@ struct gic_lr {
>      /* Virtual IRQ */
>      uint32_t virq;
>      uint8_t priority;
> -   uint8_t state;
> -   uint8_t hw_status;
> -   uint8_t grp;
> +   uint8_t source;
> +   uint8_t state:2;
> +   uint8_t hw_status:1;
> +   uint8_t grp:1;
> +   uint8_t eoi:1;
I would much prefer to introduce an union with specific information for 
the physical interrupt (i.e GICH_LR.HW = 1) in one side and purely 
virtual in the other side.
That would also help to understand the purpose of each field without 
looking at the write_lr callback details.
Also, please mention that source is GICv2 only.

Cheers,

--
Julien Grall

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