Re: [PATCH v2 12/12] hw/intc: add implementation of GICD_IIDR to Arm GIC

[Peter Maydell]

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