> From: Markus Armbruster [mailto:armbru@redhat.com]
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 09.01.2020 um 07:13 hat Pavel Dovgalyuk geschrieben:
> >> Ping.
> >
> > I think you have my Acked-by for the block-related patches in this
> > series now. If I missed something, please let me know.
> 
> Pavel, whom are you nudging to do what?

I'm not sure.
My prior patches for record/replay were pulled by Paolo.
But reverse debugging is more like a modification of things,
and not a completely new subsystem. 
Everything but gdbstub was already acked by the maintainers.

Pavel Dovgalyuk

Am 13.01.2020 um 10:35 hat Pavel Dovgalyuk geschrieben:
> > From: Markus Armbruster [mailto:armbru@redhat.com]
> > Kevin Wolf <kwolf@redhat.com> writes:
> > 
> > > Am 09.01.2020 um 07:13 hat Pavel Dovgalyuk geschrieben:
> > >> Ping.
> > >
> > > I think you have my Acked-by for the block-related patches in this
> > > series now. If I missed something, please let me know.
> > 
> > Pavel, whom are you nudging to do what?
> 
> I'm not sure.
> My prior patches for record/replay were pulled by Paolo.
> But reverse debugging is more like a modification of things,
> and not a completely new subsystem. 

In MAINTAINERS, you are listed yourself as the maintainer for
record/replay. I wonder whether you shouldn't just be sending pull
requests after getting Acked-by or Reviewed-by from the maintainers of
other subsystems you touch.

> Everything but gdbstub was already acked by the maintainers.

The GDB stub seems to be maintained by Alex Bennée, so he is probably
the one you need to nudge to give at least an Acked-by.

Kevin

On Mon, 13 Jan 2020 at 10:07, Kevin Wolf <kwolf@redhat.com> wrote:
> In MAINTAINERS, you are listed yourself as the maintainer for
> record/replay. I wonder whether you shouldn't just be sending pull
> requests after getting Acked-by or Reviewed-by from the maintainers of
> other subsystems you touch.

Ideally somebody else should be interested enough in record/replay
to review patches. "I'm a subsystem maintainer and send pull
requests" ideally shouldn't be something we give out just because
patches aren't getting code review, though I know that it
does sometimes degenerate into that...

thanks
-- PMM

Am 13.01.2020 um 11:14 hat Peter Maydell geschrieben:
> On Mon, 13 Jan 2020 at 10:07, Kevin Wolf <kwolf@redhat.com> wrote:
> > In MAINTAINERS, you are listed yourself as the maintainer for
> > record/replay. I wonder whether you shouldn't just be sending pull
> > requests after getting Acked-by or Reviewed-by from the maintainers of
> > other subsystems you touch.
> 
> Ideally somebody else should be interested enough in record/replay
> to review patches. "I'm a subsystem maintainer and send pull
> requests" ideally shouldn't be something we give out just because
> patches aren't getting code review, though I know that it
> does sometimes degenerate into that...

I had the impression that he said he had collected (almost) all of the
necessary reviews, but nobody really seems to be interested to take the
series through their tree because no matter who you ask, the majority of
changes will always be for other subsystems.

And as record/replay is already listed as a separate subsystem in
MAINTAINERS, it seems to make sense to me that it also gets its own pull
requests rather than trying to get patches merged though the trees of
various subsystem maintainers who all aren't really responsible for it.

Kevin

On Mon, 13 Jan 2020 at 10:27, Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 13.01.2020 um 11:14 hat Peter Maydell geschrieben:
> > On Mon, 13 Jan 2020 at 10:07, Kevin Wolf <kwolf@redhat.com> wrote:
> > > In MAINTAINERS, you are listed yourself as the maintainer for
> > > record/replay. I wonder whether you shouldn't just be sending pull
> > > requests after getting Acked-by or Reviewed-by from the maintainers of
> > > other subsystems you touch.
> >
> > Ideally somebody else should be interested enough in record/replay
> > to review patches. "I'm a subsystem maintainer and send pull
> > requests" ideally shouldn't be something we give out just because
> > patches aren't getting code review, though I know that it
> > does sometimes degenerate into that...
>
> I had the impression that he said he had collected (almost) all of the
> necessary reviews, but nobody really seems to be interested to take the
> series through their tree because no matter who you ask, the majority of
> changes will always be for other subsystems.

No, the series has only got acked-bys so far, except for the one patch
that touches qapi, which Markus reviewed. (The bulk of the changes
here are in replay/, and so far nobody's looked at those AFAIK.)

> And as record/replay is already listed as a separate subsystem in
> MAINTAINERS, it seems to make sense to me that it also gets its own pull
> requests rather than trying to get patches merged though the trees of
> various subsystem maintainers who all aren't really responsible for it.

Yeah, pull requests would be fine. Pull requests of whole
patchsets that are basically unreviewed are something I think
we should try to avoid if we can.

thanks
-- PMM

(looping the list back in)

Pavel Dovgalyuk <dovgaluk@ispras.ru> writes:

>> From: Alex Bennée [mailto:alex.bennee@linaro.org]
>> Pavel Dovgalyuk <dovgaluk@ispras.ru> writes:
>> 
>> >> From: Markus Armbruster [mailto:armbru@redhat.com]
>> >> Kevin Wolf <kwolf@redhat.com> writes:
>> >>
>> >> > Am 09.01.2020 um 07:13 hat Pavel Dovgalyuk geschrieben:
>> >> >> Ping.
>> >> >
>> >> > I think you have my Acked-by for the block-related patches in this
>> >> > series now. If I missed something, please let me know.
>> >>
>> >> Pavel, whom are you nudging to do what?
>> >
>> > I'm not sure.
>> > My prior patches for record/replay were pulled by Paolo.
>> > But reverse debugging is more like a modification of things,
>> > and not a completely new subsystem.
>> > Everything but gdbstub was already acked by the maintainers.
>> 
>> I'm having a look at the series now. What would help is if we can have
>> some sort of test case to ensure expected behaviour is protected. We
>> have a basic smoke test for record/replay and my gdbstub series:
>> 
>>   https://github.com/stsquad/qemu/tree/gdbstub/sve-registers-v5
>> 
>> introduces some infrastructure for testing thing via gdbstub. The main
>> thing needed now is some sort of gdb capability test to ensure the gdb
>> we are using supports whatever extensions it needs to work.
>
> The smoke test may be rather easy:
> 1. Execute qemu record with qcow2 image and rrsnapshot
> 2. Start qemu replay with -S
> 3. Connect gdb to qemu
> 4. stepi
> 5. break $pc
> 6. Save pc to X
> 7. stepi 10
> 8. Save pc to Y
> 9. stepi
> 10. reverse-stepi
> 11. pc should be Y
> 12. reverse-continue
> 13. pc should be X
>
> Is it possible with your infrastructure?

I think so - even if the reverse step instructions are not in the python
API the script can always do:

 gdb.parse_and_eval("command")

The main thing is ensuring probing the gdb for a version/feature first
so we can safely exit and skip the test if the host gdb isn't capable.
For the SVE testing I've had to use my own compiled version of GDB which
is a little ugly:

 ~/lsrc/qemu.git/tests/guest-debug/run-test.py \
   --qemu ./aarch64-linux-user/qemu-aarch64 \
   --qargs "-cpu max" \
   --bin ./tests/tcg/aarch64-linux-user/sve-ioctls \
   --test ~/lsrc/qemu.git/tests/tcg/aarch64/gdbstub/test-sve-ioctl.py \
   --gdb ~/src/tools/binutils-gdb.git/builds/all/install/bin/gdb

We should probably wrap that up into the tests/tcg configure.sh code so
the GDB used can persist with the build. Maybe via:

  ./configure --with-gdb=~/src/tools/binutils-gdb.git/builds/all/install/bin/gdb

-- 
Alex Bennée