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

From: Julien Grall <julien.grall@arm.com>

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.

With that change, write_lr is now behaving like update_lr for the group.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 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(-)

On 09/03/18 17:35, julien.grall@arm.com wrote:
> From: Julien Grall <julien.grall@arm.com>
> 
> 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@arm.com>
> ---
>  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 {
>

Hi Andre,

On 03/14/2018 06:02 PM, Andre Przywara wrote:
> On 09/03/18 17:35, julien.grall@arm.com wrote:
>> From: Julien Grall <julien.grall@arm.com>
>>
>> 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?

I had a patch to remove update_lr and use write_lr. I exercised the code 
with it but didn't send it because of some nasty bug with the priority 
in write_lr and the old vGIC.

On the old vGIC write_lr is only used when in gic_update_one_lr(...) 
combine with read_lr before. In that context we don't care about the 
group, so overwriting it is fine and the right thing to do.

Cheers,