[Qemu-devel] [PATCH] ehci: Don't fetch a NULL current qtd but advance the queue instead.

[Qemu-devel] [PATCH] ehci: Don't fetch a NULL current qtd but advance the queue instead.

[Sebastian Bauer]

Fetching qtd with the NULL address most likely makes no sense so from now
on, we handle it this case similarly as if the terminate (T) bit is not
set, which is already an exception as according to section 3.6 of the EHCI
spec there is no T bit defined for the current_qtd field.

The spec is a bit vague on how an EHCI driver should initialize these
fields: "The general operational model is that the host controller can
detect whether the overlay area contains a description of an active
transfer" (p. 49). QEMU primarily uses the QTD_TOKEN_ACTIVE bit of the
queue header to infer the activity state but there are other ways
conceivable.

This change allows QEMU to boot further into AmigaOS. The public available
version of the EHCI driver recycles queue heads in some rare conditions but
only clears the current_qtd field but not the status field. This works with
many available EHCI PCI cards but e.g., not with the Freescale USB
controller's found on the P5040. On the emulated EHCI controller of QEMU
the consequence is that some garbage was read in, which resulted in a
reset of the controller. This change fixes the problem.

Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>

---

The fix is probably not the best solution. However, it is one that is very
small.

QEMU still warns that an active qh will be changed if the qh is recycled,
so more changes could be done. But I didn't notice any negative
consequences implied by the warning so I would leave a possible fix to this
problem to a further patch.
---
 hw/usb/hcd-ehci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 0134232627..e5acfc5ba5 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1672,7 +1672,8 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
         ehci_set_state(ehci, async, EST_HORIZONTALQH);
 
     } else if ((q->qh.token & QTD_TOKEN_ACTIVE) &&
-               (NLPTR_TBIT(q->qh.current_qtd) == 0)) {
+               (NLPTR_TBIT(q->qh.current_qtd) == 0) &&
+               (q->qh.current_qtd != 0)) {
         q->qtdaddr = q->qh.current_qtd;
         ehci_set_state(ehci, async, EST_FETCHQTD);
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH] ehci: Don't fetch a NULL current qtd but advance the queue instead.

[BALATON Zoltan]

On Tue, 26 Jun 2018, Sebastian Bauer wrote:
> Fetching qtd with the NULL address most likely makes no sense so from now
> on, we handle it this case similarly as if the terminate (T) bit is not
> set, which is already an exception as according to section 3.6 of the EHCI
> spec there is no T bit defined for the current_qtd field.
>
> The spec is a bit vague on how an EHCI driver should initialize these
> fields: "The general operational model is that the host controller can
> detect whether the overlay area contains a description of an active
> transfer" (p. 49). QEMU primarily uses the QTD_TOKEN_ACTIVE bit of the
> queue header to infer the activity state but there are other ways
> conceivable.
>
> This change allows QEMU to boot further into AmigaOS. The public available
> version of the EHCI driver recycles queue heads in some rare conditions but
> only clears the current_qtd field but not the status field. This works with
> many available EHCI PCI cards but e.g., not with the Freescale USB
> controller's found on the P5040. On the emulated EHCI controller of QEMU
> the consequence is that some garbage was read in, which resulted in a
> reset of the controller. This change fixes the problem.
>
> Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>

This fixes mouse and keyboard for AmigaOS on sam460ex. It would be 
appreciated if this could be merged before the freeze. I guess it could be 
removed again before the release if found to cause any problems or fixed 
if needed but if it's not merged now then we can't make QEMU 3.0 support 
AmigaOS.

Tested-by: BALATON Zoltan <balaton@eik.bme.hu>

> ---
>
> The fix is probably not the best solution. However, it is one that is very
> small.
>
> QEMU still warns that an active qh will be changed if the qh is recycled,
> so more changes could be done. But I didn't notice any negative
> consequences implied by the warning so I would leave a possible fix to this
> problem to a further patch.
> ---
> hw/usb/hcd-ehci.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> index 0134232627..e5acfc5ba5 100644
> --- a/hw/usb/hcd-ehci.c
> +++ b/hw/usb/hcd-ehci.c
> @@ -1672,7 +1672,8 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
>         ehci_set_state(ehci, async, EST_HORIZONTALQH);
>
>     } else if ((q->qh.token & QTD_TOKEN_ACTIVE) &&
> -               (NLPTR_TBIT(q->qh.current_qtd) == 0)) {
> +               (NLPTR_TBIT(q->qh.current_qtd) == 0) &&
> +               (q->qh.current_qtd != 0)) {
>         q->qtdaddr = q->qh.current_qtd;
>         ehci_set_state(ehci, async, EST_FETCHQTD);
>
>

Re: [Qemu-devel] [PATCH] ehci: Don't fetch a NULL current qtd but advance the queue instead.

[Gerd Hoffmann]

On Tue, Jun 26, 2018 at 12:27:18AM +0200, Sebastian Bauer wrote:
> Fetching qtd with the NULL address most likely makes no sense so from now
> on, we handle it this case similarly as if the terminate (T) bit is not
> set, which is already an exception as according to section 3.6 of the EHCI
> spec there is no T bit defined for the current_qtd field.
> 
> The spec is a bit vague on how an EHCI driver should initialize these
> fields: "The general operational model is that the host controller can
> detect whether the overlay area contains a description of an active
> transfer" (p. 49). QEMU primarily uses the QTD_TOKEN_ACTIVE bit of the
> queue header to infer the activity state but there are other ways
> conceivable.
> 
> This change allows QEMU to boot further into AmigaOS. The public available
> version of the EHCI driver recycles queue heads in some rare conditions but
> only clears the current_qtd field but not the status field. This works with
> many available EHCI PCI cards but e.g., not with the Freescale USB
> controller's found on the P5040. On the emulated EHCI controller of QEMU
> the consequence is that some garbage was read in, which resulted in a
> reset of the controller. This change fixes the problem.
> 
> Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>

Added to usb queue.

thanks,
  Gerd