Re: [Qemu-devel] [PATCH qemu v15 14/17] spapr_iommu, vfio, memory: Notify IOMMU about starting/stopping being used by VFIO

[Alexey Kardashevskiy]

On 04/07/2016 10:40 AM, David Gibson wrote:
> On Mon, Apr 04, 2016 at 07:33:43PM +1000, Alexey Kardashevskiy wrote:
> > The sPAPR TCE tables manage 2 copies when VFIO is using an IOMMU -
> > a guest view of the table and a hardware TCE table. If there is no VFIO
> > presense in the address space, then just the guest view is used, if
> > this is the case, it is allocated in the KVM. However since there is no
> > support yet for VFIO in KVM TCE hypercalls, when we start using VFIO,
> > we need to move the guest view from KVM to the userspace; and we need
> > to do this for every IOMMU on a bus with VFIO devices.
> >
> > This adds vfio_start/vfio_stop callbacks in MemoryRegionIOMMUOps to
> > notifiy IOMMU about changing environment so it can reallocate the table
> > to/from KVM or (when available) hook the IOMMU groups with the logical
> > bus (LIOBN) in the KVM.
> >
> > This removes explicit spapr_tce_set_need_vfio() call from PCI hotplug
> > path as the new callbacks do this better - they notify IOMMU at
> > the exact moment when the configuration is changed, and this also
> > includes the case of PCI hot unplug.
> >
> > As there can be multiple containers attached to the same PHB/LIOBN,
> > this replaces the @need_vfio flag in sPAPRTCETable with the counter
> > of VFIO users.
> >
> > Signed-off-by: Alexey Kardashevskiy <address@hidden>
>
> This looks correct, but there's one remaining ugly.
>
> > ---
> > Changes:
> > v15:
> > * s/need_vfio/vfio-Users/g
> > ---
> >   hw/ppc/spapr_iommu.c   | 30 ++++++++++++++++++++----------
> >   hw/ppc/spapr_pci.c     |  6 ------
> >   hw/vfio/common.c       |  9 +++++++++
> >   include/exec/memory.h  |  4 ++++
> >   include/hw/ppc/spapr.h |  2 +-
> >   5 files changed, 34 insertions(+), 17 deletions(-)
> >
> > diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> > index c945dba..ea09414 100644
> > --- a/hw/ppc/spapr_iommu.c
> > +++ b/hw/ppc/spapr_iommu.c
> > @@ -155,6 +155,16 @@ static uint64_t spapr_tce_get_page_sizes(MemoryRegion 
> > *iommu)
> >       return 1ULL << tcet->page_shift;
> >   }
> >
> > +static void spapr_tce_vfio_start(MemoryRegion *iommu)
> > +{
> > +    spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu), true);
> > +}
> > +
> > +static void spapr_tce_vfio_stop(MemoryRegion *iommu)
> > +{
> > +    spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu), false);
> > +}
> > +
> >   static void spapr_tce_table_do_enable(sPAPRTCETable *tcet);
> >   static void spapr_tce_table_do_disable(sPAPRTCETable *tcet);
> >
> > @@ -239,6 +249,8 @@ static const VMStateDescription vmstate_spapr_tce_table = {
> >   static MemoryRegionIOMMUOps spapr_iommu_ops = {
> >       .translate = spapr_tce_translate_iommu,
> >       .get_page_sizes = spapr_tce_get_page_sizes,
> > +    .vfio_start = spapr_tce_vfio_start,
> > +    .vfio_stop = spapr_tce_vfio_stop,
>
> Ok, so AFAICT these callbacks are called whenever a VFIO context is
> added / removed from the gIOMMU's address space, and it's up to the
> gIOMMU code to ref count that to see if there are any current vfio
> users.  That makes "vfio_start" and "vfio_stop" not great names.
>
> But.. better than changing the names would be to move the refcounting
> to the generic code if you can manage it, so the individual gIOMMU
> backends don't need to - they just told when they need to start / stop
> providing VFIO support.

Everything is manageable...

This referencing is needed for the case of >=2 containers so 
2xvfio_listener_region_add will create 2xVFIOGuestIOMMU as they are per 
VFIOContainer so VFIOGuestIOMMU is not the right place for the reference 
counting, VFIOAddressSpace seems to be that place (=> add list of IOMMU MRs 
with refcounter). Or even IOMMU MR. Or move VFIOGuestIOMMU list from 
VFIOContainer to VFIOAddressSpace and then gIOMMU can handle refcounting?


> >   };
> >
> >   static int spapr_tce_table_realize(DeviceState *dev)
> > @@ -248,7 +260,7 @@ static int spapr_tce_table_realize(DeviceState *dev)
> >       char tmp[32];
> >
> >       tcet->fd = -1;
> > -    tcet->need_vfio = false;
> > +    tcet->vfio_users = 0;
> >       snprintf(tmp, sizeof(tmp), "tce-root-%x", tcet->liobn);
> >       memory_region_init(&tcet->root, tcetobj, tmp, UINT64_MAX);
> >
> > @@ -268,20 +280,18 @@ void spapr_tce_set_need_vfio(sPAPRTCETable *tcet, bool 
> > need_vfio)
> >       size_t table_size = tcet->nb_table * sizeof(uint64_t);
> >       void *newtable;
> >
> > -    if (need_vfio == tcet->need_vfio) {
> > -        /* Nothing to do */
> > -        return;
> > -    }
> > +    tcet->vfio_users += need_vfio ? 1 : -1;
> > +    g_assert(tcet->vfio_users >= 0);
> > +    g_assert(tcet->table);
> >
> > -    if (!need_vfio) {
> > +    if (!tcet->vfio_users) {
> >           /* FIXME: We don't support transition back to KVM accelerated
> >            * TCEs yet */
> >           return;
> >       }
> >
> > -    tcet->need_vfio = true;
> > -
> > -    if (tcet->fd < 0) {
> > +    if (tcet->vfio_users > 1) {
> > +        g_assert(tcet->fd < 0);
> >           /* Table is already in userspace, nothing to be do */
> >           return;
> >       }
> > @@ -327,7 +337,7 @@ static void spapr_tce_table_do_enable(sPAPRTCETable *tcet)
> >                                           tcet->page_shift,
> >                                           tcet->nb_table,
> >                                           &tcet->fd,
> > -                                        tcet->need_vfio);
> > +                                        tcet->vfio_users != 0);
> >
> >       memory_region_set_size(&tcet->iommu,
> >                              (uint64_t)tcet->nb_table << tcet->page_shift);
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 5497a18..f864fde 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1083,12 +1083,6 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector 
> > *drc,
> >       void *fdt = NULL;
> >       int fdt_start_offset = 0, fdt_size;
> >
> > -    if (object_dynamic_cast(OBJECT(pdev), "vfio-pci")) {
> > -        sPAPRTCETable *tcet = spapr_tce_find_by_liobn(phb->dma_liobn);
> > -
> > -        spapr_tce_set_need_vfio(tcet, true);
> > -    }
> > -
> >       if (dev->hotplugged) {
> >           fdt = create_device_tree(&fdt_size);
> >           fdt_start_offset = spapr_create_pci_child_dt(phb, pdev, fdt, 0);
> > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > index ea79311..5e5b77c 100644
> > --- a/hw/vfio/common.c
> > +++ b/hw/vfio/common.c
> > @@ -421,6 +421,9 @@ static void vfio_listener_region_add(MemoryListener 
> > *listener,
> >           QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next);
> >
> >           memory_region_register_iommu_notifier(giommu->iommu, &giommu->n);
> > +        if (section->mr->iommu_ops && section->mr->iommu_ops->vfio_start) {
> > +            section->mr->iommu_ops->vfio_start(section->mr);
> > +        }
> >           memory_region_iommu_replay(giommu->iommu, &giommu->n,
> >                                      false);
> >
> > @@ -466,6 +469,7 @@ static void vfio_listener_region_del(MemoryListener 
> > *listener,
> >       VFIOContainer *container = container_of(listener, VFIOContainer, 
> > listener);
> >       hwaddr iova, end;
> >       int ret;
> > +    MemoryRegion *iommu = NULL;
> >
> >       if (vfio_listener_skipped_section(section)) {
> >           trace_vfio_listener_region_del_skip(
> > @@ -487,6 +491,7 @@ static void vfio_listener_region_del(MemoryListener 
> > *listener,
> >           QLIST_FOREACH(giommu, &container->giommu_list, giommu_next) {
> >               if (giommu->iommu == section->mr) {
> >                   memory_region_unregister_iommu_notifier(&giommu->n);
> > +                iommu = giommu->iommu;
> >                   QLIST_REMOVE(giommu, giommu_next);
> >                   g_free(giommu);
> >                   break;
> > @@ -519,6 +524,10 @@ static void vfio_listener_region_del(MemoryListener 
> > *listener,
> >                        "0x%"HWADDR_PRIx") = %d (%m)",
> >                        container, iova, end - iova, ret);
> >       }
> > +
> > +    if (iommu && iommu->iommu_ops && iommu->iommu_ops->vfio_stop) {
> > +        iommu->iommu_ops->vfio_stop(section->mr);
> > +    }
> >   }
> >
> >   static const MemoryListener vfio_memory_listener = {
> > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > index eb5ce67..f1de133f 100644
> > --- a/include/exec/memory.h
> > +++ b/include/exec/memory.h
> > @@ -152,6 +152,10 @@ struct MemoryRegionIOMMUOps {
> >       IOMMUTLBEntry (*translate)(MemoryRegion *iommu, hwaddr addr, bool 
> > is_write);
> >       /* Returns supported page sizes */
> >       uint64_t (*get_page_sizes)(MemoryRegion *iommu);
> > +    /* Called when VFIO starts using this */
> > +    void (*vfio_start)(MemoryRegion *iommu);
> > +    /* Called when VFIO stops using this */
> > +    void (*vfio_stop)(MemoryRegion *iommu);
> >   };
> >
> >   typedef struct CoalescedMemoryRange CoalescedMemoryRange;
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index 471eb4a..5c00e38 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -548,7 +548,7 @@ struct sPAPRTCETable {
> >       uint32_t mig_nb_table;
> >       uint64_t *mig_table;
> >       bool bypass;
> > -    bool need_vfio;
> > +    int vfio_users;
> >       int fd;
> >       MemoryRegion root, iommu;
> >       struct VIOsPAPRDevice *vdev; /* for @bypass migration compatibility only 
> > */


--
Alexey