| Message ID | 20240201164412.785520-1-dwmw2@infradead.org |
|---|---|
| State | New |
| Headers | show |
On Thu, 1 Feb 2024 at 16:48, David Woodhouse <dwmw2@infradead.org> wrote: > > The following changes since commit 14639717bf379480e937716fcaf1e72b47fd4c5f: > > Merge tag 'pull-trivial-patches' of https://gitlab.com/mjt0k/qemu into staging (2024-01-31 19:53:45 +0000) > > are available in the Git repository at: > > git://git.infradead.org/users/dwmw2/qemu.git tags/pull-nic-config.for-upstream-20240201 > > for you to fetch changes up to 5382a24c9b0be4391ea91b020bb72ad15c05cc88: > > net: make nb_nics and nd_table[] static in net/net.c (2024-02-01 16:21:52 +0000) > > ---------------------------------------------------------------- > Rework matching of network devices to -nic options This fails "make check' because some of the qom-test and test-hmp checks fail when the QEMU binary segfaults. https://gitlab.com/qemu-project/qemu/-/jobs/6084552256 https://gitlab.com/qemu-project/qemu/-/jobs/6084044180 Reproduces on an x86-64 host if you do a build and 'make check' for all target archs. Generally this kind of segfault is because some machine type segfaults on startup. For instance: $ ./build/all/qemu-system-hppa -M C3700 Segmentation fault (core dumped) (It's possible that's the only machine type that has a problem; I haven't tried to exhaustively check.) -- PMM
On Fri, 2024-02-02 at 15:32 +0000, Peter Maydell wrote: > > This fails "make check' because some of the qom-test and > test-hmp checks fail when the QEMU binary segfaults. > > https://gitlab.com/qemu-project/qemu/-/jobs/6084552256 > https://gitlab.com/qemu-project/qemu/-/jobs/6084044180 Thanks. Any idea why that didn't show up in my own pipeline? https://gitlab.com/dwmw2/qemu/-/pipelines/1160949234 (The only difference in that tree is a last minute Reviewed-by:) > Reproduces on an x86-64 host if you do a build and 'make check' > for all target archs. > > Generally this kind of segfault is because some machine type > segfaults on startup. For instance: > > $ ./build/all/qemu-system-hppa -M C3700 > Segmentation fault (core dumped) > > (It's possible that's the only machine type that has a > problem; I haven't tried to exhaustively check.) Sorry about that. I'll go improve my test coverage...
On Fri, 2 Feb 2024 at 15:36, David Woodhouse <dwmw2@infradead.org> wrote: > > On Fri, 2024-02-02 at 15:32 +0000, Peter Maydell wrote: > > > > This fails "make check' because some of the qom-test and > > test-hmp checks fail when the QEMU binary segfaults. > > > > https://gitlab.com/qemu-project/qemu/-/jobs/6084552256 > > https://gitlab.com/qemu-project/qemu/-/jobs/6084044180 > > Thanks. Any idea why that didn't show up in my own pipeline? > https://gitlab.com/dwmw2/qemu/-/pipelines/1160949234 I think because the failing runners are the aarch64 and s390 host ones, which we don't let run for anything except real merge-pullreq test runs because they're limited resource. I guess that perhaps we have at some point said "we don't need to run all the guest architectures on all jobs" and not noticed that this leaves the coverage for the submaintainer only-uses-the-public-runners CI testing with gaps. CCing Alex and Thomas for possible suggestions. thanks -- PMM
On Fri, 2024-02-02 at 15:32 +0000, Peter Maydell wrote: > > $ ./build/all/qemu-system-hppa -M C3700 > Segmentation fault (core dumped) Ah, got it. Some HPPA machine types don't have a lasi_dev. --- a/hw/hppa/machine.c +++ b/hw/hppa/machine.c @@ -362,9 +362,11 @@ static void machine_HP_common_init_tail(MachineState *machine, PCIBus *pci_bus, } /* Network setup. */ - lasi_82596_init(addr_space, translate(NULL, LASI_LAN_HPA), - qdev_get_gpio_in(lasi_dev, LASI_IRQ_LAN_HPA), - enable_lasi_lan()); + if (lasi_dev) { + lasi_82596_init(addr_space, translate(NULL, LASI_LAN_HPA), + qdev_get_gpio_in(lasi_dev, LASI_IRQ_LAN_HPA), + enable_lasi_lan()); + } pci_init_nic_devices(pci_bus, mc->default_nic); New pipeline running (FWIW) at https://gitlab.com/dwmw2/qemu/-/pipelines/1162635873 What is the next step? Post the full series as a v5, or perhaps just the single fixed patch which is now at https://git.infradead.org/?p=users/dwmw2/qemu.git;a=commitdiff;h=2c20b4ee96db ... and then another pull request? The docs are fairly clear that pull requests can't have even minor changes that haven't been posted separately... and I guess the above incremental doesn't count? Sorry for the noise.
On Fri, 2 Feb 2024 at 16:01, David Woodhouse <dwmw2@infradead.org> wrote: > > On Fri, 2024-02-02 at 15:32 +0000, Peter Maydell wrote: > > > > $ ./build/all/qemu-system-hppa -M C3700 > > Segmentation fault (core dumped) > > Ah, got it. Some HPPA machine types don't have a lasi_dev. > > > --- a/hw/hppa/machine.c > +++ b/hw/hppa/machine.c > @@ -362,9 +362,11 @@ static void machine_HP_common_init_tail(MachineState *machine, PCIBus *pci_bus, > } > > /* Network setup. */ > - lasi_82596_init(addr_space, translate(NULL, LASI_LAN_HPA), > - qdev_get_gpio_in(lasi_dev, LASI_IRQ_LAN_HPA), > - enable_lasi_lan()); > + if (lasi_dev) { > + lasi_82596_init(addr_space, translate(NULL, LASI_LAN_HPA), > + qdev_get_gpio_in(lasi_dev, LASI_IRQ_LAN_HPA), > + enable_lasi_lan()); > + } > > pci_init_nic_devices(pci_bus, mc->default_nic); > > New pipeline running (FWIW) at > https://gitlab.com/dwmw2/qemu/-/pipelines/1162635873 > > What is the next step? Post the full series as a v5, or perhaps just > the single fixed patch which is now at > https://git.infradead.org/?p=users/dwmw2/qemu.git;a=commitdiff;h=2c20b4ee96db > > ... and then another pull request? If that diff above is the only change, then: * roll a new pullrequest with the fix squashed into the appropriate patch * have the subject marker be "PULL v2" * you can send just the cover-letter and the one patch that has changed, you don't need to resend the entire series (though it's not a big deal if you do send the whole set of mails again) > The docs are fairly clear that pull > requests can't have even minor changes that haven't been posted > separately... and I guess the above incremental doesn't count? Which bit of the docs is that? It's not our actual practice, so we should really fix the wording. The principle is "don't stick code into pullreqs that hasn't been through the review process", but in practice especially for submaintainers who know the system it's not uncommon to say "I'm going to squash change X in and take this" or similar rather than forcing submitters to do another round of sending out patches. There should be *something* on the list to say this change was put in, but eg the exchange in this email thread is fine for that. thanks -- PMM
On Fri, 2024-02-02 at 16:15 +0000, Peter Maydell wrote: > On Fri, 2 Feb 2024 at 16:01, David Woodhouse <dwmw2@infradead.org> wrote: > > > > What is the next step? Post the full series as a v5, or perhaps just > > the single fixed patch which is now at > > https://git.infradead.org/?p=users/dwmw2/qemu.git;a=commitdiff;h=2c20b4ee96db > > > > ... and then another pull request? > > If that diff above is the only change, then: > * roll a new pullrequest with the fix squashed into the appropriate patch > * have the subject marker be "PULL v2" > * you can send just the cover-letter and the one patch that has changed, > you don't need to resend the entire series (though it's not a big > deal if you do send the whole set of mails again) Done, thank you. > > The docs are fairly clear that pull > > requests can't have even minor changes that haven't been posted > > separately... and I guess the above incremental doesn't count? > > Which bit of the docs is that? It's not our actual practice, > so we should really fix the wording. The principle is "don't > stick code into pullreqs that hasn't been through the review > process", but in practice especially for submaintainers who > know the system it's not uncommon to say "I'm going to > squash change X in and take this" or similar rather than > forcing submitters to do another round of sending out patches. > There should be *something* on the list to say this change was > put in, but eg the exchange in this email thread is fine for that. I think I was looking at https://www.qemu.org/docs/master/devel/submitting-a-pull-request.html where it says "In particular if you’ve corrected issues in one round of code review, you need to send your fixed patch series as normal to the list; you can’t put it in a pull request until it’s gone through. (Extremely trivial fixes may be OK to just fix in passing, but if in doubt err on the side of not.)" I think the doc is actually fine and I was just reading it too conservatively. I have been making a conscious effort to pay attention, follow the process and fit in, rather than just turning up and doing whatever comes naturally from decades of working on open source. (I do not *promise* that will last.)
On 02/02/2024 16.40, Peter Maydell wrote: > On Fri, 2 Feb 2024 at 15:36, David Woodhouse <dwmw2@infradead.org> wrote: >> >> On Fri, 2024-02-02 at 15:32 +0000, Peter Maydell wrote: >>> >>> This fails "make check' because some of the qom-test and >>> test-hmp checks fail when the QEMU binary segfaults. >>> >>> https://gitlab.com/qemu-project/qemu/-/jobs/6084552256 >>> https://gitlab.com/qemu-project/qemu/-/jobs/6084044180 >> >> Thanks. Any idea why that didn't show up in my own pipeline? >> https://gitlab.com/dwmw2/qemu/-/pipelines/1160949234 > > I think because the failing runners are the aarch64 and > s390 host ones, which we don't let run for anything > except real merge-pullreq test runs because they're > limited resource. I guess that perhaps we have at some point > said "we don't need to run all the guest architectures > on all jobs" It's rather "we cannot run all the guest architectures on all jobs due to time constraints" > and not noticed that this leaves the > coverage for the submaintainer only-uses-the-public-runners > CI testing with gaps. > > CCing Alex and Thomas for possible suggestions. Well, not everybody has access to non-x86 machines, so there's not too much we can do about this, can we? The only possibility is: We still have support for travis-CI, so if you've got a github account, you can test aarch64, s390x and ppc64le there before sending a pull request. The .travis-ci.yml file needs some love, though, one of the jobs is currently timing out ... it's on my TODO list since weeks, but I just didn't find enough spare time to fix it properly yet. Thomas
On 05/02/2024 07.56, Thomas Huth wrote: > On 02/02/2024 16.40, Peter Maydell wrote: >> On Fri, 2 Feb 2024 at 15:36, David Woodhouse <dwmw2@infradead.org> wrote: >>> >>> On Fri, 2024-02-02 at 15:32 +0000, Peter Maydell wrote: >>>> >>>> This fails "make check' because some of the qom-test and >>>> test-hmp checks fail when the QEMU binary segfaults. >>>> >>>> https://gitlab.com/qemu-project/qemu/-/jobs/6084552256 >>>> https://gitlab.com/qemu-project/qemu/-/jobs/6084044180 >>> >>> Thanks. Any idea why that didn't show up in my own pipeline? >>> https://gitlab.com/dwmw2/qemu/-/pipelines/1160949234 >> >> I think because the failing runners are the aarch64 and >> s390 host ones, which we don't let run for anything >> except real merge-pullreq test runs because they're >> limited resource. I guess that perhaps we have at some point >> said "we don't need to run all the guest architectures >> on all jobs" > > It's rather "we cannot run all the guest architectures on all jobs due to > time constraints" Ah, wait, but we should still run at least "make check" for each target architecture... so there's indeed something that went wrong recently: commit 78ebc00b06813 ("gitlab: shuffle some targets and reduce avocado noise") removed the hppa-softmmu target from the ubuntu job, without making sure that it gets tested somewhere else. Alex, why did you remove it? It's now missing from all check-system-* jobs... Thomas
On Mon, 5 Feb 2024 at 10:11, Thomas Huth <thuth@redhat.com> wrote: > > On 05/02/2024 07.56, Thomas Huth wrote: > > On 02/02/2024 16.40, Peter Maydell wrote: > >> On Fri, 2 Feb 2024 at 15:36, David Woodhouse <dwmw2@infradead.org> wrote: > >>> > >>> On Fri, 2024-02-02 at 15:32 +0000, Peter Maydell wrote: > >>>> > >>>> This fails "make check' because some of the qom-test and > >>>> test-hmp checks fail when the QEMU binary segfaults. > >>>> > >>>> https://gitlab.com/qemu-project/qemu/-/jobs/6084552256 > >>>> https://gitlab.com/qemu-project/qemu/-/jobs/6084044180 > >>> > >>> Thanks. Any idea why that didn't show up in my own pipeline? > >>> https://gitlab.com/dwmw2/qemu/-/pipelines/1160949234 > >> > >> I think because the failing runners are the aarch64 and > >> s390 host ones, which we don't let run for anything > >> except real merge-pullreq test runs because they're > >> limited resource. I guess that perhaps we have at some point > >> said "we don't need to run all the guest architectures > >> on all jobs" > > > > It's rather "we cannot run all the guest architectures on all jobs due to > > time constraints" > > Ah, wait, but we should still run at least "make check" for each target > architecture... Yes, this is what I mean -- it's OK to have the target architecture checks split up between different CI jobs as long as between the jobs we cover them all, but it's not so good to be relying on the non-x86 host jobs to provide part of the coverage. -- PMM
The following changes since commit 14639717bf379480e937716fcaf1e72b47fd4c5f: Merge tag 'pull-trivial-patches' of https://gitlab.com/mjt0k/qemu into staging (2024-01-31 19:53:45 +0000) are available in the Git repository at: git://git.infradead.org/users/dwmw2/qemu.git tags/pull-nic-config.for-upstream-20240201 for you to fetch changes up to 5382a24c9b0be4391ea91b020bb72ad15c05cc88: net: make nb_nics and nd_table[] static in net/net.c (2024-02-01 16:21:52 +0000) ---------------------------------------------------------------- Rework matching of network devices to -nic options ---------------------------------------------------------------- David Woodhouse (47): net: add qemu_{configure,create}_nic_device(), qemu_find_nic_info() net: report list of available models according to platform net: add qemu_create_nic_bus_devices() hw/pci: add pci_init_nic_devices(), pci_init_nic_in_slot() hw/i386/pc: use qemu_get_nic_info() and pci_init_nic_devices() hw/xen: use qemu_create_nic_bus_devices() to instantiate Xen NICs hw/alpha/dp264: use pci_init_nic_devices() hw/arm/sbsa-ref: use pci_init_nic_devices() hw/arm/virt: use pci_init_nic_devices() hw/hppa: use pci_init_nic_devices() hw/loongarch: use pci_init_nic_devices() hw/mips/fuloong2e: use pci_init_nic_devices() hw/mips/malta: use pci_init_nic_devices() hw/mips/loongson3_virt: use pci_init_nic_devices() hw/ppc/prep: use pci_init_nic_devices() hw/ppc/spapr: use qemu_get_nic_info() and pci_init_nic_devices() hw/ppc: use pci_init_nic_devices() hw/sh4/r2d: use pci_init_nic_devices() hw/sparc64/sun4u: use pci_init_nic_devices() hw/xtensa/virt: use pci_init_nic_devices() hw/arm/allwinner: use qemu_configure_nic_device() hw/arm/aspeed: use qemu_configure_nic_device() hw/arm/exynos4: use qemu_create_nic_device() hw/arm/fsl: use qemu_configure_nic_device() hw/net/smc91c111: use qemu_configure_nic_device() hw/net/lan9118: use qemu_configure_nic_device() hw/arm/highbank: use qemu_create_nic_device() hw/arm/npcm7xx: use qemu_configure_nic_device, allow emc0/emc1 as aliases hw/arm/stellaris: use qemu_find_nic_info() hw/arm: use qemu_configure_nic_device() hw/net/etraxfs-eth: use qemu_configure_nic_device() hw/m68k/mcf5208: use qemu_create_nic_device() hw/m68k/q800: use qemu_find_nic_info() hw/microblaze: use qemu_configure_nic_device() hw/mips/mipssim: use qemu_create_nic_device() hw/mips/jazz: use qemu_find_nic_info() hw/net/lasi_i82596: Re-enable build hw/net/lasi_i82596: use qemu_create_nic_device() hw/openrisc/openrisc_sim: use qemu_create_nic_device() hw/riscv: use qemu_configure_nic_device() hw/s390x/s390-virtio-ccw: use qemu_create_nic_device() hw/sparc/sun4m: use qemu_find_nic_info() hw/xtensa/xtfpga: use qemu_create_nic_device() net: remove qemu_check_nic_model() hw/pci: remove pci_nic_init_nofail() net: remove qemu_show_nic_models(), qemu_find_nic_model() net: make nb_nics and nd_table[] static in net/net.c hw/alpha/dp264.c | 4 +- hw/arm/allwinner-a10.c | 6 +- hw/arm/allwinner-h3.c | 6 +- hw/arm/allwinner-r40.c | 27 +--- hw/arm/aspeed.c | 9 +- hw/arm/exynos4_boards.c | 6 +- hw/arm/fsl-imx25.c | 2 +- hw/arm/fsl-imx6.c | 2 +- hw/arm/fsl-imx6ul.c | 2 +- hw/arm/fsl-imx7.c | 2 +- hw/arm/gumstix.c | 6 +- hw/arm/highbank.c | 12 +- hw/arm/integratorcp.c | 5 +- hw/arm/kzm.c | 4 +- hw/arm/mainstone.c | 3 +- hw/arm/mps2-tz.c | 8 +- hw/arm/mps2.c | 2 +- hw/arm/msf2-soc.c | 6 +- hw/arm/musicpal.c | 3 +- hw/arm/npcm7xx.c | 16 +- hw/arm/realview.c | 25 ++- hw/arm/sbsa-ref.c | 4 +- hw/arm/stellaris.c | 30 +++- hw/arm/versatilepb.c | 15 +- hw/arm/vexpress.c | 4 +- hw/arm/virt.c | 4 +- hw/arm/xilinx_zynq.c | 11 +- hw/arm/xlnx-versal.c | 7 +- hw/arm/xlnx-zynqmp.c | 8 +- hw/cris/axis_dev88.c | 9 +- hw/hppa/machine.c | 14 +- hw/i386/pc.c | 38 +++-- hw/i386/pc_piix.c | 2 +- hw/i386/pc_q35.c | 2 +- hw/loongarch/virt.c | 4 +- hw/m68k/mcf5208.c | 19 +-- hw/m68k/q800.c | 29 ++-- hw/microblaze/petalogix_ml605_mmu.c | 3 +- hw/microblaze/petalogix_s3adsp1800_mmu.c | 3 +- hw/mips/fuloong2e.c | 16 +- hw/mips/jazz.c | 15 +- hw/mips/loongson3_virt.c | 4 +- hw/mips/malta.c | 15 +- hw/mips/mipssim.c | 13 +- hw/net/etraxfs_eth.c | 5 +- hw/net/lan9118.c | 5 +- hw/net/lasi_i82596.c | 13 +- hw/net/meson.build | 2 +- hw/net/smc91c111.c | 5 +- hw/openrisc/openrisc_sim.c | 18 +-- hw/pci/pci.c | 73 +++------ hw/ppc/e500.c | 4 +- hw/ppc/mac_newworld.c | 4 +- hw/ppc/mac_oldworld.c | 4 +- hw/ppc/ppc440_bamboo.c | 14 +- hw/ppc/prep.c | 8 +- hw/ppc/spapr.c | 18 +-- hw/riscv/microchip_pfsoc.c | 14 +- hw/riscv/sifive_u.c | 7 +- hw/s390x/s390-virtio-ccw.c | 11 +- hw/sh4/r2d.c | 6 +- hw/sparc/sun4m.c | 20 ++- hw/sparc64/sun4u.c | 27 +--- hw/xen/xen-bus.c | 6 +- hw/xen/xen_devconfig.c | 25 --- hw/xenpv/xen_machine_pv.c | 9 -- hw/xtensa/virt.c | 4 +- hw/xtensa/xtfpga.c | 13 +- include/hw/cris/etraxfs.h | 2 +- include/hw/i386/pc.h | 4 +- include/hw/net/lan9118.h | 2 +- include/hw/net/lasi_82596.h | 4 +- include/hw/net/ne2000-isa.h | 2 - include/hw/net/smc91c111.h | 2 +- include/hw/pci/pci.h | 7 +- include/hw/xen/xen-bus.h | 2 +- include/hw/xen/xen-legacy-backend.h | 1 - include/net/net.h | 70 ++++++++- net/net.c | 253 +++++++++++++++++++++++++------ system/globals.c | 2 - tests/qtest/npcm7xx_emc-test.c | 18 +-- 81 files changed, 547 insertions(+), 562 deletions(-)