Re: [PATCH v2 5/9] vfio: Improve error reporting when MMIO region mapping fails

["Cédric Le Goater" <clg@redhat.com>]

On 2/10/25 15:36, Philippe Mathieu-Daudé wrote:
> On 30/1/25 14:43, Cédric Le Goater wrote:
>> When the IOMMU address space width is smaller than the physical
>> address width, a MMIO region of a device can fail to map because the
>> region is outside the supported IOVA ranges of the VM. In this case,
>> PCI peer-to-peer transactions on BARs are not supported.
>>
>> This can occur with the 39-bit IOMMU address space width, as can be
>> the case on some consumer processors or when using a vIOMMU device
>> with default settings. The current error message is unclear, also
>> change the error report to a warning because it is a non fatal
>> condition for the VM.
>>
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>   hw/vfio/common.c | 17 ++++++++++++++++-
>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 62af1216fc5a9089fc718c2afe3a405d9381db32..5c9d8657d746ce30af5ae8f9122101e086a61ef5 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -555,6 +555,18 @@ static bool vfio_get_section_iova_range(VFIOContainerBase *bcontainer,
>>       return true;
>>   }
>> +static void vfio_device_error_append(VFIODevice *vbasedev, Error **errp)
>> +{
>> +    /*
>> +     * MMIO region mapping failures are not fatal but in this case PCI
>> +     * peer-to-peer transactions are broken.
>> +     */
>> +    if (vbasedev && vbasedev->type == VFIO_DEVICE_TYPE_PCI) {
>> +        error_append_hint(errp, "%s: PCI peer-to-peer transactions "
>> +                          "on BARs are not supported.\n", vbasedev->name);
>> +    }
>> +}
>> +
>>   static void vfio_listener_region_add(MemoryListener *listener,
>>                                        MemoryRegionSection *section)
>>   {
>> @@ -670,7 +682,10 @@ static void vfio_listener_region_add(MemoryListener *listener,
>>                      strerror(-ret));
>>           if (memory_region_is_ram_device(section->mr)) {
>>               /* Allow unexpected mappings not to be fatal for RAM devices */
>> -            error_report_err(err);
>> +            VFIODevice *vbasedev =
>> +                vfio_get_vfio_device(memory_region_owner(section->mr));
>> +            vfio_device_error_append(vbasedev, &err);
> 
> Having vfio_get_vfio_device() returning NULL and
> vfio_device_error_append() also checking for NULL is odd.

Shouldn't vfio_device_error_append() check that its arguments are
safe to use ?

> Maybe just inline everything here?

I plan to use it elsewhere. See last patch.


Thanks,

C.


> 
>> +            warn_report_once_err(err);
>>               return;
>>           }
>>           goto fail;
>