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 <address@hidden> 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