Re: [Qemu-devel] [PATCH qemu] RFC: spapr/iommu: Enable in-kernel TCE acceleration via VFIO KVM device

[Paolo Bonzini <pbonzini@redhat.com>]

On 19/12/2017 15:09, Alex Williamson wrote:
> On Tue, 19 Dec 2017 12:12:35 +0100
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> On 12/12/2017 06:46, Alex Williamson wrote:
>>>> +enum IOMMUMemoryRegionAttr {
>>>> +    IOMMU_ATTR_KVM_FD  
>>>
>>> You're generalizing the wrong thing here, this is specifically a
>>> SPAPR_TCE_FD, call it that.  
>>
>> ... and you're not even implementing set_attr, so let's drop it.
>>
>> My suggestion is to add a function in hw/vfio:
>>
>>     int vfio_container_attach_kvm_spapr_tce(VFIOContainer *cont,
>>                                             int tablefd);
>>
>> and an IOMMUMemoryRegionClass member:
>>
>>     int (*set_vfio_container_attrs)(IOMMUMemoryRegion *iommu,
>>                                     VFIOContainer *cont)
>>
>> Then your implementation for the latter is as simple as this:
>>
>>     if (!kvm_enabled() || !kvmppc_has_cap_spapr_vfio()) {
>>         sPAPRTCETable *tcet = container_of(iommu, sPAPRTCETable, iommu);
>>         return vfio_container_attach_kvm_spapr_tce(cont, tcet->fd);
>>     }
> 
> Ugh, exactly the sort of interface I've been trying to avoid, vfio
> specific callbacks on common data structures handing out vfio private
> data pointers,

True, VFIOContainer* is private, but in those declarations it's also opaque.

The VFIO container is the representation of the IOMMU, so it makes sense
to me to have a function to set it up according to QEMU's IOMMU object.
I don't think we will be introducing another object soon that is similar
to the VFIO container.

> requiring additional exported functions from vfio for
> each new user of it.  Why is this better?

I understand that you don't like having many exported functions, but you
are just pushing the problem on the memory.h side where you'd get many
attribute enums.

I suppose it would pay if you have many attributes and many clients,
and/or if the attributes need to be public similar to sysfs.  I mention
public vs. private because, before inventing a new mechanism for
attributes, it would make sense to look at QOM properties.  However,
here we have one client (VFIO) and one attribute (the KVM IOMMU fd) that
is pretty much internal to QEMU.

So, depending on the direction you want to pull/push the information to,
I would do either:

- as above, a generic IOMMU callback

     int (*set_vfio_container_attrs)(IOMMUMemoryRegion *iommu,
                                     VFIOContainer *cont);

- an object-type check in VFIO, followed by calling a new function

     int spapr_tce_iommu_get_kvm_fd(IOMMUMemoryRegion *iommu);

- if we foresee having more IOMMU devices in KVM, let's rename
KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE to KVM_DEV_VFIO_GROUP_ATTACH_IOMMU and
add a new function

     int iommu_memory_region_get_kvm_fd(IOMMUMemoryRegion *iommu);

that requires no object-type check in VFIO.

Paolo