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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 24/10/2022 14:22, Bertrand Marquis wrote:
> Hi Ayan,
Hi Bertrand,
> > On 24 Oct 2022, at 14:11, Ayan Kumar Halder <ayankuma@xxxxxxx> wrote:
> >
> >
> > On 24/10/2022 11:06, Bertrand Marquis wrote:
> > > Hi Ayan,
> > Hi Bertrand,
> > > > 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.
> > Oh, my understanding from the comment "We do not implement security extensions for 
> > guests" is that Xen does not allow guests to run in secure mode.
> >
> > Also, does Xen itself ever run in secure mode ? I thought it was no.
> >
> >  From https://wiki.xenproject.org/wiki/Xen_ARM_with_Virtualization_Extensions
> >
> > "The primary requirement is that the hypervisor must be launched in Non-Secure 
> > Hypervisor mode only."
> For a long time there was no EL2 in secure mode so that was not even possible.
> This has been introduced in the past year but nobody ever tested that apart 
> from the work on R82 and R52.
>
> So for now, Xen must be launched in non secure mode, any other setup is 
> unsupported (might work though).
>
> > > 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.”
> > Do you mean ?
> >
> > " Xen does *not* support *guests* to run in secure mode so emulate all registers as 
> > the hardware does in non-secure."
> A guest is always running in the same mode as Xen.
>
> There is a question for guest running in secure mode when (if) Xen will run in 
> secure mode: what rights can we give to guest ?
>  From the theory point of view, it does not make sense for a guest to play with 
> the groups I think, as interrupt management is to be done by Xen.
>
> So I think it makes sense to say that those hardware registers are not 
> accessible to Xen guests as Xen will have to be the one programming interrupt 
> to be fired in secure or non secure world.
Many thanks for the explanation. It makes sense.

I have sent out "[XEN v3 01/13] GICv3: Emulate GICD_IGRPMODR as RAZ / 
WI" with the updated commit message.
- Ayan

> Cheers
> Bertrand
>
> > - Ayan
> >
> > > 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