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 <[email protected]>
>>
>> ---
>>
>> 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 <[email protected]>
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);
>> }
>
