On Mon, 30 Sep 2019 at 18:47, Paolo Bonzini <pbonz...@redhat.com> 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
