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

[Peter Maydell]

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