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.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - checkpatch fixes.
v3
  - re-base on re-flow with if
v4
  - fix the commit message
---
 hw/intc/arm_gic.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 1a04144c38..7a34bc0998 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -973,8 +973,18 @@ static uint8_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs)
             /* GICD_TYPER byte 1 */
             return (s->security_extn << 2);
         }
-        if (offset < 0x08)
+        if (offset == 8) {
+            /* GICD_IIDR byte 0 */
+            return 0x3b; /* Arm JEP106 identity */
+        }
+        if (offset == 9) {
+            /* GICD_IIDR byte 1 */
+            return 0x04; /* Arm JEP106 identity */
+        }
+        if (offset < 0x0c) {
+            /* All other bytes in this range are RAZ */
             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
-- 
2.34.1

On 15/11/22 17:17, Alex Bennée 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.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v2
>    - checkpatch fixes.
> v3
>    - re-base on re-flow with if
> v4
>    - fix the commit message
> ---
>   hw/intc/arm_gic.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 1a04144c38..7a34bc0998 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -973,8 +973,18 @@ static uint8_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs)
>               /* GICD_TYPER byte 1 */
>               return (s->security_extn << 2);
>           }
> -        if (offset < 0x08)
> +        if (offset == 8) {
> +            /* GICD_IIDR byte 0 */
> +            return 0x3b; /* Arm JEP106 identity */
> +        }
> +        if (offset == 9) {
> +            /* GICD_IIDR byte 1 */
> +            return 0x04; /* Arm JEP106 identity */

Possible future cleanup, define JEP106_ID_ARM:

$ git grep 0x43b
hw/intc/arm_gic.c:1671:            *data = (s->revision << 16) | 0x43b;
hw/intc/gicv3_internal.h:743:    return 0x43b;
hw/misc/armv7m_ras.c:26:        *data = 0x43b;

> +        }
> +        if (offset < 0x0c) {
> +            /* All other bytes in this range are RAZ */
>               return 0;
> +        }

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

On Tue, 15 Nov 2022 at 16:29, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> Possible future cleanup, define JEP106_ID_ARM:
>
> $ git grep 0x43b
> hw/intc/arm_gic.c:1671:            *data = (s->revision << 16) | 0x43b;
> hw/intc/gicv3_internal.h:743:    return 0x43b;
> hw/misc/armv7m_ras.c:26:        *data = 0x43b;

I think that's probably not worthwhile, because the datasheet gives
you the hex value of the ID register for the device, and if we
hide that behind a #define then now to check that the value is
right you need to go look at the #define line.

Put another way, the important thing is "this register holds the
value 0x43b", and it's just a point of information that that happens
to correspond to a JEP-106 manufacturer ID code value.

thanks
-- PMM