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/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~