diff mbox series

usb: don't use 64bit accessors

Message ID 20200721131351.106014-1-lvivier@redhat.com
State New
Headers show
Series usb: don't use 64bit accessors | expand

Commit Message

Laurent Vivier July 21, 2020, 1:13 p.m. UTC
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(-)

Comments

David Gibson July 22, 2020, 10:35 a.m. UTC | #1
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)
Laurent Vivier July 22, 2020, 11:52 a.m. UTC | #2
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 mbox series

Patch

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)