Re: [Xen-devel] [PATCH 5/6] xen/arm: GIC: Only set pirq in the LR when hw_status is set

[Andre Przywara <andre.przywara@xxxxxxx>]

Hi,

On 09/03/18 16:35, julien.grall@xxxxxxx wrote:
> From: Julien Grall <julien.grall@xxxxxxx>
> 
> The field pirq should only be valid when the virtual interrupt
> is associated to a physical interrupt.
> 
> This change will help to extend gic_lr for supporting specific virtual
> interrupt field (e.g eoi, source) that clashes with the PIRQ field.

Makes some sense, yes.

> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>

Reviewed-by: Andre Przywara <andre.przywara@xxxxxxx>

Cheers,
Andre.

> ---
>  xen/arch/arm/gic-v2.c     | 13 ++++++++++---
>  xen/arch/arm/gic-v3.c     | 10 +++++++---
>  xen/include/asm-arm/gic.h |  2 +-
>  3 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 90d8f652d3..daf8c61258 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -466,20 +466,24 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
>      uint32_t lrv;
>  
>      lrv          = readl_gich(GICH_LR + lr * 4);
> -    lr_reg->pirq = (lrv >> GICH_V2_LR_PHYSICAL_SHIFT) & 
> GICH_V2_LR_PHYSICAL_MASK;
>      lr_reg->virq = (lrv >> GICH_V2_LR_VIRTUAL_SHIFT) & 
> GICH_V2_LR_VIRTUAL_MASK;
>      lr_reg->priority = (lrv >> GICH_V2_LR_PRIORITY_SHIFT) & 
> GICH_V2_LR_PRIORITY_MASK;
>      lr_reg->pending = (lrv & GICH_V2_LR_PENDING) == GICH_V2_LR_PENDING;
>      lr_reg->active = (lrv & GICH_V2_LR_ACTIVE) == GICH_V2_LR_ACTIVE;
>      lr_reg->hw_status = (lrv & GICH_V2_LR_HW) == GICH_V2_LR_HW;
> +
> +    if ( lr_reg->hw_status )
> +    {
> +        lr_reg->pirq = lrv >> GICH_V2_LR_PHYSICAL_SHIFT;
> +        lr_reg->pirq &= GICH_V2_LR_PHYSICAL_MASK;
> +    }
>  }
>  
>  static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
>  {
>      uint32_t lrv = 0;
>  
> -    lrv = ( ((lr_reg->pirq & GICH_V2_LR_PHYSICAL_MASK) << 
> GICH_V2_LR_PHYSICAL_SHIFT) |
> -          ((lr_reg->virq & GICH_V2_LR_VIRTUAL_MASK) << 
> GICH_V2_LR_VIRTUAL_SHIFT)   |
> +    lrv = (((lr_reg->virq & GICH_V2_LR_VIRTUAL_MASK) << 
> GICH_V2_LR_VIRTUAL_SHIFT)   |
>            ((uint32_t)(lr_reg->priority & GICH_V2_LR_PRIORITY_MASK)
>                                        << GICH_V2_LR_PRIORITY_SHIFT) );
>  
> @@ -490,7 +494,10 @@ static void gicv2_write_lr(int lr, const struct gic_lr 
> *lr_reg)
>          lrv |= GICH_V2_LR_PENDING;
>  
>      if ( lr_reg->hw_status )
> +    {
>          lrv |= GICH_V2_LR_HW;
> +        lrv |= lr_reg->pirq << GICH_V2_LR_PHYSICAL_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 4dbbf0afd2..f73d386df1 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1006,21 +1006,22 @@ static void gicv3_read_lr(int lr, struct gic_lr 
> *lr_reg)
>  
>      lrv = gicv3_ich_read_lr(lr);
>  
> -    lr_reg->pirq = (lrv >> ICH_LR_PHYSICAL_SHIFT) & ICH_LR_PHYSICAL_MASK;
>      lr_reg->virq = (lrv >> ICH_LR_VIRTUAL_SHIFT) & ICH_LR_VIRTUAL_MASK;
>  
>      lr_reg->priority  = (lrv >> ICH_LR_PRIORITY_SHIFT) & 
> ICH_LR_PRIORITY_MASK;
>      lr_reg->pending   = (lrv & ICH_LR_STATE_PENDING) == ICH_LR_STATE_PENDING;
>      lr_reg->active    = (lrv & ICH_LR_STATE_ACTIVE) == ICH_LR_STATE_ACTIVE;
>      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;
>  }
>  
>  static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
>  {
>      uint64_t lrv = 0;
>  
> -    lrv = ( ((u64)(lr->pirq & ICH_LR_PHYSICAL_MASK) << 
> ICH_LR_PHYSICAL_SHIFT)|
> -        ((u64)(lr->virq & ICH_LR_VIRTUAL_MASK)  << ICH_LR_VIRTUAL_SHIFT) |
> +    lrv = ( ((u64)(lr->virq & ICH_LR_VIRTUAL_MASK)  << ICH_LR_VIRTUAL_SHIFT) 
> |
>          ((u64)(lr->priority & ICH_LR_PRIORITY_MASK) << 
> ICH_LR_PRIORITY_SHIFT) );
>  
>      if ( lr->active )
> @@ -1030,7 +1031,10 @@ static void gicv3_write_lr(int lr_reg, const struct 
> gic_lr *lr)
>          lrv |= ICH_LR_STATE_PENDING;
>  
>      if ( lr->hw_status )
> +    {
>          lrv |= ICH_LR_HW;
> +        lrv |= (uint64_t)lr->pirq << ICH_LR_PHYSICAL_SHIFT;
> +    }
>  
>      /*
>       * When the guest is using vGICv3, all the IRQs are Group 1. Group 0
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index c32861d4fa..545901b120 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -204,7 +204,7 @@ union gic_state_data {
>   * The LR register format is different for GIC HW version
>   */
>  struct gic_lr {
> -   /* Physical IRQ */
> +   /* Physical IRQ -> Only set when hw_status is set. */
>     uint32_t pirq;
>     /* Virtual IRQ */
>     uint32_t virq;
> 

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