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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Ayan,

> 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.

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
>>>