diff mbox series

[PULL,13/25] hcd-ohci: Fix inconsistency when resetting ohci root hubs

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

Commit Message

Gerd Hoffmann Sept. 26, 2022, 9:54 a.m. UTC 
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(-)

Comments

Stefan Hajnoczi Sept. 27, 2022, 1:11 a.m. UTC | #1 
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
>
>
Qiang Liu Sept. 28, 2022, 12:24 p.m. UTC | #2 
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
> >
> >
>
Qiang Liu Feb. 15, 2023, 1:45 p.m. UTC | #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
Stefan Hajnoczi Feb. 15, 2023, 2:34 p.m. UTC | #4 
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
Laurent Vivier Feb. 15, 2023, 4:10 p.m. UTC | #5 
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
Laurent Vivier Feb. 15, 2023, 4:28 p.m. UTC | #6 
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
BALATON Zoltan Feb. 15, 2023, 5:05 p.m. UTC | #7 
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 mbox series

Patch

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;