Re: [Qemu-devel] [PATCH v2] bugfix: passing reference instead of value
Hi Paolo
On 12/30/2015 12:25 AM, Paolo Bonzini wrote:
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.
This patch works but the added conversion to LE is conceptually wrong.
The conversion from LE to CPU and vice versa is already done by
host_pci_config_read and pci_default_write_config. You don't have it in
host_pci_config_read because the code is not portable and x86 is
little-endian.
Do you mean, this code section(or this igd passthru device)will only run
on x86? If it is, yes, the cpu_to_le32() is not necessary. If it has
opportunity to run on BE, I think it is necessary, right?
The reason I add the conversion is that I am not sure whether it will
run on a BE machine or not, and it will no bad to LE machine like x86.
Generally, functions that accept/return integers take care themselves of
endianness conversions.
Paolo
hw/pci-host/piix.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 715208b..a9cb983 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,11 +807,11 @@ 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;
}
-pci_default_write_config(pci_dev, pos, val, len);
+pci_default_write_config(pci_dev, pos, cpu_to_le32(val), len);
}
return 0;
.
--
Yours Sincerely,
Cao Jin
Re: [Qemu-devel] [PATCH v2] bugfix: passing reference instead of value
>>> 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.
This patch works but the added conversion to LE is conceptually wrong.
The conversion from LE to CPU and vice versa is already done by
host_pci_config_read and pci_default_write_config. You don't have it in
host_pci_config_read because the code is not portable and x86 is
little-endian.
Generally, functions that accept/return integers take care themselves of
endianness conversions.
Paolo
>>> hw/pci-host/piix.c | 10 ++
>>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>>> index 715208b..a9cb983 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,11 +807,11 @@ 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;
>>> }
>>> -pci_default_write_config(pci_dev, pos, val, len);
>>> +pci_default_write_config(pci_dev, pos, cpu_to_le32(val), len);
>>> }
>>>
>>> return 0;
>>>
>>
>
Re: [Qemu-devel] [PATCH v2] bugfix: passing reference instead of value
Oops, got following feedback 2nd time:
The following message to was undeliverable.
The reason for the problem:
5.1.0 - Unknown address error 550-'#5.1.0 Address rejected.'
I guess the author address is not available anymore
On 12/29/2015 07:54 PM, Cao jin wrote:
CC the code author: Tiejun Chen
On 12/28/2015 12:42 PM, 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().
And because value in PCI config space are little-endian, use
cpu_to_le32() to
ensure it when write config.
Signed-off-by: Cao jin
---
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 | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 715208b..a9cb983 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,11 +807,11 @@ 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;
}
-pci_default_write_config(pci_dev, pos, val, len);
+pci_default_write_config(pci_dev, pos, cpu_to_le32(val), len);
}
return 0;
--
Yours Sincerely,
Cao Jin
Re: [Qemu-devel] [PATCH v2] bugfix: passing reference instead of value
CC the code author: Tiejun Chen
On 12/28/2015 12:42 PM, 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().
And because value in PCI config space are little-endian, use cpu_to_le32() to
ensure it when write config.
Signed-off-by: Cao jin
---
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 | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 715208b..a9cb983 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,11 +807,11 @@ 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;
}
-pci_default_write_config(pci_dev, pos, val, len);
+pci_default_write_config(pci_dev, pos, cpu_to_le32(val), len);
}
return 0;
--
Yours Sincerely,
Cao Jin
