[PATCH 3/3] KVM: arm64: vgic-its: Make updates to propbaser/pendbaser atomic

On Tue, Aug 09, 2016 at 12:19:53PM +0100, Andre Przywara wrote:

Hi,

On 09/08/16 11:30, Christoffer Dall wrote:

quoted

On Mon, Aug 08, 2016 at 02:30:38PM +0100, Andre Przywara wrote:

quoted

Hi,

On 03/08/16 17:13, Christoffer Dall wrote:

quoted

There are two problems with the current implementation of the MMIO
handlers for the propbaser and pendbaser:

First, the write to the value itself is not guaranteed to be an atomic
64-bit write so two concurrent writes to the structure field could be
intermixed.

Second, because we do a read-modify-update operation without any
synchronization, if we have two 32-bit accesses to separate parts of the
register, we can loose one of them.

I am still not 100% convinced that this is necessary, but leave it up to
the judgement of you senior guys.

ok, consider this case:

    reg = 0x55555555 55555555;

    CPU 0                CPU 1
    -----                -----
    tmp = reg;
                         tmp = reg;
    tmp[63:32] = ~0;
                         tmp[31:0] = 0;
    reg = tmp;
                         reg = tmp;

    print("reg is %x", reg);
      /* reg is 0x55555555 00000000 */

which is weird, because I think in hardware you'll get:
    0xffffffff 00000000

no matter how you order the two 32-bit updates.

That is, unless the architecture tells us that you could observe the
above behavior.

OK, I can see that this case is indeed broken. I was just wondering how
much software can expect if it allows unsynchronized accesses from
different CPUs to such a 64-bit register - even if it is for separate
halves of that register. I'd expect (guest) software to take a lock if
it can't atomically update prop/pendbaser and there are other agents
possibly chiming in.
And I hope we sanitize them enough now to avoid any bad things to happen
in the kernel ;-)

I don't think it's 100% unreasonable to potentially imagine independent
updates of parts of the register when the spec explicitly says this is
allowed.

I agree, that it would be a curious guest (and I raised this point once
already), but I think relying on this is a terrible approach to
emulating hardware and without any comment or anything in the kernel
saying 'this is safe and reasonable because of so and so' it just looks
too dodgy for me to live with.

-Christoffer