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

[John Snow]

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?
> 
> Pavel Dovgalyuk
> 
> 

Shouldn't change guest state all by itself.

ide_restart_bh does, though. (Changes device registers, can cause block
IO to occur, etc.)

--js