[Qemu-devel] [PATCH v2 0/2] ohci: try to mimic real hardware command latency

[Qemu-devel] [PATCH v2 0/2] ohci: try to mimic real hardware command latency

[Laurent Vivier]

OHCI linux driver has some critical sections not protected against
device interrupts. Because of real hardware latency, it is generally
not a problem as interrupts cannot be triggered fast enough to happen
during these critical sections.

But theoretically, it can happen. And with QEMU used on an overcommitted
CPU, the vCPU becomes slow enough and it happens.

This series fixes a kernel crash on boot (CPU stuck) when the OHCI driver
tries to resume or suspend the device.

v2: Address comments from Thomas
    update code comments
    split ohci_sof() to add ohci_eof_timer()

Laurent Vivier (2):
  ohci: delay first SOF interrupt
  ohci: clear pending SOF on suspend

 hw/usb/hcd-ohci.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 1/2] ohci: delay first SOF interrupt

[Laurent Vivier]

On overcommitted CPU, kernel can be so slow that an interrupt can
be triggered by the device whereas the driver is not ready to receive
it. This drives us into an infinite loop.

This does not happen on real hardware because real hardware never send
interrupt immediately after the controller has been moved to OPERATION state.

This patch tries to delay the first SOF interrupt to let driver exits from
the critical section (which is not protected against interrupts...)

Some details:

- ohci_irq(): the OHCI interrupt handler, acknowledges the SOF IRQ
  only if the state of the driver (rh_state) is OHCI_STATE_RUNNING.
  So if this interrupt happens and the driver is not in this state,
  the function is called again and again, moving the system to a
  CPU starvation.

- ohci_rh_resume(): the driver re-enables operation with OHCI_USB_OPER.
  In QEMU this start the SOF timer and QEMU starts to send IRQs. As
  the driver is not in OHCI_STATE_RUNNING and not protected against IRQ,
  the ohci_irq() can be called and the driver never moved to
  OHCI_STATE_RUNNING.

Suggested-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 hw/usb/hcd-ohci.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index 7d65818..a44fab2 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -1231,11 +1231,16 @@ static int ohci_service_ed_list(OHCIState *ohci, uint32_t head, int completion)
     return active;
 }
 
-/* Generate a SOF event, and set a timer for EOF */
-static void ohci_sof(OHCIState *ohci)
+/* set a timer for EOF */
+static void ohci_eof_timer(OHCIState *ohci)
 {
     ohci->sof_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
     timer_mod(ohci->eof_timer, ohci->sof_time + usb_frame_time);
+}
+/* Set a timer for EOF and generate a SOF event */
+static void ohci_sof(OHCIState *ohci)
+{
+    ohci_eof_timer(ohci);
     ohci_set_interrupt(ohci, OHCI_INTR_SF);
 }
 
@@ -1343,7 +1348,12 @@ static int ohci_bus_start(OHCIState *ohci)
 
     trace_usb_ohci_start(ohci->name);
 
-    ohci_sof(ohci);
+    /* Delay the first SOF event by one frame time as
+     * linux driver is not ready to receive it and
+     * can meet some race conditions
+     */
+
+    ohci_eof_timer(ohci);
 
     return 1;
 }
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 2/2] ohci: clear pending SOF on suspend

[Laurent Vivier]

On overcommitted CPU, kernel can be so slow that an interrupt can
be triggered by the device whereas the driver is not ready to receive
it. This drives us into an infinite loop.

On suspend, if a SOF interrupt is raised between the stop of the
device processing and the change of the device internal state to
OHCI_USB_SUSPEND (QEMU stops SOF timer on this state change), this
interrupt is never acknowledged.

This patch clears pending SOF interrupt on OHCI_USB_SUSPEND setting.

Some details:

- ohci_irq(): the OHCI interrupt handler, acknowledges the SOF IRQ
  only if the state of the driver (rh_state) is OHCI_STATE_RUNNING.
  So if this interrupt happens and the driver is not in this state,
  the function is called again and again, moving the system to a
  CPU starvation.

- ohci_rh_suspend(): the function stop the operation and acknowledge
  pending interrupts (but doesn't disable it). Later in the function,
  the device is moved to OHCI_SUSPEND_STATE, and the driver to
  OHCI_RH_SUSPENDED. If between the moment when the interrupt is
  acknowledged and the moment when the device is suspended a new
  interrupt is raised, it will be never acknowledged because the
  driver is now not in OHCI_RH_RUNNING state.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 hw/usb/hcd-ohci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index a44fab2..b99e44f 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -1446,6 +1446,9 @@ static void ohci_set_ctl(OHCIState *ohci, uint32_t val)
         break;
     case OHCI_USB_SUSPEND:
         ohci_bus_stop(ohci);
+        /* clear pending SF otherwise linux driver loops in ohci_irq() */
+        ohci->intr_status &= ~OHCI_INTR_SF;
+        ohci_intr_update(ohci);
         break;
     case OHCI_USB_RESUME:
         trace_usb_ohci_resume(ohci->name);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 1/2] ohci: delay first SOF interrupt

[Thomas Huth]

On 06.01.2016 20:45, Laurent Vivier wrote:
> On overcommitted CPU, kernel can be so slow that an interrupt can
> be triggered by the device whereas the driver is not ready to receive
> it. This drives us into an infinite loop.
> 
> This does not happen on real hardware because real hardware never send
> interrupt immediately after the controller has been moved to OPERATION state.
> 
> This patch tries to delay the first SOF interrupt to let driver exits from
> the critical section (which is not protected against interrupts...)
> 
> Some details:
> 
> - ohci_irq(): the OHCI interrupt handler, acknowledges the SOF IRQ
>   only if the state of the driver (rh_state) is OHCI_STATE_RUNNING.
>   So if this interrupt happens and the driver is not in this state,
>   the function is called again and again, moving the system to a
>   CPU starvation.
> 
> - ohci_rh_resume(): the driver re-enables operation with OHCI_USB_OPER.
>   In QEMU this start the SOF timer and QEMU starts to send IRQs. As
>   the driver is not in OHCI_STATE_RUNNING and not protected against IRQ,
>   the ohci_irq() can be called and the driver never moved to
>   OHCI_STATE_RUNNING.
> 
> Suggested-by: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  hw/usb/hcd-ohci.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
> index 7d65818..a44fab2 100644
> --- a/hw/usb/hcd-ohci.c
> +++ b/hw/usb/hcd-ohci.c
> @@ -1231,11 +1231,16 @@ static int ohci_service_ed_list(OHCIState *ohci, uint32_t head, int completion)
>      return active;
>  }
>  
> -/* Generate a SOF event, and set a timer for EOF */
> -static void ohci_sof(OHCIState *ohci)
> +/* set a timer for EOF */
> +static void ohci_eof_timer(OHCIState *ohci)
>  {
>      ohci->sof_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>      timer_mod(ohci->eof_timer, ohci->sof_time + usb_frame_time);
> +}
> +/* Set a timer for EOF and generate a SOF event */
> +static void ohci_sof(OHCIState *ohci)
> +{
> +    ohci_eof_timer(ohci);
>      ohci_set_interrupt(ohci, OHCI_INTR_SF);
>  }
>  
> @@ -1343,7 +1348,12 @@ static int ohci_bus_start(OHCIState *ohci)
>  
>      trace_usb_ohci_start(ohci->name);
>  
> -    ohci_sof(ohci);
> +    /* Delay the first SOF event by one frame time as
> +     * linux driver is not ready to receive it and
> +     * can meet some race conditions
> +     */
> +
> +    ohci_eof_timer(ohci);
>  
>      return 1;
>  }

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH v2 2/2] ohci: clear pending SOF on suspend

[Thomas Huth]

On 06.01.2016 20:45, Laurent Vivier wrote:
> On overcommitted CPU, kernel can be so slow that an interrupt can
> be triggered by the device whereas the driver is not ready to receive
> it. This drives us into an infinite loop.
> 
> On suspend, if a SOF interrupt is raised between the stop of the
> device processing and the change of the device internal state to
> OHCI_USB_SUSPEND (QEMU stops SOF timer on this state change), this
> interrupt is never acknowledged.
> 
> This patch clears pending SOF interrupt on OHCI_USB_SUSPEND setting.
> 
> Some details:
> 
> - ohci_irq(): the OHCI interrupt handler, acknowledges the SOF IRQ
>   only if the state of the driver (rh_state) is OHCI_STATE_RUNNING.
>   So if this interrupt happens and the driver is not in this state,
>   the function is called again and again, moving the system to a
>   CPU starvation.
> 
> - ohci_rh_suspend(): the function stop the operation and acknowledge
>   pending interrupts (but doesn't disable it). Later in the function,
>   the device is moved to OHCI_SUSPEND_STATE, and the driver to
>   OHCI_RH_SUSPENDED. If between the moment when the interrupt is
>   acknowledged and the moment when the device is suspended a new
>   interrupt is raised, it will be never acknowledged because the
>   driver is now not in OHCI_RH_RUNNING state.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  hw/usb/hcd-ohci.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
> index a44fab2..b99e44f 100644
> --- a/hw/usb/hcd-ohci.c
> +++ b/hw/usb/hcd-ohci.c
> @@ -1446,6 +1446,9 @@ static void ohci_set_ctl(OHCIState *ohci, uint32_t val)
>          break;
>      case OHCI_USB_SUSPEND:
>          ohci_bus_stop(ohci);
> +        /* clear pending SF otherwise linux driver loops in ohci_irq() */
> +        ohci->intr_status &= ~OHCI_INTR_SF;
> +        ohci_intr_update(ohci);
>          break;
>      case OHCI_USB_RESUME:
>          trace_usb_ohci_resume(ohci->name);
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>