Message ID | 20220926095509.3759409-14-kraxel@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [PULL,01/25] ui/console: Get tab completion working again in the SDL monitor vc | expand |
On Mon, 26 Sept 2022 at 06:29, Gerd Hoffmann <kraxel@redhat.com> wrote: > > From: Qiang Liu <cyruscyliu@gmail.com> > > I found an assertion failure in usb_cancel_packet() and posted my analysis in > https://gitlab.com/qemu-project/qemu/-/issues/1180. I think this issue is > because the inconsistency when resetting ohci root hubs. > > There are two ways to reset ohci root hubs: 1) through HcRhPortStatus, 2) > through HcControl. However, when the packet's status is USB_PACKET_ASYNC, > resetting through HcRhPortStatus will complete the packet and thus resetting > through HcControl will fail. That is because IMO resetting through > HcRhPortStatus should first detach the port and then invoked usb_device_reset() > just like through HcControl. Therefore, I change usb_device_reset() to > usb_port_reset() where usb_detach() and usb_device_reset() are invoked > consequently. > > Fixes: d28f4e2d8631 ("usb: kill USB_MSG_RESET") > Reported-by: Qiang Liu <cyruscyliu@gmail.com> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1180 > Signed-off-by: Qiang Liu <cyruscyliu@gmail.com> > Message-Id: <20220830033022.1164961-1-cyruscyliu@gmail.com> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > hw/usb/hcd-ohci.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) This commit breaks boot-serial-test on ppc64-softmmu. $ ./configure --enable-tcg-interpreter '--target-list=aarch64-softmmu alpha-softmmu arm-softmmu hppa-softmmu m68k-softmmu microblaze-softmmu ppc64-softmmu s390x-softmmu x86_64-softmmu' $ make && cd build && QTEST_QEMU_BINARY=./qemu-system-ppc64 ./tests/qtest/boot-serial-test; cd - (Yes, the full --target-list is needed because boot-serial-test isn't built when only ppc64-softmmu is selected.) Here is the CI failure: https://gitlab.com/qemu-project/qemu/-/jobs/3087540972#L22 Stefan > > diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c > index 72bdde92617c..28d703481515 100644 > --- a/hw/usb/hcd-ohci.c > +++ b/hw/usb/hcd-ohci.c > @@ -1436,7 +1436,7 @@ static void ohci_port_set_status(OHCIState *ohci, int portnum, uint32_t val) > > if (ohci_port_set_if_connected(ohci, portnum, val & OHCI_PORT_PRS)) { > trace_usb_ohci_port_reset(portnum); > - usb_device_reset(port->port.dev); > + usb_port_reset(&port->port); > port->ctrl &= ~OHCI_PORT_PRS; > /* ??? Should this also set OHCI_PORT_PESC. */ > port->ctrl |= OHCI_PORT_PES | OHCI_PORT_PRSC; > -- > 2.37.3 > >
Hi, I will take over this and fix it Best, Qiang On Tue, Sep 27, 2022 at 9:11 AM Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Mon, 26 Sept 2022 at 06:29, Gerd Hoffmann <kraxel@redhat.com> wrote: > > > > From: Qiang Liu <cyruscyliu@gmail.com> > > > > I found an assertion failure in usb_cancel_packet() and posted my > analysis in > > https://gitlab.com/qemu-project/qemu/-/issues/1180. I think this issue > is > > because the inconsistency when resetting ohci root hubs. > > > > There are two ways to reset ohci root hubs: 1) through HcRhPortStatus, 2) > > through HcControl. However, when the packet's status is USB_PACKET_ASYNC, > > resetting through HcRhPortStatus will complete the packet and thus > resetting > > through HcControl will fail. That is because IMO resetting through > > HcRhPortStatus should first detach the port and then invoked > usb_device_reset() > > just like through HcControl. Therefore, I change usb_device_reset() to > > usb_port_reset() where usb_detach() and usb_device_reset() are invoked > > consequently. > > > > Fixes: d28f4e2d8631 ("usb: kill USB_MSG_RESET") > > Reported-by: Qiang Liu <cyruscyliu@gmail.com> > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1180 > > Signed-off-by: Qiang Liu <cyruscyliu@gmail.com> > > Message-Id: <20220830033022.1164961-1-cyruscyliu@gmail.com> > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > --- > > hw/usb/hcd-ohci.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > This commit breaks boot-serial-test on ppc64-softmmu. > > $ ./configure --enable-tcg-interpreter > '--target-list=aarch64-softmmu alpha-softmmu arm-softmmu hppa-softmmu > m68k-softmmu microblaze-softmmu ppc64-softmmu s390x-softmmu > x86_64-softmmu' > $ make && cd build && QTEST_QEMU_BINARY=./qemu-system-ppc64 > ./tests/qtest/boot-serial-test; cd - > > (Yes, the full --target-list is needed because boot-serial-test isn't > built when only ppc64-softmmu is selected.) > > Here is the CI failure: > https://gitlab.com/qemu-project/qemu/-/jobs/3087540972#L22 > > Stefan > > > > > diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c > > index 72bdde92617c..28d703481515 100644 > > --- a/hw/usb/hcd-ohci.c > > +++ b/hw/usb/hcd-ohci.c > > @@ -1436,7 +1436,7 @@ static void ohci_port_set_status(OHCIState *ohci, > int portnum, uint32_t val) > > > > if (ohci_port_set_if_connected(ohci, portnum, val & OHCI_PORT_PRS)) > { > > trace_usb_ohci_port_reset(portnum); > > - usb_device_reset(port->port.dev); > > + usb_port_reset(&port->port); > > port->ctrl &= ~OHCI_PORT_PRS; > > /* ??? Should this also set OHCI_PORT_PESC. */ > > port->ctrl |= OHCI_PORT_PES | OHCI_PORT_PRSC; > > -- > > 2.37.3 > > > > >
Hi, > This commit breaks boot-serial-test on ppc64-softmmu. >> >> $ ./configure --enable-tcg-interpreter >> '--target-list=aarch64-softmmu alpha-softmmu arm-softmmu hppa-softmmu >> m68k-softmmu microblaze-softmmu ppc64-softmmu s390x-softmmu >> x86_64-softmmu' >> $ make && cd build && QTEST_QEMU_BINARY=./qemu-system-ppc64 >> ./tests/qtest/boot-serial-test; cd - >> >> (Yes, the full --target-list is needed because boot-serial-test isn't >> built when only ppc64-softmmu is selected.) >> >> Here is the CI failure: >> https://gitlab.com/qemu-project/qemu/-/jobs/3087540972#L22 > > I reproduced this failure and got "Out of malloc memory" error message in the [openbios-ppc]( https://github.com/openbios/openbios/blob/4a0041107b8ef77e0e8337bfcb5f8078887261a7/libopenbios/ofmem_common.c#L134). However, I'm not sure how to debug this. Have you run into this issue before? Best, Qiang
On Wed, 15 Feb 2023 at 08:45, Qiang Liu <cyruscyliu@gmail.com> wrote: > > Hi, >>> >>> This commit breaks boot-serial-test on ppc64-softmmu. >>> >>> $ ./configure --enable-tcg-interpreter >>> '--target-list=aarch64-softmmu alpha-softmmu arm-softmmu hppa-softmmu >>> m68k-softmmu microblaze-softmmu ppc64-softmmu s390x-softmmu >>> x86_64-softmmu' >>> $ make && cd build && QTEST_QEMU_BINARY=./qemu-system-ppc64 >>> ./tests/qtest/boot-serial-test; cd - >>> >>> (Yes, the full --target-list is needed because boot-serial-test isn't >>> built when only ppc64-softmmu is selected.) >>> >>> Here is the CI failure: >>> https://gitlab.com/qemu-project/qemu/-/jobs/3087540972#L22 > > I reproduced this failure and got "Out of malloc memory" error message in the [openbios-ppc](https://github.com/openbios/openbios/blob/4a0041107b8ef77e0e8337bfcb5f8078887261a7/libopenbios/ofmem_common.c#L134). However, I'm not sure how to debug this. Have you run into this issue before? I don't. Maybe Gerd has an idea? The memory allocation may be because there is either a request leak or additional USB activity as a result of this patch. Stefan
On 2/15/23 14:45, Qiang Liu wrote: > Hi, > > This commit breaks boot-serial-test on ppc64-softmmu. > > $ ./configure --enable-tcg-interpreter > '--target-list=aarch64-softmmu alpha-softmmu arm-softmmu hppa-softmmu > m68k-softmmu microblaze-softmmu ppc64-softmmu s390x-softmmu > x86_64-softmmu' > $ make && cd build && QTEST_QEMU_BINARY=./qemu-system-ppc64 > ./tests/qtest/boot-serial-test; cd - > > (Yes, the full --target-list is needed because boot-serial-test isn't > built when only ppc64-softmmu is selected.) > I think we need something like this : diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build index e97616d327c0..8203f6a71ad0 100644 --- a/tests/qtest/meson.build +++ b/tests/qtest/meson.build @@ -149,8 +149,8 @@ qtests_ppc = \ qtests_filter + \ (config_all_devices.has_key('CONFIG_ISA_TESTDEV') ? ['endianness-test'] : []) + \ (config_all_devices.has_key('CONFIG_M48T59') ? ['m48t59-test'] : []) + \ - (config_all_devices.has_key('CONFIG_TCG') ? ['prom-env-test'] : []) + \ - (config_all_devices.has_key('CONFIG_TCG') ? ['boot-serial-test'] : []) + \ + (config_host.has_key('CONFIG_TCG') ? ['prom-env-test'] : []) + \ + (config_host.has_key('CONFIG_TCG') ? ['boot-serial-test'] : []) + \ ['boot-order-test'] qtests_ppc64 = \ Thanks, Laurent
On 2/15/23 15:34, Stefan Hajnoczi wrote: > On Wed, 15 Feb 2023 at 08:45, Qiang Liu <cyruscyliu@gmail.com> wrote: >> >> Hi, >>>> >>>> This commit breaks boot-serial-test on ppc64-softmmu. >>>> >>>> $ ./configure --enable-tcg-interpreter >>>> '--target-list=aarch64-softmmu alpha-softmmu arm-softmmu hppa-softmmu >>>> m68k-softmmu microblaze-softmmu ppc64-softmmu s390x-softmmu >>>> x86_64-softmmu' >>>> $ make && cd build && QTEST_QEMU_BINARY=./qemu-system-ppc64 >>>> ./tests/qtest/boot-serial-test; cd - >>>> >>>> (Yes, the full --target-list is needed because boot-serial-test isn't >>>> built when only ppc64-softmmu is selected.) >>>> >>>> Here is the CI failure: >>>> https://gitlab.com/qemu-project/qemu/-/jobs/3087540972#L22 >> >> I reproduced this failure and got "Out of malloc memory" error message in the [openbios-ppc](https://github.com/openbios/openbios/blob/4a0041107b8ef77e0e8337bfcb5f8078887261a7/libopenbios/ofmem_common.c#L134). However, I'm not sure how to debug this. Have you run into this issue before? > > I don't. Maybe Gerd has an idea? > > The memory allocation may be because there is either a request leak or > additional USB activity as a result of this patch. It looks like a bug in openbios ohci, perhaps Zoltan can help? Thanks, Laurent
On Wed, 15 Feb 2023, Laurent Vivier wrote: > On 2/15/23 15:34, Stefan Hajnoczi wrote: >> On Wed, 15 Feb 2023 at 08:45, Qiang Liu <cyruscyliu@gmail.com> wrote: >>> >>> Hi, >>>>> >>>>> This commit breaks boot-serial-test on ppc64-softmmu. >>>>> >>>>> $ ./configure --enable-tcg-interpreter >>>>> '--target-list=aarch64-softmmu alpha-softmmu arm-softmmu hppa-softmmu >>>>> m68k-softmmu microblaze-softmmu ppc64-softmmu s390x-softmmu >>>>> x86_64-softmmu' >>>>> $ make && cd build && QTEST_QEMU_BINARY=./qemu-system-ppc64 >>>>> ./tests/qtest/boot-serial-test; cd - >>>>> >>>>> (Yes, the full --target-list is needed because boot-serial-test isn't >>>>> built when only ppc64-softmmu is selected.) >>>>> >>>>> Here is the CI failure: >>>>> https://gitlab.com/qemu-project/qemu/-/jobs/3087540972#L22 >>> >>> I reproduced this failure and got "Out of malloc memory" error message >>> in the >>> [openbios-ppc](https://github.com/openbios/openbios/blob/4a0041107b8ef77e0e8337bfcb5f8078887261a7/libopenbios/ofmem_common.c#L134). >>> However, I'm not sure how to debug this. Have you run into this issue >>> before? >> >> I don't. Maybe Gerd has an idea? >> >> The memory allocation may be because there is either a request leak or >> additional USB activity as a result of this patch. > > It looks like a bug in openbios ohci, perhaps Zoltan can help? Unfortunately I don't quite understand neither what this thread is about nor that openbios driver. Even though I've added that to openbios, all I did was porting the driver from coreboot's libpayload as noted in the copyright message of that file. So if you suspect it's a bug there you could try checking newer versions in libpayload and see if they've fixed anything that could help. On the other hand the OHCI emulation in QEMU is known to be incomplete and likely to have some bugs with guests that work on real hardware (e.g. MacOS <= 10.1) so it's more likely that QEMU behaves differently than it should assuming that the coreboot driver works on real hardware but I'm not sure about that either. Regards, BALATON Zoltan
diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c index 72bdde92617c..28d703481515 100644 --- a/hw/usb/hcd-ohci.c +++ b/hw/usb/hcd-ohci.c @@ -1436,7 +1436,7 @@ static void ohci_port_set_status(OHCIState *ohci, int portnum, uint32_t val) if (ohci_port_set_if_connected(ohci, portnum, val & OHCI_PORT_PRS)) { trace_usb_ohci_port_reset(portnum); - usb_device_reset(port->port.dev); + usb_port_reset(&port->port); port->ctrl &= ~OHCI_PORT_PRS; /* ??? Should this also set OHCI_PORT_PESC. */ port->ctrl |= OHCI_PORT_PES | OHCI_PORT_PRSC;