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 <pbonz...@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.
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
