[RFC PATCH 0/4] hw/intc: enable GICv4 memory layout for GICv3 driver

[RFC PATCH 0/4] hw/intc: enable GICv4 memory layout for GICv3 driver

[Leif Lindholm]

GICv4 sets aside 256K per redistributor configuration block, whereas GICv3
only uses 128K. However, some codebases (like TF-A, EDK2) will happily use
the GICv3 functionality only.

This set aims at enabling these codebases to run, without actually enabling
full support for GICv4. 

This creates a ... problematic ... system, which will misbehave if you try
to use the virtual LPIs. But it does help with letting me use QEMU for
modelling a platform containing a GICv4, and share firmware images with
other prototyping platforms.

Leif Lindholm (4):
  hw/intc: don't bail out gicv3 model init for revision 4
  hw/intc: add helper function to determine gicv3 redistributor size
  hw/intc: set GICD_TYPER.DVIS for GICv4
  hw/intc: make gicv3_idreg() distinguish between gicv3/gicv4

 hw/intc/arm_gicv3_common.c         |  4 ++--
 hw/intc/arm_gicv3_dist.c           |  5 ++++-
 hw/intc/arm_gicv3_redist.c         | 15 ++++++++++-----
 hw/intc/gicv3_internal.h           | 12 ++++++++++--
 include/hw/intc/arm_gicv3_common.h |  3 +++
 5 files changed, 29 insertions(+), 10 deletions(-)

-- 
2.20.1

[RFC PATCH 1/4] hw/intc: don't bail out gicv3 model init for revision 4

[Leif Lindholm]

As a first step towards GICv4 compatibility, add support for gic revision 4
to GICv3 driver (i.e. don't bail out if revision 4 is encountered).

Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
 hw/intc/arm_gicv3_common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
index 58ef65f589..7365d24873 100644
--- a/hw/intc/arm_gicv3_common.c
+++ b/hw/intc/arm_gicv3_common.c
@@ -315,7 +315,7 @@ static void arm_gicv3_common_realize(DeviceState *dev, Error **errp)
      * conditions. However, in future it could be used, for example, if we
      * implement GICv4.
      */
-    if (s->revision != 3) {
+    if (s->revision != 3 && s->revision != 4) {
         error_setg(errp, "unsupported GIC revision %d", s->revision);
         return;
     }
-- 
2.20.1

Re: [RFC PATCH 1/4] hw/intc: don't bail out gicv3 model init for revision 4

[Peter Maydell]

On Sun, 24 Jan 2021 at 02:53, Leif Lindholm <leif@nuviainc.com> wrote:
>
> As a first step towards GICv4 compatibility, add support for gic revision 4
> to GICv3 driver (i.e. don't bail out if revision 4 is encountered).
>
> Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> ---
>  hw/intc/arm_gicv3_common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
> index 58ef65f589..7365d24873 100644
> --- a/hw/intc/arm_gicv3_common.c
> +++ b/hw/intc/arm_gicv3_common.c
> @@ -315,7 +315,7 @@ static void arm_gicv3_common_realize(DeviceState *dev, Error **errp)
>       * conditions. However, in future it could be used, for example, if we
>       * implement GICv4.
>       */
> -    if (s->revision != 3) {
> +    if (s->revision != 3 && s->revision != 4) {
>          error_setg(errp, "unsupported GIC revision %d", s->revision);
>          return;
>      }

This change is obviously not-for-upstream; we can't allow the GIC
to be configured to a revision that we don't actually implement
correctly.

thanks
-- PMM

[RFC PATCH 2/4] hw/intc: add helper function to determine gicv3 redistributor size

[Leif Lindholm]

GICv3 sets aside 128K for each redistributor block, whereas GICv4 sets
aside 256K. To enable use of the gicv3 model for gicv4, abstract this
away as the helper function gicv3_redist_size() and replace the current
hardcoded locations with calls to this function.

Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
 hw/intc/arm_gicv3_common.c         |  2 +-
 hw/intc/arm_gicv3_redist.c         | 13 +++++++++----
 include/hw/intc/arm_gicv3_common.h |  3 +++
 3 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/hw/intc/arm_gicv3_common.c b/hw/intc/arm_gicv3_common.c
index 7365d24873..a8510b39a1 100644
--- a/hw/intc/arm_gicv3_common.c
+++ b/hw/intc/arm_gicv3_common.c
@@ -299,7 +299,7 @@ void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler,
 
         memory_region_init_io(&s->iomem_redist[i], OBJECT(s),
                               ops ? &ops[1] : NULL, s, name,
-                              s->redist_region_count[i] * GICV3_REDIST_SIZE);
+                              s->redist_region_count[i] * gicv3_redist_size(s));
         sysbus_init_mmio(sbd, &s->iomem_redist[i]);
         g_free(name);
     }
diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c
index 8645220d61..544f4d82ff 100644
--- a/hw/intc/arm_gicv3_redist.c
+++ b/hw/intc/arm_gicv3_redist.c
@@ -14,6 +14,11 @@
 #include "trace.h"
 #include "gicv3_internal.h"
 
+int gicv3_redist_size(GICv3State *s)
+{
+    return (s->revision == 3 ? GICV3_REDIST_SIZE : GICV4_REDIST_SIZE);
+}
+
 static uint32_t mask_group(GICv3CPUState *cs, MemTxAttrs attrs)
 {
     /* Return a 32-bit mask which should be applied for this set of 32
@@ -429,8 +434,8 @@ MemTxResult gicv3_redist_read(void *opaque, hwaddr offset, uint64_t *data,
      * want to allow splitting of redistributor pages into several
      * blocks so we can support more CPUs.
      */
-    cpuidx = offset / 0x20000;
-    offset %= 0x20000;
+    cpuidx = offset / gicv3_redist_size(s);
+    offset %= gicv3_redist_size(s);
     assert(cpuidx < s->num_cpu);
 
     cs = &s->cpu[cpuidx];
@@ -486,8 +491,8 @@ MemTxResult gicv3_redist_write(void *opaque, hwaddr offset, uint64_t data,
      * want to allow splitting of redistributor pages into several
      * blocks so we can support more CPUs.
      */
-    cpuidx = offset / 0x20000;
-    offset %= 0x20000;
+    cpuidx = offset / gicv3_redist_size(s);
+    offset %= gicv3_redist_size(s);
     assert(cpuidx < s->num_cpu);
 
     cs = &s->cpu[cpuidx];
diff --git a/include/hw/intc/arm_gicv3_common.h b/include/hw/intc/arm_gicv3_common.h
index 91491a2f66..ab88d14867 100644
--- a/include/hw/intc/arm_gicv3_common.h
+++ b/include/hw/intc/arm_gicv3_common.h
@@ -37,6 +37,7 @@
 #define GICV3_MAXSPI (GICV3_MAXIRQ - GIC_INTERNAL)
 
 #define GICV3_REDIST_SIZE 0x20000
+#define GICV4_REDIST_SIZE (GICV3_REDIST_SIZE + 0x20000)
 
 /* Number of SGI target-list bits */
 #define GICV3_TARGETLIST_BITS 16
@@ -295,4 +296,6 @@ struct ARMGICv3CommonClass {
 void gicv3_init_irqs_and_mmio(GICv3State *s, qemu_irq_handler handler,
                               const MemoryRegionOps *ops, Error **errp);
 
+int gicv3_redist_size(GICv3State *s);
+
 #endif
-- 
2.20.1

Re: [RFC PATCH 2/4] hw/intc: add helper function to determine gicv3 redistributor size

[Peter Maydell]

On Sun, 24 Jan 2021 at 02:53, Leif Lindholm <leif@nuviainc.com> wrote:
>
> GICv3 sets aside 128K for each redistributor block, whereas GICv4 sets
> aside 256K. To enable use of the gicv3 model for gicv4, abstract this
> away as the helper function gicv3_redist_size() and replace the current
> hardcoded locations with calls to this function.
>
> Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> ---

We're going to need this at some point and it's nicer than those
hard-coded 0x20000s.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[RFC PATCH 3/4] hw/intc: set GICD_TYPER.DVIS for GICv4

[Leif Lindholm]

The VLPI frames are what make the redistributor size change, so ensure
we state in GICD_TYPER that we have them.

Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
 hw/intc/arm_gicv3_dist.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/intc/arm_gicv3_dist.c b/hw/intc/arm_gicv3_dist.c
index b65f56f903..833deb0a74 100644
--- a/hw/intc/arm_gicv3_dist.c
+++ b/hw/intc/arm_gicv3_dist.c
@@ -387,6 +387,9 @@ static MemTxResult gicd_readl(GICv3State *s, hwaddr offset,
 
         *data = (1 << 25) | (1 << 24) | (sec_extn << 10) |
             (0xf << 19) | itlinesnumber;
+        if (s->revision == 4) {
+            *data |= (1 << 18);;
+        }
         return MEMTX_OK;
     }
     case GICD_IIDR:
-- 
2.20.1

Re: [RFC PATCH 3/4] hw/intc: set GICD_TYPER.DVIS for GICv4

[Peter Maydell]

On Sun, 24 Jan 2021 at 02:53, Leif Lindholm <leif@nuviainc.com> wrote:
>
> The VLPI frames are what make the redistributor size change, so ensure
> we state in GICD_TYPER that we have them.
>
> Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> ---
>  hw/intc/arm_gicv3_dist.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/intc/arm_gicv3_dist.c b/hw/intc/arm_gicv3_dist.c
> index b65f56f903..833deb0a74 100644
> --- a/hw/intc/arm_gicv3_dist.c
> +++ b/hw/intc/arm_gicv3_dist.c
> @@ -387,6 +387,9 @@ static MemTxResult gicd_readl(GICv3State *s, hwaddr offset,
>
>          *data = (1 << 25) | (1 << 24) | (sec_extn << 10) |
>              (0xf << 19) | itlinesnumber;
> +        if (s->revision == 4) {
> +            *data |= (1 << 18);;
> +        }

Double semicolon.

>          return MEMTX_OK;
>      }
>      case GICD_IIDR:

I think I'd prefer not to take this patch in mainline for the moment:
it would be "safe", in the sense that it doesn't affect anything,
but it's not the only thing in this register that changes for GIC
versions > 3, and it would make a lot more sense to go in as part
of the general support for the feature the bit is advertising.

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 <leif@nuviainc.com>
---
 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

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 <leif@nuviainc.com> wrote:
>
> Make gicv3_idreg() able to return either gicv3 or gicv4 data.
> Add a parameter to specify gic version.
>
> Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> ---
>  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

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 +0000, Peter Maydell wrote:
> On Sun, 24 Jan 2021 at 02:53, Leif Lindholm <leif@nuviainc.com> wrote:
> >
> > Make gicv3_idreg() able to return either gicv3 or gicv4 data.
> > Add a parameter to specify gic version.
> >
> > Signed-off-by: Leif Lindholm <leif@nuviainc.com>
> > ---
> >  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 0/4] hw/intc: enable GICv4 memory layout for GICv3 driver

[no-reply]

Patchew URL: https://patchew.org/QEMU/20210124025306.3949-1-leif@nuviainc.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210124025306.3949-1-leif@nuviainc.com
Subject: [RFC PATCH 0/4] hw/intc: enable GICv4 memory layout for GICv3 driver

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20210123230105.2076270-1-richard.henderson@linaro.org -> patchew/20210123230105.2076270-1-richard.henderson@linaro.org
 * [new tag]         patchew/20210124025306.3949-1-leif@nuviainc.com -> patchew/20210124025306.3949-1-leif@nuviainc.com
Switched to a new branch 'test'
22ce3eb hw/intc: make gicv3_idreg() distinguish between gicv3/gicv4
4d2400c hw/intc: set GICD_TYPER.DVIS for GICv4
bab9208 hw/intc: add helper function to determine gicv3 redistributor size
71fa48f hw/intc: don't bail out gicv3 model init for revision 4

=== OUTPUT BEGIN ===
1/4 Checking commit 71fa48fa1624 (hw/intc: don't bail out gicv3 model init for revision 4)
2/4 Checking commit bab920855fbe (hw/intc: add helper function to determine gicv3 redistributor size)
3/4 Checking commit 4d2400cda07b (hw/intc: set GICD_TYPER.DVIS for GICv4)
ERROR: superfluous trailing semicolon
#25: FILE: hw/intc/arm_gicv3_dist.c:391:
+            *data |= (1 << 18);;

total: 1 errors, 0 warnings, 9 lines checked

Patch 3/4 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

4/4 Checking commit 22ce3eb6f90a (hw/intc: make gicv3_idreg() distinguish between gicv3/gicv4)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210124025306.3949-1-leif@nuviainc.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [RFC PATCH 0/4] hw/intc: enable GICv4 memory layout for GICv3 driver

[Peter Maydell]

On Sun, 24 Jan 2021 at 02:53, Leif Lindholm <leif@nuviainc.com> wrote:
>
> GICv4 sets aside 256K per redistributor configuration block, whereas GICv3
> only uses 128K. However, some codebases (like TF-A, EDK2) will happily use
> the GICv3 functionality only.
>
> This set aims at enabling these codebases to run, without actually enabling
> full support for GICv4.
>
> This creates a ... problematic ... system, which will misbehave if you try
> to use the virtual LPIs. But it does help with letting me use QEMU for
> modelling a platform containing a GICv4, and share firmware images with
> other prototyping platforms.

So, what's your aim for this series? I think we could reasonably
take patches 2 and 4 upstream (they are changes we'll want for eventual
v4 emulation support), but I don't really want 1 and 3.
That would reduce the delta you're carrying locally, at least.

I suppose we should look at what changes QEMU needs for KVM in-kernel GICv4
support at some point...

thanks
-- PMM

Re: [RFC PATCH 0/4] hw/intc: enable GICv4 memory layout for GICv3 driver

[Leif Lindholm]

On Tue, Feb 02, 2021 at 10:39:22 +0000, Peter Maydell wrote:
> On Sun, 24 Jan 2021 at 02:53, Leif Lindholm <leif@nuviainc.com> wrote:
> >
> > GICv4 sets aside 256K per redistributor configuration block, whereas GICv3
> > only uses 128K. However, some codebases (like TF-A, EDK2) will happily use
> > the GICv3 functionality only.
> >
> > This set aims at enabling these codebases to run, without actually enabling
> > full support for GICv4.
> >
> > This creates a ... problematic ... system, which will misbehave if you try
> > to use the virtual LPIs. But it does help with letting me use QEMU for
> > modelling a platform containing a GICv4, and share firmware images with
> > other prototyping platforms.
> 
> So, what's your aim for this series? I think we could reasonably
> take patches 2 and 4 upstream (they are changes we'll want for eventual
> v4 emulation support), but I don't really want 1 and 3.
> That would reduce the delta you're carrying locally, at least.

That would be much appreciated.

In a way, I wanted to test the waters a bit with regards to whether
gicv4 emulation could be introduced gradually, and whether doing so by
extending the existing gicv3 driver was the way to go.

Anyway, for now, I'll address your comments and send out a 2-part v2,
containing 2,4/4.

Best Regards,

Leif

> I suppose we should look at what changes QEMU needs for KVM in-kernel GICv4
> support at some point...
> 
> thanks
> -- PMM