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

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

[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);
     } 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 v3] bugfix: passing reference instead of value

[Stefan Weil]

[-- Attachment #1: Type: text/plain, Size: 1525 bytes --]

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.

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.

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

Regards,
Stefan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Cao jin]

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

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

[Paolo Bonzini]

On 02/01/2016 10:06, 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.
> 
> 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.
> 
> * The len argument can obviously be 2 or 4. Will endianness handling
> work for both cases?

Not sure why this is in pci-host/piix.c, but it's only used on Intel
processors and only on Linux.

Paolo

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

[Michael S. Tsirkin]

On Sat, Jan 02, 2016 at 10:06:10AM +0100, 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.
> 
> 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 pokes at sysfs, it has no chance to work on windows anyway.

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

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

[Michael S. Tsirkin]

On Sat, Jan 02, 2016 at 04:02:20PM +0800, Cao jin wrote:
> 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.

It really should be le32_to_cpu and a separate patch,
but I think it's preferable to have it there
since people tend to copy code around.

But in any case, before merging any patches in this function I'd like to
hear a response from someone explaining why is this function necessary
at all, since it provably never did anything useful.

> 
>  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 v3] bugfix: passing reference instead of value

[Stefano Stabellini]

On Sat, 2 Jan 2016, Michael S. Tsirkin wrote:
> On Sat, Jan 02, 2016 at 04:02:20PM +0800, Cao jin wrote:
> > 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.
> 
> It really should be le32_to_cpu and a separate patch,
> but I think it's preferable to have it there
> since people tend to copy code around.
> 
> But in any case, before merging any patches in this function I'd like to
> hear a response from someone explaining why is this function necessary
> at all, since it provably never did anything useful.

If Tiejun's email address bounces, then we are unlikely to get a reply.

I think that the pass-by-value bug was introduced in one of the
rebase/resend versions, as the series is very old and originally looked
very different. I would take the patch as is to fix the obvious bug.

This is how the original code looks like:

http://xenbits.xen.org/gitweb/?p=qemu-xen-traditional.git;a=blob_plain;f=hw/pt-graphics.c;hb=HEAD

See the function named igd_pci_read.





> > 
> >  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 v3] bugfix: passing reference instead of value

[Michael S. Tsirkin]

On Mon, Jan 04, 2016 at 02:14:48PM +0000, Stefano Stabellini wrote:
> On Sat, 2 Jan 2016, Michael S. Tsirkin wrote:
> > On Sat, Jan 02, 2016 at 04:02:20PM +0800, Cao jin wrote:
> > > 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.
> > 
> > It really should be le32_to_cpu and a separate patch,
> > but I think it's preferable to have it there
> > since people tend to copy code around.
> > 
> > But in any case, before merging any patches in this function I'd like to
> > hear a response from someone explaining why is this function necessary
> > at all, since it provably never did anything useful.
> 
> If Tiejun's email address bounces, then we are unlikely to get a reply.
> 
> I think that the pass-by-value bug was introduced in one of the
> rebase/resend versions, as the series is very old and originally looked
> very different. I would take the patch as is to fix the obvious bug.

Yes but with this bug in place, we know no one is using this
device. And if no one can be bothered to test it,
maybe we should rip out the code and be done with it.

OTOH Gerd has apparently been looking at making it
work for kvm, maybe this will bring in testers/users.

I'll apply the fix for now.


> This is how the original code looks like:
> 
> http://xenbits.xen.org/gitweb/?p=qemu-xen-traditional.git;a=blob_plain;f=hw/pt-graphics.c;hb=HEAD
> 
> See the function named igd_pci_read.
> 
> 
> 
> 
> 
> > > 
> > >  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
> > > 
> > > 
> >