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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2678 bytes --]

On Tue, Dec 19, 2017 at 03:59:59PM +0100, Paolo Bonzini wrote:
> 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.

It's more than just enums, doing it the other way around is putting
fairly intimate knowledge of a specific guest IOMMU workings into the
VFIO code.

Fundamentally this *requires* linking vfio knowledge to guest iommu
(kvm) knowledge, so some cross linkage we'd usually want to avoid is
inevitable.  I don't see that there's a strong argument for whether we
put the bit of vfio knowledge into the spapr viommu or the bit of
spapr viommu knowledge into vfio.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]