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é <f4...@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";
>> }
>>
>
