Re: Lockup with --accel tcg,thread=single

[Peter Maydell]

On Mon, 30 Sep 2019 at 18:47, Paolo Bonzini <address@hidden> wrote:
> On 30/09/19 17:37, Peter Maydell wrote:
> > Side note -- this use of run_on_cpu() means that we now drop
> > the iothread lock within memory_region_snapshot_and_clear_dirty(),
> > which we didn't before. This means that a vCPU thread can now
> > get in and execute an access to the device registers of
> > hw/display/vga.c, updating its state in a way I suspect that the
> > device model code is not expecting... So maybe the right answer
> > here should be to come up with a fix for the race that 9458a9a1
> > addresses that doesn't require us to drop the iothread lock in
> > memory_region_snapshot_and_clear_dirty() ? Alternatively we need
> > to audit the callers and flag in the documentation that this
> > function has the unexpected side effect of briefly dropping the
> > iothread lock.
>
> Yes, this is intended.  There shouldn't be side effects other than
> possibly a one-frame flash for all callers.

This seems quite tricky to audit to me -- for instance is the code
in vga_draw_graphic() really designed for and expecting to cope with
races where information it reads from s->foo after the call to
memory_region_snapshot_and_clear_dirty() might have been updated
by the guest and not be consistent with the information it read
from s->bar before the call and cached in a local variable?
I think most people write device code assuming that while execution
is within a function in the device model the device won't have to
deal with possible reentrancy where another thread comes in and
alters the device state underfoot.

thanks
-- PMM