[Qemu-devel] [PATCH v8 0/4] vfio on spapr-ppc64

[Qemu-devel] [PATCH v8 0/4] vfio on spapr-ppc64

[Alexey Kardashevskiy]

Yet another try with VFIO on SPAPR (server PPC64).

This adds VFIO support on SPAPR for the existing VFIO-SPAPR-TCE driver
in the upstream kernel.

Individual patches have more detailed commit logs.

Please comment. Thanks!


Changes:
v8:
* added placeholder for KVM_CAP_SPAPR_TCE_VFIO capability
* added generic helper to call ioctl on container's fd.

v7:
* cleaned and rebased on agraf/ppc-next tree (which is on its way to upstream)

v6:
* initial set was split into 3

v5:
* rebase on top of the current upstream

v4:
* addressed all comments from Alex Williamson
* moved spapr-pci-phb-vfio-phb to new file
* split spapr-pci-phb-vfio to many smaller patches


Alexey Kardashevskiy (4):
  spapr_iommu: Make in-kernel TCE table optional
  vfio: Add vfio_container_ioctl()
  spapr_pci_vfio: Add spapr-pci-vfio-host-bridge to support vfio
  vfio: Enable for SPAPR

 hw/misc/vfio.c              |  56 ++++++++++++++++++++++++
 hw/ppc/Makefile.objs        |   3 ++
 hw/ppc/spapr_iommu.c        |   7 ++-
 hw/ppc/spapr_pci.c          |   2 +-
 hw/ppc/spapr_pci_vfio.c     | 102 ++++++++++++++++++++++++++++++++++++++++++++
 hw/ppc/spapr_vio.c          |   2 +-
 include/hw/misc/vfio.h      |   9 ++++
 include/hw/pci-host/spapr.h |  11 +++++
 include/hw/ppc/spapr.h      |   4 +-
 target-ppc/kvm.c            |   7 ++-
 target-ppc/kvm_ppc.h        |   6 ++-
 11 files changed, 200 insertions(+), 9 deletions(-)
 create mode 100644 hw/ppc/spapr_pci_vfio.c
 create mode 100644 include/hw/misc/vfio.h

-- 
2.0.0

[Qemu-devel] [PATCH v8 1/4] spapr_iommu: Make in-kernel TCE table optional

[Alexey Kardashevskiy]

POWER KVM supports an KVM_CAP_SPAPR_TCE capability which allows allocating
TCE tables in the host kernel memory and handle H_PUT_TCE requests
targeted to specific LIOBN (logical bus number) right in the host without
switching to QEMU. At the moment this is used for emulated devices only
and the handler only puts TCE to the table. If the in-kernel H_PUT_TCE
handler finds a LIOBN and corresponding table, it will put a TCE to
the table and complete hypercall execution. The user space will not be
notified.

Upcoming VFIO support is going to use the same sPAPRTCETable device class
so KVM_CAP_SPAPR_TCE is going to be used as well. That means that TCE
tables for VFIO are going to be allocated in the host as well.
However VFIO operates with real IOMMU tables and simple copying of
a TCE to the real hardware TCE table will not work as guest physical
to host physical address translation is requited.

So until the host kernel gets VFIO support for H_PUT_TCE, we better not
to register VFIO's TCE in the host.

This adds a place holder for KVM_CAP_SPAPR_TCE_VFIO capability. It is not
in upstream yet and being discussed so now it is always false which means
that in-kernel VFIO acceleration is not supported.

This adds a bool @vfio_accel flag to the sPAPRTCETable device telling
that sPAPRTCETable should not try allocating TCE table in the host kernel
for VFIO. The flag is false now as at the moment there is no VFIO.

This adds an vfio_accel parameter to spapr_tce_new_table(), the semantic
is the same. Since there is only emulated PCI and VIO now, the flag is set
to false. Upcoming VFIO support will set it to true.

This is a preparation patch so no change in behaviour is expected

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v8:
* s/kvm_accel/vfio_accel/
* s/true/false/ in spapr_tce_new_table() invocations
* added  placeholder for cap_spapr_vfio
---
 hw/ppc/spapr_iommu.c   | 7 +++++--
 hw/ppc/spapr_pci.c     | 2 +-
 hw/ppc/spapr_vio.c     | 2 +-
 include/hw/ppc/spapr.h | 4 +++-
 target-ppc/kvm.c       | 7 +++++--
 target-ppc/kvm_ppc.h   | 6 ++++--
 6 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
index 3b6e373..855c37a 100644
--- a/hw/ppc/spapr_iommu.c
+++ b/hw/ppc/spapr_iommu.c
@@ -119,7 +119,8 @@ static int spapr_tce_table_realize(DeviceState *dev)
         tcet->table = kvmppc_create_spapr_tce(tcet->liobn,
                                               tcet->nb_table <<
                                               tcet->page_shift,
-                                              &tcet->fd);
+                                              &tcet->fd,
+                                              tcet->vfio_accel);
     }
 
     if (!tcet->table) {
@@ -143,7 +144,8 @@ static int spapr_tce_table_realize(DeviceState *dev)
 sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
                                    uint64_t bus_offset,
                                    uint32_t page_shift,
-                                   uint32_t nb_table)
+                                   uint32_t nb_table,
+                                   bool vfio_accel)
 {
     sPAPRTCETable *tcet;
 
@@ -162,6 +164,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;
 
     object_property_add_child(OBJECT(owner), "tce-table", OBJECT(tcet), NULL);
 
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index ddfd8bb..658ddcb 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -658,7 +658,7 @@ static void spapr_phb_finish_realize(sPAPRPHBState *sphb, Error **errp)
     tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn,
                                0,
                                SPAPR_TCE_PAGE_SHIFT,
-                               0x40000000 >> SPAPR_TCE_PAGE_SHIFT);
+                               0x40000000 >> SPAPR_TCE_PAGE_SHIFT, false);
     if (!tcet) {
         error_setg(errp, "Unable to create TCE table for %s",
                    sphb->dtbusname);
diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index 48b0125..5dfa145 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -460,7 +460,7 @@ static int spapr_vio_busdev_init(DeviceState *qdev)
                                         0,
                                         SPAPR_TCE_PAGE_SHIFT,
                                         pc->rtce_window_size >>
-                                        SPAPR_TCE_PAGE_SHIFT);
+                                        SPAPR_TCE_PAGE_SHIFT, false);
         address_space_init(&dev->as, spapr_tce_get_iommu(dev->tcet), qdev->id);
     }
 
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 08c301f..fe1eff6 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -407,6 +407,7 @@ struct sPAPRTCETable {
     uint32_t page_shift;
     uint64_t *table;
     bool bypass;
+    bool vfio_accel;
     int fd;
     MemoryRegion iommu;
     QLIST_ENTRY(sPAPRTCETable) list;
@@ -418,7 +419,8 @@ int spapr_h_cas_compose_response(target_ulong addr, target_ulong size);
 sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
                                    uint64_t bus_offset,
                                    uint32_t page_shift,
-                                   uint32_t nb_table);
+                                   uint32_t nb_table,
+                                   bool vfio_accel);
 MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet);
 void spapr_tce_set_bypass(sPAPRTCETable *tcet, bool bypass);
 int spapr_dma_dt(void *fdt, int node_off, const char *propname,
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index dfa5a26..f83bdbe 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -63,6 +63,7 @@ static int cap_ppc_smt;
 static int cap_ppc_rma;
 static int cap_spapr_tce;
 static int cap_spapr_multitce;
+static int cap_spapr_vfio;
 static int cap_hior;
 static int cap_one_reg;
 static int cap_epr;
@@ -101,6 +102,7 @@ int kvm_arch_init(KVMState *s)
     cap_ppc_rma = kvm_check_extension(s, KVM_CAP_PPC_RMA);
     cap_spapr_tce = kvm_check_extension(s, KVM_CAP_SPAPR_TCE);
     cap_spapr_multitce = kvm_check_extension(s, KVM_CAP_SPAPR_MULTITCE);
+    cap_spapr_vfio = false;
     cap_one_reg = kvm_check_extension(s, KVM_CAP_ONE_REG);
     cap_hior = kvm_check_extension(s, KVM_CAP_PPC_HIOR);
     cap_epr = kvm_check_extension(s, KVM_CAP_PPC_EPR);
@@ -1660,7 +1662,8 @@ bool kvmppc_spapr_use_multitce(void)
     return cap_spapr_multitce;
 }
 
-void *kvmppc_create_spapr_tce(uint32_t liobn, uint32_t window_size, int *pfd)
+void *kvmppc_create_spapr_tce(uint32_t liobn, uint32_t window_size, int *pfd,
+                              bool vfio_accel)
 {
     struct kvm_create_spapr_tce args = {
         .liobn = liobn,
@@ -1674,7 +1677,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) {
+    if (!cap_spapr_tce || (vfio_accel && !cap_spapr_vfio)) {
         return NULL;
     }
 
diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
index 412cc7f..1118122 100644
--- a/target-ppc/kvm_ppc.h
+++ b/target-ppc/kvm_ppc.h
@@ -33,7 +33,8 @@ int kvmppc_booke_watchdog_enable(PowerPCCPU *cpu);
 #ifndef CONFIG_USER_ONLY
 off_t kvmppc_alloc_rma(const char *name, MemoryRegion *sysmem);
 bool kvmppc_spapr_use_multitce(void);
-void *kvmppc_create_spapr_tce(uint32_t liobn, uint32_t window_size, int *pfd);
+void *kvmppc_create_spapr_tce(uint32_t liobn, uint32_t window_size, int *pfd,
+                              bool vfio_accel);
 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);
@@ -144,7 +145,8 @@ static inline bool kvmppc_spapr_use_multitce(void)
 }
 
 static inline void *kvmppc_create_spapr_tce(uint32_t liobn,
-                                            uint32_t window_size, int *fd)
+                                            uint32_t window_size, int *fd,
+                                            bool vfio_accel)
 {
     return NULL;
 }
-- 
2.0.0

[Qemu-devel] [PATCH v8 2/4] vfio: Add vfio_container_ioctl()

[Alexey Kardashevskiy]

While most operations with VFIO IOMMU driver are generic and used inside
vfio.c, there are still some operations which only specific VFIO IOMMU
drivers implement. The first example of it will be reading a DMA window
start from the host.

This adds a helper which passes an ioctl request to the container's fd.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v8:
* s/vfio_container_spapr_get_info/vfio_container_ioctl/ - now it is
generalized

v7:
* do not return a group fd from the helper

v6:
* added dup() to protect group_fd from accidental disposal

v5:
* reworked to reflect change in vfio_get_group() from one
of previous patches change

v4:
* fixed possible leaks on error paths
---
 hw/misc/vfio.c         | 28 ++++++++++++++++++++++++++++
 include/hw/misc/vfio.h |  9 +++++++++
 2 files changed, 37 insertions(+)
 create mode 100644 include/hw/misc/vfio.h

diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
index 7437c2e..bb77934 100644
--- a/hw/misc/vfio.c
+++ b/hw/misc/vfio.c
@@ -39,6 +39,7 @@
 #include "qemu/range.h"
 #include "sysemu/kvm.h"
 #include "sysemu/sysemu.h"
+#include "hw/misc/vfio.h"
 
 /* #define DEBUG_VFIO */
 #ifdef DEBUG_VFIO
@@ -4318,3 +4319,30 @@ static void register_vfio_pci_dev_type(void)
 }
 
 type_init(register_vfio_pci_dev_type)
+
+int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
+                         int req, void *param)
+{
+    VFIOGroup *group;
+    VFIOContainer *container;
+    int ret = -1;
+
+    group = vfio_get_group(groupid, as);
+    if (!group) {
+        error_report("vfio: group %d not registered", groupid);
+        return ret;
+    }
+
+    container = group->container;
+    if (group->container) {
+        ret = ioctl(container->fd, req, param);
+        if (ret < 0) {
+            error_report("vfio: failed to ioctl container: ret=%d, %s",
+                         ret, strerror(errno));
+        }
+    }
+
+    vfio_put_group(group);
+
+    return ret;
+}
diff --git a/include/hw/misc/vfio.h b/include/hw/misc/vfio.h
new file mode 100644
index 0000000..0b26cd8
--- /dev/null
+++ b/include/hw/misc/vfio.h
@@ -0,0 +1,9 @@
+#ifndef VFIO_API_H
+#define VFIO_API_H
+
+#include "qemu/typedefs.h"
+
+extern int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
+                                int req, void *param);
+
+#endif
-- 
2.0.0

Re: [Qemu-devel] [PATCH v8 2/4] vfio: Add vfio_container_ioctl()

[Alex Williamson]

On Fri, 2014-06-06 at 13:34 +1000, Alexey Kardashevskiy wrote:
> While most operations with VFIO IOMMU driver are generic and used inside
> vfio.c, there are still some operations which only specific VFIO IOMMU
> drivers implement. The first example of it will be reading a DMA window
> start from the host.
> 
> This adds a helper which passes an ioctl request to the container's fd.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> Changes:
> v8:
> * s/vfio_container_spapr_get_info/vfio_container_ioctl/ - now it is
> generalized
> 
> v7:
> * do not return a group fd from the helper
> 
> v6:
> * added dup() to protect group_fd from accidental disposal
> 
> v5:
> * reworked to reflect change in vfio_get_group() from one
> of previous patches change
> 
> v4:
> * fixed possible leaks on error paths
> ---
>  hw/misc/vfio.c         | 28 ++++++++++++++++++++++++++++
>  include/hw/misc/vfio.h |  9 +++++++++
>  2 files changed, 37 insertions(+)
>  create mode 100644 include/hw/misc/vfio.h
> 
> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> index 7437c2e..bb77934 100644
> --- a/hw/misc/vfio.c
> +++ b/hw/misc/vfio.c
> @@ -39,6 +39,7 @@
>  #include "qemu/range.h"
>  #include "sysemu/kvm.h"
>  #include "sysemu/sysemu.h"
> +#include "hw/misc/vfio.h"
>  
>  /* #define DEBUG_VFIO */
>  #ifdef DEBUG_VFIO
> @@ -4318,3 +4319,30 @@ static void register_vfio_pci_dev_type(void)
>  }
>  
>  type_init(register_vfio_pci_dev_type)
> +
> +int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
> +                         int req, void *param)
> +{
> +    VFIOGroup *group;
> +    VFIOContainer *container;
> +    int ret = -1;
> +
> +    group = vfio_get_group(groupid, as);
> +    if (!group) {
> +        error_report("vfio: group %d not registered", groupid);
> +        return ret;
> +    }

Gavin's version of this walked existing groups and returned an error if
not found.  vfio_get_group() will go out and create a group and connect
it to a container.  I'm not sure we really want to expose that sort of
capability out to the reset of QEMU.  Thanks,

Alex

> +
> +    container = group->container;
> +    if (group->container) {
> +        ret = ioctl(container->fd, req, param);
> +        if (ret < 0) {
> +            error_report("vfio: failed to ioctl container: ret=%d, %s",
> +                         ret, strerror(errno));
> +        }
> +    }
> +
> +    vfio_put_group(group);
> +
> +    return ret;
> +}
> diff --git a/include/hw/misc/vfio.h b/include/hw/misc/vfio.h
> new file mode 100644
> index 0000000..0b26cd8
> --- /dev/null
> +++ b/include/hw/misc/vfio.h
> @@ -0,0 +1,9 @@
> +#ifndef VFIO_API_H
> +#define VFIO_API_H
> +
> +#include "qemu/typedefs.h"
> +
> +extern int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
> +                                int req, void *param);
> +
> +#endif

Re: [Qemu-devel] [PATCH v8 2/4] vfio: Add vfio_container_ioctl()

[Alexey Kardashevskiy]

On 06/07/2014 02:48 AM, Alex Williamson wrote:
> On Fri, 2014-06-06 at 13:34 +1000, Alexey Kardashevskiy wrote:
>> While most operations with VFIO IOMMU driver are generic and used inside
>> vfio.c, there are still some operations which only specific VFIO IOMMU
>> drivers implement. The first example of it will be reading a DMA window
>> start from the host.
>>
>> This adds a helper which passes an ioctl request to the container's fd.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> Changes:
>> v8:
>> * s/vfio_container_spapr_get_info/vfio_container_ioctl/ - now it is
>> generalized
>>
>> v7:
>> * do not return a group fd from the helper
>>
>> v6:
>> * added dup() to protect group_fd from accidental disposal
>>
>> v5:
>> * reworked to reflect change in vfio_get_group() from one
>> of previous patches change
>>
>> v4:
>> * fixed possible leaks on error paths
>> ---
>>  hw/misc/vfio.c         | 28 ++++++++++++++++++++++++++++
>>  include/hw/misc/vfio.h |  9 +++++++++
>>  2 files changed, 37 insertions(+)
>>  create mode 100644 include/hw/misc/vfio.h
>>
>> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
>> index 7437c2e..bb77934 100644
>> --- a/hw/misc/vfio.c
>> +++ b/hw/misc/vfio.c
>> @@ -39,6 +39,7 @@
>>  #include "qemu/range.h"
>>  #include "sysemu/kvm.h"
>>  #include "sysemu/sysemu.h"
>> +#include "hw/misc/vfio.h"
>>  
>>  /* #define DEBUG_VFIO */
>>  #ifdef DEBUG_VFIO
>> @@ -4318,3 +4319,30 @@ static void register_vfio_pci_dev_type(void)
>>  }
>>  
>>  type_init(register_vfio_pci_dev_type)
>> +
>> +int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
>> +                         int req, void *param)
>> +{
>> +    VFIOGroup *group;
>> +    VFIOContainer *container;
>> +    int ret = -1;
>> +
>> +    group = vfio_get_group(groupid, as);
>> +    if (!group) {
>> +        error_report("vfio: group %d not registered", groupid);
>> +        return ret;
>> +    }
> 
> Gavin's version of this walked existing groups and returned an error if
> not found.  vfio_get_group() will go out and create a group and connect
> it to a container.  I'm not sure we really want to expose that sort of
> capability out to the reset of QEMU.  Thanks,


If it is not created here, then it can only be created from vfio_initfn()
which is too late as I need group to get DMA window properties for PHB when
PHB is being realized.

Add another helper?


> 
> Alex
> 
>> +
>> +    container = group->container;
>> +    if (group->container) {
>> +        ret = ioctl(container->fd, req, param);
>> +        if (ret < 0) {
>> +            error_report("vfio: failed to ioctl container: ret=%d, %s",
>> +                         ret, strerror(errno));
>> +        }
>> +    }
>> +
>> +    vfio_put_group(group);
>> +
>> +    return ret;
>> +}
>> diff --git a/include/hw/misc/vfio.h b/include/hw/misc/vfio.h
>> new file mode 100644
>> index 0000000..0b26cd8
>> --- /dev/null
>> +++ b/include/hw/misc/vfio.h
>> @@ -0,0 +1,9 @@
>> +#ifndef VFIO_API_H
>> +#define VFIO_API_H
>> +
>> +#include "qemu/typedefs.h"
>> +
>> +extern int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
>> +                                int req, void *param);
>> +
>> +#endif
> 
> 
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH v8 2/4] vfio: Add vfio_container_ioctl()

[Alex Williamson]

On Sat, 2014-06-07 at 02:54 +1000, Alexey Kardashevskiy wrote:
> On 06/07/2014 02:48 AM, Alex Williamson wrote:
> > On Fri, 2014-06-06 at 13:34 +1000, Alexey Kardashevskiy wrote:
> >> While most operations with VFIO IOMMU driver are generic and used inside
> >> vfio.c, there are still some operations which only specific VFIO IOMMU
> >> drivers implement. The first example of it will be reading a DMA window
> >> start from the host.
> >>
> >> This adds a helper which passes an ioctl request to the container's fd.
> >>
> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >> ---
> >> Changes:
> >> v8:
> >> * s/vfio_container_spapr_get_info/vfio_container_ioctl/ - now it is
> >> generalized
> >>
> >> v7:
> >> * do not return a group fd from the helper
> >>
> >> v6:
> >> * added dup() to protect group_fd from accidental disposal
> >>
> >> v5:
> >> * reworked to reflect change in vfio_get_group() from one
> >> of previous patches change
> >>
> >> v4:
> >> * fixed possible leaks on error paths
> >> ---
> >>  hw/misc/vfio.c         | 28 ++++++++++++++++++++++++++++
> >>  include/hw/misc/vfio.h |  9 +++++++++
> >>  2 files changed, 37 insertions(+)
> >>  create mode 100644 include/hw/misc/vfio.h
> >>
> >> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> >> index 7437c2e..bb77934 100644
> >> --- a/hw/misc/vfio.c
> >> +++ b/hw/misc/vfio.c
> >> @@ -39,6 +39,7 @@
> >>  #include "qemu/range.h"
> >>  #include "sysemu/kvm.h"
> >>  #include "sysemu/sysemu.h"
> >> +#include "hw/misc/vfio.h"
> >>  
> >>  /* #define DEBUG_VFIO */
> >>  #ifdef DEBUG_VFIO
> >> @@ -4318,3 +4319,30 @@ static void register_vfio_pci_dev_type(void)
> >>  }
> >>  
> >>  type_init(register_vfio_pci_dev_type)
> >> +
> >> +int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
> >> +                         int req, void *param)
> >> +{
> >> +    VFIOGroup *group;
> >> +    VFIOContainer *container;
> >> +    int ret = -1;
> >> +
> >> +    group = vfio_get_group(groupid, as);
> >> +    if (!group) {
> >> +        error_report("vfio: group %d not registered", groupid);
> >> +        return ret;
> >> +    }
> > 
> > Gavin's version of this walked existing groups and returned an error if
> > not found.  vfio_get_group() will go out and create a group and connect
> > it to a container.  I'm not sure we really want to expose that sort of
> > capability out to the reset of QEMU.  Thanks,
> 
> 
> If it is not created here, then it can only be created from vfio_initfn()
> which is too late as I need group to get DMA window properties for PHB when
> PHB is being realized.
> 
> Add another helper?

Urgh.  No, let's not make another helper.  Can we at least keep the
filtering Gavin had so we don't just allow any ioctl through here
though?  At least we can make vfio.c involved if someone what's to do
something crazy here.  Thanks,

Alex

> >> +
> >> +    container = group->container;
> >> +    if (group->container) {
> >> +        ret = ioctl(container->fd, req, param);
> >> +        if (ret < 0) {
> >> +            error_report("vfio: failed to ioctl container: ret=%d, %s",
> >> +                         ret, strerror(errno));
> >> +        }
> >> +    }
> >> +
> >> +    vfio_put_group(group);
> >> +
> >> +    return ret;
> >> +}
> >> diff --git a/include/hw/misc/vfio.h b/include/hw/misc/vfio.h
> >> new file mode 100644
> >> index 0000000..0b26cd8
> >> --- /dev/null
> >> +++ b/include/hw/misc/vfio.h
> >> @@ -0,0 +1,9 @@
> >> +#ifndef VFIO_API_H
> >> +#define VFIO_API_H
> >> +
> >> +#include "qemu/typedefs.h"
> >> +
> >> +extern int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
> >> +                                int req, void *param);
> >> +
> >> +#endif
> > 
> > 
> > 
> 
> 

Re: [Qemu-devel] [PATCH v8 2/4] vfio: Add vfio_container_ioctl()

[Alexey Kardashevskiy]

On 06/07/2014 03:35 AM, Alex Williamson wrote:
> On Sat, 2014-06-07 at 02:54 +1000, Alexey Kardashevskiy wrote:
>> On 06/07/2014 02:48 AM, Alex Williamson wrote:
>>> On Fri, 2014-06-06 at 13:34 +1000, Alexey Kardashevskiy wrote:
>>>> While most operations with VFIO IOMMU driver are generic and used inside
>>>> vfio.c, there are still some operations which only specific VFIO IOMMU
>>>> drivers implement. The first example of it will be reading a DMA window
>>>> start from the host.
>>>>
>>>> This adds a helper which passes an ioctl request to the container's fd.
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> ---
>>>> Changes:
>>>> v8:
>>>> * s/vfio_container_spapr_get_info/vfio_container_ioctl/ - now it is
>>>> generalized
>>>>
>>>> v7:
>>>> * do not return a group fd from the helper
>>>>
>>>> v6:
>>>> * added dup() to protect group_fd from accidental disposal
>>>>
>>>> v5:
>>>> * reworked to reflect change in vfio_get_group() from one
>>>> of previous patches change
>>>>
>>>> v4:
>>>> * fixed possible leaks on error paths
>>>> ---
>>>>  hw/misc/vfio.c         | 28 ++++++++++++++++++++++++++++
>>>>  include/hw/misc/vfio.h |  9 +++++++++
>>>>  2 files changed, 37 insertions(+)
>>>>  create mode 100644 include/hw/misc/vfio.h
>>>>
>>>> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
>>>> index 7437c2e..bb77934 100644
>>>> --- a/hw/misc/vfio.c
>>>> +++ b/hw/misc/vfio.c
>>>> @@ -39,6 +39,7 @@
>>>>  #include "qemu/range.h"
>>>>  #include "sysemu/kvm.h"
>>>>  #include "sysemu/sysemu.h"
>>>> +#include "hw/misc/vfio.h"
>>>>  
>>>>  /* #define DEBUG_VFIO */
>>>>  #ifdef DEBUG_VFIO
>>>> @@ -4318,3 +4319,30 @@ static void register_vfio_pci_dev_type(void)
>>>>  }
>>>>  
>>>>  type_init(register_vfio_pci_dev_type)
>>>> +
>>>> +int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
>>>> +                         int req, void *param)
>>>> +{
>>>> +    VFIOGroup *group;
>>>> +    VFIOContainer *container;
>>>> +    int ret = -1;
>>>> +
>>>> +    group = vfio_get_group(groupid, as);
>>>> +    if (!group) {
>>>> +        error_report("vfio: group %d not registered", groupid);
>>>> +        return ret;
>>>> +    }


>>>
>>> Gavin's version of this walked existing groups and returned an error if
>>> not found.  vfio_get_group() will go out and create a group and connect
>>> it to a container.  I'm not sure we really want to expose that sort of
>>> capability out to the reset of QEMU.  Thanks,

>>
>>
>> If it is not created here, then it can only be created from vfio_initfn()
>> which is too late as I need group to get DMA window properties for PHB when
>> PHB is being realized.
>>
>> Add another helper?
> 
> Urgh.  No, let's not make another helper.  Can we at least keep the
> filtering Gavin had so we don't just allow any ioctl through here
> though?  At least we can make vfio.c involved if someone what's to do
> something crazy here.  Thanks,


if (req != CHECK_EXTENSION) &&
   (req != ENABLE) &&
   (reg != EEH_OP)
	return -1;

Like that (roughly)?




> 
> Alex
> 
>>>> +
>>>> +    container = group->container;
>>>> +    if (group->container) {
>>>> +        ret = ioctl(container->fd, req, param);
>>>> +        if (ret < 0) {
>>>> +            error_report("vfio: failed to ioctl container: ret=%d, %s",
>>>> +                         ret, strerror(errno));
>>>> +        }
>>>> +    }
>>>> +
>>>> +    vfio_put_group(group);
>>>> +
>>>> +    return ret;
>>>> +}
>>>> diff --git a/include/hw/misc/vfio.h b/include/hw/misc/vfio.h
>>>> new file mode 100644
>>>> index 0000000..0b26cd8
>>>> --- /dev/null
>>>> +++ b/include/hw/misc/vfio.h
>>>> @@ -0,0 +1,9 @@
>>>> +#ifndef VFIO_API_H
>>>> +#define VFIO_API_H
>>>> +
>>>> +#include "qemu/typedefs.h"
>>>> +
>>>> +extern int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
>>>> +                                int req, void *param);
>>>> +
>>>> +#endif
>>>
>>>
>>>
>>
>>
> 
> 
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH v8 2/4] vfio: Add vfio_container_ioctl()

[Alex Williamson]

On Sat, 2014-06-07 at 08:50 +1000, Alexey Kardashevskiy wrote:
> On 06/07/2014 03:35 AM, Alex Williamson wrote:
> > On Sat, 2014-06-07 at 02:54 +1000, Alexey Kardashevskiy wrote:
> >> On 06/07/2014 02:48 AM, Alex Williamson wrote:
> >>> On Fri, 2014-06-06 at 13:34 +1000, Alexey Kardashevskiy wrote:
> >>>> While most operations with VFIO IOMMU driver are generic and used inside
> >>>> vfio.c, there are still some operations which only specific VFIO IOMMU
> >>>> drivers implement. The first example of it will be reading a DMA window
> >>>> start from the host.
> >>>>
> >>>> This adds a helper which passes an ioctl request to the container's fd.
> >>>>
> >>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>>> ---
> >>>> Changes:
> >>>> v8:
> >>>> * s/vfio_container_spapr_get_info/vfio_container_ioctl/ - now it is
> >>>> generalized
> >>>>
> >>>> v7:
> >>>> * do not return a group fd from the helper
> >>>>
> >>>> v6:
> >>>> * added dup() to protect group_fd from accidental disposal
> >>>>
> >>>> v5:
> >>>> * reworked to reflect change in vfio_get_group() from one
> >>>> of previous patches change
> >>>>
> >>>> v4:
> >>>> * fixed possible leaks on error paths
> >>>> ---
> >>>>  hw/misc/vfio.c         | 28 ++++++++++++++++++++++++++++
> >>>>  include/hw/misc/vfio.h |  9 +++++++++
> >>>>  2 files changed, 37 insertions(+)
> >>>>  create mode 100644 include/hw/misc/vfio.h
> >>>>
> >>>> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> >>>> index 7437c2e..bb77934 100644
> >>>> --- a/hw/misc/vfio.c
> >>>> +++ b/hw/misc/vfio.c
> >>>> @@ -39,6 +39,7 @@
> >>>>  #include "qemu/range.h"
> >>>>  #include "sysemu/kvm.h"
> >>>>  #include "sysemu/sysemu.h"
> >>>> +#include "hw/misc/vfio.h"
> >>>>  
> >>>>  /* #define DEBUG_VFIO */
> >>>>  #ifdef DEBUG_VFIO
> >>>> @@ -4318,3 +4319,30 @@ static void register_vfio_pci_dev_type(void)
> >>>>  }
> >>>>  
> >>>>  type_init(register_vfio_pci_dev_type)
> >>>> +
> >>>> +int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
> >>>> +                         int req, void *param)
> >>>> +{
> >>>> +    VFIOGroup *group;
> >>>> +    VFIOContainer *container;
> >>>> +    int ret = -1;
> >>>> +
> >>>> +    group = vfio_get_group(groupid, as);
> >>>> +    if (!group) {
> >>>> +        error_report("vfio: group %d not registered", groupid);
> >>>> +        return ret;
> >>>> +    }
> 
> 
> >>>
> >>> Gavin's version of this walked existing groups and returned an error if
> >>> not found.  vfio_get_group() will go out and create a group and connect
> >>> it to a container.  I'm not sure we really want to expose that sort of
> >>> capability out to the reset of QEMU.  Thanks,
> 
> >>
> >>
> >> If it is not created here, then it can only be created from vfio_initfn()
> >> which is too late as I need group to get DMA window properties for PHB when
> >> PHB is being realized.
> >>
> >> Add another helper?
> > 
> > Urgh.  No, let's not make another helper.  Can we at least keep the
> > filtering Gavin had so we don't just allow any ioctl through here
> > though?  At least we can make vfio.c involved if someone what's to do
> > something crazy here.  Thanks,
> 
> 
> if (req != CHECK_EXTENSION) &&
>    (req != ENABLE) &&
>    (reg != EEH_OP)
> 	return -1;
> 
> Like that (roughly)?

A switch statement seems a lot cleaner.
>  
> >>>> +
> >>>> +    container = group->container;
> >>>> +    if (group->container) {
> >>>> +        ret = ioctl(container->fd, req, param);
> >>>> +        if (ret < 0) {
> >>>> +            error_report("vfio: failed to ioctl container: ret=%d, %s",
> >>>> +                         ret, strerror(errno));
> >>>> +        }
> >>>> +    }
> >>>> +
> >>>> +    vfio_put_group(group);
> >>>> +
> >>>> +    return ret;
> >>>> +}
> >>>> diff --git a/include/hw/misc/vfio.h b/include/hw/misc/vfio.h
> >>>> new file mode 100644
> >>>> index 0000000..0b26cd8
> >>>> --- /dev/null
> >>>> +++ b/include/hw/misc/vfio.h
> >>>> @@ -0,0 +1,9 @@
> >>>> +#ifndef VFIO_API_H
> >>>> +#define VFIO_API_H
> >>>> +
> >>>> +#include "qemu/typedefs.h"
> >>>> +
> >>>> +extern int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
> >>>> +                                int req, void *param);
> >>>> +
> >>>> +#endif
> >>>
> >>>
> >>>
> >>
> >>
> > 
> > 
> > 
> 
> 

Re: [Qemu-devel] [PATCH v8 2/4] vfio: Add vfio_container_ioctl()

[Alexey Kardashevskiy]

On 06/07/2014 08:55 AM, Alex Williamson wrote:
> On Sat, 2014-06-07 at 08:50 +1000, Alexey Kardashevskiy wrote:
>> On 06/07/2014 03:35 AM, Alex Williamson wrote:
>>> On Sat, 2014-06-07 at 02:54 +1000, Alexey Kardashevskiy wrote:
>>>> On 06/07/2014 02:48 AM, Alex Williamson wrote:
>>>>> On Fri, 2014-06-06 at 13:34 +1000, Alexey Kardashevskiy wrote:
>>>>>> While most operations with VFIO IOMMU driver are generic and used inside
>>>>>> vfio.c, there are still some operations which only specific VFIO IOMMU
>>>>>> drivers implement. The first example of it will be reading a DMA window
>>>>>> start from the host.
>>>>>>
>>>>>> This adds a helper which passes an ioctl request to the container's fd.
>>>>>>
>>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>>> ---
>>>>>> Changes:
>>>>>> v8:
>>>>>> * s/vfio_container_spapr_get_info/vfio_container_ioctl/ - now it is
>>>>>> generalized
>>>>>>
>>>>>> v7:
>>>>>> * do not return a group fd from the helper
>>>>>>
>>>>>> v6:
>>>>>> * added dup() to protect group_fd from accidental disposal
>>>>>>
>>>>>> v5:
>>>>>> * reworked to reflect change in vfio_get_group() from one
>>>>>> of previous patches change
>>>>>>
>>>>>> v4:
>>>>>> * fixed possible leaks on error paths
>>>>>> ---
>>>>>>  hw/misc/vfio.c         | 28 ++++++++++++++++++++++++++++
>>>>>>  include/hw/misc/vfio.h |  9 +++++++++
>>>>>>  2 files changed, 37 insertions(+)
>>>>>>  create mode 100644 include/hw/misc/vfio.h
>>>>>>
>>>>>> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
>>>>>> index 7437c2e..bb77934 100644
>>>>>> --- a/hw/misc/vfio.c
>>>>>> +++ b/hw/misc/vfio.c
>>>>>> @@ -39,6 +39,7 @@
>>>>>>  #include "qemu/range.h"
>>>>>>  #include "sysemu/kvm.h"
>>>>>>  #include "sysemu/sysemu.h"
>>>>>> +#include "hw/misc/vfio.h"
>>>>>>  
>>>>>>  /* #define DEBUG_VFIO */
>>>>>>  #ifdef DEBUG_VFIO
>>>>>> @@ -4318,3 +4319,30 @@ static void register_vfio_pci_dev_type(void)
>>>>>>  }
>>>>>>  
>>>>>>  type_init(register_vfio_pci_dev_type)
>>>>>> +
>>>>>> +int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
>>>>>> +                         int req, void *param)
>>>>>> +{
>>>>>> +    VFIOGroup *group;
>>>>>> +    VFIOContainer *container;
>>>>>> +    int ret = -1;
>>>>>> +
>>>>>> +    group = vfio_get_group(groupid, as);
>>>>>> +    if (!group) {
>>>>>> +        error_report("vfio: group %d not registered", groupid);
>>>>>> +        return ret;
>>>>>> +    }
>>
>>
>>>>>
>>>>> Gavin's version of this walked existing groups and returned an error if
>>>>> not found.  vfio_get_group() will go out and create a group and connect
>>>>> it to a container.  I'm not sure we really want to expose that sort of
>>>>> capability out to the reset of QEMU.  Thanks,
>>
>>>>
>>>>
>>>> If it is not created here, then it can only be created from vfio_initfn()
>>>> which is too late as I need group to get DMA window properties for PHB when
>>>> PHB is being realized.
>>>>
>>>> Add another helper?
>>>
>>> Urgh.  No, let's not make another helper.  Can we at least keep the
>>> filtering Gavin had so we don't just allow any ioctl through here
>>> though?  At least we can make vfio.c involved if someone what's to do
>>> something crazy here.  Thanks,
>>
>>
>> if (req != CHECK_EXTENSION) &&
>>    (req != ENABLE) &&
>>    (reg != EEH_OP)
>> 	return -1;
>>
>> Like that (roughly)?
> 
> A switch statement seems a lot cleaner.


Like that (#2 adds empty switch{}, #4 enables two ioctls, below is #4)?

 int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
                          int req, void *param)
 {
     VFIOGroup *group;
     VFIOContainer *container;
     int ret = -1;

     switch (req) {
+    case VFIO_CHECK_EXTENSION:
+    case VFIO_IOMMU_SPAPR_TCE_GET_INFO:
+        break;
     default:
         /* Return an error on unknown requests */
         return ret;
     }
     group = vfio_get_group(groupid, as);
     if (!group) {



>>  
>>>>>> +
>>>>>> +    container = group->container;
>>>>>> +    if (group->container) {
>>>>>> +        ret = ioctl(container->fd, req, param);
>>>>>> +        if (ret < 0) {
>>>>>> +            error_report("vfio: failed to ioctl container: ret=%d, %s",
>>>>>> +                         ret, strerror(errno));
>>>>>> +        }
>>>>>> +    }
>>>>>> +
>>>>>> +    vfio_put_group(group);
>>>>>> +
>>>>>> +    return ret;
>>>>>> +}
>>>>>> diff --git a/include/hw/misc/vfio.h b/include/hw/misc/vfio.h
>>>>>> new file mode 100644
>>>>>> index 0000000..0b26cd8
>>>>>> --- /dev/null
>>>>>> +++ b/include/hw/misc/vfio.h
>>>>>> @@ -0,0 +1,9 @@
>>>>>> +#ifndef VFIO_API_H
>>>>>> +#define VFIO_API_H
>>>>>> +
>>>>>> +#include "qemu/typedefs.h"
>>>>>> +
>>>>>> +extern int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
>>>>>> +                                int req, void *param);
>>>>>> +
>>>>>> +#endif
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>>
> 
> 
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH v8 2/4] vfio: Add vfio_container_ioctl()

[Alex Williamson]

On Sun, 2014-06-08 at 09:48 +1000, Alexey Kardashevskiy wrote:
> On 06/07/2014 08:55 AM, Alex Williamson wrote:
> > On Sat, 2014-06-07 at 08:50 +1000, Alexey Kardashevskiy wrote:
> >> On 06/07/2014 03:35 AM, Alex Williamson wrote:
> >>> On Sat, 2014-06-07 at 02:54 +1000, Alexey Kardashevskiy wrote:
> >>>> On 06/07/2014 02:48 AM, Alex Williamson wrote:
> >>>>> On Fri, 2014-06-06 at 13:34 +1000, Alexey Kardashevskiy wrote:
> >>>>>> While most operations with VFIO IOMMU driver are generic and used inside
> >>>>>> vfio.c, there are still some operations which only specific VFIO IOMMU
> >>>>>> drivers implement. The first example of it will be reading a DMA window
> >>>>>> start from the host.
> >>>>>>
> >>>>>> This adds a helper which passes an ioctl request to the container's fd.
> >>>>>>
> >>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>>>>> ---
> >>>>>> Changes:
> >>>>>> v8:
> >>>>>> * s/vfio_container_spapr_get_info/vfio_container_ioctl/ - now it is
> >>>>>> generalized
> >>>>>>
> >>>>>> v7:
> >>>>>> * do not return a group fd from the helper
> >>>>>>
> >>>>>> v6:
> >>>>>> * added dup() to protect group_fd from accidental disposal
> >>>>>>
> >>>>>> v5:
> >>>>>> * reworked to reflect change in vfio_get_group() from one
> >>>>>> of previous patches change
> >>>>>>
> >>>>>> v4:
> >>>>>> * fixed possible leaks on error paths
> >>>>>> ---
> >>>>>>  hw/misc/vfio.c         | 28 ++++++++++++++++++++++++++++
> >>>>>>  include/hw/misc/vfio.h |  9 +++++++++
> >>>>>>  2 files changed, 37 insertions(+)
> >>>>>>  create mode 100644 include/hw/misc/vfio.h
> >>>>>>
> >>>>>> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> >>>>>> index 7437c2e..bb77934 100644
> >>>>>> --- a/hw/misc/vfio.c
> >>>>>> +++ b/hw/misc/vfio.c
> >>>>>> @@ -39,6 +39,7 @@
> >>>>>>  #include "qemu/range.h"
> >>>>>>  #include "sysemu/kvm.h"
> >>>>>>  #include "sysemu/sysemu.h"
> >>>>>> +#include "hw/misc/vfio.h"
> >>>>>>  
> >>>>>>  /* #define DEBUG_VFIO */
> >>>>>>  #ifdef DEBUG_VFIO
> >>>>>> @@ -4318,3 +4319,30 @@ static void register_vfio_pci_dev_type(void)
> >>>>>>  }
> >>>>>>  
> >>>>>>  type_init(register_vfio_pci_dev_type)
> >>>>>> +
> >>>>>> +int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
> >>>>>> +                         int req, void *param)
> >>>>>> +{
> >>>>>> +    VFIOGroup *group;
> >>>>>> +    VFIOContainer *container;
> >>>>>> +    int ret = -1;
> >>>>>> +
> >>>>>> +    group = vfio_get_group(groupid, as);
> >>>>>> +    if (!group) {
> >>>>>> +        error_report("vfio: group %d not registered", groupid);
> >>>>>> +        return ret;
> >>>>>> +    }
> >>
> >>
> >>>>>
> >>>>> Gavin's version of this walked existing groups and returned an error if
> >>>>> not found.  vfio_get_group() will go out and create a group and connect
> >>>>> it to a container.  I'm not sure we really want to expose that sort of
> >>>>> capability out to the reset of QEMU.  Thanks,
> >>
> >>>>
> >>>>
> >>>> If it is not created here, then it can only be created from vfio_initfn()
> >>>> which is too late as I need group to get DMA window properties for PHB when
> >>>> PHB is being realized.
> >>>>
> >>>> Add another helper?
> >>>
> >>> Urgh.  No, let's not make another helper.  Can we at least keep the
> >>> filtering Gavin had so we don't just allow any ioctl through here
> >>> though?  At least we can make vfio.c involved if someone what's to do
> >>> something crazy here.  Thanks,
> >>
> >>
> >> if (req != CHECK_EXTENSION) &&
> >>    (req != ENABLE) &&
> >>    (reg != EEH_OP)
> >> 	return -1;
> >>
> >> Like that (roughly)?
> > 
> > A switch statement seems a lot cleaner.
> 
> 
> Like that (#2 adds empty switch{}, #4 enables two ioctls, below is #4)?
> 
>  int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
>                           int req, void *param)
>  {
>      VFIOGroup *group;
>      VFIOContainer *container;
>      int ret = -1;
> 
>      switch (req) {
> +    case VFIO_CHECK_EXTENSION:
> +    case VFIO_IOMMU_SPAPR_TCE_GET_INFO:
> +        break;
>      default:
>          /* Return an error on unknown requests */
>          return ret;
>      }
>      group = vfio_get_group(groupid, as);
>      if (!group) {
> 

Sure, I might have chosen to move the body to a separate static
function, making this a simple wrapper to avoid awkward code flow, but
it's up to you.  Thanks,

Alex

[Qemu-devel] [PATCH v8 3/4] spapr_pci_vfio: Add spapr-pci-vfio-host-bridge to support vfio

[Alexey Kardashevskiy]

The patch adds a spapr-pci-vfio-host-bridge device type
which is a PCI Host Bridge with VFIO support. The new device
inherits from the spapr-pci-host-bridge device and adds an "iommu"
property which is an IOMMU id. This ID represents a minimal entity
for which IOMMU isolation can be guaranteed. In SPAPR architecture IOMMU
group is called a Partitionable Endpoint (PE).

Current implementation supports one IOMMU id per QEMU VFIO PHB. Since
SPAPR allows multiple PHB for no extra cost, this does not seem to
be a problem. This limitation may change in the future though.

Example of use:
Configure and Add 3 functions of a multifunctional device to QEMU:
(the NEC PCI USB card is used as an example here):
-device spapr-pci-vfio-host-bridge,id=USB,iommu=4,index=7 \
-device vfio-pci,host=4:0:1.0,addr=1.0,bus=USB,multifunction=true
-device vfio-pci,host=4:0:1.1,addr=1.1,bus=USB
-device vfio-pci,host=4:0:1.2,addr=1.2,bus=USB

where:
* index=7 is a QEMU PHB index (used as source for MMIO/MSI/IO windows
offset);
* iommu=4 is an IOMMU id which can be found in sysfs:
[aik@vpl2 ~]$ cd /sys/bus/pci/devices/0004:00:00.0/
[aik@vpl2 0004:00:00.0]$ ls -l iommu_group
lrwxrwxrwx 1 root root 0 Jun  5 12:49 iommu_group -> ../../../kernel/iommu_groups/4

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v8:
* s/vfio_container_spapr_get_info/vfio_container_ioctl/
* spapr_tce_new_table() now receives vfio_accel=true instead of
kvm_accel=false

v7:
* remove bunch of properties from VFIO PHB such as "scan", "multifunction",
"force_addr" - let management softwsare deal with it
* removed traces used in scan() (which is also removed)
* updated license
* disables in-kernel TCE table ("false" in spapr_tce_new_table())

v5:
* added handling of possible failure of spapr_vfio_new_table()

v4:
* moved IOMMU changes to separate patches
* moved spapr-pci-vfio-host-bridge to new file
---
 hw/ppc/Makefile.objs        |   3 ++
 hw/ppc/spapr_pci_vfio.c     | 102 ++++++++++++++++++++++++++++++++++++++++++++
 include/hw/pci-host/spapr.h |  11 +++++
 3 files changed, 116 insertions(+)
 create mode 100644 hw/ppc/spapr_pci_vfio.c

diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
index ea747f0..edd44d0 100644
--- a/hw/ppc/Makefile.objs
+++ b/hw/ppc/Makefile.objs
@@ -4,6 +4,9 @@ obj-y += ppc.o ppc_booke.o
 obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
 obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
 obj-$(CONFIG_PSERIES) += spapr_pci.o
+ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
+obj-y += spapr_pci_vfio.o
+endif
 # PowerPC 4xx boards
 obj-y += ppc405_boards.o ppc4xx_devs.o ppc405_uc.o ppc440_bamboo.o
 obj-y += ppc4xx_pci.o
diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
new file mode 100644
index 0000000..d3bddf2
--- /dev/null
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -0,0 +1,102 @@
+/*
+ * QEMU sPAPR PCI host for VFIO
+ *
+ * Copyright (c) 2011-2014 Alexey Kardashevskiy, IBM Corporation.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License,
+ *  or (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "hw/ppc/spapr.h"
+#include "hw/pci-host/spapr.h"
+#include "linux/vfio.h"
+#include "hw/misc/vfio.h"
+
+static Property spapr_phb_vfio_properties[] = {
+    DEFINE_PROP_INT32("iommu", sPAPRPHBVFIOState, iommugroupid, -1),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+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;
+
+    if (svphb->iommugroupid == -1) {
+        error_setg(errp, "Wrong IOMMU group ID %d", svphb->iommugroupid);
+        return;
+    }
+
+    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;
+    }
+
+    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;
+    }
+
+    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;
+    }
+
+    /* 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_reset(DeviceState *qdev)
+{
+    /* Do nothing */
+}
+
+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;
+    dc->reset = spapr_phb_vfio_reset;
+    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),
+    .class_init    = spapr_phb_vfio_class_init,
+    .class_size    = sizeof(sPAPRPHBClass),
+};
+
+static void spapr_pci_vfio_register_types(void)
+{
+    type_register_static(&spapr_phb_vfio_info);
+}
+
+type_init(spapr_pci_vfio_register_types)
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index 0934518..6808e96 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -30,10 +30,14 @@
 #define SPAPR_MSIX_MAX_DEVS 32
 
 #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) \
@@ -41,6 +45,7 @@
 
 typedef struct sPAPRPHBClass sPAPRPHBClass;
 typedef struct sPAPRPHBState sPAPRPHBState;
+typedef struct sPAPRPHBVFIOState sPAPRPHBVFIOState;
 
 struct sPAPRPHBClass {
     PCIHostBridgeClass parent_class;
@@ -76,6 +81,12 @@ struct sPAPRPHBState {
     QLIST_ENTRY(sPAPRPHBState) list;
 };
 
+struct sPAPRPHBVFIOState {
+    sPAPRPHBState phb;
+
+    int32_t iommugroupid;
+};
+
 #define SPAPR_PCI_BASE_BUID          0x800000020000000ULL
 
 #define SPAPR_PCI_WINDOW_BASE        0x10000000000ULL
-- 
2.0.0

[Qemu-devel] [PATCH v8 4/4] vfio: Enable for SPAPR

[Alexey Kardashevskiy]

This turns the sPAPR support on and enables VFIO container use
in the kernel.

This extends vfio_connect_container to support VFIO_SPAPR_TCE_IOMMU type
in the host kernel.

This registers a memory listener which sPAPR IOMMU will notify when
executing H_PUT_TCE/etc DMA calls. The listener then will notify the host
kernel about DMA map/unmap operation via VFIO_IOMMU_MAP_DMA/
VFIO_IOMMU_UNMAP_DMA ioctls.

This executes VFIO_IOMMU_ENABLE ioctl to make sure that the IOMMU is free
of mappings and can be exclusively given to the user. At the moment SPAPR
is the only platform requiring this call to be implemented.

Note that the host kernel function implementing VFIO_IOMMU_DISABLE
is called automatically when container's fd is closed so there is
no need to call it explicitly from QEMU. This may change in the future.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v8:
* added note about VFIO_IOMMU_DISABLE in the commit log

v7:
* added more details in commit log

v5:
* multiple returns converted to gotos

v4:
* fixed format string to use %m which is a glibc extension:
"Print output of strerror(errno). No argument is required."
---
 hw/misc/vfio.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
index bb77934..78d2045 100644
--- a/hw/misc/vfio.c
+++ b/hw/misc/vfio.c
@@ -3650,6 +3650,34 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
 
         container->iommu_data.type1.initialized = true;
 
+    } else if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_IOMMU)) {
+        ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd);
+        if (ret) {
+            error_report("vfio: failed to set group container: %m");
+            ret = -errno;
+            goto free_container_exit;
+        }
+
+        ret = ioctl(fd, VFIO_SET_IOMMU, VFIO_SPAPR_TCE_IOMMU);
+        if (ret) {
+            error_report("vfio: failed to set iommu for container: %m");
+            ret = -errno;
+            goto free_container_exit;
+        }
+
+        ret = ioctl(fd, VFIO_IOMMU_ENABLE);
+        if (ret) {
+            error_report("vfio: failed to enable container: %m");
+            ret = -errno;
+            goto free_container_exit;
+        }
+
+        container->iommu_data.type1.listener = vfio_memory_listener;
+        container->iommu_data.release = vfio_listener_release;
+
+        memory_listener_register(&container->iommu_data.type1.listener,
+                                 container->space->as);
+
     } else {
         error_report("vfio: No available IOMMU models");
         ret = -EINVAL;
-- 
2.0.0

Re: [Qemu-devel] [PATCH v8 4/4] vfio: Enable for SPAPR

[Alex Williamson]

On Fri, 2014-06-06 at 13:34 +1000, Alexey Kardashevskiy wrote:
> This turns the sPAPR support on and enables VFIO container use
> in the kernel.
> 
> This extends vfio_connect_container to support VFIO_SPAPR_TCE_IOMMU type
> in the host kernel.
> 
> This registers a memory listener which sPAPR IOMMU will notify when
> executing H_PUT_TCE/etc DMA calls. The listener then will notify the host
> kernel about DMA map/unmap operation via VFIO_IOMMU_MAP_DMA/
> VFIO_IOMMU_UNMAP_DMA ioctls.
> 
> This executes VFIO_IOMMU_ENABLE ioctl to make sure that the IOMMU is free
> of mappings and can be exclusively given to the user. At the moment SPAPR
> is the only platform requiring this call to be implemented.
> 
> Note that the host kernel function implementing VFIO_IOMMU_DISABLE
> is called automatically when container's fd is closed so there is
> no need to call it explicitly from QEMU. This may change in the future.

So you're saying we rely on a behavior that may change in the future...
The kernel must do cleanup, it can never rely on userspace to do the
right thing or we have a bug.  Does that mean we can remove the "may
change in the future" part of this comment or is that directed at QEMU's
behavior and not the kernel's?  A comment in the code where we setup the
release handler or call ENABLE would be a good idea too.  Thanks,

Alex

> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> Changes:
> v8:
> * added note about VFIO_IOMMU_DISABLE in the commit log
> 
> v7:
> * added more details in commit log
> 
> v5:
> * multiple returns converted to gotos
> 
> v4:
> * fixed format string to use %m which is a glibc extension:
> "Print output of strerror(errno). No argument is required."
> ---
>  hw/misc/vfio.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> index bb77934..78d2045 100644
> --- a/hw/misc/vfio.c
> +++ b/hw/misc/vfio.c
> @@ -3650,6 +3650,34 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>  
>          container->iommu_data.type1.initialized = true;
>  
> +    } else if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_IOMMU)) {
> +        ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd);
> +        if (ret) {
> +            error_report("vfio: failed to set group container: %m");
> +            ret = -errno;
> +            goto free_container_exit;
> +        }
> +
> +        ret = ioctl(fd, VFIO_SET_IOMMU, VFIO_SPAPR_TCE_IOMMU);
> +        if (ret) {
> +            error_report("vfio: failed to set iommu for container: %m");
> +            ret = -errno;
> +            goto free_container_exit;
> +        }
> +
> +        ret = ioctl(fd, VFIO_IOMMU_ENABLE);
> +        if (ret) {
> +            error_report("vfio: failed to enable container: %m");
> +            ret = -errno;
> +            goto free_container_exit;
> +        }
> +
> +        container->iommu_data.type1.listener = vfio_memory_listener;
> +        container->iommu_data.release = vfio_listener_release;
> +
> +        memory_listener_register(&container->iommu_data.type1.listener,
> +                                 container->space->as);
> +
>      } else {
>          error_report("vfio: No available IOMMU models");
>          ret = -EINVAL;

Re: [Qemu-devel] [PATCH v8 4/4] vfio: Enable for SPAPR

[Alexey Kardashevskiy]

On 06/07/2014 02:57 AM, Alex Williamson wrote:
> On Fri, 2014-06-06 at 13:34 +1000, Alexey Kardashevskiy wrote:
>> This turns the sPAPR support on and enables VFIO container use
>> in the kernel.
>>
>> This extends vfio_connect_container to support VFIO_SPAPR_TCE_IOMMU type
>> in the host kernel.
>>
>> This registers a memory listener which sPAPR IOMMU will notify when
>> executing H_PUT_TCE/etc DMA calls. The listener then will notify the host
>> kernel about DMA map/unmap operation via VFIO_IOMMU_MAP_DMA/
>> VFIO_IOMMU_UNMAP_DMA ioctls.
>>
>> This executes VFIO_IOMMU_ENABLE ioctl to make sure that the IOMMU is free
>> of mappings and can be exclusively given to the user. At the moment SPAPR
>> is the only platform requiring this call to be implemented.
>>
>> Note that the host kernel function implementing VFIO_IOMMU_DISABLE
>> is called automatically when container's fd is closed so there is
>> no need to call it explicitly from QEMU. This may change in the future.
> 
> So you're saying we rely on a behavior that may change in the future...
> The kernel must do cleanup, it can never rely on userspace to do the
> right thing or we have a bug.  Does that mean we can remove the "may
> change in the future" part of this comment or is that directed at QEMU's
> behavior and not the kernel's? 


I wanted to say that if/when we support PCI hotplug or dynamic IOMMU group
reconfiguration (we might be able to do so in POWER9 or later, do not know
details), we will call DISABLE explicitly.


> A comment in the code where we setup the
> release handler or call ENABLE would be a good idea too.  Thanks,




> 
> Alex
> 
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> Changes:
>> v8:
>> * added note about VFIO_IOMMU_DISABLE in the commit log
>>
>> v7:
>> * added more details in commit log
>>
>> v5:
>> * multiple returns converted to gotos
>>
>> v4:
>> * fixed format string to use %m which is a glibc extension:
>> "Print output of strerror(errno). No argument is required."
>> ---
>>  hw/misc/vfio.c | 28 ++++++++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>>
>> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
>> index bb77934..78d2045 100644
>> --- a/hw/misc/vfio.c
>> +++ b/hw/misc/vfio.c
>> @@ -3650,6 +3650,34 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>>  
>>          container->iommu_data.type1.initialized = true;
>>  
>> +    } else if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_IOMMU)) {
>> +        ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd);
>> +        if (ret) {
>> +            error_report("vfio: failed to set group container: %m");
>> +            ret = -errno;
>> +            goto free_container_exit;
>> +        }
>> +
>> +        ret = ioctl(fd, VFIO_SET_IOMMU, VFIO_SPAPR_TCE_IOMMU);
>> +        if (ret) {
>> +            error_report("vfio: failed to set iommu for container: %m");
>> +            ret = -errno;
>> +            goto free_container_exit;
>> +        }
>> +
>> +        ret = ioctl(fd, VFIO_IOMMU_ENABLE);
>> +        if (ret) {
>> +            error_report("vfio: failed to enable container: %m");
>> +            ret = -errno;
>> +            goto free_container_exit;
>> +        }
>> +
>> +        container->iommu_data.type1.listener = vfio_memory_listener;
>> +        container->iommu_data.release = vfio_listener_release;
>> +
>> +        memory_listener_register(&container->iommu_data.type1.listener,
>> +                                 container->space->as);
>> +
>>      } else {
>>          error_report("vfio: No available IOMMU models");
>>          ret = -EINVAL;
> 
> 
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH v8 4/4] vfio: Enable for SPAPR

[Alexey Kardashevskiy]

On 06/07/2014 08:47 AM, Alexey Kardashevskiy wrote:
> On 06/07/2014 02:57 AM, Alex Williamson wrote:
>> On Fri, 2014-06-06 at 13:34 +1000, Alexey Kardashevskiy wrote:
>>> This turns the sPAPR support on and enables VFIO container use
>>> in the kernel.
>>>
>>> This extends vfio_connect_container to support VFIO_SPAPR_TCE_IOMMU type
>>> in the host kernel.
>>>
>>> This registers a memory listener which sPAPR IOMMU will notify when
>>> executing H_PUT_TCE/etc DMA calls. The listener then will notify the host
>>> kernel about DMA map/unmap operation via VFIO_IOMMU_MAP_DMA/
>>> VFIO_IOMMU_UNMAP_DMA ioctls.
>>>
>>> This executes VFIO_IOMMU_ENABLE ioctl to make sure that the IOMMU is free
>>> of mappings and can be exclusively given to the user. At the moment SPAPR
>>> is the only platform requiring this call to be implemented.
>>>
>>> Note that the host kernel function implementing VFIO_IOMMU_DISABLE
>>> is called automatically when container's fd is closed so there is
>>> no need to call it explicitly from QEMU. This may change in the future.
>>
>> So you're saying we rely on a behavior that may change in the future...
>> The kernel must do cleanup, it can never rely on userspace to do the
>> right thing or we have a bug.  Does that mean we can remove the "may
>> change in the future" part of this comment or is that directed at QEMU's
>> behavior and not the kernel's? 
> 
> 
> I wanted to say that if/when we support PCI hotplug or dynamic IOMMU group
> reconfiguration (we might be able to do so in POWER9 or later, do not know
> details), we will call DISABLE explicitly.


Any better?
===
    Note that the host kernel function implementing VFIO_IOMMU_DISABLE
    is called automatically when container's fd is closed so there is
    no need to call it explicitly from QEMU. We may need to call
    VFIO_IOMMU_DISABLE explicitely in the future for some sort of dynamic
    reconfiguration (PCI hotplug or dynamic IOMMU group management).
===

Thanks!

> 
> 
>> A comment in the code where we setup the
>> release handler or call ENABLE would be a good idea too.  Thanks,
> 
> 
> 
> 
>>
>> Alex
>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>> Changes:
>>> v8:
>>> * added note about VFIO_IOMMU_DISABLE in the commit log
>>>
>>> v7:
>>> * added more details in commit log
>>>
>>> v5:
>>> * multiple returns converted to gotos
>>>
>>> v4:
>>> * fixed format string to use %m which is a glibc extension:
>>> "Print output of strerror(errno). No argument is required."
>>> ---
>>>  hw/misc/vfio.c | 28 ++++++++++++++++++++++++++++
>>>  1 file changed, 28 insertions(+)
>>>
>>> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
>>> index bb77934..78d2045 100644
>>> --- a/hw/misc/vfio.c
>>> +++ b/hw/misc/vfio.c
>>> @@ -3650,6 +3650,34 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>>>  
>>>          container->iommu_data.type1.initialized = true;
>>>  
>>> +    } else if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_IOMMU)) {
>>> +        ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd);
>>> +        if (ret) {
>>> +            error_report("vfio: failed to set group container: %m");
>>> +            ret = -errno;
>>> +            goto free_container_exit;
>>> +        }
>>> +
>>> +        ret = ioctl(fd, VFIO_SET_IOMMU, VFIO_SPAPR_TCE_IOMMU);
>>> +        if (ret) {
>>> +            error_report("vfio: failed to set iommu for container: %m");
>>> +            ret = -errno;
>>> +            goto free_container_exit;
>>> +        }
>>> +
>>> +        ret = ioctl(fd, VFIO_IOMMU_ENABLE);
>>> +        if (ret) {
>>> +            error_report("vfio: failed to enable container: %m");
>>> +            ret = -errno;
>>> +            goto free_container_exit;
>>> +        }
>>> +
>>> +        container->iommu_data.type1.listener = vfio_memory_listener;
>>> +        container->iommu_data.release = vfio_listener_release;
>>> +
>>> +        memory_listener_register(&container->iommu_data.type1.listener,
>>> +                                 container->space->as);
>>> +
>>>      } else {
>>>          error_report("vfio: No available IOMMU models");
>>>          ret = -EINVAL;
>>
>>
>>
> 
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH v8 4/4] vfio: Enable for SPAPR

[Alex Williamson]

On Sun, 2014-06-08 at 09:49 +1000, Alexey Kardashevskiy wrote:
> On 06/07/2014 08:47 AM, Alexey Kardashevskiy wrote:
> > On 06/07/2014 02:57 AM, Alex Williamson wrote:
> >> On Fri, 2014-06-06 at 13:34 +1000, Alexey Kardashevskiy wrote:
> >>> This turns the sPAPR support on and enables VFIO container use
> >>> in the kernel.
> >>>
> >>> This extends vfio_connect_container to support VFIO_SPAPR_TCE_IOMMU type
> >>> in the host kernel.
> >>>
> >>> This registers a memory listener which sPAPR IOMMU will notify when
> >>> executing H_PUT_TCE/etc DMA calls. The listener then will notify the host
> >>> kernel about DMA map/unmap operation via VFIO_IOMMU_MAP_DMA/
> >>> VFIO_IOMMU_UNMAP_DMA ioctls.
> >>>
> >>> This executes VFIO_IOMMU_ENABLE ioctl to make sure that the IOMMU is free
> >>> of mappings and can be exclusively given to the user. At the moment SPAPR
> >>> is the only platform requiring this call to be implemented.
> >>>
> >>> Note that the host kernel function implementing VFIO_IOMMU_DISABLE
> >>> is called automatically when container's fd is closed so there is
> >>> no need to call it explicitly from QEMU. This may change in the future.
> >>
> >> So you're saying we rely on a behavior that may change in the future...
> >> The kernel must do cleanup, it can never rely on userspace to do the
> >> right thing or we have a bug.  Does that mean we can remove the "may
> >> change in the future" part of this comment or is that directed at QEMU's
> >> behavior and not the kernel's? 
> > 
> > 
> > I wanted to say that if/when we support PCI hotplug or dynamic IOMMU group
> > reconfiguration (we might be able to do so in POWER9 or later, do not know
> > details), we will call DISABLE explicitly.
> 
> 
> Any better?
> ===
>     Note that the host kernel function implementing VFIO_IOMMU_DISABLE
>     is called automatically when container's fd is closed so there is
>     no need to call it explicitly from QEMU. We may need to call
>     VFIO_IOMMU_DISABLE explicitely in the future for some sort of dynamic
>     reconfiguration (PCI hotplug or dynamic IOMMU group management).
> ===

That sounds better.  I'd also appreciate a comment in the code where
enable is called stating that disable is implicit in closing the
container fd.  Thanks,

Alex

> > 
> >> A comment in the code where we setup the
> >> release handler or call ENABLE would be a good idea too.  Thanks,
> > 
> > 
> > 
> > 
> >>
> >> Alex
> >>
> >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>> ---
> >>> Changes:
> >>> v8:
> >>> * added note about VFIO_IOMMU_DISABLE in the commit log
> >>>
> >>> v7:
> >>> * added more details in commit log
> >>>
> >>> v5:
> >>> * multiple returns converted to gotos
> >>>
> >>> v4:
> >>> * fixed format string to use %m which is a glibc extension:
> >>> "Print output of strerror(errno). No argument is required."
> >>> ---
> >>>  hw/misc/vfio.c | 28 ++++++++++++++++++++++++++++
> >>>  1 file changed, 28 insertions(+)
> >>>
> >>> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> >>> index bb77934..78d2045 100644
> >>> --- a/hw/misc/vfio.c
> >>> +++ b/hw/misc/vfio.c
> >>> @@ -3650,6 +3650,34 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
> >>>  
> >>>          container->iommu_data.type1.initialized = true;
> >>>  
> >>> +    } else if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_SPAPR_TCE_IOMMU)) {
> >>> +        ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd);
> >>> +        if (ret) {
> >>> +            error_report("vfio: failed to set group container: %m");
> >>> +            ret = -errno;
> >>> +            goto free_container_exit;
> >>> +        }
> >>> +
> >>> +        ret = ioctl(fd, VFIO_SET_IOMMU, VFIO_SPAPR_TCE_IOMMU);
> >>> +        if (ret) {
> >>> +            error_report("vfio: failed to set iommu for container: %m");
> >>> +            ret = -errno;
> >>> +            goto free_container_exit;
> >>> +        }
> >>> +
> >>> +        ret = ioctl(fd, VFIO_IOMMU_ENABLE);
> >>> +        if (ret) {
> >>> +            error_report("vfio: failed to enable container: %m");
> >>> +            ret = -errno;
> >>> +            goto free_container_exit;
> >>> +        }
> >>> +
> >>> +        container->iommu_data.type1.listener = vfio_memory_listener;
> >>> +        container->iommu_data.release = vfio_listener_release;
> >>> +
> >>> +        memory_listener_register(&container->iommu_data.type1.listener,
> >>> +                                 container->space->as);
> >>> +
> >>>      } else {
> >>>          error_report("vfio: No available IOMMU models");
> >>>          ret = -EINVAL;
> >>
> >>
> >>
> > 
> > 
> 
>