> From: John Snow [mailto:js...@redhat.com] > On 09/14/2018 03:27 AM, Pavel Dovgalyuk wrote: > >> From: Pavel Dovgalyuk [mailto:dovga...@ispras.ru] > >>> From: John Snow [mailto:js...@redhat.com] > >>> On 09/12/2018 04:19 AM, Pavel Dovgalyuk wrote: > >>>> This patch makes IDE trim BH deterministic, because it affects > >>>> the device state. Therefore its invocation should be replayed > >>>> instead of running at the random moment. > >>>> > >>>> Signed-off-by: Pavel Dovgalyuk <pavel.dovga...@ispras.ru> > >>>> Reviewed-by: Paolo Bonzini <pbonz...@redhat.com> > >>>> --- > >>>> hw/ide/core.c | 3 ++- > >>>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/hw/ide/core.c b/hw/ide/core.c > >>>> index 2c62efc..04e22e7 100644 > >>>> --- a/hw/ide/core.c > >>>> +++ b/hw/ide/core.c > >>>> @@ -35,6 +35,7 @@ > >>>> #include "sysemu/block-backend.h" > >>>> #include "qapi/error.h" > >>>> #include "qemu/cutils.h" > >>>> +#include "sysemu/replay.h" > >>>> > >>>> #include "hw/ide/internal.h" > >>>> #include "trace.h" > >>>> @@ -479,7 +480,7 @@ static void ide_issue_trim_cb(void *opaque, int ret) > >>>> done: > >>>> iocb->aiocb = NULL; > >>>> if (iocb->bh) { > >>>> - qemu_bh_schedule(iocb->bh); > >>>> + replay_bh_schedule_event(iocb->bh); > >>>> } > >>>> } > >>>> > >>> Just passing by: Why do we need to change this call, but nothing else in > >>> IDE? > >> > >> This call is responsible for a bug that was reproducible. > >> > >>> I don't mind conceptually, but it's odd to me that of all the calls I > >>> make in this emulator that change state somewhere that this is the only > >>> one you need to hijack for the replay feature. > >>> > >>> Is this a necessarily complete change? > > > > > > I found one more BH in ide/core: > > > > static void ide_restart_cb(void *opaque, int running, RunState state) > > { > > IDEBus *bus = opaque; > > > > if (!running) > > return; > > > > if (!bus->bh) { > > bus->bh = qemu_bh_new(ide_restart_bh, bus); > > qemu_bh_schedule(bus->bh); > > } > > } > > > > void ide_register_restart_cb(IDEBus *bus) > > { > > if (bus->dma->ops->restart_dma) { > > bus->vmstate = qemu_add_vm_change_state_handler(ide_restart_cb, > > bus); > > } > > } > > > > As I understand, it is called when VM start/stop event happen. > > These events are not related to the guest state. > > > > Does this BH change the guest state somehow? > > Shouldn't change guest state all by itself. > > ide_restart_bh does, though. (Changes device registers, can cause block > IO to occur, etc.)
Why is this needed? I mean that start/stop should not change the state of the guest. Why should we restart IDE controller operations? Pavel Dovgalyuk
