| Message ID | 20221111145529.4020801-13-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | testing, docs, plugins, arm pre-PR | expand |
On Fri, 11 Nov 2022 at 14:55, Alex Bennée <alex.bennee@linaro.org> wrote: > > a66a24585f (hw/intc/arm_gic: Implement read of GICC_IIDR) implemented > this for the CPU interface register. The fact we don't implement it > shows up when running Xen with -d guest_error which is definitely > wrong because the guest is perfectly entitled to read it. > > Lightly re-factor this region of registers and also add a comment to > the function in case anyway was under the illusion we only return > bytes from a function called readb. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > > --- > v2 > - checkpatch fixes. > --- > hw/intc/arm_gic.c | 44 ++++++++++++++++++++++++++++++-------------- > 1 file changed, 30 insertions(+), 14 deletions(-) > > diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c > index 492b2421ab..65b1ef7151 100644 > --- a/hw/intc/arm_gic.c > +++ b/hw/intc/arm_gic.c > @@ -941,6 +941,10 @@ static void gic_complete_irq(GICState *s, int cpu, int irq, MemTxAttrs attrs) > gic_update(s); > } > > +/* > + * Although this is named a byte read we don't always return bytes and > + * rely on the calling function oring bits together. > + */ Rather than documenting this, maybe it would be better to fix the weirdness? We only do this for exactly one register, the GICD_TYPER. Everything else is naturally byte-based. (The GICD_CTLR looks like it is also doing this, but the only non-zero bits are in the low byte, so it isn't really.) The GICD_TYPER returning bigger than a byte's worth of data I think is a bug we introduced in commit 5543d1abb6e2 when we added the security extensions support -- before that all the bits we needed to return were in the low byte. So I think we can fix this with just (untested): --- a/hw/intc/arm_gic.c +++ b/hw/intc/arm_gic.c @@ -955,6 +955,7 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs) cm = 1 << cpu; if (offset < 0x100) { if (offset == 0) { /* GICD_CTLR */ + /* We rely here on the only non-zero bits being in byte 0 */ if (s->security_extn && !attrs.secure) { /* The NS bank of this register is just an alias of the * EnableGrp1 bit in the S bank version. @@ -964,11 +965,14 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs) return s->ctlr; } } - if (offset == 4) - /* Interrupt Controller Type Register */ - return ((s->num_irq / 32) - 1) - | ((s->num_cpu - 1) << 5) - | (s->security_extn << 10); + if (offset == 4) { + /* GICD_TYPER byte 0 */ + return ((s->num_irq / 32) - 1) | ((s->num_cpu - 1) << 5); + } + if (offset == 5) { + /* GICD_TYPER byte 1 */ + return (s->security_extn << 2); + } if (offset < 0x08) return 0; if (offset >= 0x80) { (you can add my Signed-off-by: if you want to turn that into a proper patch.) thanks -- PMM
diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c index 492b2421ab..65b1ef7151 100644 --- a/hw/intc/arm_gic.c +++ b/hw/intc/arm_gic.c @@ -941,6 +941,10 @@ static void gic_complete_irq(GICState *s, int cpu, int irq, MemTxAttrs attrs) gic_update(s); } +/* + * Although this is named a byte read we don't always return bytes and + * rely on the calling function oring bits together. + */ static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs) { GICState *s = (GICState *)opaque; @@ -954,23 +958,35 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs) cpu = gic_get_current_cpu(s); cm = 1 << cpu; if (offset < 0x100) { - if (offset == 0) { /* GICD_CTLR */ - if (s->security_extn && !attrs.secure) { - /* The NS bank of this register is just an alias of the - * EnableGrp1 bit in the S bank version. - */ - return extract32(s->ctlr, 1, 1); - } else { - return s->ctlr; + if (offset < 0xc) { + switch (offset) { + case 0: /* GICD_CTLR[7:0] */ + { + if (s->security_extn && !attrs.secure) { + /* + * The NS bank of this register is just an alias of the + * EnableGrp1 bit in the S bank version. + */ + return extract32(s->ctlr, 1, 1); + } else { + return s->ctlr; + } } - } - if (offset == 4) - /* Interrupt Controller Type Register */ - return ((s->num_irq / 32) - 1) + case 4: /* GIC_TYPER - Interrupt Controller Type Register */ + { + return ((s->num_irq / 32) - 1) | ((s->num_cpu - 1) << 5) | (s->security_extn << 10); - if (offset < 0x08) - return 0; + } + case 8: /* GICD_IIDR - Implementer ID Register */ + { + return 0x43b; /* Arm JEP106 identity */ + } + default: + /* return 0 for high bits of above */ + return 0; + } + } if (offset >= 0x80) { /* Interrupt Group Registers: these RAZ/WI if this is an NS * access to a GIC with the security extensions, or if the GIC
a66a24585f (hw/intc/arm_gic: Implement read of GICC_IIDR) implemented this for the CPU interface register. The fact we don't implement it shows up when running Xen with -d guest_error which is definitely wrong because the guest is perfectly entitled to read it. Lightly re-factor this region of registers and also add a comment to the function in case anyway was under the illusion we only return bytes from a function called readb. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- v2 - checkpatch fixes. --- hw/intc/arm_gic.c | 44 ++++++++++++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 14 deletions(-)