Re: [Qemu-devel] [PATCH qemu v14 16/18] spapr_iommu, vfio, memory: Notify IOMMU about starting/stopping being used by VFIO

[Alexey Kardashevskiy]

On 03/22/2016 03:45 PM, David Gibson wrote:
> On Mon, Mar 21, 2016 at 06:47:04PM +1100, 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.
> >
> > TODO: split into 2 or 3 patches, per maintainership area.
> >
> > Signed-off-by: Alexey Kardashevskiy <address@hidden>
>
> I'm finding this one much easier to follow than the previous revision.
>
> > ---
> >   hw/ppc/spapr_iommu.c  | 12 ++++++++++++
> >   hw/ppc/spapr_pci.c    |  6 ------
> >   hw/vfio/common.c      |  9 +++++++++
> >   include/exec/memory.h |  4 ++++
> >   4 files changed, 25 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> > index 6dc3c45..702075d 100644
> > --- a/hw/ppc/spapr_iommu.c
> > +++ b/hw/ppc/spapr_iommu.c
> > @@ -151,6 +151,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);
> > +}
>
> Wonder if a single callback which takes a boolean might be a little
> less clunky.

I have a feeling that at least once I was asked to do the opposite and now 
we have take_ownership/release_ownership. This does not seem to be much 
different and the existing names are more self-documenting than the 
previous vfio_notify() or whatever name I could think of.


> >   static void spapr_tce_table_do_enable(sPAPRTCETable *tcet);
> >   static void spapr_tce_table_do_disable(sPAPRTCETable *tcet);
> >
> > @@ -211,6 +221,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,
> >   };
> >
> >   static int spapr_tce_table_realize(DeviceState *dev)
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index bfcafdf..af99a36 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1121,12 +1121,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 b257655..4e873b7 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);
> > +    }
>
> IIRC there can be multiple containers listening on the same PCI
> address space.  In that case, this won't be correct, because once one
> of the VFIO containers is removed, it will call vfio_stop, even though
> the other VFIO container still needs the guest IOMMU to support it.
>
> So I think you need some sort of refcounting here.


Right, missed this bit, good finding.


> >   }
> >
> >   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;


--
Alexey