xhci: move command stop and command abort flag check to the case when the crcr_low register is set

Commit Message

Jaroslav Jindrák July 31, 2017, 11:48 p.m. UTC

Hello,

I'd like to submit a patch to the xhci subsystem of QEMU. Currently, when
the command stop or command abort flags in the crcr_low register are set,
nothing happens. This is because the part of the code that tests those two
flags (and performs command ring abort/stop) is in the crcr_high case. This
error has a simple workaround - after writing to the crcr_low register with
either of these two flags set, one can write the value of crcr_high to
crcr_high, so I assume this fix does not have that big of a priority, but a
driver that follows the specification strictly would misbehave in this kind
of situation (stopping/aborting the command ring).

The patch is contained in the attachement.

​Kind​ regards,
Jaroslav Jindrak

Comments

Gerd Hoffmann Aug. 21, 2017, 12:23 p.m. UTC | #1

On Tue, 2017-08-01 at 01:48 +0200, Jaroslav Jindrák wrote:
> Hello,
> 
> I'd like to submit a patch to the xhci subsystem of QEMU. Currently,
> when the command stop or command abort flags in the crcr_low register
> are set, nothing happens. This is because the part of the code that
> tests those two flags (and performs command ring abort/stop) is in
> the crcr_high case. This error has a simple workaround - after
> writing to the crcr_low register with either of these two flags set,
> one can write the value of crcr_high to crcr_high, so I
> assume this fix does not have that big of a priority, but a driver
> that follows the specification strictly would misbehave in this kind
> of situation (stopping/aborting the command ring).

Specs says (section 5.1, Register Conventions):

<quote>
If the xHC supports 64-bit addressing (AC64 = ‘1’), then software
should write registers containing 64-bit address fields using only
Qword accesses. If a system is incapable of issuing Qword accesses,
then writes to the 64-bit address fields shall be performed using 2
Dword accesses; low Dword-first, high-Dword second.
</quote>

So I think the guest must write both crcr_low and crcr_high, and the
qemu behavior is correct.

Are there any guests which actually have problems?

cheers,
  Gerd

Jaroslav Jindrák Aug. 21, 2017, 6:31 p.m. UTC | #2

Hi,

that was an error on my part, I was debugging the non-working command ring
abort/stop functionality and as
the xhci driver implementation I was working on used crcr_low and crcr_high
as two separate registers it did not
hit me it was actually my way of accessing them that caused the abort/stop
to not work.

Thank you very much for the correction and I apologise for disturbing you,
hope I didn't eat too much of your time.

Have a nice day,
Jaroslav Jindrak

S pozdravem,
Jaroslav Jindrak

On 21 August 2017 at 14:23, Gerd Hoffmann <[e-mail]> wrote:

> On Tue, 2017-08-01 at 01:48 +0200, Jaroslav Jindrák wrote:
> > Hello,
> >
> > I'd like to submit a patch to the xhci subsystem of QEMU. Currently,
> > when the command stop or command abort flags in the crcr_low register
> > are set, nothing happens. This is because the part of the code that
> > tests those two flags (and performs command ring abort/stop) is in
> > the crcr_high case. This error has a simple workaround - after
> > writing to the crcr_low register with either of these two flags set,
> > one can write the value of crcr_high to crcr_high, so I
> > assume this fix does not have that big of a priority, but a driver
> > that follows the specification strictly would misbehave in this kind
> > of situation (stopping/aborting the command ring).
>
> Specs says (section 5.1, Register Conventions):
>
> <quote>
> If the xHC supports 64-bit addressing (AC64 = ‘1’), then software
> should write registers containing 64-bit address fields using only
> Qword accesses. If a system is incapable of issuing Qword accesses,
> then writes to the 64-bit address fields shall be performed using 2
> Dword accesses; low Dword-first, high-Dword second.
> </quote>
>
> So I think the guest must write both crcr_low and crcr_high, and the
> qemu behavior is correct.
>
> Are there any guests which actually have problems?
>
> cheers,
>   Gerd
>
>

Patch

From b18a165f3c70c6154944706bd096fb002a9b4461 Mon Sep 17 00:00:00 2001
From: Dzejrou <[e-mail]>
Date: Tue, 1 Aug 2017 01:30:49 +0200
Subject: [PATCH 1/1] xhci: move command stop and command abort flag check to
 the case when the crcr_low register is set

Signed-off-by: Dzejrou <[e-mail]>
---
 hw/usb/hcd-xhci.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 204ea69..9eb3c83 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -2937,9 +2937,6 @@  static void xhci_oper_write(void *ptr, hwaddr reg,
         break;
     case 0x18: /* CRCR low */
         xhci->crcr_low = (val & 0xffffffcf) | (xhci->crcr_low & CRCR_CRR);
-        break;
-    case 0x1c: /* CRCR high */
-        xhci->crcr_high = val;
         if (xhci->crcr_low & (CRCR_CA|CRCR_CS) && (xhci->crcr_low & CRCR_CRR)) {
             XHCIEvent event = {ER_COMMAND_COMPLETE, CC_COMMAND_RING_STOPPED};
             xhci->crcr_low &= ~CRCR_CRR;
@@ -2951,6 +2948,9 @@  static void xhci_oper_write(void *ptr, hwaddr reg,
         }
         xhci->crcr_low &= ~(CRCR_CA | CRCR_CS);
         break;
+    case 0x1c: /* CRCR high */
+        xhci->crcr_high = val;
+        break;
     case 0x30: /* DCBAAP low */
         xhci->dcbaap_low = val & 0xffffffc0;
         break;
-- 
2.4.11