Re: [Qemu-devel] [PATCH v3] bugfix: passing reference instead of value

[Cao jin <caoj.fnst@cn.fujitsu.com>]

Hi,
     Happy new year:)

On 01/02/2016 05:06 PM, Stefan Weil wrote:
> Am 02.01.2016 um 09:02 schrieb Cao jin:
>> Fix the bug introduced by 595a4f07: function host_pci_config_read() should be
>> pass-by-reference, not value.
>>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>> v3 changelog:
>> 1. Remove cpu_to_le32() since the code only runs on X86.
>>
>>   hw/pci-host/piix.c | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>> index 715208b..924f0fa 100644
>> --- a/hw/pci-host/piix.c
>> +++ b/hw/pci-host/piix.c
>> @@ -761,7 +761,7 @@ static const IGDHostInfo igd_host_bridge_infos[] = {
>>       {0xa8, 4},  /* SNB: base of GTT stolen memory */
>>   };
>>
>> -static int host_pci_config_read(int pos, int len, uint32_t val)
>> +static int host_pci_config_read(int pos, int len, uint32_t *val)
>>   {
>>       char path[PATH_MAX];
>>       int config_fd;
>> @@ -784,12 +784,14 @@ static int host_pci_config_read(int pos, int len, uint32_t val)
>>           ret = -errno;
>>           goto out;
>>       }
>> +
>>       do {
>> -        rc = read(config_fd, (uint8_t *)&val, len);
>> +        rc = read(config_fd, (uint8_t *)val, len);
>
> The type cast is not needed here, because read accepts any pointer
> type for the buffer argument.
>

I guess so, since in function read() prototype, buffer is void *

> While looking at that code, I noticed more potential issues:
>
> * The open statement needs O_RDWR | O_BINARY, otherwise the code won't
> work on Windows.
>

I am not quite familiar with things on windows:-[ Let`s see what will 
other people say.

> * The len argument can obviously be 2 or 4. Will endianness handling
> work for both cases?
>

I noticed what you find, and after analysing, I think it will works for 
both case:

take vendor ID in config space for example(PCI config space is 
little-endian), assume vendor ID = 0x1234, so in config space, it will 
be laid out as: (lo)34 12(hi).

host_pci_config_read() use read(fd, (uint8_t *)val, len) to get host 
device space value, I guess read() will read it from low address to high 
address, byte by byte(not quite sure about it). So after reading, the 
value in that integer buffer is laid out as: (lo)34,12,0,0(hi)

For (lo)34,12,0,0(hi), a LE machine like X86 will interpret it as number 
0x00001234; A BE machine interpret it as 0x34120000

since the code only runs on x86, now we have val = 0x00001234(len = 2) 
passed to pci_default_write_config(),  it is going to write the value 
into config space like this way: for (i = 0; i < len; val >>= 8, ++i). 
So the endianness is ok.

> Regards,
> Stefan
>
>

-- 
Yours Sincerely,

Cao Jin