Re: [Xen-devel] [PATCH 2/6] xen/arm: vgic: Override the group in lr everytime

[Andre Przywara <andre.przywara@xxxxxxx>]

On 09/03/18 17:35, julien.grall@xxxxxxx wrote:
> From: Julien Grall <julien.grall@xxxxxxx>
> 
> At the moment, write_lr is assuming the caller will set correctly the
> group. However the group should always be 0 when the guest is using
> vGICv2 and 1 for vGICv3. As the caller should not care about the group,
> override it directly.

I think that makes sense, mostly because this is what KVM does as well.
And it's a good idea to do this in write_lr().

I understand that this is effectively what I did in the new VGIC, but it
would be good to double check that this is the right thing to do
for the old VGIC as well. Did you test this on some GICv3 h/w or the
model? Or do we always call update_lr() anyway?

Cheers,
Andre.

> With that change, write_lr is now behaving like update_lr for the group.
> 
> Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> ---
>  xen/arch/arm/gic-v2.c     |  4 +---
>  xen/arch/arm/gic-v3.c     | 11 ++++++++---
>  xen/include/asm-arm/gic.h |  1 -
>  3 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index f16e17c1a3..fc105c08b8 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -469,7 +469,6 @@ static void gicv2_read_lr(int lr, struct gic_lr *lr_reg)
>      lr_reg->priority = (lrv >> GICH_V2_LR_PRIORITY_SHIFT) & 
> GICH_V2_LR_PRIORITY_MASK;
>      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;
>  }
>  
>  static void gicv2_write_lr(int lr, const struct gic_lr *lr_reg)
> @@ -483,8 +482,7 @@ static void gicv2_write_lr(int lr, const struct gic_lr 
> *lr_reg)
>            ((uint32_t)(lr_reg->state & GICH_V2_LR_STATE_MASK)
>                                     << GICH_V2_LR_STATE_SHIFT) |
>            ((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) );
> +                                       << GICH_V2_LR_HW_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 09b49a07d5..0dfa1a1e08 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -1012,7 +1012,6 @@ static void gicv3_read_lr(int lr, struct gic_lr *lr_reg)
>      lr_reg->priority  = (lrv >> ICH_LR_PRIORITY_SHIFT) & 
> ICH_LR_PRIORITY_MASK;
>      lr_reg->state     = (lrv >> ICH_LR_STATE_SHIFT) & ICH_LR_STATE_MASK;
>      lr_reg->hw_status = (lrv >> ICH_LR_HW_SHIFT) & ICH_LR_HW_MASK;
> -    lr_reg->grp       = (lrv >> ICH_LR_GRP_SHIFT) & ICH_LR_GRP_MASK;
>  }
>  
>  static void gicv3_write_lr(int lr_reg, const struct gic_lr *lr)
> @@ -1023,8 +1022,14 @@ static void gicv3_write_lr(int lr_reg, const struct 
> gic_lr *lr)
>          ((u64)(lr->virq & ICH_LR_VIRTUAL_MASK)  << ICH_LR_VIRTUAL_SHIFT) |
>          ((u64)(lr->priority & ICH_LR_PRIORITY_MASK) << 
> ICH_LR_PRIORITY_SHIFT)|
>          ((u64)(lr->state & ICH_LR_STATE_MASK) << ICH_LR_STATE_SHIFT) |
> -        ((u64)(lr->hw_status & ICH_LR_HW_MASK) << ICH_LR_HW_SHIFT)  |
> -        ((u64)(lr->grp & ICH_LR_GRP_MASK) << ICH_LR_GRP_SHIFT) );
> +        ((u64)(lr->hw_status & ICH_LR_HW_MASK) << ICH_LR_HW_SHIFT) );
> +
> +    /*
> +     * When the guest is using vGICv3, all the IRQs are Group 1. Group 0
> +     * would result in a FIQ, which will not be expected by the guest OS.
> +     */
> +    if ( current->domain->arch.vgic.version == GIC_V3 )
> +        lrv |= ICH_LR_GRP1;
>  
>      gicv3_ich_write_lr(lr_reg, lrv);
>  }
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 49cb94f792..1eb08b856e 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -211,7 +211,6 @@ struct gic_lr {
>     uint8_t priority;
>     uint8_t state;
>     uint8_t hw_status;
> -   uint8_t grp;
>  };
>  
>  enum gic_version {
> 


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