| Message ID | 20220201193207.2771604-7-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/intc/arm_gicv3_its: more cleanups, bugfixes | expand |
On 2/2/22 06:32, Peter Maydell wrote: > In get_ite() and update_ite() we work with a 12-byte in-guest-memory > table entry, which we intend to handle as an 8-byte value followed by > a 4-byte value. Unfortunately the calculation of the address of the > 4-byte value is wrong, because we write it as: > > table_base_address + (index * entrysize) + 4 > (obfuscated by the way the expression has been written) > > when it should be + 8. This bug meant that we overwrote the top > bytes of the 8-byte value with the 4-byte value. There are no > guest-visible effects because the top half of the 8-byte value > contains only the doorbell interrupt field, which is used only in > GICv4, and the two bugs in the "write ITE" and "read ITE" codepaths > cancel each other out. > > We can't simply change the calculation, because this would break > migration of a (TCG) guest from the old version of QEMU which had > in-guest-memory interrupt tables written using the buggy version of > update_ite(). We must also at the same time change the layout of the > fields within the ITE_L and ITE_H values so that the in-memory > locations of the fields we care about (VALID, INTTYPE, INTID and > ICID) stay the same. > > Signed-off-by: Peter Maydell<peter.maydell@linaro.org> > --- > hw/intc/gicv3_internal.h | 19 ++++++++++--------- > hw/intc/arm_gicv3_its.c | 28 +++++++++++----------------- > 2 files changed, 21 insertions(+), 26 deletions(-) This is confusing: 5-3 is titled "example of the number of bits that might be stored in an ITE"? Surely there must be a true architected format for this table, the one real hardware uses. Surely tcg will simply have to suck it up and break migration to fix this properly. r~
On Thu, 3 Feb 2022 at 03:59, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 2/2/22 06:32, Peter Maydell wrote: > > In get_ite() and update_ite() we work with a 12-byte in-guest-memory > > table entry, which we intend to handle as an 8-byte value followed by > > a 4-byte value. Unfortunately the calculation of the address of the > > 4-byte value is wrong, because we write it as: > > > > table_base_address + (index * entrysize) + 4 > > (obfuscated by the way the expression has been written) > > > > when it should be + 8. This bug meant that we overwrote the top > > bytes of the 8-byte value with the 4-byte value. There are no > > guest-visible effects because the top half of the 8-byte value > > contains only the doorbell interrupt field, which is used only in > > GICv4, and the two bugs in the "write ITE" and "read ITE" codepaths > > cancel each other out. > > > > We can't simply change the calculation, because this would break > > migration of a (TCG) guest from the old version of QEMU which had > > in-guest-memory interrupt tables written using the buggy version of > > update_ite(). We must also at the same time change the layout of the > > fields within the ITE_L and ITE_H values so that the in-memory > > locations of the fields we care about (VALID, INTTYPE, INTID and > > ICID) stay the same. > This is confusing: 5-3 is titled "example of the number of bits that might be stored in an > ITE"? Surely there must be a true architected format for this table, the one real > hardware uses. Surely tcg will simply have to suck it up and break migration to fix this > properly. No, the ITE format is implementation-defined, like that of the other in-guest-memory tables the ITS uses. It's UNPREDICTABLE for a guest to try to directly access the tables in memory -- they are only ever written or read by the ITS itself in response to incoming commands, so it's not a problem for the format in memory to be impdef. This flexibility in the spec allows implementations to minimize the size of their data tables based on how large an ID size they support and other potentially-configurable parameters. For instance if you look at the values for the GITS_BASER* for the GIC-700 in its TRM you can see that its Collection Table entry size is only 2 bytes, since it uses the "rdbase is a CPU number" format; an ITS that used the "rdbase is a physical address" implementation choice would need more bytes there. (QEMU also uses "rdbase is a CPU number", but we have rather profligately opted to use 8 bytes per collection table entry.) thanks -- PMM
On 2/3/22 21:45, Peter Maydell wrote: > On Thu, 3 Feb 2022 at 03:59, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> On 2/2/22 06:32, Peter Maydell wrote: >>> In get_ite() and update_ite() we work with a 12-byte in-guest-memory >>> table entry, which we intend to handle as an 8-byte value followed by >>> a 4-byte value. Unfortunately the calculation of the address of the >>> 4-byte value is wrong, because we write it as: >>> >>> table_base_address + (index * entrysize) + 4 >>> (obfuscated by the way the expression has been written) >>> >>> when it should be + 8. This bug meant that we overwrote the top >>> bytes of the 8-byte value with the 4-byte value. There are no >>> guest-visible effects because the top half of the 8-byte value >>> contains only the doorbell interrupt field, which is used only in >>> GICv4, and the two bugs in the "write ITE" and "read ITE" codepaths >>> cancel each other out. >>> >>> We can't simply change the calculation, because this would break >>> migration of a (TCG) guest from the old version of QEMU which had >>> in-guest-memory interrupt tables written using the buggy version of >>> update_ite(). We must also at the same time change the layout of the >>> fields within the ITE_L and ITE_H values so that the in-memory >>> locations of the fields we care about (VALID, INTTYPE, INTID and >>> ICID) stay the same. > >> This is confusing: 5-3 is titled "example of the number of bits that might be stored in an >> ITE"? Surely there must be a true architected format for this table, the one real >> hardware uses. Surely tcg will simply have to suck it up and break migration to fix this >> properly. > > No, the ITE format is implementation-defined, like that of the other > in-guest-memory tables the ITS uses. It's UNPREDICTABLE for a guest > to try to directly access the tables in memory -- they are only ever > written or read by the ITS itself in response to incoming commands, > so it's not a problem for the format in memory to be impdef. This > flexibility in the spec allows implementations to minimize the size > of their data tables based on how large an ID size they support and > other potentially-configurable parameters. For instance if you look > at the values for the GITS_BASER* for the GIC-700 in its TRM you > can see that its Collection Table entry size is only 2 bytes, since > it uses the "rdbase is a CPU number" format; an ITS that used the > "rdbase is a physical address" implementation choice would need > more bytes there. (QEMU also uses "rdbase is a CPU number", but > we have rather profligately opted to use 8 bytes per collection > table entry.) Ah, right. In which case, Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
diff --git a/hw/intc/gicv3_internal.h b/hw/intc/gicv3_internal.h index 60c8617e4e4..2bf1baef047 100644 --- a/hw/intc/gicv3_internal.h +++ b/hw/intc/gicv3_internal.h @@ -370,22 +370,23 @@ FIELD(MOVI_2, ICID, 0, 16) * 12 bytes Interrupt translation Table Entry size * as per Table 5.3 in GICv3 spec * ITE Lower 8 Bytes - * Bits: | 49 ... 26 | 25 ... 2 | 1 | 0 | - * Values: | Doorbell | IntNum | IntType | Valid | + * Bits: | 63 ... 48 | 47 ... 32 | 31 ... 26 | 25 ... 2 | 1 | 0 | + * Values: | vPEID | ICID | unused | IntNum | IntType | Valid | * ITE Higher 4 Bytes - * Bits: | 31 ... 16 | 15 ...0 | - * Values: | vPEID | ICID | - * (When Doorbell is unused, as it always is in GICv3, it is 1023) + * Bits: | 31 ... 25 | 24 ... 0 | + * Values: | unused | Doorbell | + * (When Doorbell is unused, as it always is for INTYPE_PHYSICAL, + * 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 FIELD(ITE_L, VALID, 0, 1) FIELD(ITE_L, INTTYPE, 1, 1) FIELD(ITE_L, INTID, 2, 24) -FIELD(ITE_L, DOORBELL, 26, 24) - -FIELD(ITE_H, ICID, 0, 16) -FIELD(ITE_H, VPEID, 16, 16) +FIELD(ITE_L, ICID, 32, 16) +FIELD(ITE_L, VPEID, 48, 16) +FIELD(ITE_H, DOORBELL, 0, 24) /* Possible values for ITE_L INTTYPE */ #define ITE_INTTYPE_VIRTUAL 0 diff --git a/hw/intc/arm_gicv3_its.c b/hw/intc/arm_gicv3_its.c index b94775fd379..48eaf20a6c9 100644 --- a/hw/intc/arm_gicv3_its.c +++ b/hw/intc/arm_gicv3_its.c @@ -173,14 +173,12 @@ 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; - address_space_stq_le(as, dte->ittaddr + (eventid * (sizeof(uint64_t) + - sizeof(uint32_t))), ite.itel, MEMTXATTRS_UNSPECIFIED, - &res); + address_space_stq_le(as, iteaddr, ite.itel, MEMTXATTRS_UNSPECIFIED, &res); if (res == MEMTX_OK) { - address_space_stl_le(as, dte->ittaddr + (eventid * (sizeof(uint64_t) + - sizeof(uint32_t))) + sizeof(uint32_t), ite.iteh, + address_space_stl_le(as, iteaddr + 8, ite.iteh, MEMTXATTRS_UNSPECIFIED, &res); } if (res != MEMTX_OK) { @@ -196,16 +194,12 @@ static bool get_ite(GICv3ITSState *s, uint32_t eventid, const DTEntry *dte, AddressSpace *as = &s->gicv3->dma_as; bool status = false; IteEntry ite = {}; + hwaddr iteaddr = dte->ittaddr + eventid * ITS_ITT_ENTRY_SIZE; - ite.itel = address_space_ldq_le(as, dte->ittaddr + - (eventid * (sizeof(uint64_t) + - sizeof(uint32_t))), MEMTXATTRS_UNSPECIFIED, - res); + ite.itel = address_space_ldq_le(as, iteaddr, MEMTXATTRS_UNSPECIFIED, res); if (*res == MEMTX_OK) { - ite.iteh = address_space_ldl_le(as, dte->ittaddr + - (eventid * (sizeof(uint64_t) + - sizeof(uint32_t))) + sizeof(uint32_t), + ite.iteh = address_space_ldl_le(as, iteaddr + 8, MEMTXATTRS_UNSPECIFIED, res); if (*res == MEMTX_OK) { @@ -213,7 +207,7 @@ static bool get_ite(GICv3ITSState *s, uint32_t eventid, const DTEntry *dte, int inttype = FIELD_EX64(ite.itel, ITE_L, INTTYPE); if (inttype == ITE_INTTYPE_PHYSICAL) { *pIntid = FIELD_EX64(ite.itel, ITE_L, INTID); - *icid = FIELD_EX32(ite.iteh, ITE_H, ICID); + *icid = FIELD_EX64(ite.itel, ITE_L, ICID); status = true; } } @@ -412,8 +406,8 @@ static ItsCmdResult process_mapti(GICv3ITSState *s, const uint64_t *cmdpkt, ite.itel = FIELD_DP64(ite.itel, ITE_L, VALID, true); ite.itel = FIELD_DP64(ite.itel, ITE_L, INTTYPE, ITE_INTTYPE_PHYSICAL); ite.itel = FIELD_DP64(ite.itel, ITE_L, INTID, pIntid); - ite.itel = FIELD_DP64(ite.itel, ITE_L, DOORBELL, INTID_SPURIOUS); - ite.iteh = FIELD_DP32(ite.iteh, ITE_H, ICID, icid); + ite.itel = FIELD_DP64(ite.itel, ITE_L, ICID, icid); + ite.iteh = FIELD_DP32(ite.iteh, ITE_H, DOORBELL, INTID_SPURIOUS); return update_ite(s, eventid, &dte, ite) ? CMD_CONTINUE : CMD_STALL; } @@ -688,8 +682,8 @@ static ItsCmdResult process_movi(GICv3ITSState *s, const uint64_t *cmdpkt) ite.itel = FIELD_DP64(ite.itel, ITE_L, VALID, 1); ite.itel = FIELD_DP64(ite.itel, ITE_L, INTTYPE, ITE_INTTYPE_PHYSICAL); ite.itel = FIELD_DP64(ite.itel, ITE_L, INTID, intid); - ite.itel = FIELD_DP64(ite.itel, ITE_L, DOORBELL, INTID_SPURIOUS); - ite.iteh = FIELD_DP32(ite.iteh, ITE_H, ICID, new_icid); + ite.itel = FIELD_DP64(ite.itel, ITE_L, ICID, new_icid); + ite.iteh = FIELD_DP32(ite.iteh, ITE_H, DOORBELL, INTID_SPURIOUS); return update_ite(s, eventid, &dte, ite) ? CMD_CONTINUE : CMD_STALL; }
In get_ite() and update_ite() we work with a 12-byte in-guest-memory table entry, which we intend to handle as an 8-byte value followed by a 4-byte value. Unfortunately the calculation of the address of the 4-byte value is wrong, because we write it as: table_base_address + (index * entrysize) + 4 (obfuscated by the way the expression has been written) when it should be + 8. This bug meant that we overwrote the top bytes of the 8-byte value with the 4-byte value. There are no guest-visible effects because the top half of the 8-byte value contains only the doorbell interrupt field, which is used only in GICv4, and the two bugs in the "write ITE" and "read ITE" codepaths cancel each other out. We can't simply change the calculation, because this would break migration of a (TCG) guest from the old version of QEMU which had in-guest-memory interrupt tables written using the buggy version of update_ite(). We must also at the same time change the layout of the fields within the ITE_L and ITE_H values so that the in-memory locations of the fields we care about (VALID, INTTYPE, INTID and ICID) stay the same. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/intc/gicv3_internal.h | 19 ++++++++++--------- hw/intc/arm_gicv3_its.c | 28 +++++++++++----------------- 2 files changed, 21 insertions(+), 26 deletions(-)