[PATCH 0/2] hw/intc/arm_gicv3: Bump ITT entry size to 16

[PATCH 0/2] hw/intc/arm_gicv3: Bump ITT entry size to 16

[Alexander Graf]

While trying to make Windows work with GICv3 emulation, I stumbled over
the fact that it only supports ITT entry sizes that are power of 2 sized.

While the spec allows arbitrary sizes, in practice hardware will always
expose power of 2 sizes and so this limitation is not really a problem
in real world scenarios. However, we only expose a 12 byte ITT entry size
which makes Windows blue screen on boot.

The easy way to get around that problem is to bump the size to 16. That
is a power of 2, basically is what hardware would expose given the amount
of bits we need per entry and doesn't break any existing scenarios. To
play it safe, this patch set only bumps them on newer machine types.

Alexander Graf (2):
  hw/intc/arm_gicv3: Make ITT entry size configurable
  hw/intc/arm_gicv3: Bump ITT entry size to 16

 hw/core/machine.c                      |  4 +++-
 hw/intc/arm_gicv3_its.c                | 13 ++++++++++---
 hw/intc/gicv3_internal.h               |  2 +-
 include/hw/intc/arm_gicv3_its_common.h |  1 +
 4 files changed, 15 insertions(+), 5 deletions(-)

-- 
2.37.1 (Apple Git-137.1)

[PATCH 1/2] hw/intc/arm_gicv3: Make ITT entry size configurable

[Alexander Graf]

An ITT entry is opaque to the OS. The only thing it does get told by HW is
its size. In theory, that size can be any byte aligned number, in practice
HW will always use power of 2s to simplify offset calculation. We currently
expose the size as 12, which is not a power of 2.

To prepare for a future where we expose power of 2 sized entry sizes, let's
make the size itself configurable. We only need to watch out that we don't
have an entry be smaller than the fields we want to access inside. Bigger
is always fine.

Signed-off-by: Alexander Graf <agraf@csgraf.de>
---
 hw/intc/arm_gicv3_its.c                | 14 +++++++++++---
 hw/intc/gicv3_internal.h               |  2 +-
 include/hw/intc/arm_gicv3_its_common.h |  1 +
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index 57c79da5c5..e7cabeb46c 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -215,7 +215,7 @@ static bool update_ite(GICv3ITSState *s, uint32_t eventid, const DTEntry *dte,
 {
     AddressSpace *as = &s->gicv3->dma_as;
     MemTxResult res = MEMTX_OK;
-    hwaddr iteaddr = dte->ittaddr + eventid * ITS_ITT_ENTRY_SIZE;
+    hwaddr iteaddr = dte->ittaddr + eventid * s->itt_entry_size;
     uint64_t itel = 0;
     uint32_t iteh = 0;
 
@@ -253,7 +253,7 @@ static MemTxResult get_ite(GICv3ITSState *s, uint32_t eventid,
     MemTxResult res = MEMTX_OK;
     uint64_t itel;
     uint32_t iteh;
-    hwaddr iteaddr = dte->ittaddr + eventid * ITS_ITT_ENTRY_SIZE;
+    hwaddr iteaddr = dte->ittaddr + eventid * s->itt_entry_size;
 
     itel = address_space_ldq_le(as, iteaddr, MEMTXATTRS_UNSPECIFIED, &res);
     if (res != MEMTX_OK) {
@@ -1934,6 +1934,12 @@ static void gicv3_arm_its_realize(DeviceState *dev, Error **errp)
         }
     }
 
+    if (s->itt_entry_size < MIN_ITS_ITT_ENTRY_SIZE) {
+        error_setg(errp, "ITT entry size must be at least %d",
+                   MIN_ITS_ITT_ENTRY_SIZE);
+        return;
+    }
+
     gicv3_add_its(s->gicv3, dev);
 
     gicv3_its_init_mmio(s, &gicv3_its_control_ops, &gicv3_its_translation_ops);
@@ -1941,7 +1947,7 @@ static void gicv3_arm_its_realize(DeviceState *dev, Error **errp)
     /* set the ITS default features supported */
     s->typer = FIELD_DP64(s->typer, GITS_TYPER, PHYSICAL, 1);
     s->typer = FIELD_DP64(s->typer, GITS_TYPER, ITT_ENTRY_SIZE,
-                          ITS_ITT_ENTRY_SIZE - 1);
+                          s->itt_entry_size - 1);
     s->typer = FIELD_DP64(s->typer, GITS_TYPER, IDBITS, ITS_IDBITS);
     s->typer = FIELD_DP64(s->typer, GITS_TYPER, DEVBITS, ITS_DEVBITS);
     s->typer = FIELD_DP64(s->typer, GITS_TYPER, CIL, 1);
@@ -2008,6 +2014,8 @@ static void gicv3_its_post_load(GICv3ITSState *s)
 static Property gicv3_its_props[] = {
     DEFINE_PROP_LINK("parent-gicv3", GICv3ITSState, gicv3, "arm-gicv3",
                      GICv3State *),
+    DEFINE_PROP_UINT8("itt-entry-size", GICv3ITSState, itt_entry_size,
+                      MIN_ITS_ITT_ENTRY_SIZE),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h
index 29d5cdc1b6..2aca1ba095 100644
--- a/hw/intc/gicv3_internal.h
+++ b/hw/intc/gicv3_internal.h
@@ -450,7 +450,7 @@ FIELD(VINVALL_1, VPEID, 32, 16)
  * the value of that field in memory cannot be relied upon -- older
  * versions of QEMU did not correctly write to that memory.)
  */
-#define ITS_ITT_ENTRY_SIZE            0xC
+#define MIN_ITS_ITT_ENTRY_SIZE            0xC
 
 FIELD(ITE_L, VALID, 0, 1)
 FIELD(ITE_L, INTTYPE, 1, 1)
diff --git a/include/hw/intc/arm_gicv3_its_common.h b/include/hw/intc/arm_gicv3_its_common.h
index a11a0f6654..e730a5482c 100644
--- a/include/hw/intc/arm_gicv3_its_common.h
+++ b/include/hw/intc/arm_gicv3_its_common.h
@@ -66,6 +66,7 @@ struct GICv3ITSState {
     int dev_fd; /* kvm device fd if backed by kvm vgic support */
     uint64_t gits_translater_gpa;
     bool translater_gpa_known;
+    uint8_t itt_entry_size;
 
     /* Registers */
     uint32_t ctlr;
-- 
2.37.1 (Apple Git-137.1)

[PATCH 2/2] hw/intc/arm_gicv3: Bump ITT entry size to 16

[Alexander Graf]

Some Operating Systems (like Windows) can only deal with ITT entry sizes
that are a power of 2. While the spec allows arbitrarily sized ITT entry
sizes, in practice all hardware will use power of 2 because that
simplifies offset calculation and ensures that a power of 2 sized region
can hold a set of entries without gap at the end.

So let's just bump the entry size to 16. That gives us enough space for
the 12 bytes of data that we want to have in each ITT entry and makes
QEMU look a bit more like real hardware.

Signed-off-by: Alexander Graf <agraf@csgraf.de>
---
 hw/core/machine.c       | 4 +++-
 hw/intc/arm_gicv3_its.c | 3 +--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index f589b92909..d9a3f01ed9 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -40,7 +40,9 @@
 #include "hw/virtio/virtio-pci.h"
 #include "qom/object_interfaces.h"
 
-GlobalProperty hw_compat_7_2[] = {};
+GlobalProperty hw_compat_7_2[] = {
+    { "arm-gicv3-its", "itt-entry-size", "12" },
+};
 const size_t hw_compat_7_2_len = G_N_ELEMENTS(hw_compat_7_2);
 
 GlobalProperty hw_compat_7_1[] = {
diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c
index e7cabeb46c..6754523321 100644
--- a/hw/intc/arm_gicv3_its.c
+++ b/hw/intc/arm_gicv3_its.c
@@ -2014,8 +2014,7 @@ static void gicv3_its_post_load(GICv3ITSState *s)
 static Property gicv3_its_props[] = {
     DEFINE_PROP_LINK("parent-gicv3", GICv3ITSState, gicv3, "arm-gicv3",
                      GICv3State *),
-    DEFINE_PROP_UINT8("itt-entry-size", GICv3ITSState, itt_entry_size,
-                      MIN_ITS_ITT_ENTRY_SIZE),
+    DEFINE_PROP_UINT8("itt-entry-size", GICv3ITSState, itt_entry_size, 16),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.37.1 (Apple Git-137.1)

Re: [PATCH 0/2] hw/intc/arm_gicv3: Bump ITT entry size to 16

[Philippe Mathieu-Daudé]

On 23/12/22 09:50, Alexander Graf wrote:
> While trying to make Windows work with GICv3 emulation, I stumbled over
> the fact that it only supports ITT entry sizes that are power of 2 sized.
> 
> While the spec allows arbitrary sizes, in practice hardware will always
> expose power of 2 sizes and so this limitation is not really a problem
> in real world scenarios. However, we only expose a 12 byte ITT entry size
> which makes Windows blue screen on boot.
> 
> The easy way to get around that problem is to bump the size to 16. That
> is a power of 2, basically is what hardware would expose given the amount
> of bits we need per entry and doesn't break any existing scenarios. To
> play it safe, this patch set only bumps them on newer machine types.
> 
> Alexander Graf (2):
>    hw/intc/arm_gicv3: Make ITT entry size configurable
>    hw/intc/arm_gicv3: Bump ITT entry size to 16

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

Re: [PATCH 0/2] hw/intc/arm_gicv3: Bump ITT entry size to 16

[Peter Maydell]

On Fri, 23 Dec 2022 at 08:50, Alexander Graf <agraf@csgraf.de> wrote:
>
> While trying to make Windows work with GICv3 emulation, I stumbled over
> the fact that it only supports ITT entry sizes that are power of 2 sized.
>
> While the spec allows arbitrary sizes, in practice hardware will always
> expose power of 2 sizes and so this limitation is not really a problem
> in real world scenarios. However, we only expose a 12 byte ITT entry size
> which makes Windows blue screen on boot.
>
> The easy way to get around that problem is to bump the size to 16. That
> is a power of 2, basically is what hardware would expose given the amount
> of bits we need per entry and doesn't break any existing scenarios. To
> play it safe, this patch set only bumps them on newer machine types.

This is a Windows bug and should IMHO be fixed in that guest OS.
Changing the ITT entry size of QEMU's implementation introduces
an unnecessary incompatibility in migration and wastes memory
(we're already a bit unnecessarily profligate with ITT entries
compared to real hardware).

thanks
-- PMM

Re: [PATCH 0/2] hw/intc/arm_gicv3: Bump ITT entry size to 16

[Alexander Graf]

Hi Peter,

On 03.01.23 18:41, Peter Maydell wrote:
> On Fri, 23 Dec 2022 at 08:50, Alexander Graf <agraf@csgraf.de> wrote:
>> While trying to make Windows work with GICv3 emulation, I stumbled over
>> the fact that it only supports ITT entry sizes that are power of 2 sized.
>>
>> While the spec allows arbitrary sizes, in practice hardware will always
>> expose power of 2 sizes and so this limitation is not really a problem
>> in real world scenarios. However, we only expose a 12 byte ITT entry size
>> which makes Windows blue screen on boot.
>>
>> The easy way to get around that problem is to bump the size to 16. That
>> is a power of 2, basically is what hardware would expose given the amount
>> of bits we need per entry and doesn't break any existing scenarios. To
>> play it safe, this patch set only bumps them on newer machine types.
> This is a Windows bug and should IMHO be fixed in that guest OS.


I don't have access to the Windows source code, but the compiled binary 
very explicitly checks and validates that an ITT entry is Po2 sized. 
That means the MS folks deliberately decided to make simplifying 
assumptions that hardware will never use any other sizes.

After thinking about it for a while, I ended up with the same 
conclusion: Hardware would never use anything but Po2 sizes because 
those are trivial to map to indexes in hardware, while anything even 
remotely odd is much more costly (in die space and/or time) to extract 
an index from.

So while I'm really curious about the rationale they had here, I doubt 
it's a bug. It's a deliberate decision. And one that makes sense in the 
context of hardware. I don't see a good reason for them to change the 
behavior, given that there's a close-to-0 chance we will ever see real 
hardware ITS structures with ITT entries that are not Po2 sized.


> Changing the ITT entry size of QEMU's implementation introduces
> an unnecessary incompatibility in migration and wastes memory

The patch set deals with migration through machine versions. We do these 
type of changes all the time, why would it be a problem here?

As for memory waste, I agree. If I understand the ITS code correctly, 
basically all of the contents that are >8 bytes is GICv4 related and 
useless in a GICv3 vGIC. So I think if we really care strongly about 
memory waste, we could try to condense it down to 8 bytes in the GICv3 
case and make it 16 only for GICv4.

I think keeping GICv3 and GICv4 code paths identical does have its 
attractiveness though, so I'd prefer not to do it.


> (we're already a bit unnecessarily profligate with ITT entries
> compared to real hardware).

Do you mean the number of entries or the size per entry?


Alex

Re: [PATCH 1/2] hw/intc/arm_gicv3: Make ITT entry size configurable

[Joelle van Dyne]

On Fri, Dec 23, 2022 at 12:50 AM Alexander Graf <agraf@csgraf.de> wrote:
>
> An ITT entry is opaque to the OS. The only thing it does get told by HW is
> its size. In theory, that size can be any byte aligned number, in practice
> HW will always use power of 2s to simplify offset calculation. We currently
> expose the size as 12, which is not a power of 2.
>
> To prepare for a future where we expose power of 2 sized entry sizes, let's
> make the size itself configurable. We only need to watch out that we don't
> have an entry be smaller than the fields we want to access inside. Bigger
> is always fine.
>
> Signed-off-by: Alexander Graf <agraf@csgraf.de>
> ---

Tested-by: Joelle van Dyne <j@getutm.app>

Re: [PATCH 0/2] hw/intc/arm_gicv3: Bump ITT entry size to 16

[Alexander Graf]

On 03.01.23 18:41, Peter Maydell wrote:
> On Fri, 23 Dec 2022 at 08:50, Alexander Graf <agraf@csgraf.de> wrote:
>> While trying to make Windows work with GICv3 emulation, I stumbled over
>> the fact that it only supports ITT entry sizes that are power of 2 sized.
>>
>> While the spec allows arbitrary sizes, in practice hardware will always
>> expose power of 2 sizes and so this limitation is not really a problem
>> in real world scenarios. However, we only expose a 12 byte ITT entry size
>> which makes Windows blue screen on boot.
>>
>> The easy way to get around that problem is to bump the size to 16. That
>> is a power of 2, basically is what hardware would expose given the amount
>> of bits we need per entry and doesn't break any existing scenarios. To
>> play it safe, this patch set only bumps them on newer machine types.
> This is a Windows bug and should IMHO be fixed in that guest OS.
> Changing the ITT entry size of QEMU's implementation introduces
> an unnecessary incompatibility in migration and wastes memory
> (we're already a bit unnecessarily profligate with ITT entries
> compared to real hardware).


Follow-up on this: Microsoft has fixed the issue in Windows. That won't 
make older versions work, but the current should be fine with GICv3:

https://fosstodon.org/@itanium/109909281184181276


Alex