Re: [PATCH 06/13] hw/intc/arm_gicv3_its: Fix address calculation in get_ite() and update_ite()

[Richard Henderson <richard.henderson@linaro.org>]

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~