[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-ppc] [PATCH qemu] RFC: spapr/iommu: Enable in-kernel TCE accel

From: Alexey Kardashevskiy
Subject: Re: [Qemu-ppc] [PATCH qemu] RFC: spapr/iommu: Enable in-kernel TCE acceleration via VFIO KVM device
Date: Wed, 20 Dec 2017 12:47:07 +1100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0

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


reply via email to

[Prev in Thread] Current Thread [Next in Thread]