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

[John Snow <jsnow@redhat.com>]

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?