Message ID | 20200721083322.90651-1-lvivier@redhat.com |
---|---|
State | New |
Headers | show |
Series | [for-5.1] xhci: fix valid.max_access_size to access address registers | expand |
On 7/21/20 10:33 AM, Laurent Vivier wrote: > QEMU XHCI advertises AC64 (64-bit addressing) but doesn't allow > 64-bit mode access in "runtime" and "operational" MemoryRegionOps. > > Set the max_access_size based on sizeof(dma_addr_t) as AC64 is set. > > XHCI specs: > "If the xHC supports 64-bit addressing (AC64 = ‘1’), then software > should write 64-bit registers 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. If the xHC supports 32-bit > addressing (AC64 = ‘0’), then the high Dword of registers containing > 64-bit address fields are unused and software should write addresses > using only Dword accesses" You only describe the WRITE path. Is the READ path similar? > > The problem has been detected with SLOF, as linux kernel always accesses > registers using 32-bit access even if AC64 is set and revealed by > 5d971f9e6725 ("memory: Revert "memory: accept mismatching sizes in memory_region_access_valid"") > > Suggested-by: Alexey Kardashevskiy <aik@au1.ibm.com> > Signed-off-by: Laurent Vivier <lvivier@redhat.com> > --- > hw/usb/hcd-xhci.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c > index b330e36fe6cc..67a18fe2b64c 100644 > --- a/hw/usb/hcd-xhci.c > +++ b/hw/usb/hcd-xhci.c > @@ -3184,7 +3184,7 @@ static const MemoryRegionOps xhci_oper_ops = { > .read = xhci_oper_read, > .write = xhci_oper_write, > .valid.min_access_size = 4, > - .valid.max_access_size = 4, > + .valid.max_access_size = sizeof(dma_addr_t), > .endianness = DEVICE_LITTLE_ENDIAN, > }; > > @@ -3200,7 +3200,7 @@ static const MemoryRegionOps xhci_runtime_ops = { > .read = xhci_runtime_read, > .write = xhci_runtime_write, > .valid.min_access_size = 4, > - .valid.max_access_size = 4, > + .valid.max_access_size = sizeof(dma_addr_t), > .endianness = DEVICE_LITTLE_ENDIAN, > }; I wonder if we shouldn't check the access size now, something like: bool xhci_check_access_size(void *opaque, hwaddr addr, unsigned size, bool is_write, MemTxAttrs attrs); { XHCIState *xhci = opaque; /* FIXME only for is_write??? */ return xhci->ac64 || size == 4; } And add to both MemoryRegionOps: .accepts = xhci_check_access_size,
On 21/07/2020 11:17, Philippe Mathieu-Daudé wrote: > On 7/21/20 10:33 AM, Laurent Vivier wrote: >> QEMU XHCI advertises AC64 (64-bit addressing) but doesn't allow >> 64-bit mode access in "runtime" and "operational" MemoryRegionOps. >> >> Set the max_access_size based on sizeof(dma_addr_t) as AC64 is set. >> >> XHCI specs: >> "If the xHC supports 64-bit addressing (AC64 = ‘1’), then software >> should write 64-bit registers 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. If the xHC supports 32-bit >> addressing (AC64 = ‘0’), then the high Dword of registers containing >> 64-bit address fields are unused and software should write addresses >> using only Dword accesses" > > You only describe the WRITE path. Is the READ path similar? The specs text comes from Alexey. So I don't know. But I don't see any reason to not have 64bit read if we have 64bit write. > >> >> The problem has been detected with SLOF, as linux kernel always accesses >> registers using 32-bit access even if AC64 is set and revealed by >> 5d971f9e6725 ("memory: Revert "memory: accept mismatching sizes in memory_region_access_valid"") >> >> Suggested-by: Alexey Kardashevskiy <aik@au1.ibm.com> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com> >> --- >> hw/usb/hcd-xhci.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c >> index b330e36fe6cc..67a18fe2b64c 100644 >> --- a/hw/usb/hcd-xhci.c >> +++ b/hw/usb/hcd-xhci.c >> @@ -3184,7 +3184,7 @@ static const MemoryRegionOps xhci_oper_ops = { >> .read = xhci_oper_read, >> .write = xhci_oper_write, >> .valid.min_access_size = 4, >> - .valid.max_access_size = 4, >> + .valid.max_access_size = sizeof(dma_addr_t), >> .endianness = DEVICE_LITTLE_ENDIAN, >> }; >> >> @@ -3200,7 +3200,7 @@ static const MemoryRegionOps xhci_runtime_ops = { >> .read = xhci_runtime_read, >> .write = xhci_runtime_write, >> .valid.min_access_size = 4, >> - .valid.max_access_size = 4, >> + .valid.max_access_size = sizeof(dma_addr_t), >> .endianness = DEVICE_LITTLE_ENDIAN, >> }; > > I wonder if we shouldn't check the access size now, something like: > > bool xhci_check_access_size(void *opaque, hwaddr addr, > unsigned size, bool is_write, > MemTxAttrs attrs); > { > XHCIState *xhci = opaque; > > /* FIXME only for is_write??? */ > return xhci->ac64 || size == 4; I don't think it's needed as AC64 (in fact a bit in HCCPARAMS) is set only if sizeof(dma_addr_t) != 4... but I'm checking source code, and dma_addr_t is always uint64_t. I think it should rely instead on TARGET_PHYS_ADDR_SPACE_BITS. But this check has been removed by David in: 59a70ccd3be2 ("usb-xhci: Use PCI DMA helper functions") Thanks, Laurent
Hi, > >> - .valid.max_access_size = 4, > >> + .valid.max_access_size = sizeof(dma_addr_t), > I don't think it's needed as AC64 (in fact a bit in HCCPARAMS) is set > only if sizeof(dma_addr_t) != 4... So both AC64 bit and max_access_size are in sync, good. Patch queued. thanks, Gerd
diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index b330e36fe6cc..67a18fe2b64c 100644 --- a/hw/usb/hcd-xhci.c +++ b/hw/usb/hcd-xhci.c @@ -3184,7 +3184,7 @@ static const MemoryRegionOps xhci_oper_ops = { .read = xhci_oper_read, .write = xhci_oper_write, .valid.min_access_size = 4, - .valid.max_access_size = 4, + .valid.max_access_size = sizeof(dma_addr_t), .endianness = DEVICE_LITTLE_ENDIAN, }; @@ -3200,7 +3200,7 @@ static const MemoryRegionOps xhci_runtime_ops = { .read = xhci_runtime_read, .write = xhci_runtime_write, .valid.min_access_size = 4, - .valid.max_access_size = 4, + .valid.max_access_size = sizeof(dma_addr_t), .endianness = DEVICE_LITTLE_ENDIAN, };
QEMU XHCI advertises AC64 (64-bit addressing) but doesn't allow 64-bit mode access in "runtime" and "operational" MemoryRegionOps. Set the max_access_size based on sizeof(dma_addr_t) as AC64 is set. XHCI specs: "If the xHC supports 64-bit addressing (AC64 = ‘1’), then software should write 64-bit registers 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. If the xHC supports 32-bit addressing (AC64 = ‘0’), then the high Dword of registers containing 64-bit address fields are unused and software should write addresses using only Dword accesses" The problem has been detected with SLOF, as linux kernel always accesses registers using 32-bit access even if AC64 is set and revealed by 5d971f9e6725 ("memory: Revert "memory: accept mismatching sizes in memory_region_access_valid"") Suggested-by: Alexey Kardashevskiy <aik@au1.ibm.com> Signed-off-by: Laurent Vivier <lvivier@redhat.com> --- hw/usb/hcd-xhci.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)