On Fri, 11 Nov 2022 at 14:55, Alex Bennée <alex.ben...@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.ben...@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