Re: [Qemu-devel] [PATCH v6 21/25] replay: replay BH for IDE trim operation

[Pavel Dovgalyuk]

> From: John Snow [mailto:address@hidden
> On 09/14/2018 03:27 AM, Pavel Dovgalyuk wrote:
> >> From: Pavel Dovgalyuk [mailto:address@hidden
> >>> From: John Snow [mailto:address@hidden
> >>> 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 <address@hidden>
> >>>> Reviewed-by: Paolo Bonzini <address@hidden>
> >>>> ---
> >>>>  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