[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 21/25] replay: replay BH for IDE trim operati
From: |
Pavel Dovgalyuk |
Subject: |
Re: [Qemu-devel] [PATCH v6 21/25] replay: replay BH for IDE trim operation |
Date: |
Mon, 17 Sep 2018 15:00:20 +0300 |
> 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
- [Qemu-devel] [PATCH v6 18/25] replay: describe reverse debugging in docs/replay.txt, (continued)
- Re: [Qemu-devel] [PATCH v6 21/25] replay: replay BH for IDE trim operation, John Snow, 2018/09/14
- Re: [Qemu-devel] [PATCH v6 21/25] replay: replay BH for IDE trim operation, Paolo Bonzini, 2018/09/14
- Re: [Qemu-devel] [PATCH v6 21/25] replay: replay BH for IDE trim operation, Peter Maydell, 2018/09/14
- Re: [Qemu-devel] [PATCH v6 21/25] replay: replay BH for IDE trim operation, Pavel Dovgalyuk, 2018/09/17
- Re: [Qemu-devel] [PATCH v6 21/25] replay: replay BH for IDE trim operation, Paolo Bonzini, 2018/09/17
[Qemu-devel] [PATCH v6 22/25] replay: add BH oneshot event for block layer, Pavel Dovgalyuk, 2018/09/12