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.Dovgaluk@ispras.ru>
Reviewed-by: Paolo Bonzini <pbonzini@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);
     }
 }
 

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.Dovgaluk@ispras.ru>
> Reviewed-by: Paolo Bonzini <pbonzini@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?

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?

--js

> From: John Snow [mailto:jsnow@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.Dovgaluk@ispras.ru>
> > Reviewed-by: Paolo Bonzini <pbonzini@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?

Maybe not. We can hardly analyze all peripheral devices code and fix all the calls.
But I think we can improve that patch and at least look through ide core to fix other calls.

Pavel Dovgalyuk

On 09/14/2018 01:48 AM, Pavel Dovgalyuk wrote:
>> From: John Snow [mailto:jsnow@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.Dovgaluk@ispras.ru>
>>> Reviewed-by: Paolo Bonzini <pbonzini@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?
> 
> Maybe not. We can hardly analyze all peripheral devices code and fix all the calls.
> But I think we can improve that patch and at least look through ide core to fix other calls.
> 
> Pavel Dovgalyuk
> 

It just seems odd that if you're working on a replay mechanism that
requires you to intercept my QEMU API calls that you're only changing a
trim callback.

I'd kind of expect that you don't need to intercept any, unless these
are legacy calls that I shouldn't be making at all and you have a more
generic intercept somewhere deeper in the codebase.

In that case, I really ought to hustle off of my use of legacy calls.

What are the criteria for things you need to intercept/wrap?

On 14/09/2018 16:00, John Snow wrote:
>> Maybe not. We can hardly analyze all peripheral devices code and fix all the calls.
>> But I think we can improve that patch and at least look through ide core to fix other calls.
>>
>> Pavel Dovgalyuk
>>
> It just seems odd that if you're working on a replay mechanism that
> requires you to intercept my QEMU API calls that you're only changing a
> trim callback.

You need it only here because the block layer is already calling
replay_bh_schedule_event (actually replay_bh_schedule_oneshot_event
after patch 22 of this series) on reads and writes.

Paolo

> I'd kind of expect that you don't need to intercept any, unless these
> are legacy calls that I shouldn't be making at all and you have a more
> generic intercept somewhere deeper in the codebase.
> 
> In that case, I really ought to hustle off of my use of legacy calls.
> 
> What are the criteria for things you need to intercept/wrap?

On 14 September 2018 at 16:19, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 14/09/2018 16:00, John Snow wrote:
>>> Maybe not. We can hardly analyze all peripheral devices code and fix all the calls.
>>> But I think we can improve that patch and at least look through ide core to fix other calls.
>>>
>>> Pavel Dovgalyuk
>>>
>> It just seems odd that if you're working on a replay mechanism that
>> requires you to intercept my QEMU API calls that you're only changing a
>> trim callback.
>
> You need it only here because the block layer is already calling
> replay_bh_schedule_event (actually replay_bh_schedule_oneshot_event
> after patch 22 of this series) on reads and writes.

Do we have documentation describing when a device model needs to care
about record/replay ?

thanks
-- PMM

> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> On 14 September 2018 at 16:19, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > On 14/09/2018 16:00, John Snow wrote:
> >>> Maybe not. We can hardly analyze all peripheral devices code and fix all the calls.
> >>> But I think we can improve that patch and at least look through ide core to fix other
> calls.
> >>>
> >>> Pavel Dovgalyuk
> >>>
> >> It just seems odd that if you're working on a replay mechanism that
> >> requires you to intercept my QEMU API calls that you're only changing a
> >> trim callback.
> >
> > You need it only here because the block layer is already calling
> > replay_bh_schedule_event (actually replay_bh_schedule_oneshot_event
> > after patch 22 of this series) on reads and writes.
> 
> Do we have documentation describing when a device model needs to care
> about record/replay ?

Not yet.
But how we are suppose this doc to be seen by everyone?

Pavel Dovgalyuk

On 17/09/2018 13:57, Pavel Dovgalyuk wrote:
>> From: Peter Maydell [mailto:peter.maydell@linaro.org]
>> On 14 September 2018 at 16:19, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> On 14/09/2018 16:00, John Snow wrote:
>>>>> Maybe not. We can hardly analyze all peripheral devices code and fix all the calls.
>>>>> But I think we can improve that patch and at least look through ide core to fix other
>> calls.
>>>>>
>>>>> Pavel Dovgalyuk
>>>>>
>>>> It just seems odd that if you're working on a replay mechanism that
>>>> requires you to intercept my QEMU API calls that you're only changing a
>>>> trim callback.
>>>
>>> You need it only here because the block layer is already calling
>>> replay_bh_schedule_event (actually replay_bh_schedule_oneshot_event
>>> after patch 22 of this series) on reads and writes.
>>
>> Do we have documentation describing when a device model needs to care
>> about record/replay ?
> 
> Not yet.
> But how we are suppose this doc to be seen by everyone?

Put it in docs/devel.

Paolo

> From: Pavel Dovgalyuk [mailto:dovgaluk@ispras.ru]
> > From: John Snow [mailto:jsnow@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.Dovgaluk@ispras.ru>
> > > Reviewed-by: Paolo Bonzini <pbonzini@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?

Pavel Dovgalyuk

On 09/14/2018 03:27 AM, Pavel Dovgalyuk wrote:
>> From: Pavel Dovgalyuk [mailto:dovgaluk@ispras.ru]
>>> From: John Snow [mailto:jsnow@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.Dovgaluk@ispras.ru>
>>>> Reviewed-by: Paolo Bonzini <pbonzini@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?
> 
> 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

> From: John Snow [mailto:jsnow@redhat.com]
> On 09/14/2018 03:27 AM, Pavel Dovgalyuk wrote:
> >> From: Pavel Dovgalyuk [mailto:dovgaluk@ispras.ru]
> >>> From: John Snow [mailto:jsnow@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.Dovgaluk@ispras.ru>
> >>>> Reviewed-by: Paolo Bonzini <pbonzini@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

On 09/17/2018 08:00 AM, Pavel Dovgalyuk wrote:
>> From: John Snow [mailto:jsnow@redhat.com]
>> On 09/14/2018 03:27 AM, Pavel Dovgalyuk wrote:
>>>> From: Pavel Dovgalyuk [mailto:dovgaluk@ispras.ru]
>>>>> From: John Snow [mailto:jsnow@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.Dovgaluk@ispras.ru>
>>>>>> Reviewed-by: Paolo Bonzini <pbonzini@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
> 

This is part of the rerror/werror=stop model where if we hit a host IO
problem we pause the guest instead of reporting the error. When we
re-engage the VM, the IO is re-submitted, which may change guest-visible
data.

BTW, I'm fine with the changes presented so far, I was just trying to
understand them.

Paolo, please go ahead.

--js