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

[Alexey Kardashevskiy]

On 20/12/17 01:59, Paolo Bonzini wrote:
> On 19/12/2017 15:09, Alex Williamson wrote:
>> On Tue, 19 Dec 2017 12:12:35 +0100
>> Paolo Bonzini <address@hidden> 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.

This is how it started and the comment was that this KVM fd needs to have a
well defined semantic which I struggle to provide.

I could just implement a "spapr-kvm-fd" QOM property on sPAPR's IOMMU MR if
this is acceptable.



-- 
Alexey