| Message ID | 1473145893-17088-4-git-send-email-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Auger <eric.auger@redhat.com> writes: > Let's expand the usage of QEMU Error objects to vfio_msix_early_setup. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > --- > hw/vfio/pci.c | 28 ++++++++++++++++------------ > 1 file changed, 16 insertions(+), 12 deletions(-) > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index f7768e9..b132bf1 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -1275,7 +1275,7 @@ static void vfio_pci_fixup_msix_region(VFIOPCIDevice *vdev) > * need to first look for where the MSI-X table lives. So we > * unfortunately split MSI-X setup across two functions. > */ > -static int vfio_msix_early_setup(VFIOPCIDevice *vdev) > +static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp) > { > uint8_t pos; > uint16_t ctrl; > @@ -1285,22 +1285,25 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev) > > pos = pci_find_capability(&vdev->pdev, PCI_CAP_ID_MSIX); > if (!pos) { > - return 0; > + return; > } > > if (pread(fd, &ctrl, sizeof(ctrl), > vdev->config_offset + pos + PCI_MSIX_FLAGS) != sizeof(ctrl)) { > - return -errno; > + error_setg_errno(errp, -errno, "failed to read PCI MSIX FLAGS"); > + return; Looks like a bug fix to me: before your series, this is a silent failure. PATCH 1 makes it non-silent, with a generic error message, and this patch replaces it with a specific error message. > } > > if (pread(fd, &table, sizeof(table), > vdev->config_offset + pos + PCI_MSIX_TABLE) != sizeof(table)) { > - return -errno; > + error_setg_errno(errp, -errno, "failed to read PCI MSIX TABLE"); > + return; Likewise. > } > > if (pread(fd, &pba, sizeof(pba), > vdev->config_offset + pos + PCI_MSIX_PBA) != sizeof(pba)) { > - return -errno; > + error_setg_errno(errp, -errno, "failed to read PCI MSIX PBA"); > + return; Likewise. > } > > ctrl = le16_to_cpu(ctrl); > @@ -1330,10 +1333,11 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev) > (vdev->device_id & 0xff00) == 0x5800) { > msix->pba_offset = 0x1000; > } else { > - error_report("vfio: Hardware reports invalid configuration, " > - "MSIX PBA outside of specified BAR"); > + error_setg_errno(errp, -EINVAL, > + "hardware reports invalid configuration, " > + "MSIX PBA outside of specified BAR"); > g_free(msix); > - return -EINVAL; > + return; > } > } > > @@ -1343,7 +1347,7 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev) > > vfio_pci_fixup_msix_region(vdev); > > - return 0; > + return; > } > > static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos) > @@ -2642,9 +2646,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > > vfio_pci_size_rom(vdev); > > - ret = vfio_msix_early_setup(vdev); > - if (ret) { > - error_setg_errno(errp, ret, "msix early setup failure"); > + vfio_msix_early_setup(vdev, &err); > + if (err) { > + error_propagate(errp, err); > goto error; > } vfio_realize() still calls a few functions that report errors with error_report(), including vfio_get_group(), vfio_get_device(), vfio_pci_igd_opregion_init(), vfio_intx_enable(). If you prefer not to complete the job right now, please mark such calls FIXME.
Hi Markus, On 12/09/2016 14:58, Markus Armbruster wrote: > Eric Auger <eric.auger@redhat.com> writes: > >> Let's expand the usage of QEMU Error objects to vfio_msix_early_setup. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> --- >> hw/vfio/pci.c | 28 ++++++++++++++++------------ >> 1 file changed, 16 insertions(+), 12 deletions(-) >> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index f7768e9..b132bf1 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -1275,7 +1275,7 @@ static void vfio_pci_fixup_msix_region(VFIOPCIDevice *vdev) >> * need to first look for where the MSI-X table lives. So we >> * unfortunately split MSI-X setup across two functions. >> */ >> -static int vfio_msix_early_setup(VFIOPCIDevice *vdev) >> +static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp) >> { >> uint8_t pos; >> uint16_t ctrl; >> @@ -1285,22 +1285,25 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev) >> >> pos = pci_find_capability(&vdev->pdev, PCI_CAP_ID_MSIX); >> if (!pos) { >> - return 0; >> + return; >> } >> >> if (pread(fd, &ctrl, sizeof(ctrl), >> vdev->config_offset + pos + PCI_MSIX_FLAGS) != sizeof(ctrl)) { >> - return -errno; >> + error_setg_errno(errp, -errno, "failed to read PCI MSIX FLAGS"); >> + return; > > Looks like a bug fix to me: before your series, this is a silent > failure. PATCH 1 makes it non-silent, with a generic error message, and > this patch replaces it with a specific error message. Actually the init process was stopped without reporting the actual issue. But that's the goal of the series. I will improve the commit message. > >> } >> >> if (pread(fd, &table, sizeof(table), >> vdev->config_offset + pos + PCI_MSIX_TABLE) != sizeof(table)) { >> - return -errno; >> + error_setg_errno(errp, -errno, "failed to read PCI MSIX TABLE"); >> + return; > > Likewise. > >> } >> >> if (pread(fd, &pba, sizeof(pba), >> vdev->config_offset + pos + PCI_MSIX_PBA) != sizeof(pba)) { >> - return -errno; >> + error_setg_errno(errp, -errno, "failed to read PCI MSIX PBA"); >> + return; > > Likewise. > >> } >> >> ctrl = le16_to_cpu(ctrl); >> @@ -1330,10 +1333,11 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev) >> (vdev->device_id & 0xff00) == 0x5800) { >> msix->pba_offset = 0x1000; >> } else { >> - error_report("vfio: Hardware reports invalid configuration, " >> - "MSIX PBA outside of specified BAR"); >> + error_setg_errno(errp, -EINVAL, >> + "hardware reports invalid configuration, " >> + "MSIX PBA outside of specified BAR"); >> g_free(msix); >> - return -EINVAL; >> + return; >> } >> } >> >> @@ -1343,7 +1347,7 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev) >> >> vfio_pci_fixup_msix_region(vdev); >> >> - return 0; >> + return; >> } >> >> static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos) >> @@ -2642,9 +2646,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) >> >> vfio_pci_size_rom(vdev); >> >> - ret = vfio_msix_early_setup(vdev); >> - if (ret) { >> - error_setg_errno(errp, ret, "msix early setup failure"); >> + vfio_msix_early_setup(vdev, &err); >> + if (err) { >> + error_propagate(errp, err); >> goto error; >> } > > vfio_realize() still calls a few functions that report errors with > error_report(), including vfio_get_group(), vfio_get_device(), > vfio_pci_igd_opregion_init(), vfio_intx_enable(). If you prefer not to > complete the job right now, please mark such calls FIXME. I will complete the job but I just wanted to get some first feedbacks. Hopefully you already pointed out some issues! Thank you for your time. Best Regards Eric >
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index f7768e9..b132bf1 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -1275,7 +1275,7 @@ static void vfio_pci_fixup_msix_region(VFIOPCIDevice *vdev) * need to first look for where the MSI-X table lives. So we * unfortunately split MSI-X setup across two functions. */ -static int vfio_msix_early_setup(VFIOPCIDevice *vdev) +static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp) { uint8_t pos; uint16_t ctrl; @@ -1285,22 +1285,25 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev) pos = pci_find_capability(&vdev->pdev, PCI_CAP_ID_MSIX); if (!pos) { - return 0; + return; } if (pread(fd, &ctrl, sizeof(ctrl), vdev->config_offset + pos + PCI_MSIX_FLAGS) != sizeof(ctrl)) { - return -errno; + error_setg_errno(errp, -errno, "failed to read PCI MSIX FLAGS"); + return; } if (pread(fd, &table, sizeof(table), vdev->config_offset + pos + PCI_MSIX_TABLE) != sizeof(table)) { - return -errno; + error_setg_errno(errp, -errno, "failed to read PCI MSIX TABLE"); + return; } if (pread(fd, &pba, sizeof(pba), vdev->config_offset + pos + PCI_MSIX_PBA) != sizeof(pba)) { - return -errno; + error_setg_errno(errp, -errno, "failed to read PCI MSIX PBA"); + return; } ctrl = le16_to_cpu(ctrl); @@ -1330,10 +1333,11 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev) (vdev->device_id & 0xff00) == 0x5800) { msix->pba_offset = 0x1000; } else { - error_report("vfio: Hardware reports invalid configuration, " - "MSIX PBA outside of specified BAR"); + error_setg_errno(errp, -EINVAL, + "hardware reports invalid configuration, " + "MSIX PBA outside of specified BAR"); g_free(msix); - return -EINVAL; + return; } } @@ -1343,7 +1347,7 @@ static int vfio_msix_early_setup(VFIOPCIDevice *vdev) vfio_pci_fixup_msix_region(vdev); - return 0; + return; } static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos) @@ -2642,9 +2646,9 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) vfio_pci_size_rom(vdev); - ret = vfio_msix_early_setup(vdev); - if (ret) { - error_setg_errno(errp, ret, "msix early setup failure"); + vfio_msix_early_setup(vdev, &err); + if (err) { + error_propagate(errp, err); goto error; }
Let's expand the usage of QEMU Error objects to vfio_msix_early_setup. Signed-off-by: Eric Auger <eric.auger@redhat.com> --- hw/vfio/pci.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-)