On Fri, 6 Mar 2026 at 16:16, Jonathan Cameron <[email protected]> wrote: > > On Mon, 23 Feb 2026 17:01:16 +0000 > Peter Maydell <[email protected]> wrote: > > > Implement the IRS frame ID registers IRS_IDR[0-7], IRS_IIDR and > > IRS_AIDR. These are all 32-bit registers. > > > > We make these fields in the GIC state struct rather than just > > hardcoding them in the register read function so that we can later > > code "do this only if X is implemented" as a test on the ID register > > value. > > > > Signed-off-by: Peter Maydell <[email protected]> > > --- > > hw/intc/arm_gicv5.c | 112 +++++++++++++++++++++++++++++ > > include/hw/intc/arm_gicv5_common.h | 39 ++++++++++ > > 2 files changed, 151 insertions(+) > > > > diff --git a/hw/intc/arm_gicv5.c b/hw/intc/arm_gicv5.c > > index db754e7681..f34bb81966 100644 > > --- a/hw/intc/arm_gicv5.c > > +++ b/hw/intc/arm_gicv5.c > > @@ -268,6 +268,62 @@ REG64(IRS_SWERR_SYNDROMER1, 0x3d0) > > static bool config_readl(GICv5 *s, GICv5Domain domain, hwaddr offset, > > uint64_t *data, MemTxAttrs attrs) > > { > > + GICv5Common *cs = ARM_GICV5_COMMON(s); > > + uint32_t v = 0; > > + > > + switch (offset) { > > + case A_IRS_IDR0: > > + v = cs->irs_idr0; > > + /* INT_DOM reports the domain this register is for */ > > + v = FIELD_DP32(v, IRS_IDR0, INT_DOM, domain); > > + if (domain != GICV5_ID_REALM) { > > + /* MEC field RES0 except for the Realm domain */ > > + v &= ~R_IRS_IDR0_MEC_MASK; > > + } > > + if (domain == GICV5_ID_EL3) { > > + /* VIRT is RES0 for EL3 domain */ > > + v &= ~R_IRS_IDR0_VIRT_MASK; > > + } > > There are some more complex RES0 conditions that kind of build > off these, like VIRT_ONE_N is RES0 if VIRT is 0, including > I think if VIRT is RES0 as a result of the above. That particular > condition is perhaps worth encoding in here as you can see we may > have it implemented for everything other than EL3.
As it happens, I have no intention of implementing 1ofN support. But yes, for consistency we should mask it out to follow a rule of "mask stuff that will appear differently in the view of this register in the different register frames". > This is similar to what you do for the whole of IDR3. > > > + return true; > > + > > + case A_IRS_IDR1: > > + *data = cs->irs_idr1; > > + return true; > > + > > + case A_IRS_IDR2: > > + *data = cs->irs_idr2; > > + return true; > > + > > + case A_IRS_IDR3: > > + /* In EL3 IDR0.VIRT is 0 so this is RES0 */ > > + *data = domain == GICV5_ID_EL3 ? 0 : cs->irs_idr3; > > The spec has that condition on a field by field bases. I wonder > if it would be clearer to mask out each field rather than set > whole thing to 0. I think that the whole IDR3 register is clearly deliberately "things related to virtualization" and it's clearer to zero it all at once rather than have multiple lines and leave the reader wondering if any fields don't get zeroed. > > + return true; > > + > > + case A_IRS_IDR4: > > + /* In EL3 IDR0.VIRT is 0 so this is RES0 */ > > + *data = domain == GICV5_ID_EL3 ? 0 : cs->irs_idr4; > Similar for this one. Most of register is currently res0 and > we don't know if those bits will be used later for stuff that > isn't dependent on IRS_IDR0.VIRT being 1. There's a lot of spare space in the register offset map after IDR7 and before the next register, so I imagine the intention is to add more ID registers if there's a future need for more ID information that's not related to the existing fields in any ID register. thanks -- PMM
