Message ID | 20220528192057.30910-1-shentey@gmail.com (mailing list archive) |
---|---|
Headers | show |
Series | QOM'ify PIIX southbridge creation | expand |
On 28/05/2022 20:20, Bernhard Beschow wrote: > v3: > * Rebase onto 'hw/acpi/piix4: remove legacy piix4_pm_init() function' (Mark) [1] > * Use embedded structs for touched PCI devices (Mark) > * Fix piix4's rtc embedded struct to be initialized by > object_initialize_child() (Peter) [2] > > Testing done: > > 1) > `make check-avocado` for --target-list=x86_64-softmmu,mips-softmmu > Result: All pass. > > 2) > * `qemu-system-x86_64 -M pc -m 2G -cdrom archlinux-2022.05.01-x86_64.iso` > * `qemu-system-mipsel -M malta -kernel vmlinux-3.2.0-4-4kc-malta -hda > debian_wheezy_mipsel_standard.qcow2 -append "root=/dev/sda1 console=tty0"` > > In both cases the system booted successfully and it was possible to shut down > the system using the `poweroff` command. > > > v2: > * Preserve `DeviceState *` as return value of piix4_create() (Mark) > * Aggregate all type name movements into first commit (Mark) > * Have piix4 southbridge rather than malta board instantiate piix4 pm (me) > > Testing done: > > 1) > `make check-avocado` for --target-list=x86_64-softmmu,mips-softmmu > Result: All pass. > > 2) > Modify pci_piix3_realize() to start with > error_setg(errp, "This is a test"); > Then start `qemu-system-x86_64 -M pc -m 1G -accel kvm -cpu host -cdrom > archlinux-2022.05.01-x86_64.iso`. > Result: qemu-system-x86_64 aborts with: "This is a test" > > > v1: > The piix3 and piix4 southbridge devices still rely on create() functions which > are deprecated. This series resolves these functions piece by piece to > modernize the code. > > Both devices are modified in lockstep where possible to provide more context. > > Testing done: > * `qemu-system-x86_64 -M pc -m 2G -cdrom archlinux-2022.05.01-x86_64.iso` > * `qemu-system-mipsel -M malta -kernel vmlinux-3.2.0-4-4kc-malta -hda > debian_wheezy_mipsel_standard.qcow2 -append "root=/dev/sda1 console=tty0"` > > In both cases the system booted successfully and it was possible to shut down > the system using the `poweroff` command. > > [1] https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg05686.html > [2] https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg01128.html > > Bernhard Beschow (7): > include/hw/southbridge/piix: Aggregate all PIIX soughbridge type names > hw/isa/piix4: Use object_initialize_child() for embedded struct > hw/isa/piix{3,4}: Move pci_map_irq_fn's near pci_set_irq_fn's > hw/isa/piix{3,4}: QOM'ify PCI device creation and wiring > hw/isa/piix{3,4}: Factor out ISABus retrieval from create() functions > hw/isa/piix4: QOM'ify PIIX4 PM creation > hw/isa/piix{3,4}: Inline and remove create() functions > > hw/i386/pc_piix.c | 7 +- > hw/isa/piix3.c | 98 ++++++++++++++------------- > hw/isa/piix4.c | 120 +++++++++++++++++----------------- > hw/mips/malta.c | 7 +- > include/hw/isa/isa.h | 2 - > include/hw/southbridge/piix.h | 6 +- > 6 files changed, 127 insertions(+), 113 deletions(-) Hi Bernhard, I've spotted a couple of small things, but once those are fixed this series looks good to me so I'm happy to give a: Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> Thanks for your patience with these series too: you've done some good work here, however patchsets like this can sometimes take a while to get reviewed because they both i) touch legacy code/APIs and ii) cut across multiple machines and maintainers. I'd really like to get this work, along with your RTC updates, merged soon as it is a great improvement. One reason that you may not get many reviews is because you've not added the relevant maintainers on To/CC. Due to the large volume of emails on qemu-devel, quite a few maintainers will filter based upon their own email address so it is definitely worth adding them in. Fortunately you can easily find the relevant maintainer email addresses by running "./scripts/get_maintainer.pl <path-to-git-patch-dir>" on your git format-patch directory. I'd recommend doing this, and also dropping qemu-trivial since I would say these patches are now beyond the trivial threshold. ATB, Mark.
On 29/05/2022 10:46, Mark Cave-Ayland wrote: > On 28/05/2022 20:20, Bernhard Beschow wrote: > >> v3: >> * Rebase onto 'hw/acpi/piix4: remove legacy piix4_pm_init() function' (Mark) [1] >> * Use embedded structs for touched PCI devices (Mark) >> * Fix piix4's rtc embedded struct to be initialized by >> object_initialize_child() (Peter) [2] >> >> Testing done: >> >> 1) >> `make check-avocado` for --target-list=x86_64-softmmu,mips-softmmu >> Result: All pass. >> >> 2) >> * `qemu-system-x86_64 -M pc -m 2G -cdrom archlinux-2022.05.01-x86_64.iso` >> * `qemu-system-mipsel -M malta -kernel vmlinux-3.2.0-4-4kc-malta -hda >> debian_wheezy_mipsel_standard.qcow2 -append "root=/dev/sda1 console=tty0"` >> >> In both cases the system booted successfully and it was possible to shut down >> the system using the `poweroff` command. >> >> >> v2: >> * Preserve `DeviceState *` as return value of piix4_create() (Mark) >> * Aggregate all type name movements into first commit (Mark) >> * Have piix4 southbridge rather than malta board instantiate piix4 pm (me) >> >> Testing done: >> >> 1) >> `make check-avocado` for --target-list=x86_64-softmmu,mips-softmmu >> Result: All pass. >> >> 2) >> Modify pci_piix3_realize() to start with >> error_setg(errp, "This is a test"); >> Then start `qemu-system-x86_64 -M pc -m 1G -accel kvm -cpu host -cdrom >> archlinux-2022.05.01-x86_64.iso`. >> Result: qemu-system-x86_64 aborts with: "This is a test" >> >> >> v1: >> The piix3 and piix4 southbridge devices still rely on create() functions which >> are deprecated. This series resolves these functions piece by piece to >> modernize the code. >> >> Both devices are modified in lockstep where possible to provide more context. >> >> Testing done: >> * `qemu-system-x86_64 -M pc -m 2G -cdrom archlinux-2022.05.01-x86_64.iso` >> * `qemu-system-mipsel -M malta -kernel vmlinux-3.2.0-4-4kc-malta -hda >> debian_wheezy_mipsel_standard.qcow2 -append "root=/dev/sda1 console=tty0"` >> >> In both cases the system booted successfully and it was possible to shut down >> the system using the `poweroff` command. >> >> [1] https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg05686.html >> [2] https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg01128.html >> >> Bernhard Beschow (7): >> include/hw/southbridge/piix: Aggregate all PIIX soughbridge type names >> hw/isa/piix4: Use object_initialize_child() for embedded struct >> hw/isa/piix{3,4}: Move pci_map_irq_fn's near pci_set_irq_fn's >> hw/isa/piix{3,4}: QOM'ify PCI device creation and wiring >> hw/isa/piix{3,4}: Factor out ISABus retrieval from create() functions >> hw/isa/piix4: QOM'ify PIIX4 PM creation >> hw/isa/piix{3,4}: Inline and remove create() functions >> >> hw/i386/pc_piix.c | 7 +- >> hw/isa/piix3.c | 98 ++++++++++++++------------- >> hw/isa/piix4.c | 120 +++++++++++++++++----------------- >> hw/mips/malta.c | 7 +- >> include/hw/isa/isa.h | 2 - >> include/hw/southbridge/piix.h | 6 +- >> 6 files changed, 127 insertions(+), 113 deletions(-) > > Hi Bernhard, > > I've spotted a couple of small things, but once those are fixed this series looks > good to me so I'm happy to give a: > > Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > > Thanks for your patience with these series too: you've done some good work here, > however patchsets like this can sometimes take a while to get reviewed because they > both i) touch legacy code/APIs and ii) cut across multiple machines and maintainers. > I'd really like to get this work, along with your RTC updates, merged soon as it is a > great improvement. > > One reason that you may not get many reviews is because you've not added the relevant > maintainers on To/CC. Due to the large volume of emails on qemu-devel, quite a few > maintainers will filter based upon their own email address so it is definitely worth > adding them in. > > Fortunately you can easily find the relevant maintainer email addresses by running > "./scripts/get_maintainer.pl <path-to-git-patch-dir>" on your git format-patch > directory. I'd recommend doing this, and also dropping qemu-trivial since I would say > these patches are now beyond the trivial threshold. Oh wait - I see now it's just the cover letter which is missing the additional maintainer addresses :) If you could add them into the cover letter for your next revision that would be great, since it gives context for maintainers to help with the review process. ATB, Mark.
On Sun, May 29, 2022 at 12:06 PM Mark Cave-Ayland < mark.cave-ayland@ilande.co.uk> wrote: > On 29/05/2022 10:46, Mark Cave-Ayland wrote: > > > On 28/05/2022 20:20, Bernhard Beschow wrote: > > > >> v3: > >> * Rebase onto 'hw/acpi/piix4: remove legacy piix4_pm_init() function' > (Mark) [1] > >> * Use embedded structs for touched PCI devices (Mark) > >> * Fix piix4's rtc embedded struct to be initialized by > >> object_initialize_child() (Peter) [2] > >> > >> Testing done: > >> > >> 1) > >> `make check-avocado` for --target-list=x86_64-softmmu,mips-softmmu > >> Result: All pass. > >> > >> 2) > >> * `qemu-system-x86_64 -M pc -m 2G -cdrom > archlinux-2022.05.01-x86_64.iso` > >> * `qemu-system-mipsel -M malta -kernel vmlinux-3.2.0-4-4kc-malta -hda > >> debian_wheezy_mipsel_standard.qcow2 -append "root=/dev/sda1 > console=tty0"` > >> > >> In both cases the system booted successfully and it was possible to > shut down > >> the system using the `poweroff` command. > >> > >> > >> v2: > >> * Preserve `DeviceState *` as return value of piix4_create() (Mark) > >> * Aggregate all type name movements into first commit (Mark) > >> * Have piix4 southbridge rather than malta board instantiate piix4 pm > (me) > >> > >> Testing done: > >> > >> 1) > >> `make check-avocado` for --target-list=x86_64-softmmu,mips-softmmu > >> Result: All pass. > >> > >> 2) > >> Modify pci_piix3_realize() to start with > >> error_setg(errp, "This is a test"); > >> Then start `qemu-system-x86_64 -M pc -m 1G -accel kvm -cpu host -cdrom > >> archlinux-2022.05.01-x86_64.iso`. > >> Result: qemu-system-x86_64 aborts with: "This is a test" > >> > >> > >> v1: > >> The piix3 and piix4 southbridge devices still rely on create() > functions which > >> are deprecated. This series resolves these functions piece by piece to > >> modernize the code. > >> > >> Both devices are modified in lockstep where possible to provide more > context. > >> > >> Testing done: > >> * `qemu-system-x86_64 -M pc -m 2G -cdrom > archlinux-2022.05.01-x86_64.iso` > >> * `qemu-system-mipsel -M malta -kernel vmlinux-3.2.0-4-4kc-malta -hda > >> debian_wheezy_mipsel_standard.qcow2 -append "root=/dev/sda1 > console=tty0"` > >> > >> In both cases the system booted successfully and it was possible to > shut down > >> the system using the `poweroff` command. > >> > >> [1] https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg05686.html > >> [2] https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg01128.html > >> > >> Bernhard Beschow (7): > >> include/hw/southbridge/piix: Aggregate all PIIX soughbridge type > names > >> hw/isa/piix4: Use object_initialize_child() for embedded struct > >> hw/isa/piix{3,4}: Move pci_map_irq_fn's near pci_set_irq_fn's > >> hw/isa/piix{3,4}: QOM'ify PCI device creation and wiring > >> hw/isa/piix{3,4}: Factor out ISABus retrieval from create() functions > >> hw/isa/piix4: QOM'ify PIIX4 PM creation > >> hw/isa/piix{3,4}: Inline and remove create() functions > >> > >> hw/i386/pc_piix.c | 7 +- > >> hw/isa/piix3.c | 98 ++++++++++++++------------- > >> hw/isa/piix4.c | 120 +++++++++++++++++----------------- > >> hw/mips/malta.c | 7 +- > >> include/hw/isa/isa.h | 2 - > >> include/hw/southbridge/piix.h | 6 +- > >> 6 files changed, 127 insertions(+), 113 deletions(-) > > > > Hi Bernhard, > > > > I've spotted a couple of small things, but once those are fixed this > series looks > > good to me so I'm happy to give a: > > > > Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > > > > Thanks for your patience with these series too: you've done some good > work here, > > however patchsets like this can sometimes take a while to get reviewed > because they > > both i) touch legacy code/APIs and ii) cut across multiple machines and > maintainers. > > I'd really like to get this work, along with your RTC updates, merged > soon as it is a > > great improvement. > > > > One reason that you may not get many reviews is because you've not added > the relevant > > maintainers on To/CC. Due to the large volume of emails on qemu-devel, > quite a few > > maintainers will filter based upon their own email address so it is > definitely worth > > adding them in. > > > > Fortunately you can easily find the relevant maintainer email addresses > by running > > "./scripts/get_maintainer.pl <path-to-git-patch-dir>" on your git > format-patch > > directory. I'd recommend doing this, and also dropping qemu-trivial > since I would say > > these patches are now beyond the trivial threshold. > > Oh wait - I see now it's just the cover letter which is missing the > additional > maintainer addresses :) If you could add them into the cover letter for > your next > revision that would be great, since it gives context for maintainers to > help with the > review process. > Hi Mark, Thanks for your great work you put into reviews and the useful insights! It seems to me that the time it takes for patches to be queued depends on the subsystem - some are faster, some are slower... I've automated my setup as described in [1]. However, it doesn't seem to work for the cover letter which I'd expect to be sent to the union of all reviewers of all patches. Any idea how to fix this? Best regards, Bernhard [1] https://www.qemu.org/docs/master/devel/submitting-a-patch.html#cc-the-relevant-maintainer > > > ATB, > > Mark. >
On 29/05/2022 14:02, Bernhard Beschow wrote: > On Sun, May 29, 2022 at 12:06 PM Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk > <mailto:mark.cave-ayland@ilande.co.uk>> wrote: > > On 29/05/2022 10:46, Mark Cave-Ayland wrote: > > > On 28/05/2022 20:20, Bernhard Beschow wrote: > > > >> v3: > >> * Rebase onto 'hw/acpi/piix4: remove legacy piix4_pm_init() function' (Mark) [1] > >> * Use embedded structs for touched PCI devices (Mark) > >> * Fix piix4's rtc embedded struct to be initialized by > >> object_initialize_child() (Peter) [2] > >> > >> Testing done: > >> > >> 1) > >> `make check-avocado` for --target-list=x86_64-softmmu,mips-softmmu > >> Result: All pass. > >> > >> 2) > >> * `qemu-system-x86_64 -M pc -m 2G -cdrom archlinux-2022.05.01-x86_64.iso` > >> * `qemu-system-mipsel -M malta -kernel vmlinux-3.2.0-4-4kc-malta -hda > >> debian_wheezy_mipsel_standard.qcow2 -append "root=/dev/sda1 console=tty0"` > >> > >> In both cases the system booted successfully and it was possible to shut down > >> the system using the `poweroff` command. > >> > >> > >> v2: > >> * Preserve `DeviceState *` as return value of piix4_create() (Mark) > >> * Aggregate all type name movements into first commit (Mark) > >> * Have piix4 southbridge rather than malta board instantiate piix4 pm (me) > >> > >> Testing done: > >> > >> 1) > >> `make check-avocado` for --target-list=x86_64-softmmu,mips-softmmu > >> Result: All pass. > >> > >> 2) > >> Modify pci_piix3_realize() to start with > >> error_setg(errp, "This is a test"); > >> Then start `qemu-system-x86_64 -M pc -m 1G -accel kvm -cpu host -cdrom > >> archlinux-2022.05.01-x86_64.iso`. > >> Result: qemu-system-x86_64 aborts with: "This is a test" > >> > >> > >> v1: > >> The piix3 and piix4 southbridge devices still rely on create() functions which > >> are deprecated. This series resolves these functions piece by piece to > >> modernize the code. > >> > >> Both devices are modified in lockstep where possible to provide more context. > >> > >> Testing done: > >> * `qemu-system-x86_64 -M pc -m 2G -cdrom archlinux-2022.05.01-x86_64.iso` > >> * `qemu-system-mipsel -M malta -kernel vmlinux-3.2.0-4-4kc-malta -hda > >> debian_wheezy_mipsel_standard.qcow2 -append "root=/dev/sda1 console=tty0"` > >> > >> In both cases the system booted successfully and it was possible to shut down > >> the system using the `poweroff` command. > >> > >> [1] https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg05686.html > <https://lists.gnu.org/archive/html/qemu-devel/2022-05/msg05686.html> > >> [2] https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg01128.html > <https://lists.gnu.org/archive/html/qemu-devel/2022-02/msg01128.html> > >> > >> Bernhard Beschow (7): > >> include/hw/southbridge/piix: Aggregate all PIIX soughbridge type names > >> hw/isa/piix4: Use object_initialize_child() for embedded struct > >> hw/isa/piix{3,4}: Move pci_map_irq_fn's near pci_set_irq_fn's > >> hw/isa/piix{3,4}: QOM'ify PCI device creation and wiring > >> hw/isa/piix{3,4}: Factor out ISABus retrieval from create() functions > >> hw/isa/piix4: QOM'ify PIIX4 PM creation > >> hw/isa/piix{3,4}: Inline and remove create() functions > >> > >> hw/i386/pc_piix.c | 7 +- > >> hw/isa/piix3.c | 98 ++++++++++++++------------- > >> hw/isa/piix4.c | 120 +++++++++++++++++----------------- > >> hw/mips/malta.c | 7 +- > >> include/hw/isa/isa.h | 2 - > >> include/hw/southbridge/piix.h | 6 +- > >> 6 files changed, 127 insertions(+), 113 deletions(-) > > > > Hi Bernhard, > > > > I've spotted a couple of small things, but once those are fixed this series looks > > good to me so I'm happy to give a: > > > > Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk > <mailto:mark.cave-ayland@ilande.co.uk>> > > > > Thanks for your patience with these series too: you've done some good work here, > > however patchsets like this can sometimes take a while to get reviewed because > they > > both i) touch legacy code/APIs and ii) cut across multiple machines and > maintainers. > > I'd really like to get this work, along with your RTC updates, merged soon as > it is a > > great improvement. > > > > One reason that you may not get many reviews is because you've not added the > relevant > > maintainers on To/CC. Due to the large volume of emails on qemu-devel, quite a > few > > maintainers will filter based upon their own email address so it is definitely > worth > > adding them in. > > > > Fortunately you can easily find the relevant maintainer email addresses by > running > > "./scripts/get_maintainer.pl <http://get_maintainer.pl> > <path-to-git-patch-dir>" on your git format-patch > > directory. I'd recommend doing this, and also dropping qemu-trivial since I > would say > > these patches are now beyond the trivial threshold. > > Oh wait - I see now it's just the cover letter which is missing the additional > maintainer addresses :) If you could add them into the cover letter for your next > revision that would be great, since it gives context for maintainers to help with > the > review process. > > > Hi Mark, > > Thanks for your great work you put into reviews and the useful insights! It seems to > me that the time it takes for patches to be queued depends on the subsystem - some > are faster, some are slower... > > I've automated my setup as described in [1]. However, it doesn't seem to work for the > cover letter which I'd expect to be sent to the union of all reviewers of all > patches. Any idea how to fix this? > > Best regards, > Bernhard > > [1] > https://www.qemu.org/docs/master/devel/submitting-a-patch.html#cc-the-relevant-maintainer > <https://www.qemu.org/docs/master/devel/submitting-a-patch.html#cc-the-relevant-maintainer> Good question. I tend to do "git format-patch -o /tmp/foo --cover-letter" to generate the series, fill in the cover letter, and then use "git send-email /tmp/foo" to send out the emails (entering in the results of get_maintainer.pl by hand). I'm not sure why the cover letter isn't being generated correctly in your case I'm afraid. ATB, Mark.
On 30/5/22 21:11, Mark Cave-Ayland wrote: > On 29/05/2022 14:02, Bernhard Beschow wrote: >> Oh wait - I see now it's just the cover letter which is missing >> the additional >> maintainer addresses :) If you could add them into the cover >> letter for your next >> revision that would be great, since it gives context for >> maintainers to help with >> the >> review process. >> >> >> Hi Mark, >> >> Thanks for your great work you put into reviews and the useful >> insights! It seems to me that the time it takes for patches to be >> queued depends on the subsystem - some are faster, some are slower... >> >> I've automated my setup as described in [1]. However, it doesn't seem >> to work for the cover letter which I'd expect to be sent to the union >> of all reviewers of all patches. Any idea how to fix this? >> >> Best regards, >> Bernhard >> >> [1] >> https://www.qemu.org/docs/master/devel/submitting-a-patch.html#cc-the-relevant-maintainer >> <https://www.qemu.org/docs/master/devel/submitting-a-patch.html#cc-the-relevant-maintainer> > > > Good question. I tend to do "git format-patch -o /tmp/foo > --cover-letter" to generate the series, fill in the cover letter, and > then use "git send-email /tmp/foo" to send out the emails (entering in > the results of get_maintainer.pl by hand). I'm not sure why the cover > letter isn't being generated correctly in your case I'm afraid. Or try git-publish :) It does a first pass collecting Cc for each patch (calling get_maintainer.pl) then use that set on the cover. https://github.com/stefanha/git-publish
Am 30. Mai 2022 19:45:26 UTC schrieb "Philippe Mathieu-Daudé" <f4bug@amsat.org>: >On 30/5/22 21:11, Mark Cave-Ayland wrote: >> On 29/05/2022 14:02, Bernhard Beschow wrote: > >>> Oh wait - I see now it's just the cover letter which is missing the additional >>> maintainer addresses :) If you could add them into the cover letter for your next >>> revision that would be great, since it gives context for maintainers to help with >>> the >>> review process. >>> >>> >>> Hi Mark, >>> >>> Thanks for your great work you put into reviews and the useful insights! It seems to me that the time it takes for patches to be queued depends on the subsystem - some are faster, some are slower... >>> >>> I've automated my setup as described in [1]. However, it doesn't seem to work for the cover letter which I'd expect to be sent to the union of all reviewers of all patches. Any idea how to fix this? >>> >>> Best regards, >>> Bernhard >>> >>> [1] https://www.qemu.org/docs/master/devel/submitting-a-patch.html#cc-the-relevant-maintainer <https://www.qemu.org/docs/master/devel/submitting-a-patch.html#cc-the-relevant-maintainer> >> >> >> Good question. I tend to do "git format-patch -o /tmp/foo --cover-letter" to generate the series, fill in the cover letter, and then use "git send-email /tmp/foo" to send out the emails (entering in the results of get_maintainer.pl by hand). I'm not sure why the cover letter isn't being generated correctly in your case I'm afraid. > >Or try git-publish :) It does a first pass collecting Cc for each patch >(calling get_maintainer.pl) then use that set on the cover. > >https://github.com/stefanha/git-publish Yes, that worked. What an improvement! Best regards, Bernhard