| Message ID | 20231003101530.288864-5-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Prerequisite changes for IOMMUFD support | expand |
On 10/3/23 12:14, Eric Auger wrote: > In the VFIO_SPAPR_TCE_v2_IOMMU container case, when > KVM_SET_DEVICE_ATTR fails, we currently don't propagate the > error as we do on the vfio_spapr_create_window() failure > case. Let's align the code. Take the opportunity to > reword the error message and make it more explicit. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > > --- > > I think thise should end up in the > if (!container->initialized) { > if (!container->error) { > path and call the error_propagate_prepend() We could have this case also if (memory_region_is_ram_device(section->mr)) { error_report("failed to vfio_dma_map. pci p2p may not work"); return; } which was added by commit 567b5b309abe ("vfio/pci: Relax DMA map errors for MMIO regions"). There were a few changes in the failure path, like commit ac6dc3894fbb ("vfio: Generalize vfio_listener_region_add failure path") and it is unclear to me which one will be used. Anyhow, this needs some cleanup and this is what this patchset is proposing. Let's move on : Reviewed-by: Cédric Le Goater <clg@redhat.com> Thanks, C. > --- > hw/vfio/common.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 4e122fc4e4..c54a72ec80 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -878,11 +878,11 @@ static void vfio_listener_region_add(MemoryListener *listener, > QLIST_FOREACH(group, &container->group_list, container_next) { > param.groupfd = group->fd; > if (ioctl(vfio_kvm_device_fd, KVM_SET_DEVICE_ATTR, &attr)) { > - error_report("vfio: failed to setup fd %d " > - "for a group with fd %d: %s", > - param.tablefd, param.groupfd, > - strerror(errno)); > - return; > + error_setg_errno(&err, errno, > + "vfio: failed GROUP_SET_SPAPR_TCE for " > + "KVM VFIO device %d and group fd %d", > + param.tablefd, param.groupfd); > + goto fail; > } > trace_vfio_spapr_group_attach(param.groupfd, param.tablefd); > }
Hi Cédric, On 10/3/23 16:53, Cédric Le Goater wrote: > On 10/3/23 12:14, Eric Auger wrote: >> In the VFIO_SPAPR_TCE_v2_IOMMU container case, when >> KVM_SET_DEVICE_ATTR fails, we currently don't propagate the >> error as we do on the vfio_spapr_create_window() failure >> case. Let's align the code. Take the opportunity to >> reword the error message and make it more explicit. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> >> --- >> >> I think thise should end up in the >> if (!container->initialized) { >> if (!container->error) { >> path and call the error_propagate_prepend() > > We could have this case also > > if (memory_region_is_ram_device(section->mr)) { > error_report("failed to vfio_dma_map. pci p2p may not work"); > return; > } > > which was added by commit 567b5b309abe ("vfio/pci: Relax DMA map errors > for MMIO regions"). There were a few changes in the failure path, like > commit ac6dc3894fbb ("vfio: Generalize vfio_listener_region_add failure > path") and it is unclear to me which one will be used. Anyhow, this needs > some cleanup and this is what this patchset is proposing. Let's move on : > > Reviewed-by: Cédric Le Goater <clg@redhat.com> yep the original error_report() looks strange in that context, especially without comment. Let's hope the lack of propagation was not done on purpose. I will add the original committer to the cc list. Eric > > Thanks, > > C. > > >> --- >> hw/vfio/common.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index 4e122fc4e4..c54a72ec80 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -878,11 +878,11 @@ static void >> vfio_listener_region_add(MemoryListener *listener, >> QLIST_FOREACH(group, &container->group_list, >> container_next) { >> param.groupfd = group->fd; >> if (ioctl(vfio_kvm_device_fd, >> KVM_SET_DEVICE_ATTR, &attr)) { >> - error_report("vfio: failed to setup fd %d " >> - "for a group with fd %d: %s", >> - param.tablefd, param.groupfd, >> - strerror(errno)); >> - return; >> + error_setg_errno(&err, errno, >> + "vfio: failed >> GROUP_SET_SPAPR_TCE for " >> + "KVM VFIO device %d and >> group fd %d", >> + param.tablefd, param.groupfd); >> + goto fail; >> } >> trace_vfio_spapr_group_attach(param.groupfd, >> param.tablefd); >> } >
diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 4e122fc4e4..c54a72ec80 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -878,11 +878,11 @@ static void vfio_listener_region_add(MemoryListener *listener, QLIST_FOREACH(group, &container->group_list, container_next) { param.groupfd = group->fd; if (ioctl(vfio_kvm_device_fd, KVM_SET_DEVICE_ATTR, &attr)) { - error_report("vfio: failed to setup fd %d " - "for a group with fd %d: %s", - param.tablefd, param.groupfd, - strerror(errno)); - return; + error_setg_errno(&err, errno, + "vfio: failed GROUP_SET_SPAPR_TCE for " + "KVM VFIO device %d and group fd %d", + param.tablefd, param.groupfd); + goto fail; } trace_vfio_spapr_group_attach(param.groupfd, param.tablefd); }
In the VFIO_SPAPR_TCE_v2_IOMMU container case, when KVM_SET_DEVICE_ATTR fails, we currently don't propagate the error as we do on the vfio_spapr_create_window() failure case. Let's align the code. Take the opportunity to reword the error message and make it more explicit. Signed-off-by: Eric Auger <eric.auger@redhat.com> --- I think thise should end up in the if (!container->initialized) { if (!container->error) { path and call the error_propagate_prepend() --- hw/vfio/common.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)