diff mbox

[10/12] spapr_pci: (Mostly) remove spapr-pci-vfio-host-bridge

Message ID 1456486323-8047-11-git-send-email-david@gibson.dropbear.id.au
State New, archived
Headers show

Commit Message

David Gibson Feb. 26, 2016, 11:32 a.m. UTC
Now that the regular spapr-pci-host-bridge can handle EEH, there are only
two things that spapr-pci-vfio-host-bridge does differently:
    1. automatically sizes its DMA window to match the host IOMMU
    2. checks if the attached VFIO container is backed by the
       VFIO_SPAPR_TCE_IOMMU type on the host

(1) is not particularly useful, since the default window used by the
regular host bridge will work with the host IOMMU configuration on all
current systems anyway.

Plus, automatically changing guest visible configuration (such as the DMA
window) based on host settings is generally a bad idea.  It's not
definitively broken, since spapr-pci-vfio-host-bridge is only supposed to
support VFIO devices which can't be migrated anyway, but still.

(2) is not really useful, because if a guest tries to configure EEH on a
different host IOMMU, the first call will fail and that will be that.

It's possible there are scripts or tools out there which expect
spapr-pci-vfio-host-bridge, so we don't remove it entirely.  This patch
reduces it to just a stub for backwards compatibility.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci_vfio.c     | 64 ++++++++++++---------------------------------
 include/hw/pci-host/spapr.h | 11 --------
 2 files changed, 17 insertions(+), 58 deletions(-)

Comments

Alexey Kardashevskiy Feb. 29, 2016, 1:42 a.m. UTC | #1
On 02/26/2016 10:32 PM, David Gibson wrote:
> Now that the regular spapr-pci-host-bridge can handle EEH, there are only
> two things that spapr-pci-vfio-host-bridge does differently:
>      1. automatically sizes its DMA window to match the host IOMMU
>      2. checks if the attached VFIO container is backed by the
>         VFIO_SPAPR_TCE_IOMMU type on the host
>
> (1) is not particularly useful, since the default window used by the
> regular host bridge will work with the host IOMMU configuration on all
> current systems anyway.
>
> Plus, automatically changing guest visible configuration (such as the DMA
> window) based on host settings is generally a bad idea.

btw why exactly is it a bad idea?


Anyway,
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
David Gibson Feb. 29, 2016, 3:06 a.m. UTC | #2
On Mon, Feb 29, 2016 at 12:42:28PM +1100, Alexey Kardashevskiy wrote:
> On 02/26/2016 10:32 PM, David Gibson wrote:
> >Now that the regular spapr-pci-host-bridge can handle EEH, there are only
> >two things that spapr-pci-vfio-host-bridge does differently:
> >     1. automatically sizes its DMA window to match the host IOMMU
> >     2. checks if the attached VFIO container is backed by the
> >        VFIO_SPAPR_TCE_IOMMU type on the host
> >
> >(1) is not particularly useful, since the default window used by the
> >regular host bridge will work with the host IOMMU configuration on all
> >current systems anyway.
> >
> >Plus, automatically changing guest visible configuration (such as the DMA
> >window) based on host settings is generally a bad idea.
> 
> btw why exactly is it a bad idea?

So, as noted, in this case it's probably ok.  But in general changing
configuration based on host state will break migration, because the
source and destination VMs, with apparently identical setup won't
actually be the same.

It also makes it more difficult for management layers to know exactly
what they're constructing, which is potentially an issue even for this
non-migratable device.
> 
> 
> Anyway,
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> 
>
diff mbox

Patch

diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
index ecbcc5c..41d9e5a 100644
--- a/hw/ppc/spapr_pci_vfio.c
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -20,74 +20,44 @@ 
 #include "qemu/osdep.h"
 #include "hw/ppc/spapr.h"
 #include "hw/pci-host/spapr.h"
-#include "hw/pci/msix.h"
-#include "linux/vfio.h"
-#include "hw/vfio/vfio.h"
+#include "qemu/error-report.h"
 
-static Property spapr_phb_vfio_properties[] = {
-    DEFINE_PROP_INT32("iommu", sPAPRPHBVFIOState, iommugroupid, -1),
-    DEFINE_PROP_END_OF_LIST(),
-};
+#define TYPE_SPAPR_PCI_VFIO_HOST_BRIDGE "spapr-pci-vfio-host-bridge"
 
-static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp)
-{
-    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
-    struct vfio_iommu_spapr_tce_info info = { .argsz = sizeof(info) };
-    int ret;
-    sPAPRTCETable *tcet;
-    uint32_t liobn = svphb->phb.dma_liobn;
+#define SPAPR_PCI_VFIO_HOST_BRIDGE(obj) \
+    OBJECT_CHECK(sPAPRPHBVFIOState, (obj), TYPE_SPAPR_PCI_VFIO_HOST_BRIDGE)
 
-    if (svphb->iommugroupid == -1) {
-        error_setg(errp, "Wrong IOMMU group ID %d", svphb->iommugroupid);
-        return;
-    }
+typedef struct sPAPRPHBVFIOState sPAPRPHBVFIOState;
 
-    ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
-                               VFIO_CHECK_EXTENSION,
-                               (void *) VFIO_SPAPR_TCE_IOMMU);
-    if (ret != 1) {
-        error_setg_errno(errp, -ret,
-                         "spapr-vfio: SPAPR extension is not supported");
-        return;
-    }
+struct sPAPRPHBVFIOState {
+    sPAPRPHBState phb;
 
-    ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
-                               VFIO_IOMMU_SPAPR_TCE_GET_INFO, &info);
-    if (ret) {
-        error_setg_errno(errp, -ret,
-                         "spapr-vfio: get info from container failed");
-        return;
-    }
+    int32_t iommugroupid;
+};
 
-    tcet = spapr_tce_new_table(DEVICE(sphb), liobn, info.dma32_window_start,
-                               SPAPR_TCE_PAGE_SHIFT,
-                               info.dma32_window_size >> SPAPR_TCE_PAGE_SHIFT,
-                               true);
-    if (!tcet) {
-        error_setg(errp, "spapr-vfio: failed to create VFIO TCE table");
-        return;
-    }
+static Property spapr_phb_vfio_properties[] = {
+    DEFINE_PROP_INT32("iommu", sPAPRPHBVFIOState, iommugroupid, -1),
+    DEFINE_PROP_END_OF_LIST(),
+};
 
-    /* Register default 32bit DMA window */
-    memory_region_add_subregion(&sphb->iommu_root, tcet->bus_offset,
-                                spapr_tce_get_iommu(tcet));
+static void spapr_phb_vfio_instance_init(Object *obj)
+{
+    error_report("spapr-pci-vfio-host-bridge is deprecated");
 }
 
 static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
-    sPAPRPHBClass *spc = SPAPR_PCI_HOST_BRIDGE_CLASS(klass);
 
     dc->props = spapr_phb_vfio_properties;
-    spc->finish_realize = spapr_phb_vfio_finish_realize;
 }
 
 static const TypeInfo spapr_phb_vfio_info = {
     .name          = TYPE_SPAPR_PCI_VFIO_HOST_BRIDGE,
     .parent        = TYPE_SPAPR_PCI_HOST_BRIDGE,
     .instance_size = sizeof(sPAPRPHBVFIOState),
+    .instance_init = spapr_phb_vfio_instance_init,
     .class_init    = spapr_phb_vfio_class_init,
-    .class_size    = sizeof(sPAPRPHBClass),
 };
 
 static void spapr_pci_vfio_register_types(void)
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index 9313209..7110222 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -28,14 +28,10 @@ 
 #include "hw/ppc/xics.h"
 
 #define TYPE_SPAPR_PCI_HOST_BRIDGE "spapr-pci-host-bridge"
-#define TYPE_SPAPR_PCI_VFIO_HOST_BRIDGE "spapr-pci-vfio-host-bridge"
 
 #define SPAPR_PCI_HOST_BRIDGE(obj) \
     OBJECT_CHECK(sPAPRPHBState, (obj), TYPE_SPAPR_PCI_HOST_BRIDGE)
 
-#define SPAPR_PCI_VFIO_HOST_BRIDGE(obj) \
-    OBJECT_CHECK(sPAPRPHBVFIOState, (obj), TYPE_SPAPR_PCI_VFIO_HOST_BRIDGE)
-
 #define SPAPR_PCI_HOST_BRIDGE_CLASS(klass) \
      OBJECT_CLASS_CHECK(sPAPRPHBClass, (klass), TYPE_SPAPR_PCI_HOST_BRIDGE)
 #define SPAPR_PCI_HOST_BRIDGE_GET_CLASS(obj) \
@@ -43,7 +39,6 @@ 
 
 typedef struct sPAPRPHBClass sPAPRPHBClass;
 typedef struct sPAPRPHBState sPAPRPHBState;
-typedef struct sPAPRPHBVFIOState sPAPRPHBVFIOState;
 
 struct sPAPRPHBClass {
     PCIHostBridgeClass parent_class;
@@ -90,12 +85,6 @@  struct sPAPRPHBState {
     QLIST_ENTRY(sPAPRPHBState) list;
 };
 
-struct sPAPRPHBVFIOState {
-    sPAPRPHBState phb;
-
-    int32_t iommugroupid;
-};
-
 #define SPAPR_PCI_MAX_INDEX          255
 
 #define SPAPR_PCI_BASE_BUID          0x800000020000000ULL