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

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

[Cao jin]

Fix the bug introduced by 595a4f07. Function host_pci_config_read() should be
passed by a reference, not a value, for the later pci_default_write_config().

Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
Separated from previous "igd-passthru convert to realize" patch. Since these
two don`t have dependency, can send it solely.

Not test since it is easy to find out if reading carefully, just compiled.

 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);
     } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
     if (rc != len) {
         ret = -errno;
     }
+
 out:
     close(config_fd);
     return ret;
@@ -805,7 +807,7 @@ static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
     for (i = 0; i < num; i++) {
         pos = igd_host_bridge_infos[i].offset;
         len = igd_host_bridge_infos[i].len;
-        rc = host_pci_config_read(pos, len, val);
+        rc = host_pci_config_read(pos, len, &val);
         if (rc) {
             return -ENODEV;
         }
-- 
2.1.0

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

[Michael S. Tsirkin]

On Mon, Dec 28, 2015 at 10:54:23AM +0800, Cao jin wrote:
> Fix the bug introduced by 595a4f07. Function host_pci_config_read() should be
> passed by a reference, not a value, for the later pci_default_write_config().

What's the effect of the bug? Does it break igd assignment?
How come it worked for people?
If the function is never called, mayber we can get rid
of it completely?

Stefano?

> 
> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
> ---
> Separated from previous "igd-passthru convert to realize" patch. Since these
> two don`t have dependency, can send it solely.
> 
> Not test since it is easy to find out if reading carefully, just compiled.
> 
>  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);
>      } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
>      if (rc != len) {
>          ret = -errno;
>      }
> +
>  out:
>      close(config_fd);
>      return ret;
> @@ -805,7 +807,7 @@ static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
>      for (i = 0; i < num; i++) {
>          pos = igd_host_bridge_infos[i].offset;
>          len = igd_host_bridge_infos[i].len;
> -        rc = host_pci_config_read(pos, len, val);
> +        rc = host_pci_config_read(pos, len, &val);
>          if (rc) {
>              return -ENODEV;
>          }
> -- 
> 2.1.0
> 
> 

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

[Cao jin]

On 12/28/2015 07:50 PM, Michael S. Tsirkin wrote:
> On Mon, Dec 28, 2015 at 10:54:23AM +0800, Cao jin wrote:
>> Fix the bug introduced by 595a4f07. Function host_pci_config_read() should be
>> passed by a reference, not a value, for the later pci_default_write_config().
>
> What's the effect of the bug? Does it break igd assignment?
> How come it worked for people?
> If the function is never called, mayber we can get rid
> of it completely?
>

sorry if I didn`t explain it clearly to you. let me try the explanation 
again: This function is called only when using 
TYPE_IGD_PASSTHROUGH_I440FX_PCI_DEVICE(when realize it)

the effect of the bug:
pci_default_write_config(pci_dev, pos, val, len);
                                         ^ *its value is always 0*

I think it won`t break igd assignment, but just give a wrong register 
value(forever 0) in PCI config space(wrong register value may results in 
abnormal working state?). the register should get its value by 
host_pci_config_read()

Because my bad English description, Let me do a analogy, here is the 
imitation of original code:

void swap(unsigned int val)  //this is host_pci_config_read()
{
     unsigned int org = 2;
     memcpy(&val, &org, sizeof(unsigned int));
}

int main()
{
     unsigned int val = 0;

     swap(val);

     printf("val = %d\n", val);
     return 0;
}

author want to get: val = 2. but it will always: val = 0; This is 
exactly the bug I find.

> Stefano?
>
>>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>> Separated from previous "igd-passthru convert to realize" patch. Since these
>> two don`t have dependency, can send it solely.
>>
>> Not test since it is easy to find out if reading carefully, just compiled.
>>
>>   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);
>>       } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
>>       if (rc != len) {
>>           ret = -errno;
>>       }
>> +
>>   out:
>>       close(config_fd);
>>       return ret;
>> @@ -805,7 +807,7 @@ static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
>>       for (i = 0; i < num; i++) {
>>           pos = igd_host_bridge_infos[i].offset;
>>           len = igd_host_bridge_infos[i].len;
>> -        rc = host_pci_config_read(pos, len, val);
>> +        rc = host_pci_config_read(pos, len, &val);
>>           if (rc) {
>>               return -ENODEV;
>>           }
>> --
>> 2.1.0
>>
>>
>
>
> .
>

-- 
Yours Sincerely,

Cao Jin

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

[Cao jin]

BTW, I send the v2 version of this patch, the changelog of v2 is:

ensure the value writen into register of pci config space is always 
little endian, using cpu_to_le32().

So, the actual change by v2 is following:

-        pci_default_write_config(pci_dev, pos, val, len);
+        pci_default_write_config(pci_dev, pos, cpu_to_le32(val), len);

Maybe we can discuss in v2 thread.

On 12/28/2015 07:50 PM, Michael S. Tsirkin wrote:
> On Mon, Dec 28, 2015 at 10:54:23AM +0800, Cao jin wrote:
>> Fix the bug introduced by 595a4f07. Function host_pci_config_read() should be
>> passed by a reference, not a value, for the later pci_default_write_config().
>
> What's the effect of the bug? Does it break igd assignment?
> How come it worked for people?
> If the function is never called, mayber we can get rid
> of it completely?
>
> Stefano?
>
>>
>> Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
>> ---
>> Separated from previous "igd-passthru convert to realize" patch. Since these
>> two don`t have dependency, can send it solely.
>>
>> Not test since it is easy to find out if reading carefully, just compiled.
>>
>>   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);
>>       } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
>>       if (rc != len) {
>>           ret = -errno;
>>       }
>> +
>>   out:
>>       close(config_fd);
>>       return ret;
>> @@ -805,7 +807,7 @@ static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
>>       for (i = 0; i < num; i++) {
>>           pos = igd_host_bridge_infos[i].offset;
>>           len = igd_host_bridge_infos[i].len;
>> -        rc = host_pci_config_read(pos, len, val);
>> +        rc = host_pci_config_read(pos, len, &val);
>>           if (rc) {
>>               return -ENODEV;
>>           }
>> --
>> 2.1.0
>>
>>
>
>
> .
>

-- 
Yours Sincerely,

Cao Jin