Re: [XEN v2] GICv3: Emulate GICD_IGRPMODR as RAZ / WI

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Ayan,

> On 20 Oct 2022, at 11:41, Ayan Kumar Halder <ayankuma@xxxxxxx> wrote:
> 
> Refer GIC v3 specification (Arm IHI 0069H ID020922), IGRPMODR (similar to
> IGROUPR) is relevant only when the guests run in secure/non-secure mode.

This sentence is a bit misleading as guests are always running in either secure 
or non-secure.
We should just say that we do not want guest to change the group of interrupts 
so we do as if all guests are running in non-secure.

> As Xen does not implement security extensions for guests, so the registers
> are emulated as read as zero/write ignore.

I would rephrase this as “Xen does support to run in secure mode so emulate all 
registers as the hardware does in non-secure.”

On a side note, the question might come at some point if we support to run from 
secure mode on hardware supporting it, it could be that dom0 or Xen itself 
would need to modify those.

The code is ok, just the commit message would need a bit of rework I think.

Cheers
Bertrand

> 
> Signed-off-by: Ayan Kumar Halder <ayankuma@xxxxxxx>
> ---
> 
> Observed the issue while running Zephyr on R52.
> Also, found that KVM has similar behaviour.
> 
> Changes from:-
> v1 - Moved the definitions of GICD_IGRPMODR, GICD_IGRPMODRN to gic_v3
> specific header.
> 
> xen/arch/arm/include/asm/gic_v3_defs.h | 2 ++
> xen/arch/arm/vgic-v3.c                 | 4 ++++
> 2 files changed, 6 insertions(+)
> 
> diff --git a/xen/arch/arm/include/asm/gic_v3_defs.h 
> b/xen/arch/arm/include/asm/gic_v3_defs.h
> index 34ed5f857d..728e28d5e5 100644
> --- a/xen/arch/arm/include/asm/gic_v3_defs.h
> +++ b/xen/arch/arm/include/asm/gic_v3_defs.h
> @@ -30,6 +30,8 @@
> #define GICD_CLRSPI_NSR              (0x048)
> #define GICD_SETSPI_SR               (0x050)
> #define GICD_CLRSPI_SR               (0x058)
> +#define GICD_IGRPMODR                (0xD00)
> +#define GICD_IGRPMODRN               (0xD7C)
> #define GICD_IROUTER                 (0x6000)
> #define GICD_IROUTER32               (0x6100)
> #define GICD_IROUTER1019             (0x7FD8)
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 7fb99a9ff2..0c23f6df9d 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -685,6 +685,7 @@ static int __vgic_v3_distr_common_mmio_read(const char 
> *name, struct vcpu *v,
>     switch ( reg )
>     {
>     case VRANGE32(GICD_IGROUPR, GICD_IGROUPRN):
> +    case VRANGE32(GICD_IGRPMODR, GICD_IGRPMODRN):
>         /* We do not implement security extensions for guests, read zero */
>         if ( dabt.size != DABT_WORD ) goto bad_width;
>         goto read_as_zero;
> @@ -781,6 +782,7 @@ static int __vgic_v3_distr_common_mmio_write(const char 
> *name, struct vcpu *v,
>     switch ( reg )
>     {
>     case VRANGE32(GICD_IGROUPR, GICD_IGROUPRN):
> +    case VRANGE32(GICD_IGRPMODR, GICD_IGRPMODRN):
>         /* We do not implement security extensions for guests, write ignore */
>         goto write_ignore_32;
> 
> @@ -1192,6 +1194,7 @@ static int vgic_v3_distr_mmio_read(struct vcpu *v, 
> mmio_info_t *info,
>     case VRANGE32(GICD_ICACTIVER, GICD_ICACTIVERN):
>     case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
>     case VRANGE32(GICD_ICFGR, GICD_ICFGRN):
> +    case VRANGE32(GICD_IGRPMODR, GICD_IGRPMODRN):
>         /*
>          * Above all register are common with GICR and GICD
>          * Manage in common
> @@ -1379,6 +1382,7 @@ static int vgic_v3_distr_mmio_write(struct vcpu *v, 
> mmio_info_t *info,
>     case VRANGE32(GICD_ICACTIVER, GICD_ICACTIVERN):
>     case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
>     case VRANGE32(GICD_ICFGR, GICD_ICFGRN):
> +    case VRANGE32(GICD_IGRPMODR, GICD_IGRPMODRN):
>         /* Above registers are common with GICR and GICD
>          * Manage in common */
>         return __vgic_v3_distr_common_mmio_write("vGICD", v, info,
> -- 
> 2.17.1
>