Re: [RFC PATCH 4/4] hw/intc: make gicv3_idreg() distinguish between gicv3/gicv4

[Leif Lindholm]

On Tue, Feb 02, 2021 at 10:31:16 +, Peter Maydell wrote:
> On Sun, 24 Jan 2021 at 02:53, Leif Lindholm  wrote:
> >
> > Make gicv3_idreg() able to return either gicv3 or gicv4 data.
> > Add a parameter to specify gic version.
> >
> > Signed-off-by: Leif Lindholm 
> > ---
> >  hw/intc/arm_gicv3_dist.c   |  2 +-
> >  hw/intc/arm_gicv3_redist.c |  2 +-
> >  hw/intc/gicv3_internal.h   | 12 ++--
> >  3 files changed, 12 insertions(+), 4 deletions(-)
> 
> > -static inline uint32_t gicv3_idreg(int regoffset)
> > +static inline uint32_t gicv3_idreg(int regoffset, int revision)
> 
> I would prefer to pass in the GICv3State* and let the function
> look at s->revision.

Yeah, that'd be neater.

> >  {
> >  /* Return the value of the CoreSight ID register at the specified
> >   * offset from the first ID register (as found in the distributor
> > @@ -331,7 +331,15 @@ static inline uint32_t gicv3_idreg(int regoffset)
> >  static const uint8_t gicd_ids[] = {
> >  0x44, 0x00, 0x00, 0x00, 0x92, 0xB4, 0x3B, 0x00, 0x0D, 0xF0, 0x05, 
> > 0xB1
> >  };
> > -return gicd_ids[regoffset / 4];
> > +static const uint8_t gicdv4_ids[] = {
> > +0x44, 0x00, 0x00, 0x00, 0x92, 0xB4, 0x4B, 0x00, 0x0D, 0xF0, 0x05, 
> > 0xB1
> > +};
> > +
> > +if (revision == 3) {
> > +return gicd_ids[regoffset / 4];
> > +} else {
> > +return gicdv4_ids[regoffset / 4];
> > +}
> >  }
> 
> Updating the comment "These values indicate an ARM implementation of a GICv3"
> to add a note about what the new values are indicating would be nice.

Will do.

Regards,

Leif

Re: [RFC PATCH 4/4] hw/intc: make gicv3_idreg() distinguish between gicv3/gicv4

[Peter Maydell]

On Sun, 24 Jan 2021 at 02:53, Leif Lindholm  wrote:
>
> Make gicv3_idreg() able to return either gicv3 or gicv4 data.
> Add a parameter to specify gic version.
>
> Signed-off-by: Leif Lindholm 
> ---
>  hw/intc/arm_gicv3_dist.c   |  2 +-
>  hw/intc/arm_gicv3_redist.c |  2 +-
>  hw/intc/gicv3_internal.h   | 12 ++--
>  3 files changed, 12 insertions(+), 4 deletions(-)

> -static inline uint32_t gicv3_idreg(int regoffset)
> +static inline uint32_t gicv3_idreg(int regoffset, int revision)

I would prefer to pass in the GICv3State* and let the function
look at s->revision.

>  {
>  /* Return the value of the CoreSight ID register at the specified
>   * offset from the first ID register (as found in the distributor
> @@ -331,7 +331,15 @@ static inline uint32_t gicv3_idreg(int regoffset)
>  static const uint8_t gicd_ids[] = {
>  0x44, 0x00, 0x00, 0x00, 0x92, 0xB4, 0x3B, 0x00, 0x0D, 0xF0, 0x05, 
> 0xB1
>  };
> -return gicd_ids[regoffset / 4];
> +static const uint8_t gicdv4_ids[] = {
> +0x44, 0x00, 0x00, 0x00, 0x92, 0xB4, 0x4B, 0x00, 0x0D, 0xF0, 0x05, 
> 0xB1
> +};
> +
> +if (revision == 3) {
> +return gicd_ids[regoffset / 4];
> +} else {
> +return gicdv4_ids[regoffset / 4];
> +}
>  }

Updating the comment "These values indicate an ARM implementation of a GICv3"
to add a note about what the new values are indicating would be nice.

thanks
-- PMM

[RFC PATCH 4/4] hw/intc: make gicv3_idreg() distinguish between gicv3/gicv4

[Leif Lindholm]

Make gicv3_idreg() able to return either gicv3 or gicv4 data.
Add a parameter to specify gic version.

Signed-off-by: Leif Lindholm 
---
 hw/intc/arm_gicv3_dist.c   |  2 +-
 hw/intc/arm_gicv3_redist.c |  2 +-
 hw/intc/gicv3_internal.h   | 12 ++--
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/hw/intc/arm_gicv3_dist.c b/hw/intc/arm_gicv3_dist.c
index 833deb0a74..d32a1d5f48 100644
--- a/hw/intc/arm_gicv3_dist.c
+++ b/hw/intc/arm_gicv3_dist.c
@@ -544,7 +544,7 @@ static MemTxResult gicd_readl(GICv3State *s, hwaddr offset,
 }
 case GICD_IDREGS ... GICD_IDREGS + 0x2f:
 /* ID registers */
-*data = gicv3_idreg(offset - GICD_IDREGS);
+*data = gicv3_idreg(offset - GICD_IDREGS, s->revision);
 return MEMTX_OK;
 case GICD_SGIR:
 /* WO registers, return unknown value */
diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c
index 544f4d82ff..faa68c9a71 100644
--- a/hw/intc/arm_gicv3_redist.c
+++ b/hw/intc/arm_gicv3_redist.c
@@ -239,7 +239,7 @@ static MemTxResult gicr_readl(GICv3CPUState *cs, hwaddr 
offset,
 *data = cs->gicr_nsacr;
 return MEMTX_OK;
 case GICR_IDREGS ... GICR_IDREGS + 0x2f:
-*data = gicv3_idreg(offset - GICR_IDREGS);
+*data = gicv3_idreg(offset - GICR_IDREGS, cs->gic->revision);
 return MEMTX_OK;
 default:
 return MEMTX_ERROR;
diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h
index 05303a55c8..ded2df66eb 100644
--- a/hw/intc/gicv3_internal.h
+++ b/hw/intc/gicv3_internal.h
@@ -321,7 +321,7 @@ static inline uint32_t gicv3_iidr(void)
 return 0x43b;
 }
 
-static inline uint32_t gicv3_idreg(int regoffset)
+static inline uint32_t gicv3_idreg(int regoffset, int revision)
 {
 /* Return the value of the CoreSight ID register at the specified
  * offset from the first ID register (as found in the distributor
@@ -331,7 +331,15 @@ static inline uint32_t gicv3_idreg(int regoffset)
 static const uint8_t gicd_ids[] = {
 0x44, 0x00, 0x00, 0x00, 0x92, 0xB4, 0x3B, 0x00, 0x0D, 0xF0, 0x05, 0xB1
 };
-return gicd_ids[regoffset / 4];
+static const uint8_t gicdv4_ids[] = {
+0x44, 0x00, 0x00, 0x00, 0x92, 0xB4, 0x4B, 0x00, 0x0D, 0xF0, 0x05, 0xB1
+};
+
+if (revision == 3) {
+return gicd_ids[regoffset / 4];
+} else {
+return gicdv4_ids[regoffset / 4];
+}
 }
 
 /**
-- 
2.20.1