[Qemu-devel] [PATCH 0/4] Allow VFIO devices on spapr-pci-host-bridge

[Qemu-devel] [PATCH 0/4] Allow VFIO devices on spapr-pci-host-bridge

[David Gibson]

At present, VFIO PCI devices attached to an spapr-pci-host-bridge
device (the regular PCI host bridge for the pseries machine type)
won't quite work properly.

I've recently posted some extensions to VFIO that are almost enough to
allow these to work - this series contains the final necessary changes
to the host bridge implementation itself to let that work.

NOTE: This must be applied on top of my recent series of patches to
      the VFIO core code.  I'm sending this out now, in the hopes that
      it can be reviewed and ready to apply as soon as the VFIO
      changes are merged.

David Gibson (4):
  spapr_pci: Allow PCI host bridge DMA window to be configured
  spapr_iommu: Rename vfio_accel parameter
  spapr_iommu: Provide a function to switch a TCE table to allowing VFIO
  spapr_pci: Allow VFIO devices to work on the normal PCI host bridge

 hw/ppc/spapr_iommu.c        | 38 +++++++++++++++++++++++++++++++++++---
 hw/ppc/spapr_pci.c          | 13 +++++++++++--
 include/hw/pci-host/spapr.h |  3 +--
 include/hw/ppc/spapr.h      |  6 ++++--
 target-ppc/kvm.c            |  4 ++--
 target-ppc/kvm_ppc.h        |  2 +-
 6 files changed, 54 insertions(+), 12 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH 1/4] spapr_pci: Allow PCI host bridge DMA window to be configured

[David Gibson]

At present the PCI host bridge (PHB) for the pseries machine type has a
fixed DMA window from 0..1GB (in PCI address space) which is mapped to real
memory via the PAPR paravirtualized IOMMU.

For better support of VFIO devices, we're going to want to allow for
different configurations of the DMA window.

Eventually we'll want to allow the guest itself to reconfigure the window
via the PAPR dynamic DMA window interface, but as a preliminary this patch
allows the user to reconfigure the window with new properties on the PHB
device.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/ppc/spapr_pci.c          | 7 +++++--
 include/hw/pci-host/spapr.h | 3 +--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 617b7f3..cb7c351 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1387,7 +1387,7 @@ static void spapr_phb_finish_realize(sPAPRPHBState *sphb, Error **errp)
     sPAPRTCETable *tcet;
     uint32_t nb_table;
 
-    nb_table = SPAPR_PCI_DMA32_SIZE >> SPAPR_TCE_PAGE_SHIFT;
+    nb_table = sphb->dma_win_size >> SPAPR_TCE_PAGE_SHIFT;
     tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn,
                                0, SPAPR_TCE_PAGE_SHIFT, nb_table, false);
     if (!tcet) {
@@ -1397,7 +1397,7 @@ static void spapr_phb_finish_realize(sPAPRPHBState *sphb, Error **errp)
     }
 
     /* Register default 32bit DMA window */
-    memory_region_add_subregion(&sphb->iommu_root, 0,
+    memory_region_add_subregion(&sphb->iommu_root, sphb->dma_win_addr,
                                 spapr_tce_get_iommu(tcet));
 }
 
@@ -1430,6 +1430,9 @@ static Property spapr_phb_properties[] = {
                        SPAPR_PCI_IO_WIN_SIZE),
     DEFINE_PROP_BOOL("dynamic-reconfiguration", sPAPRPHBState, dr_enabled,
                      true),
+    /* Default DMA window is 0..1GB */
+    DEFINE_PROP_UINT64("dma_win_addr", sPAPRPHBState, dma_win_addr, 0),
+    DEFINE_PROP_UINT64("dma_win_size", sPAPRPHBState, dma_win_size, 0x40000000),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index 5322b56..7de5e02 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -78,6 +78,7 @@ struct sPAPRPHBState {
     MemoryRegion memwindow, iowindow, msiwindow;
 
     uint32_t dma_liobn;
+    hwaddr dma_win_addr, dma_win_size;
     AddressSpace iommu_as;
     MemoryRegion iommu_root;
 
@@ -115,8 +116,6 @@ struct sPAPRPHBVFIOState {
 
 #define SPAPR_PCI_MSI_WINDOW         0x40000000000ULL
 
-#define SPAPR_PCI_DMA32_SIZE         0x40000000
-
 static inline qemu_irq spapr_phb_lsi_qirq(struct sPAPRPHBState *phb, int pin)
 {
     sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
-- 
2.4.3

[Qemu-devel] [PATCH 2/4] spapr_iommu: Rename vfio_accel parameter

[David Gibson]

The vfio_accel parameter used when creating a new TCE table (guest IOMMU
context) has a confusing name.  What it really means is whether we need the
TCE table created to be able to support VFIO devices.

VFIO is relevant, because when available we use in-kernel acceleration of
the TCE table, but that may not work with VFIO devices because updates to
the table are handled in kernel, bypass qemu and so don't hit qemu's
infrastructure for keeping the VFIO host IOMMU state in sync with the guest
IOMMU state.

Rename the parameter to "need_vfio" throughout.  This is a cosmetic change,
with no impact on the logic.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_iommu.c   | 6 +++---
 include/hw/ppc/spapr.h | 4 ++--
 target-ppc/kvm.c       | 4 ++--
 target-ppc/kvm_ppc.h   | 2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
index f61504e..5166cde 100644
--- a/hw/ppc/spapr_iommu.c
+++ b/hw/ppc/spapr_iommu.c
@@ -146,7 +146,7 @@ static int spapr_tce_table_realize(DeviceState *dev)
         tcet->table = kvmppc_create_spapr_tce(tcet->liobn,
                                               window_size,
                                               &tcet->fd,
-                                              tcet->vfio_accel);
+                                              tcet->need_vfio);
     }
 
     if (!tcet->table) {
@@ -172,7 +172,7 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
                                    uint64_t bus_offset,
                                    uint32_t page_shift,
                                    uint32_t nb_table,
-                                   bool vfio_accel)
+                                   bool need_vfio)
 {
     sPAPRTCETable *tcet;
     char tmp[64];
@@ -192,7 +192,7 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
     tcet->bus_offset = bus_offset;
     tcet->page_shift = page_shift;
     tcet->nb_table = nb_table;
-    tcet->vfio_accel = vfio_accel;
+    tcet->need_vfio = need_vfio;
 
     snprintf(tmp, sizeof(tmp), "tce-table-%x", liobn);
     object_property_add_child(OBJECT(owner), tmp, OBJECT(tcet), NULL);
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 56c5b0b..27d65d5 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -563,7 +563,7 @@ struct sPAPRTCETable {
     uint32_t page_shift;
     uint64_t *table;
     bool bypass;
-    bool vfio_accel;
+    bool need_vfio;
     int fd;
     MemoryRegion iommu;
     struct VIOsPAPRDevice *vdev; /* for @bypass migration compatibility only */
@@ -588,7 +588,7 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
                                    uint64_t bus_offset,
                                    uint32_t page_shift,
                                    uint32_t nb_table,
-                                   bool vfio_accel);
+                                   bool need_vfio);
 MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet);
 int spapr_dma_dt(void *fdt, int node_off, const char *propname,
                  uint32_t liobn, uint64_t window, uint32_t size);
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index e641680..04ce614 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -2071,7 +2071,7 @@ bool kvmppc_spapr_use_multitce(void)
 }
 
 void *kvmppc_create_spapr_tce(uint32_t liobn, uint32_t window_size, int *pfd,
-                              bool vfio_accel)
+                              bool need_vfio)
 {
     struct kvm_create_spapr_tce args = {
         .liobn = liobn,
@@ -2085,7 +2085,7 @@ void *kvmppc_create_spapr_tce(uint32_t liobn, uint32_t window_size, int *pfd,
      * destroying the table, which the upper layers -will- do
      */
     *pfd = -1;
-    if (!cap_spapr_tce || (vfio_accel && !cap_spapr_vfio)) {
+    if (!cap_spapr_tce || (need_vfio && !cap_spapr_vfio)) {
         return NULL;
     }
 
diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
index 470f6d6..309cbe0 100644
--- a/target-ppc/kvm_ppc.h
+++ b/target-ppc/kvm_ppc.h
@@ -36,7 +36,7 @@ int kvmppc_booke_watchdog_enable(PowerPCCPU *cpu);
 off_t kvmppc_alloc_rma(void **rma);
 bool kvmppc_spapr_use_multitce(void);
 void *kvmppc_create_spapr_tce(uint32_t liobn, uint32_t window_size, int *pfd,
-                              bool vfio_accel);
+                              bool need_vfio);
 int kvmppc_remove_spapr_tce(void *table, int pfd, uint32_t window_size);
 int kvmppc_reset_htab(int shift_hint);
 uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift);
-- 
2.4.3

[Qemu-devel] [PATCH 3/4] spapr_iommu: Provide a function to switch a TCE table to allowing VFIO

[David Gibson]

Because of the way non-VFIO guest IOMMU operations are KVM accelerated, not
all TCE tables (guest IOMMU contexts) can support VFIO devices.  Currently,
this is decided at creation time.

To support hotplug of VFIO devices, we need to allow a TCE table which
previously didn't allow VFIO devices to be switched so that it can.  This
patch adds an spapr_tce_set_need_vfio() function to do this, by
reallocating the table in userspace if necessary.

Currently this doesn't allow the KVM acceleration to be re-enabled if all
the VFIO devices are removed.  That's an optimization for another time.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_iommu.c   | 32 ++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h |  2 ++
 2 files changed, 34 insertions(+)

diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
index 5166cde..8d60f8b 100644
--- a/hw/ppc/spapr_iommu.c
+++ b/hw/ppc/spapr_iommu.c
@@ -168,6 +168,38 @@ static int spapr_tce_table_realize(DeviceState *dev)
     return 0;
 }
 
+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;
+    }
+
+    if (!need_vfio) {
+        /* FIXME: We don't support transition back to KVM accelerated
+         * TCEs yet */
+        return;
+    }
+
+    tcet->need_vfio = true;
+
+    if (tcet->fd < 0) {
+        /* Table is already in userspace, nothing to be do */
+        return;
+    }
+
+    newtable = g_malloc0(table_size);
+    memcpy(newtable, tcet->table, table_size);
+
+    kvmppc_remove_spapr_tce(tcet->table, tcet->fd, tcet->nb_table);
+
+    tcet->fd = -1;
+    tcet->table = newtable;
+}
+
 sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
                                    uint64_t bus_offset,
                                    uint32_t page_shift,
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 27d65d5..5baa906 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -589,6 +589,8 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
                                    uint32_t page_shift,
                                    uint32_t nb_table,
                                    bool need_vfio);
+void spapr_tce_set_need_vfio(sPAPRTCETable *tcet, bool need_vfio);
+
 MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet);
 int spapr_dma_dt(void *fdt, int node_off, const char *propname,
                  uint32_t liobn, uint64_t window, uint32_t size);
-- 
2.4.3

[Qemu-devel] [PATCH 4/4] spapr_pci: Allow VFIO devices to work on the normal PCI host bridge

[David Gibson]

The core VFIO infrastructure more or less allows VFIO devices to work
on any normal guest PCI host bridge (PHB) without extra logic.
However, the "spapr-pci-host-bridge" device (as opposed to the special
"spapr-pci-vfio-host-bridge" device) breaks this by using a partially
KVM accelerated implementation of the guest kernel IOMMU which won't
work with VFIO devices, without additional kernel support.

This patch allows VFIO devices to work on the spapr-pci-host-bridge,
by having it switch off KVM TCE acceleration when a VFIO device is
added to the PHB (either on startup, or by hotplug).

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index cb7c351..55fa8db 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1083,6 +1083,12 @@ 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);
-- 
2.4.3

Re: [Qemu-devel] [PATCH 2/4] spapr_iommu: Rename vfio_accel parameter

[Laurent Vivier]

On 30/09/2015 05:48, David Gibson wrote:
> The vfio_accel parameter used when creating a new TCE table (guest IOMMU
> context) has a confusing name.  What it really means is whether we need the
> TCE table created to be able to support VFIO devices.
> 
> VFIO is relevant, because when available we use in-kernel acceleration of
> the TCE table, but that may not work with VFIO devices because updates to
> the table are handled in kernel, bypass qemu and so don't hit qemu's
> infrastructure for keeping the VFIO host IOMMU state in sync with the guest
> IOMMU state.
> 
> Rename the parameter to "need_vfio" throughout.  This is a cosmetic change,
> with no impact on the logic.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_iommu.c   | 6 +++---
>  include/hw/ppc/spapr.h | 4 ++--
>  target-ppc/kvm.c       | 4 ++--
>  target-ppc/kvm_ppc.h   | 2 +-
>  4 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> index f61504e..5166cde 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -146,7 +146,7 @@ static int spapr_tce_table_realize(DeviceState *dev)
>          tcet->table = kvmppc_create_spapr_tce(tcet->liobn,
>                                                window_size,
>                                                &tcet->fd,
> -                                              tcet->vfio_accel);
> +                                              tcet->need_vfio);
>      }
>  
>      if (!tcet->table) {
> @@ -172,7 +172,7 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
>                                     uint64_t bus_offset,
>                                     uint32_t page_shift,
>                                     uint32_t nb_table,
> -                                   bool vfio_accel)
> +                                   bool need_vfio)
>  {
>      sPAPRTCETable *tcet;
>      char tmp[64];
> @@ -192,7 +192,7 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
>      tcet->bus_offset = bus_offset;
>      tcet->page_shift = page_shift;
>      tcet->nb_table = nb_table;
> -    tcet->vfio_accel = vfio_accel;
> +    tcet->need_vfio = need_vfio;
>  
>      snprintf(tmp, sizeof(tmp), "tce-table-%x", liobn);
>      object_property_add_child(OBJECT(owner), tmp, OBJECT(tcet), NULL);
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 56c5b0b..27d65d5 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -563,7 +563,7 @@ struct sPAPRTCETable {
>      uint32_t page_shift;
>      uint64_t *table;
>      bool bypass;
> -    bool vfio_accel;
> +    bool need_vfio;
>      int fd;
>      MemoryRegion iommu;
>      struct VIOsPAPRDevice *vdev; /* for @bypass migration compatibility only */
> @@ -588,7 +588,7 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
>                                     uint64_t bus_offset,
>                                     uint32_t page_shift,
>                                     uint32_t nb_table,
> -                                   bool vfio_accel);
> +                                   bool need_vfio);
>  MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet);
>  int spapr_dma_dt(void *fdt, int node_off, const char *propname,
>                   uint32_t liobn, uint64_t window, uint32_t size);
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index e641680..04ce614 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -2071,7 +2071,7 @@ bool kvmppc_spapr_use_multitce(void)
>  }
>  
>  void *kvmppc_create_spapr_tce(uint32_t liobn, uint32_t window_size, int *pfd,
> -                              bool vfio_accel)
> +                              bool need_vfio)
>  {
>      struct kvm_create_spapr_tce args = {
>          .liobn = liobn,
> @@ -2085,7 +2085,7 @@ void *kvmppc_create_spapr_tce(uint32_t liobn, uint32_t window_size, int *pfd,
>       * destroying the table, which the upper layers -will- do
>       */
>      *pfd = -1;
> -    if (!cap_spapr_tce || (vfio_accel && !cap_spapr_vfio)) {
> +    if (!cap_spapr_tce || (need_vfio && !cap_spapr_vfio)) {
>          return NULL;
>      }
>  
> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
> index 470f6d6..309cbe0 100644
> --- a/target-ppc/kvm_ppc.h
> +++ b/target-ppc/kvm_ppc.h
> @@ -36,7 +36,7 @@ int kvmppc_booke_watchdog_enable(PowerPCCPU *cpu);
>  off_t kvmppc_alloc_rma(void **rma);
>  bool kvmppc_spapr_use_multitce(void);
>  void *kvmppc_create_spapr_tce(uint32_t liobn, uint32_t window_size, int *pfd,
> -                              bool vfio_accel);
> +                              bool need_vfio);
>  int kvmppc_remove_spapr_tce(void *table, int pfd, uint32_t window_size);
>  int kvmppc_reset_htab(int shift_hint);
>  uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift);
> 
Reviewed-by: Laurent Vivier <lvivier@redhat.com>

Re: [Qemu-devel] [PATCH 3/4] spapr_iommu: Provide a function to switch a TCE table to allowing VFIO

[Laurent Vivier]

On 30/09/2015 05:48, David Gibson wrote:
> Because of the way non-VFIO guest IOMMU operations are KVM accelerated, not
> all TCE tables (guest IOMMU contexts) can support VFIO devices.  Currently,
> this is decided at creation time.
> 
> To support hotplug of VFIO devices, we need to allow a TCE table which
> previously didn't allow VFIO devices to be switched so that it can.  This
> patch adds an spapr_tce_set_need_vfio() function to do this, by
> reallocating the table in userspace if necessary.
> 
> Currently this doesn't allow the KVM acceleration to be re-enabled if all
> the VFIO devices are removed.  That's an optimization for another time.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_iommu.c   | 32 ++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h |  2 ++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> index 5166cde..8d60f8b 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -168,6 +168,38 @@ static int spapr_tce_table_realize(DeviceState *dev)
>      return 0;
>  }
>  
> +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;
> +    }
> +
> +    if (!need_vfio) {
> +        /* FIXME: We don't support transition back to KVM accelerated
> +         * TCEs yet */

Report some warnings ?

> +        return;
> +    }
> +
> +    tcet->need_vfio = true;
> +
> +    if (tcet->fd < 0) {
> +        /* Table is already in userspace, nothing to be do */
> +        return;
> +    }
> +
> +    newtable = g_malloc0(table_size);
> +    memcpy(newtable, tcet->table, table_size);
> +
> +    kvmppc_remove_spapr_tce(tcet->table, tcet->fd, tcet->nb_table);
> +
> +    tcet->fd = -1;
> +    tcet->table = newtable;
> +}
> +
>  sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
>                                     uint64_t bus_offset,
>                                     uint32_t page_shift,
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 27d65d5..5baa906 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -589,6 +589,8 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
>                                     uint32_t page_shift,
>                                     uint32_t nb_table,
>                                     bool need_vfio);
> +void spapr_tce_set_need_vfio(sPAPRTCETable *tcet, bool need_vfio);
> +
>  MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet);
>  int spapr_dma_dt(void *fdt, int node_off, const char *propname,
>                   uint32_t liobn, uint64_t window, uint32_t size);
> 

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

Re: [Qemu-devel] [PATCH 4/4] spapr_pci: Allow VFIO devices to work on the normal PCI host bridge

[Laurent Vivier]

On 30/09/2015 05:48, David Gibson wrote:
> The core VFIO infrastructure more or less allows VFIO devices to work
> on any normal guest PCI host bridge (PHB) without extra logic.
> However, the "spapr-pci-host-bridge" device (as opposed to the special
> "spapr-pci-vfio-host-bridge" device) breaks this by using a partially
> KVM accelerated implementation of the guest kernel IOMMU which won't
> work with VFIO devices, without additional kernel support.
> 
> This patch allows VFIO devices to work on the spapr-pci-host-bridge,
> by having it switch off KVM TCE acceleration when a VFIO device is
> added to the PHB (either on startup, or by hotplug).
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_pci.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index cb7c351..55fa8db 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1083,6 +1083,12 @@ 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);
> 

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

Re: [Qemu-devel] [PATCH 3/4] spapr_iommu: Provide a function to switch a TCE table to allowing VFIO

[Thomas Huth]

On 30/09/15 05:48, David Gibson wrote:
> Because of the way non-VFIO guest IOMMU operations are KVM accelerated, not
> all TCE tables (guest IOMMU contexts) can support VFIO devices.  Currently,
> this is decided at creation time.
> 
> To support hotplug of VFIO devices, we need to allow a TCE table which
> previously didn't allow VFIO devices to be switched so that it can.  This
> patch adds an spapr_tce_set_need_vfio() function to do this, by
> reallocating the table in userspace if necessary.
> 
> Currently this doesn't allow the KVM acceleration to be re-enabled if all
> the VFIO devices are removed.  That's an optimization for another time.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_iommu.c   | 32 ++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h |  2 ++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> index 5166cde..8d60f8b 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -168,6 +168,38 @@ static int spapr_tce_table_realize(DeviceState *dev)
>      return 0;
>  }
>  
> +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;
> +    }
> +
> +    if (!need_vfio) {
> +        /* FIXME: We don't support transition back to KVM accelerated
> +         * TCEs yet */
> +        return;
> +    }
> +
> +    tcet->need_vfio = true;
> +
> +    if (tcet->fd < 0) {
> +        /* Table is already in userspace, nothing to be do */
> +        return;
> +    }
> +
> +    newtable = g_malloc0(table_size);

Since you immediately fill the whole table with the memcpy below, you do
not need to zero the memory here, i.e. g_malloc instead of g_malloc0
should be sufficient.

> +    memcpy(newtable, tcet->table, table_size);
> +
> +    kvmppc_remove_spapr_tce(tcet->table, tcet->fd, tcet->nb_table);
> +
> +    tcet->fd = -1;
> +    tcet->table = newtable;
> +}

 Thomas

Re: [Qemu-devel] [PATCH 4/4] spapr_pci: Allow VFIO devices to work on the normal PCI host bridge

[Thomas Huth]

On 30/09/15 05:48, David Gibson wrote:
> The core VFIO infrastructure more or less allows VFIO devices to work
> on any normal guest PCI host bridge (PHB) without extra logic.
> However, the "spapr-pci-host-bridge" device (as opposed to the special
> "spapr-pci-vfio-host-bridge" device) breaks this by using a partially
> KVM accelerated implementation of the guest kernel IOMMU which won't
> work with VFIO devices, without additional kernel support.
> 
> This patch allows VFIO devices to work on the spapr-pci-host-bridge,
> by having it switch off KVM TCE acceleration when a VFIO device is
> added to the PHB (either on startup, or by hotplug).
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_pci.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index cb7c351..55fa8db 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1083,6 +1083,12 @@ 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);

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 1/4] spapr_pci: Allow PCI host bridge DMA window to be configured

[Laurent Vivier]

On 30/09/2015 05:48, David Gibson wrote:
> At present the PCI host bridge (PHB) for the pseries machine type has a
> fixed DMA window from 0..1GB (in PCI address space) which is mapped to real
> memory via the PAPR paravirtualized IOMMU.
> 
> For better support of VFIO devices, we're going to want to allow for
> different configurations of the DMA window.
> 
> Eventually we'll want to allow the guest itself to reconfigure the window
> via the PAPR dynamic DMA window interface, but as a preliminary this patch
> allows the user to reconfigure the window with new properties on the PHB
> device.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/ppc/spapr_pci.c          | 7 +++++--
>  include/hw/pci-host/spapr.h | 3 +--
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 617b7f3..cb7c351 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1387,7 +1387,7 @@ static void spapr_phb_finish_realize(sPAPRPHBState *sphb, Error **errp)
>      sPAPRTCETable *tcet;
>      uint32_t nb_table;
>  
> -    nb_table = SPAPR_PCI_DMA32_SIZE >> SPAPR_TCE_PAGE_SHIFT;
> +    nb_table = sphb->dma_win_size >> SPAPR_TCE_PAGE_SHIFT;
>      tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn,
>                                 0, SPAPR_TCE_PAGE_SHIFT, nb_table, false);
>      if (!tcet) {
> @@ -1397,7 +1397,7 @@ static void spapr_phb_finish_realize(sPAPRPHBState *sphb, Error **errp)
>      }
>  
>      /* Register default 32bit DMA window */
> -    memory_region_add_subregion(&sphb->iommu_root, 0,
> +    memory_region_add_subregion(&sphb->iommu_root, sphb->dma_win_addr,
>                                  spapr_tce_get_iommu(tcet));
>  }
>  
> @@ -1430,6 +1430,9 @@ static Property spapr_phb_properties[] = {
>                         SPAPR_PCI_IO_WIN_SIZE),
>      DEFINE_PROP_BOOL("dynamic-reconfiguration", sPAPRPHBState, dr_enabled,
>                       true),
> +    /* Default DMA window is 0..1GB */
> +    DEFINE_PROP_UINT64("dma_win_addr", sPAPRPHBState, dma_win_addr, 0),
> +    DEFINE_PROP_UINT64("dma_win_size", sPAPRPHBState, dma_win_size, 0x40000000),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> index 5322b56..7de5e02 100644
> --- a/include/hw/pci-host/spapr.h
> +++ b/include/hw/pci-host/spapr.h
> @@ -78,6 +78,7 @@ struct sPAPRPHBState {
>      MemoryRegion memwindow, iowindow, msiwindow;
>  
>      uint32_t dma_liobn;
> +    hwaddr dma_win_addr, dma_win_size;
>      AddressSpace iommu_as;
>      MemoryRegion iommu_root;
>  
> @@ -115,8 +116,6 @@ struct sPAPRPHBVFIOState {
>  
>  #define SPAPR_PCI_MSI_WINDOW         0x40000000000ULL
>  
> -#define SPAPR_PCI_DMA32_SIZE         0x40000000
> -
>  static inline qemu_irq spapr_phb_lsi_qirq(struct sPAPRPHBState *phb, int pin)
>  {
>      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> 
Reviewed-by: Laurent Vivier <lvivier@redhat.com>

Re: [Qemu-devel] [PATCH 3/4] spapr_iommu: Provide a function to switch a TCE table to allowing VFIO

[David Gibson]

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

On Wed, Sep 30, 2015 at 11:33:41AM +0200, Laurent Vivier wrote:
> 
> 
> On 30/09/2015 05:48, David Gibson wrote:
> > Because of the way non-VFIO guest IOMMU operations are KVM accelerated, not
> > all TCE tables (guest IOMMU contexts) can support VFIO devices.  Currently,
> > this is decided at creation time.
> > 
> > To support hotplug of VFIO devices, we need to allow a TCE table which
> > previously didn't allow VFIO devices to be switched so that it can.  This
> > patch adds an spapr_tce_set_need_vfio() function to do this, by
> > reallocating the table in userspace if necessary.
> > 
> > Currently this doesn't allow the KVM acceleration to be re-enabled if all
> > the VFIO devices are removed.  That's an optimization for another time.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr_iommu.c   | 32 ++++++++++++++++++++++++++++++++
> >  include/hw/ppc/spapr.h |  2 ++
> >  2 files changed, 34 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> > index 5166cde..8d60f8b 100644
> > --- a/hw/ppc/spapr_iommu.c
> > +++ b/hw/ppc/spapr_iommu.c
> > @@ -168,6 +168,38 @@ static int spapr_tce_table_realize(DeviceState *dev)
> >      return 0;
> >  }
> >  
> > +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;
> > +    }
> > +
> > +    if (!need_vfio) {
> > +        /* FIXME: We don't support transition back to KVM accelerated
> > +         * TCEs yet */
> 
> Report some warnings ?

I don't see much value in it.  At present nothing calls this function
with need_vfio == false anyway.  Switching back is harmless, just not
optimal.

> 
> > +        return;
> > +    }
> > +
> > +    tcet->need_vfio = true;
> > +
> > +    if (tcet->fd < 0) {
> > +        /* Table is already in userspace, nothing to be do */
> > +        return;
> > +    }
> > +
> > +    newtable = g_malloc0(table_size);
> > +    memcpy(newtable, tcet->table, table_size);
> > +
> > +    kvmppc_remove_spapr_tce(tcet->table, tcet->fd, tcet->nb_table);
> > +
> > +    tcet->fd = -1;
> > +    tcet->table = newtable;
> > +}
> > +
> >  sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
> >                                     uint64_t bus_offset,
> >                                     uint32_t page_shift,
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index 27d65d5..5baa906 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -589,6 +589,8 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
> >                                     uint32_t page_shift,
> >                                     uint32_t nb_table,
> >                                     bool need_vfio);
> > +void spapr_tce_set_need_vfio(sPAPRTCETable *tcet, bool need_vfio);
> > +
> >  MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet);
> >  int spapr_dma_dt(void *fdt, int node_off, const char *propname,
> >                   uint32_t liobn, uint64_t window, uint32_t size);
> > 
> 
> Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> 

-- 
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: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 3/4] spapr_iommu: Provide a function to switch a TCE table to allowing VFIO

[David Gibson]

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

On Wed, Sep 30, 2015 at 12:07:39PM +0200, Thomas Huth wrote:
> On 30/09/15 05:48, David Gibson wrote:
> > Because of the way non-VFIO guest IOMMU operations are KVM accelerated, not
> > all TCE tables (guest IOMMU contexts) can support VFIO devices.  Currently,
> > this is decided at creation time.
> > 
> > To support hotplug of VFIO devices, we need to allow a TCE table which
> > previously didn't allow VFIO devices to be switched so that it can.  This
> > patch adds an spapr_tce_set_need_vfio() function to do this, by
> > reallocating the table in userspace if necessary.
> > 
> > Currently this doesn't allow the KVM acceleration to be re-enabled if all
> > the VFIO devices are removed.  That's an optimization for another time.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr_iommu.c   | 32 ++++++++++++++++++++++++++++++++
> >  include/hw/ppc/spapr.h |  2 ++
> >  2 files changed, 34 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> > index 5166cde..8d60f8b 100644
> > --- a/hw/ppc/spapr_iommu.c
> > +++ b/hw/ppc/spapr_iommu.c
> > @@ -168,6 +168,38 @@ static int spapr_tce_table_realize(DeviceState *dev)
> >      return 0;
> >  }
> >  
> > +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;
> > +    }
> > +
> > +    if (!need_vfio) {
> > +        /* FIXME: We don't support transition back to KVM accelerated
> > +         * TCEs yet */
> > +        return;
> > +    }
> > +
> > +    tcet->need_vfio = true;
> > +
> > +    if (tcet->fd < 0) {
> > +        /* Table is already in userspace, nothing to be do */
> > +        return;
> > +    }
> > +
> > +    newtable = g_malloc0(table_size);
> 
> Since you immediately fill the whole table with the memcpy below, you do
> not need to zero the memory here, i.e. g_malloc instead of g_malloc0
> should be sufficient.

True, I just used malloc0 out of habit.

-- 
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: Type: application/pgp-signature, Size: 819 bytes --]