RE: [PATCH v1] vfio/common: Separate vfio-pci ranges

[Duan, Zhenzhong]

>-----Original Message-----
>From: Joao Martins <joao.m.martins@oracle.com>
>Sent: Monday, September 11, 2023 6:20 PM
>To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu-devel@nongnu.org
>Cc: Alex Williamson <alex.williamson@redhat.com>; Cedric Le Goater
><clg@redhat.com>; Avihai Horon <avihaih@nvidia.com>; Gerd Hoffmann
><kraxel@redhat.com>
>Subject: Re: [PATCH v1] vfio/common: Separate vfio-pci ranges
>
>On 11/09/2023 10:48, Duan, Zhenzhong wrote:
>>>>> +static bool vfio_section_is_vfio_pci(MemoryRegionSection *section,
>>>>> +                                     VFIOContainer *container)
>>>>> +{
>>>>> +    VFIOPCIDevice *pcidev;
>>>>> +    VFIODevice *vbasedev;
>>>>> +    VFIOGroup *group;
>>>>> +    Object *owner;
>>>>> +
>>>>> +    owner = memory_region_owner(section->mr);
>>>>> +
>>>>> +    QLIST_FOREACH(group, &container->group_list, container_next) {
>>>>> +        QLIST_FOREACH(vbasedev, &group->device_list, next) {
>>>>> +            if (vbasedev->type != VFIO_DEVICE_TYPE_PCI) {
>>>>> +                continue;
>>>>> +            }
>>>>> +            pcidev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
>>>>> +            if (OBJECT(pcidev) == owner) {
>>>>> +                return true;
>>>>> +            }
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    return false;
>>>>> +}
>>>>
>>>> What about simplify it with memory_region_is_ram_device()?
>>>> This way vdpa device could also be included.
>>>>
>>>
>>> Note that the check is not interested in RAM (or any other kinda of memory
>like
>>> VGA). That's covered in the 32-64 ranges. But rather in any PCI device RAM
>that
>>> would fall in the 64-bit PCI hole. Would memory_region_is_ram_device() 
>>> really
>>> cover it? If so, I am all for the simplification.
>>
>> Ram device is used not only by vfio pci bars but also host notifier of vdpa 
>> and
>vhost-user.
>
>My only concern is whether this is all part of the pci-hole64 or not e.g. if we
>expand to general memory_region_is_ram_device() would we go back to the
>initial bug where we create an enourmous range.

Ok, I have no idea if memory_region_is_ram_device() will be expanded for other
usage in the future. Anyway looks better to keep your code for secure.

> The latter that you mentioned should be
>mainly virtio-net devices as presented to the guest (regardless of backend is
>vdpa, or vhost-user) and perhaps they are all in the hole32 PCI hole?
Agree, that's possible.

Thanks
Zhenzhong