[Qemu-devel] [PATCH] Changed the type of val argument of the function gt64120_writel() from uint32_t to uint64_t, so change the corresponding bswap32() to bswap64().

[Qemu-devel] [PATCH] Changed the type of val argument of the function gt64120_writel() from uint32_t to uint64_t, so change the corresponding bswap32() to bswap64().

[Ray Wang]

From: Xianlei Wang <raywang@linux.vnet.ibm.com>


Signed-off-by: Xianlei Wang <raywang@linux.vnet.ibm.com>
---
 hw/gt64xxx.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
index 1c34253..d0a31d2 100644
--- a/hw/gt64xxx.c
+++ b/hw/gt64xxx.c
@@ -312,7 +312,7 @@ static void gt64120_writel (void *opaque, target_phys_addr_t addr,
     uint32_t saddr;
 
     if (!(s->regs[GT_CPU] & 0x00001000))
-        val = bswap32(val);
+        val = bswap64(val);
 
     saddr = (addr & 0xfff) >> 2;
     switch (saddr) {
@@ -533,7 +533,7 @@ static void gt64120_writel (void *opaque, target_phys_addr_t addr,
         break;
     case GT_PCI0_CFGDATA:
         if (!(s->regs[GT_PCI0_CMD] & 1) && (s->pci.config_reg & 0x00fff800))
-            val = bswap32(val);
+            val = bswap64(val);
         if (s->pci.config_reg & (1u << 31))
             pci_data_write(s->pci.bus, s->pci.config_reg, val, 4);
         break;
-- 
1.7.4.1

Re: [Qemu-devel] [PATCH] Changed the type of val argument of the function gt64120_writel() from uint32_t to uint64_t, so change the corresponding bswap32() to bswap64().

[Peter Maydell]

On 26 September 2011 07:22, Ray Wang <raywang@linux.vnet.ibm.com> wrote:
> diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
> index 1c34253..d0a31d2 100644
> --- a/hw/gt64xxx.c
> +++ b/hw/gt64xxx.c
> @@ -312,7 +312,7 @@ static void gt64120_writel (void *opaque, target_phys_addr_t addr,
>     uint32_t saddr;
>
>     if (!(s->regs[GT_CPU] & 0x00001000))
> -        val = bswap32(val);
> +        val = bswap64(val);
>
>     saddr = (addr & 0xfff) >> 2;
>     switch (saddr) {
> @@ -533,7 +533,7 @@ static void gt64120_writel (void *opaque, target_phys_addr_t addr,
>         break;
>     case GT_PCI0_CFGDATA:
>         if (!(s->regs[GT_PCI0_CMD] & 1) && (s->pci.config_reg & 0x00fff800))
> -            val = bswap32(val);
> +            val = bswap64(val);
>         if (s->pci.config_reg & (1u << 31))
>             pci_data_write(s->pci.bus, s->pci.config_reg, val, 4);
>         break;

I don't know this device, but this looks a bit suspicious. If
you do a bswap64() on the value for a 32-bit write this will put
the data into the high 32 bits and zeros into the low half; then
storing into s->regs[] will just write a zero (since regs[] is
32 bits), won't it? Changing only writel and not readl also looks
odd.

What is the bug this change is trying to fix?

-- PMM

Re: [Qemu-devel] [PATCH] Changed the type of val argument of the function gt64120_writel() from uint32_t to uint64_t, so change the corresponding bswap32() to bswap64().

[Ray Wang]

On 9/26/2011 5:46 PM, Peter Maydell wrote:
> On 26 September 2011 07:22, Ray Wang<raywang@linux.vnet.ibm.com>  wrote:
>> diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
>> index 1c34253..d0a31d2 100644
>> --- a/hw/gt64xxx.c
>> +++ b/hw/gt64xxx.c
>> @@ -312,7 +312,7 @@ static void gt64120_writel (void *opaque, target_phys_addr_t addr,
>>      uint32_t saddr;
>>
>>      if (!(s->regs[GT_CPU]&  0x00001000))
>> -        val = bswap32(val);
>> +        val = bswap64(val);
>>
>>      saddr = (addr&  0xfff)>>  2;
>>      switch (saddr) {
>> @@ -533,7 +533,7 @@ static void gt64120_writel (void *opaque, target_phys_addr_t addr,
>>          break;
>>      case GT_PCI0_CFGDATA:
>>          if (!(s->regs[GT_PCI0_CMD]&  1)&&  (s->pci.config_reg&  0x00fff800))
>> -            val = bswap32(val);
>> +            val = bswap64(val);
>>          if (s->pci.config_reg&  (1u<<  31))
>>              pci_data_write(s->pci.bus, s->pci.config_reg, val, 4);
>>          break;
> I don't know this device, but this looks a bit suspicious. If
> you do a bswap64() on the value for a 32-bit write this will put
> the data into the high 32 bits and zeros into the low half; then
> storing into s->regs[] will just write a zero (since regs[] is
> 32 bits), won't it? Changing only writel and not readl also looks
> odd.
    Yes, you're right. However, if a 64-bit value with effective high 32 
bits is considered as a 32-bit one, some significant information will be 
lost.
>
> What is the bug this change is trying to fix?
    It is always a potential bug to process a 64-bit value as 32-bit 
one, isn't it?
>
> -- PMM
>


-- 
Best Regards,
-------------------------------------------------
Ray Wang
Linux Technology Center, KVM China
IBM Corp., Beijing, China
xianleiw@cn.ibm.com

Re: [Qemu-devel] [PATCH] Changed the type of val argument of the function gt64120_writel() from uint32_t to uint64_t, so change the corresponding bswap32() to bswap64().

[Peter Maydell]

On 27 September 2011 02:20, Ray Wang <raywang@linux.vnet.ibm.com> wrote:
> On 9/26/2011 5:46 PM, Peter Maydell wrote:
>> I don't know this device, but this looks a bit suspicious. If
>> you do a bswap64() on the value for a 32-bit write this will put
>> the data into the high 32 bits and zeros into the low half; then
>> storing into s->regs[] will just write a zero (since regs[] is
>> 32 bits), won't it? Changing only writel and not readl also looks
>> odd.
>
>   Yes, you're right. However, if a 64-bit value with effective high 32 bits
> is considered as a 32-bit one, some significant information will be lost.

Yes, but this function is never passed in values with the high part
set. The memory region API gives each device a single function which
implements all widths of memory access (1,2,4 and at some point we
will implement 8 byte widths), so the 'value' parameter is 64 bits
wide. However for accesses at sizes less than 8 only the bottom part
has interesting data, the top part is always zero.

Clearly when we get support for 64-bit accesses, this device will
need some bugfixes for that to work (for instance, it will need to
support passing on the value for pci config writes at the right
width rather than always passing 4). I had a quick scan of the
datasheet for the chip, and I couldn't see anything saying what
happens if you try to access the 32 bit registers at 64 bits, though.
A patch adding support for 64 bit accesses would have to be done
so as not to break 32 bit accesses, obviously.

>> What is the bug this change is trying to fix?
>
>   It is always a potential bug to process a 64-bit value as 32-bit one,
> isn't it?

No. Sometimes a 64 bit type contains 32 bit data. If the code is
wrong then it should be possible to demonstrate that QEMU behaves
wrongly (ie differently from the hardware) as a result.

-- PMM