Re: [PATCH v3 04/15] vfio/common: Propagate KVM_SET_DEVICE_ATTR error if any

[Eric Auger <eric.auger@redhat.com>]

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);
>>                   }
>