| Message ID | CAE-Qwfz-RqR_qiOPmv8XF9SO-+4nroai0tNhE_CboEge9ufy+w@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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 <kraxel@redhat.com> 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 > >
From b18a165f3c70c6154944706bd096fb002a9b4461 Mon Sep 17 00:00:00 2001 From: Dzejrou <dzejrou@gmail.com> 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 <dzejrou@gmail.com> --- 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