| Message ID | 20200721131351.106014-1-lvivier@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: don't use 64bit accessors | expand |
On Tue, Jul 21, 2020 at 03:13:51PM +0200, Laurent Vivier wrote: > A recent change in QEMU[1] breaks the 64bit access for XHCI and makes it > unusable. > QEMU always reports the AC64 bit so 64bit should be supported, but in fact > SLOF doesn't check for this bit and always does a 64bit access. > > We need to fix QEMU to allow 64bit access when it reports AC64, but we need > also to fix SLOF to not use 64bit access when the AC64 bit is not set. > > The easiest way to do that is, like linux kernel in xhci_read_64() and > xhci_write_64() (see drivers/usb/host/xhci.h), to access always a 64bit > register using two 32bit accesses, as the specs allow that. > > [1] 5d971f9e6725 ("memory: Revert "memory: accept mismatching sizes in memory_region_access_valid"") > > Signed-off-by: Laurent Vivier <lvivier@redhat.com> Now that we've fixed this on the qemu side, this isn't so necessary. Alexey seems to think there might be some value to it for robustness, but we don't need to rush to get this into the SLOF that ships with qemu 5.1 > --- > lib/libusb/tools.h | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/lib/libusb/tools.h b/lib/libusb/tools.h > index f531175c10a6..5758334b0247 100644 > --- a/lib/libusb/tools.h > +++ b/lib/libusb/tools.h > @@ -54,13 +54,21 @@ static inline void write_reg16(uint16_t *reg, uint16_t value) > > static inline uint64_t read_reg64(uint64_t *reg) > { > - return bswap_64(ci_read_64(reg)); > + uint32_t *p = (uint32_t*)reg; > + uint32_t low, high; > + > + low = read_reg32(p); > + high = read_reg32(p + 1); > + > + return low + ((uint64_t)high << 32); > } > > static inline void write_reg64(uint64_t *reg, uint64_t value) > { > - mb(); > - ci_write_64(reg, bswap_64(value)); > + uint32_t *p = (uint32_t*)reg; > + > + write_reg32(p, value); > + write_reg32(p + 1, value >> 32); > } > > static inline uint32_t ci_read_reg(uint32_t *reg)
On 22/07/2020 12:35, David Gibson wrote: > On Tue, Jul 21, 2020 at 03:13:51PM +0200, Laurent Vivier wrote: >> A recent change in QEMU[1] breaks the 64bit access for XHCI and makes it >> unusable. >> QEMU always reports the AC64 bit so 64bit should be supported, but in fact >> SLOF doesn't check for this bit and always does a 64bit access. >> >> We need to fix QEMU to allow 64bit access when it reports AC64, but we need >> also to fix SLOF to not use 64bit access when the AC64 bit is not set. >> >> The easiest way to do that is, like linux kernel in xhci_read_64() and >> xhci_write_64() (see drivers/usb/host/xhci.h), to access always a 64bit >> register using two 32bit accesses, as the specs allow that. >> >> [1] 5d971f9e6725 ("memory: Revert "memory: accept mismatching sizes in memory_region_access_valid"") >> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com> > > Now that we've fixed this on the qemu side, this isn't so necessary. > Alexey seems to think there might be some value to it for robustness, > but we don't need to rush to get this into the SLOF that ships with > qemu 5.1 I agree. I've posted the patch because I started by fixing SLOF so it was already written. Thanks, Laurent >> --- >> lib/libusb/tools.h | 14 +++++++++++--- >> 1 file changed, 11 insertions(+), 3 deletions(-) >> >> diff --git a/lib/libusb/tools.h b/lib/libusb/tools.h >> index f531175c10a6..5758334b0247 100644 >> --- a/lib/libusb/tools.h >> +++ b/lib/libusb/tools.h >> @@ -54,13 +54,21 @@ static inline void write_reg16(uint16_t *reg, uint16_t value) >> >> static inline uint64_t read_reg64(uint64_t *reg) >> { >> - return bswap_64(ci_read_64(reg)); >> + uint32_t *p = (uint32_t*)reg; >> + uint32_t low, high; >> + >> + low = read_reg32(p); >> + high = read_reg32(p + 1); >> + >> + return low + ((uint64_t)high << 32); >> } >> >> static inline void write_reg64(uint64_t *reg, uint64_t value) >> { >> - mb(); >> - ci_write_64(reg, bswap_64(value)); >> + uint32_t *p = (uint32_t*)reg; >> + >> + write_reg32(p, value); >> + write_reg32(p + 1, value >> 32); >> } >> >> static inline uint32_t ci_read_reg(uint32_t *reg) >
diff --git a/lib/libusb/tools.h b/lib/libusb/tools.h index f531175c10a6..5758334b0247 100644 --- a/lib/libusb/tools.h +++ b/lib/libusb/tools.h @@ -54,13 +54,21 @@ static inline void write_reg16(uint16_t *reg, uint16_t value) static inline uint64_t read_reg64(uint64_t *reg) { - return bswap_64(ci_read_64(reg)); + uint32_t *p = (uint32_t*)reg; + uint32_t low, high; + + low = read_reg32(p); + high = read_reg32(p + 1); + + return low + ((uint64_t)high << 32); } static inline void write_reg64(uint64_t *reg, uint64_t value) { - mb(); - ci_write_64(reg, bswap_64(value)); + uint32_t *p = (uint32_t*)reg; + + write_reg32(p, value); + write_reg32(p + 1, value >> 32); } static inline uint32_t ci_read_reg(uint32_t *reg)
A recent change in QEMU[1] breaks the 64bit access for XHCI and makes it unusable. QEMU always reports the AC64 bit so 64bit should be supported, but in fact SLOF doesn't check for this bit and always does a 64bit access. We need to fix QEMU to allow 64bit access when it reports AC64, but we need also to fix SLOF to not use 64bit access when the AC64 bit is not set. The easiest way to do that is, like linux kernel in xhci_read_64() and xhci_write_64() (see drivers/usb/host/xhci.h), to access always a 64bit register using two 32bit accesses, as the specs allow that. [1] 5d971f9e6725 ("memory: Revert "memory: accept mismatching sizes in memory_region_access_valid"") Signed-off-by: Laurent Vivier <lvivier@redhat.com> --- lib/libusb/tools.h | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)