Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
hw/pci-host/piix.c | 31 +++++++++++++++----------------
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index a684a7cca9..0153f32a32 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -804,7 +804,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 void host_pci_config_read(int pos, int len, uint32_t *val, Error **errp)
{
char path[PATH_MAX];
int config_fd;
@@ -812,19 +812,19 @@ static int host_pci_config_read(int pos, int len, uint32_t *val)
/* Access real host bridge. */
int rc = snprintf(path, size, "/sys/bus/pci/devices/%04x:%02x:%02x.%d/%s",
0, 0, 0, 0, "config");
- int ret = 0;
if (rc >= size || rc < 0) {
- return -ENODEV;
+ error_setg(&error_abort, "Invalid PCI device");
}
config_fd = open(path, O_RDWR);
if (config_fd < 0) {
- return -ENODEV;
+ error_setg_errno(errp, errno, "Failed to open: %s", path);
+ return;
}
if (lseek(config_fd, pos, SEEK_SET) != pos) {
- ret = -errno;
+ error_setg_errno(errp, errno, "Failed to seek: %s", path);
goto out;
}
@@ -832,32 +832,31 @@ static int host_pci_config_read(int pos, int len, uint32_t *val)
rc = read(config_fd, (uint8_t *)val, len);
} while (rc < 0 && (errno == EINTR || errno == EAGAIN));
if (rc != len) {
- ret = -errno;
+ error_setg_errno(errp, errno, "Failed to read: %s", path);
}
out:
close(config_fd);
- return ret;
}
-static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
+static void igd_pt_i440fx_realize(PCIDevice *pci, Error **errp)
{
uint32_t val = 0;
- int rc, i, num;
+ int i, num;
int pos, len;
+ Error *local_err = NULL;
num = ARRAY_SIZE(igd_host_bridge_infos);
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);
- if (rc) {
- return -ENODEV;
+ host_pci_config_read(pos, len, &val, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ return;
}
- pci_default_write_config(pci_dev, pos, val, len);
+ pci_default_write_config(pci, pos, val, len);
}
-
- return 0;
}
static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
@@ -865,7 +864,7 @@ static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
DeviceClass *dc = DEVICE_CLASS(klass);
PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
- k->init = igd_pt_i440fx_initfn;
+ k->realize = igd_pt_i440fx_realize;
dc->desc = "IGD Passthrough Host bridge";
}
--
2.15.1
Hi Philippe,
Thanks for the patch.
On 17/12/2017 22:49, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/pci-host/piix.c | 31 +++++++++++++++----------------
> 1 file changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index a684a7cca9..0153f32a32 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -804,7 +804,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 void host_pci_config_read(int pos, int len, uint32_t *val, Error **errp)
> {
> char path[PATH_MAX];
> int config_fd;
> @@ -812,19 +812,19 @@ static int host_pci_config_read(int pos, int len, uint32_t *val)
> /* Access real host bridge. */
> int rc = snprintf(path, size, "/sys/bus/pci/devices/%04x:%02x:%02x.%d/%s",
> 0, 0, 0, 0, "config");
> - int ret = 0;
>
> if (rc >= size || rc < 0) {
> - return -ENODEV;
> + error_setg(&error_abort, "Invalid PCI device");
I think a return statement is missing here.
> }
>
> config_fd = open(path, O_RDWR);
> if (config_fd < 0) {
> - return -ENODEV;
> + error_setg_errno(errp, errno, "Failed to open: %s", path);
> + return;
> }
>
> if (lseek(config_fd, pos, SEEK_SET) != pos) {
> - ret = -errno;
> + error_setg_errno(errp, errno, "Failed to seek: %s", path);
> goto out;
> }
>
> @@ -832,32 +832,31 @@ static int host_pci_config_read(int pos, int len, uint32_t *val)
> rc = read(config_fd, (uint8_t *)val, len);
> } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
> if (rc != len) {
> - ret = -errno;
> + error_setg_errno(errp, errno, "Failed to read: %s", path);
> }
>
> out:
> close(config_fd);
> - return ret;
> }
>
> -static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
> +static void igd_pt_i440fx_realize(PCIDevice *pci, Error **errp)
I would keep pci_dev, pci is too "broad".
Also is kind of convention, try to grep the source code.
Other than that the patch looks good to me.
Thanks,
Marcel
> {
> uint32_t val = 0;
> - int rc, i, num;
> + int i, num;
> int pos, len;
> + Error *local_err = NULL;
>
> num = ARRAY_SIZE(igd_host_bridge_infos);
> 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);
> - if (rc) {
> - return -ENODEV;
> + host_pci_config_read(pos, len, &val, &local_err);
> + if (local_err) {
> + error_propagate(errp, local_err);
> + return;
> }
> - pci_default_write_config(pci_dev, pos, val, len);
> + pci_default_write_config(pci, pos, val, len);
> }
> -
> - return 0;
> }
>
> static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
> @@ -865,7 +864,7 @@ static void igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
> DeviceClass *dc = DEVICE_CLASS(klass);
> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>
> - k->init = igd_pt_i440fx_initfn;
> + k->realize = igd_pt_i440fx_realize;
> dc->desc = "IGD Passthrough Host bridge";
> }
>
>
Hi Marcel,
On 12/18/2017 04:05 AM, Marcel Apfelbaum wrote:
> Hi Philippe,
>
> Thanks for the patch.
>
> On 17/12/2017 22:49, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> hw/pci-host/piix.c | 31 +++++++++++++++----------------
>> 1 file changed, 15 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
>> index a684a7cca9..0153f32a32 100644
>> --- a/hw/pci-host/piix.c
>> +++ b/hw/pci-host/piix.c
>> @@ -804,7 +804,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 void host_pci_config_read(int pos, int len, uint32_t *val,
>> Error **errp)
>> {
>> char path[PATH_MAX];
>> int config_fd;
>> @@ -812,19 +812,19 @@ static int host_pci_config_read(int pos, int
>> len, uint32_t *val)
>> /* Access real host bridge. */
>> int rc = snprintf(path, size,
>> "/sys/bus/pci/devices/%04x:%02x:%02x.%d/%s",
>> 0, 0, 0, 0, "config");
>> - int ret = 0;
>> if (rc >= size || rc < 0) {
>> - return -ENODEV;
>> + error_setg(&error_abort, "Invalid PCI device");
>
> I think a return statement is missing here.
Well, error_setg(&error_abort) will never return, however checking the
documentation again I noticed I forgot these recommendations:
* Please don't error_setg(&error_fatal, ...), use error_report() and
* exit(), because that's more obvious.
* Likewise, don't error_setg(&error_abort, ...), use assert().
abort() might be OK but a bit harsh, so I'll use error_report() + exit()
here.
>> }
>> config_fd = open(path, O_RDWR);
>> if (config_fd < 0) {
>> - return -ENODEV;
>> + error_setg_errno(errp, errno, "Failed to open: %s", path);
>> + return;
>> }
>> if (lseek(config_fd, pos, SEEK_SET) != pos) {
>> - ret = -errno;
>> + error_setg_errno(errp, errno, "Failed to seek: %s", path);
>> goto out;
>> }
>> @@ -832,32 +832,31 @@ static int host_pci_config_read(int pos, int
>> len, uint32_t *val)
>> rc = read(config_fd, (uint8_t *)val, len);
>> } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
>> if (rc != len) {
>> - ret = -errno;
>> + error_setg_errno(errp, errno, "Failed to read: %s", path);
>> }
>> out:
>> close(config_fd);
>> - return ret;
>> }
>> -static int igd_pt_i440fx_initfn(struct PCIDevice *pci_dev)
>> +static void igd_pt_i440fx_realize(PCIDevice *pci, Error **errp)
>
> I would keep pci_dev, pci is too "broad".
> Also is kind of convention, try to grep the source code.
Ok.
> Other than that the patch looks good to me.
Thanks!
> Thanks,
> Marcel
>
>> {
>> uint32_t val = 0;
>> - int rc, i, num;
>> + int i, num;
>> int pos, len;
>> + Error *local_err = NULL;
>> num = ARRAY_SIZE(igd_host_bridge_infos);
>> 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);
>> - if (rc) {
>> - return -ENODEV;
>> + host_pci_config_read(pos, len, &val, &local_err);
>> + if (local_err) {
>> + error_propagate(errp, local_err);
>> + return;
>> }
>> - pci_default_write_config(pci_dev, pos, val, len);
>> + pci_default_write_config(pci, pos, val, len);
>> }
>> -
>> - return 0;
>> }
>> static void igd_passthrough_i440fx_class_init(ObjectClass *klass,
>> void *data)
>> @@ -865,7 +864,7 @@ static void
>> igd_passthrough_i440fx_class_init(ObjectClass *klass, void *data)
>> DeviceClass *dc = DEVICE_CLASS(klass);
>> PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>> - k->init = igd_pt_i440fx_initfn;
>> + k->realize = igd_pt_i440fx_realize;
>> dc->desc = "IGD Passthrough Host bridge";
>> }
>>
>
[...]
>>> -static int host_pci_config_read(int pos, int len, uint32_t *val)
>>> +static void host_pci_config_read(int pos, int len, uint32_t *val,
>>> Error **errp)
>>> {
>>> char path[PATH_MAX];
>>> int config_fd;
>>> @@ -812,19 +812,19 @@ static int host_pci_config_read(int pos, int
>>> len, uint32_t *val)
>>> /* Access real host bridge. */
>>> int rc = snprintf(path, size,
>>> "/sys/bus/pci/devices/%04x:%02x:%02x.%d/%s",
>>> 0, 0, 0, 0, "config");
>>> - int ret = 0;
>>> if (rc >= size || rc < 0) {
>>> - return -ENODEV;
>>> + error_setg(&error_abort, "Invalid PCI device");
>>
>> I think a return statement is missing here.
>
> Well, error_setg(&error_abort) will never return, however checking the
> documentation again I noticed I forgot these recommendations:
>
> * Please don't error_setg(&error_fatal, ...), use error_report() and
> * exit(), because that's more obvious.
> * Likewise, don't error_setg(&error_abort, ...), use assert().
>
> abort() might be OK but a bit harsh, so I'll use error_report() + exit()
> here.
Actually g_strdup_printf() is cleaner.
>>> }
>>> config_fd = open(path, O_RDWR);
>>> if (config_fd < 0) {
>>> - return -ENODEV;
>>> + error_setg_errno(errp, errno, "Failed to open: %s", path);
>>> + return;
>>> }
>>> if (lseek(config_fd, pos, SEEK_SET) != pos) {
>>> - ret = -errno;
>>> + error_setg_errno(errp, errno, "Failed to seek: %s", path);
>>> goto out;
>>> }
>>> @@ -832,32 +832,31 @@ static int host_pci_config_read(int pos, int
>>> len, uint32_t *val)
>>> rc = read(config_fd, (uint8_t *)val, len);
>>> } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
>>> if (rc != len) {
>>> - ret = -errno;
>>> + error_setg_errno(errp, errno, "Failed to read: %s", path);
>>> }
>>> out:
>>> close(config_fd);
>>> - return ret;
>>> }
[...]
© 2016 - 2025 Red Hat, Inc.