| Message ID | 1451271263-8275-1-git-send-email-caoj.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
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 > >
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 >> >> > > > . >
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 >> >> > > > . >
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; }
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(-)