Re: [PULL 00/47] nic-config.for-upstream queue
On Mon, 5 Feb 2024 at 10:11, Thomas Huth 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 wrote: > >>> > >>> On Fri, 2024-02-02 at 15:32 +, 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
Re: [PULL 00/47] nic-config.for-upstream queue
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 wrote:
On Fri, 2024-02-02 at 15:32 +, 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
Re: [PULL 00/47] nic-config.for-upstream queue
On 02/02/2024 16.40, Peter Maydell wrote: On Fri, 2 Feb 2024 at 15:36, David Woodhouse wrote: On Fri, 2024-02-02 at 15:32 +, 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
Re: [PULL 00/47] nic-config.for-upstream queue
On Fri, 2024-02-02 at 16:15 +, Peter Maydell wrote: > On Fri, 2 Feb 2024 at 16:01, David Woodhouse 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.) smime.p7s Description: S/MIME cryptographic signature
Re: [PULL 00/47] nic-config.for-upstream queue
On Fri, 2 Feb 2024 at 16:01, David Woodhouse wrote:
>
> On Fri, 2024-02-02 at 15:32 +, 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
Re: [PULL 00/47] nic-config.for-upstream queue
On Fri, 2024-02-02 at 15:32 +, 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.
smime.p7s
Description: S/MIME cryptographic signature
Re: [PULL 00/47] nic-config.for-upstream queue
On Fri, 2 Feb 2024 at 15:36, David Woodhouse wrote: > > On Fri, 2024-02-02 at 15:32 +, 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
Re: [PULL 00/47] nic-config.for-upstream queue
On Fri, 2024-02-02 at 15:32 +, 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... smime.p7s Description: S/MIME cryptographic signature
Re: [PULL 00/47] nic-config.for-upstream queue
On Thu, 1 Feb 2024 at 16:48, David Woodhouse 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 +) > > 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 > +) > > > 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
