The token field contains the (guest-filled) state of the qtd, which
indicates whenever the other fields are valid or not.  So make sure
we read the token first, otherwise we may end up with an stale next
pointer:

  (1) ehci reads next
  (2) guest writes next
  (3) guest writes token
  (4) ehci reads token
  (5) ehci operates with stale next.

Typical effect is that qemu doesn't notice that the guest appends new
qtds to the end of the queue.  Looks like the usb device stopped
responding.  Linux can recover from that, but leaves a message in the
kernel log that it did reset the usb device in question.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-ehci.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index e5acfc5ba5..8d44d483df 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1783,9 +1783,17 @@ static int ehci_state_fetchqtd(EHCIQueue *q)
     EHCIqtd qtd;
     EHCIPacket *p;
     int again = 1;
+    uint32_t addr;
 
-    if (get_dwords(q->ehci, NLPTR_GET(q->qtdaddr), (uint32_t *) &qtd,
-                   sizeof(EHCIqtd) >> 2) < 0) {
+    addr = NLPTR_GET(q->qtdaddr);
+    if (get_dwords(q->ehci, addr +  8, &qtd.token,   1) < 0) {
+        return 0;
+    }
+    barrier();
+    if (get_dwords(q->ehci, addr +  0, &qtd.next,    1) < 0 ||
+        get_dwords(q->ehci, addr +  4, &qtd.altnext, 1) < 0 ||
+        get_dwords(q->ehci, addr + 12, qtd.bufptr,
+                   ARRAY_SIZE(qtd.bufptr)) < 0) {
         return 0;
     }
     ehci_trace_qtd(q, NLPTR_GET(q->qtdaddr), &qtd);
-- 
2.9.3

在 2018/11/26 18:08, Gerd Hoffmann 写道:
> The token field contains the (guest-filled) state of the qtd, which
> indicates whenever the other fields are valid or not.  So make sure
> we read the token first, otherwise we may end up with an stale next
> pointer:
>
>    (1) ehci reads next
>    (2) guest writes next
>    (3) guest writes token
>    (4) ehci reads token
>    (5) ehci operates with stale next.

Hello Gerd,

Could you please explain how this can happen?

IMO, the device emulation holds the BQL and the guest can't execute, how 
can the guest

write this when QEMU is in 'ehci_state_fetchqtd'?


Thanks,

Li Qiang


> Typical effect is that qemu doesn't notice that the guest appends new
> qtds to the end of the queue.  Looks like the usb device stopped
> responding.  Linux can recover from that, but leaves a message in the
> kernel log that it did reset the usb device in question.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   hw/usb/hcd-ehci.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> index e5acfc5ba5..8d44d483df 100644
> --- a/hw/usb/hcd-ehci.c
> +++ b/hw/usb/hcd-ehci.c
> @@ -1783,9 +1783,17 @@ static int ehci_state_fetchqtd(EHCIQueue *q)
>       EHCIqtd qtd;
>       EHCIPacket *p;
>       int again = 1;
> +    uint32_t addr;
>   
> -    if (get_dwords(q->ehci, NLPTR_GET(q->qtdaddr), (uint32_t *) &qtd,
> -                   sizeof(EHCIqtd) >> 2) < 0) {
> +    addr = NLPTR_GET(q->qtdaddr);
> +    if (get_dwords(q->ehci, addr +  8, &qtd.token,   1) < 0) {
> +        return 0;
> +    }
> +    barrier();
> +    if (get_dwords(q->ehci, addr +  0, &qtd.next,    1) < 0 ||
> +        get_dwords(q->ehci, addr +  4, &qtd.altnext, 1) < 0 ||
> +        get_dwords(q->ehci, addr + 12, qtd.bufptr,
> +                   ARRAY_SIZE(qtd.bufptr)) < 0) {
>           return 0;
>       }
>       ehci_trace_qtd(q, NLPTR_GET(q->qtdaddr), &qtd);

On Mon, Nov 26, 2018 at 10:34:13AM +0000, li qiang wrote:
> 
> 在 2018/11/26 18:08, Gerd Hoffmann 写道:
> > The token field contains the (guest-filled) state of the qtd, which
> > indicates whenever the other fields are valid or not.  So make sure
> > we read the token first, otherwise we may end up with an stale next
> > pointer:
> >
> >    (1) ehci reads next
> >    (2) guest writes next
> >    (3) guest writes token
> >    (4) ehci reads token
> >    (5) ehci operates with stale next.
> 
> Hello Gerd,
> 
> Could you please explain how this can happen?
> 
> IMO, the device emulation holds the BQL and the guest can't execute, how 
> can the guest

No.  vcpus can run in parallel to the iothread (at least as long as they
are running in guest context, when they vmexit they have to wait for the
BQL).

cheers,
  Gerd