Re: [Qemu-ppc] [PATCH qemu 1/2] spapr_pci_vfio: Remove redundant spapr-pci-vfio-host-bridge

[David Gibson]

On Wed, Sep 02, 2015 at 07:44:07PM +1000, Alexey Kardashevskiy wrote:
> On 09/02/2015 06:16 PM, Alexey Kardashevskiy wrote:
> >sPAPRTCETable is handling 2 TCE tables already:
> >
> >1) guest view of the TCE table - emulated devices use only this table;
> >
> >2) hardware IOMMU table - VFIO PCI devices use it for actual work but
> >it does not replace 1) and it is not visible to the guest.
> >The initialization of this table is driven by vfio-pci device,
> >DMA map/unmap requests are handled via MemoryListener so there is very
> >little to do in spapr-pci-vfio-host-bridge.
> >
> >This moves VFIO bits to the generic spapr-pci-host-bridge which allows
> >putting emulated and VFIO devices on the same PHB. It is still possible
> >to create multiple PHBs and avoid sharing PHB resouces for emulated and
> >VFIO devices.
> >
> >If there is no VFIO-PCI device attaches, no special ioctls will be called.
> >If there are some VFIO-PCI devices attached, PHB may refuse to attach
> >another VFIO-PCI device if a VFIO container on the host kernel side
> >does not support container sharing.
> >
> >This makes spapr-pci-vfio-host-bridge type equal to spapr-pci-host-bridge
> >except it has an additional "iommu" property so spapr-pci-vfio-host-bridge
> >still should be used for VFIO devices. The next patch will remove IOMMU ID
> >property and allow putting VFIO-PCI devices onto spapr-pci-host-bridge.
> >
> >This adds a number of VFIO-PCI devices currently attached to a PHB as
> >PHB needs to know whether to do DMA setup for VFIO or not. Since
> >at the moment of the PHB's realize() invocation we cannot tell yet
> >how many VFIO-PCI devices are there (they are not attached yet),
> >this moves DMA setup to the reset handler.
> >
> >This moves PCI device lookup from spapr_phb_vfio_eeh_set_option() to
> >rtas_ibm_set_eeh_option() as we need to know if the device is "vfio-pci"
> >and decide whether to call spapr_phb_vfio_eeh_set_option() or not.
> >
> >This should cause no behavioural change.

[snip]

> >  static void spapr_phb_reset(DeviceState *qdev)
> >  {
> >+    sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(qdev);
> >+    sPAPRTCETable *tcet;
> >+
> >      /* Reset the IOMMU state */
> >      object_child_foreach(OBJECT(qdev), spapr_phb_children_reset, NULL);
> >+
> >+    if (spapr_phb_dma_capabilities_update(sphb)) {
> >+        return;
> >+    }
> >+
> >+    /* Register default 32bit DMA window */
> >+    tcet = spapr_tce_find_by_liobn(sphb->dma_liobn);
> >+    if (!tcet) {
> >+        const unsigned nb = sphb->dma32_window_size >> SPAPR_TCE_PAGE_SHIFT;
> >+        tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn,
> >+                                   sphb->dma32_window_start,
> >+                                   SPAPR_TCE_PAGE_SHIFT, nb,
> >+                                   sphb->vfio_num > 0);
> >+        if (!tcet) {
> >+            error_report("No default TCE table for %s", sphb->dtbusname);
> >+            return;
> >+        }
> >+
> >+        memory_region_add_subregion(&sphb->iommu_root, 0,
> 
> 
> Of course bug... should be bus_offset instead of 0. I'll wait a bit for more
> reviews and respin later.

Are you sure?  I thought the idea of the iommu_root MR was that it was
always at offset 0, and the "real" IOMMU region sat inside it at the
right offset, allowing things to be moved around in the case of DDW.

It was at offset 0 in the original realize() time
memory_region_add_subregion().

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

pgppcbS9ahAwd.pgp
Description: PGP signature