[Qemu-devel] [RFC PATCH 00/14] Allow EEH on "normal" sPAPR PCI host bridge

[Qemu-devel] [RFC PATCH 00/14] Allow EEH on "normal" sPAPR PCI host bridge

[David Gibson]

I've already posted a patch series that allows VFIO devices on
spapr-pci-host-bridge instead of just on spapr-pci-vfio-host-bridge.
However. it still didn't allow EEH on VFIO devices on the "normal"
host bridge.

This series addresses that, making a number of cleanup along the way.

Because of limitations in both the qemu code and the kernel VFIO+EEH
interface, it only allows EEH if there is just one IOMMU group with
VFIO devices on the host bridge.  This isn't more limited than
spapr-pci-vfio-host-bridge, because it also only supported a single
VFIO group (although it didn't properly enforce that).

David Gibson (14):
  vfio: Start adding VFIO/EEH interface
  spapr_pci: Switch EEH to vfio_eeh_op() interface
  spapr_pci: Expose and generalize spapr_phb_check_vfio_group()
  spapr_pci: Fold spapr_phb_vfio_eeh_configure() into spapr_pci code
  spapr_pci: Fold spapr_phb_vfio_eeh_reset() into spapr_pci code
  spapr_pci: Fold spapr_phb_vfio_eeh_get_state() into spapr_pci code
  spapr_pci: Fold spapr_phb_vfio_eeh_set_option() into spapr_pci code
  spapr_pci: Fold spapr_phb_vfio_eeh_configure() into spapr_pci code
  vfio: Expose a VFIO PCI device's group for EEH
  spapr_pci: Track guest Partitionable Endpoints
  spapr_pci: Allow EEH on spapr-pci-host-bridge
  spapr_pci: (Mostly) remove spapr-pci-vfio-host-bridge
  spapr_pci: Remove finish_realize hook
  vfio: Eliminate vfio_container_ioctl()

 hw/ppc/spapr_pci.c          | 290 ++++++++++++++++++++++++++++++++++++--------
 hw/ppc/spapr_pci_vfio.c     | 245 +++----------------------------------
 hw/vfio/Makefile.objs       |   1 +
 hw/vfio/common.c            |  45 -------
 hw/vfio/eeh.c               |  64 ++++++++++
 hw/vfio/pci.c               |  13 ++
 include/hw/pci-host/spapr.h |  36 ++----
 include/hw/vfio/vfio-eeh.h  |  43 +++++++
 include/hw/vfio/vfio.h      |   3 -
 9 files changed, 383 insertions(+), 357 deletions(-)
 create mode 100644 hw/vfio/eeh.c
 create mode 100644 include/hw/vfio/vfio-eeh.h

-- 
2.4.3

[Qemu-devel] [RFC PATCH 01/14] vfio: Start adding VFIO/EEH interface

[David Gibson]

At present the code handling IBM's Enhanced Error Handling (EEH) interface
on VFIO devices operates by bypassing the usual VFIO logic with
vfio_container_ioctl().  That's a poorly designed interface with unclear
semantics about exactly what can be operated on.

As a first step to cleaning that up, start creating a new VFIO interface
for EEH operations.  Because EEH operates in units of a "Partitionable
Endpoint" (PE) - a group of devices that can't be mutually isolated), it
needs to expose host PEs (that is, IOMMU groups) to the guest.  This means
EEH needs VFIO concepts exposed that other VFIO users don't.

At present all EEH ioctl()s in use operate conceptually on a single PE and
take no parameters except the opcode itself.  So, expose a vfio_eeh_op()
function to apply a single no-argument operation to a VFIO group.

At present the kernel VFIO/EEH interface is broken, because it assumes
there is only one VFIO group per container, which is no longer always the
case.  So, add logic to detect this case and warn.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/vfio/Makefile.objs      |  1 +
 hw/vfio/eeh.c              | 64 ++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/vfio/vfio-eeh.h | 42 ++++++++++++++++++++++++++++++
 3 files changed, 107 insertions(+)
 create mode 100644 hw/vfio/eeh.c
 create mode 100644 include/hw/vfio/vfio-eeh.h

diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs
index d540c9d..384c702 100644
--- a/hw/vfio/Makefile.objs
+++ b/hw/vfio/Makefile.objs
@@ -3,4 +3,5 @@ obj-$(CONFIG_SOFTMMU) += common.o
 obj-$(CONFIG_PCI) += pci.o
 obj-$(CONFIG_SOFTMMU) += platform.o
 obj-$(CONFIG_SOFTMMU) += calxeda-xgmac.o
+obj-$(CONFIG_PSERIES) += eeh.o
 endif
diff --git a/hw/vfio/eeh.c b/hw/vfio/eeh.c
new file mode 100644
index 0000000..35bd06c
--- /dev/null
+++ b/hw/vfio/eeh.c
@@ -0,0 +1,64 @@
+/*
+ * EEH (IBM Enhanced Error Handling) functions for VFIO devices
+ *
+ * Copyright Red Hat, Inc. 2015
+ *
+ * Authors:
+ *  David Gibson <dgibson@redhat.com>
+ *
+ * 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/>.
+ *
+ * Based on earlier EEH implementations:
+ * Copyright (c) 2011-2014 Alexey Kardashevskiy, IBM Corporation.
+ */
+#include <sys/ioctl.h>
+#include <linux/vfio.h>
+
+#include "qemu/error-report.h"
+
+#include "hw/vfio/vfio-common.h"
+#include "hw/vfio/vfio-eeh.h"
+
+int vfio_eeh_op(VFIOGroup *group, uint32_t op)
+{
+    VFIOContainer *container = group->container;
+    struct vfio_eeh_pe_op pe_op = {
+        .argsz = sizeof(pe_op),
+        .op = op,
+    };
+    int ret;
+
+    if (!container) {
+        error_report("vfio/eeh: EEH_PE_OP 0x%x called on unattached group %d",
+                     op, group->groupid);
+        return -ENODEV;
+    }
+
+    /* A broken kernel interface means EEH operations can't work
+     * correctly if there are multiple groups in a container */
+    if ((QLIST_FIRST(&container->group_list) != group)
+        || QLIST_NEXT(group, container_next)) {
+        error_report("vfio/eeh: EEH_PE_OP 0x%x called on container"
+                     " with multiple groups", op);
+        return -ENOSPC;
+    }
+
+    ret = ioctl(container->fd, VFIO_EEH_PE_OP, &pe_op);
+    if (ret < 0) {
+        error_report("vfio/eeh: EEH_PE_OP 0x%x failed on group %d: %m",
+                     op, group->groupid);
+    }
+
+    return ret;
+}
diff --git a/include/hw/vfio/vfio-eeh.h b/include/hw/vfio/vfio-eeh.h
new file mode 100644
index 0000000..d7356f2
--- /dev/null
+++ b/include/hw/vfio/vfio-eeh.h
@@ -0,0 +1,42 @@
+/*
+ * EEH (IBM Enhanced Error Handling) functions for VFIO devices
+ *
+ * Copyright Red Hat, Inc. 2015
+ *
+ * Authors:
+ *  David Gibson <dgibson@redhat.com>
+ *
+ * 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/>.
+ *
+ * Based on earlier EEH implementations:
+ * Copyright (c) 2011-2014 Alexey Kardashevskiy, IBM Corporation.
+ */
+#ifndef VFIO_EEH_H
+#define VFIO_EEH_H
+
+#include <linux/vfio.h>
+
+/*
+ * EEH (Enhanced Error Handling) used in IBM Power guests, needs extra
+ * hooks in VFIO to correctly pass error handling operations between
+ * guest and host.
+ */
+
+/* EEH needs to be aware of VFIOGroups as a concept, though not the
+ * internals */
+typedef struct VFIOGroup VFIOGroup;
+
+int vfio_eeh_op(VFIOGroup *group, uint32_t op);
+
+#endif /* VFIO_EEH_H */
-- 
2.4.3

Re: [Qemu-devel] [RFC PATCH 01/14] vfio: Start adding VFIO/EEH interface

[Alex Williamson]

On Sat, 2015-09-19 at 17:18 +1000, David Gibson wrote:
> At present the code handling IBM's Enhanced Error Handling (EEH) interface
> on VFIO devices operates by bypassing the usual VFIO logic with
> vfio_container_ioctl().  That's a poorly designed interface with unclear
> semantics about exactly what can be operated on.
> 
> As a first step to cleaning that up, start creating a new VFIO interface
> for EEH operations.  Because EEH operates in units of a "Partitionable
> Endpoint" (PE) - a group of devices that can't be mutually isolated), it
> needs to expose host PEs (that is, IOMMU groups) to the guest.  This means
> EEH needs VFIO concepts exposed that other VFIO users don't.
> 
> At present all EEH ioctl()s in use operate conceptually on a single PE and
> take no parameters except the opcode itself.  So, expose a vfio_eeh_op()
> function to apply a single no-argument operation to a VFIO group.
> 
> At present the kernel VFIO/EEH interface is broken, because it assumes
> there is only one VFIO group per container, which is no longer always the
> case.  So, add logic to detect this case and warn.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/vfio/Makefile.objs      |  1 +
>  hw/vfio/eeh.c              | 64 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/vfio/vfio-eeh.h | 42 ++++++++++++++++++++++++++++++
>  3 files changed, 107 insertions(+)
>  create mode 100644 hw/vfio/eeh.c
>  create mode 100644 include/hw/vfio/vfio-eeh.h
> 
> diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs
> index d540c9d..384c702 100644
> --- a/hw/vfio/Makefile.objs
> +++ b/hw/vfio/Makefile.objs
> @@ -3,4 +3,5 @@ obj-$(CONFIG_SOFTMMU) += common.o
>  obj-$(CONFIG_PCI) += pci.o
>  obj-$(CONFIG_SOFTMMU) += platform.o
>  obj-$(CONFIG_SOFTMMU) += calxeda-xgmac.o
> +obj-$(CONFIG_PSERIES) += eeh.o
>  endif
> diff --git a/hw/vfio/eeh.c b/hw/vfio/eeh.c
> new file mode 100644
> index 0000000..35bd06c
> --- /dev/null
> +++ b/hw/vfio/eeh.c
> @@ -0,0 +1,64 @@
> +/*
> + * EEH (IBM Enhanced Error Handling) functions for VFIO devices
> + *
> + * Copyright Red Hat, Inc. 2015
> + *
> + * Authors:
> + *  David Gibson <dgibson@redhat.com>
> + *
> + * 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/>.
> + *
> + * Based on earlier EEH implementations:
> + * Copyright (c) 2011-2014 Alexey Kardashevskiy, IBM Corporation.
> + */
> +#include <sys/ioctl.h>
> +#include <linux/vfio.h>
> +
> +#include "qemu/error-report.h"
> +
> +#include "hw/vfio/vfio-common.h"
> +#include "hw/vfio/vfio-eeh.h"
> +
> +int vfio_eeh_op(VFIOGroup *group, uint32_t op)
> +{
> +    VFIOContainer *container = group->container;
> +    struct vfio_eeh_pe_op pe_op = {
> +        .argsz = sizeof(pe_op),
> +        .op = op,
> +    };
> +    int ret;
> +
> +    if (!container) {
> +        error_report("vfio/eeh: EEH_PE_OP 0x%x called on unattached group %d",
> +                     op, group->groupid);
> +        return -ENODEV;
> +    }
> +
> +    /* A broken kernel interface means EEH operations can't work
> +     * correctly if there are multiple groups in a container */
> +    if ((QLIST_FIRST(&container->group_list) != group)
> +        || QLIST_NEXT(group, container_next)) {
> +        error_report("vfio/eeh: EEH_PE_OP 0x%x called on container"
> +                     " with multiple groups", op);
> +        return -ENOSPC;

-EINVAL really seems more appropriate

> +    }
> +
> +    ret = ioctl(container->fd, VFIO_EEH_PE_OP, &pe_op);
> +    if (ret < 0) {
> +        error_report("vfio/eeh: EEH_PE_OP 0x%x failed on group %d: %m",
> +                     op, group->groupid);
> +    }
> +
> +    return ret;

Would -errno be more interesting in the failure case?

> +}
> diff --git a/include/hw/vfio/vfio-eeh.h b/include/hw/vfio/vfio-eeh.h
> new file mode 100644
> index 0000000..d7356f2
> --- /dev/null
> +++ b/include/hw/vfio/vfio-eeh.h
> @@ -0,0 +1,42 @@
> +/*
> + * EEH (IBM Enhanced Error Handling) functions for VFIO devices
> + *
> + * Copyright Red Hat, Inc. 2015
> + *
> + * Authors:
> + *  David Gibson <dgibson@redhat.com>
> + *
> + * 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/>.
> + *
> + * Based on earlier EEH implementations:
> + * Copyright (c) 2011-2014 Alexey Kardashevskiy, IBM Corporation.
> + */
> +#ifndef VFIO_EEH_H
> +#define VFIO_EEH_H
> +
> +#include <linux/vfio.h>
> +
> +/*
> + * EEH (Enhanced Error Handling) used in IBM Power guests, needs extra
> + * hooks in VFIO to correctly pass error handling operations between
> + * guest and host.
> + */
> +
> +/* EEH needs to be aware of VFIOGroups as a concept, though not the
> + * internals */
> +typedef struct VFIOGroup VFIOGroup;
> +
> +int vfio_eeh_op(VFIOGroup *group, uint32_t op);
> +
> +#endif /* VFIO_EEH_H */

Re: [Qemu-devel] [RFC PATCH 01/14] vfio: Start adding VFIO/EEH interface

[David Gibson]

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

On Wed, Sep 23, 2015 at 11:28:29AM -0600, Alex Williamson wrote:
> On Sat, 2015-09-19 at 17:18 +1000, David Gibson wrote:
> > At present the code handling IBM's Enhanced Error Handling (EEH) interface
> > on VFIO devices operates by bypassing the usual VFIO logic with
> > vfio_container_ioctl().  That's a poorly designed interface with unclear
> > semantics about exactly what can be operated on.
> > 
> > As a first step to cleaning that up, start creating a new VFIO interface
> > for EEH operations.  Because EEH operates in units of a "Partitionable
> > Endpoint" (PE) - a group of devices that can't be mutually isolated), it
> > needs to expose host PEs (that is, IOMMU groups) to the guest.  This means
> > EEH needs VFIO concepts exposed that other VFIO users don't.
> > 
> > At present all EEH ioctl()s in use operate conceptually on a single PE and
> > take no parameters except the opcode itself.  So, expose a vfio_eeh_op()
> > function to apply a single no-argument operation to a VFIO group.
> > 
> > At present the kernel VFIO/EEH interface is broken, because it assumes
> > there is only one VFIO group per container, which is no longer always the
> > case.  So, add logic to detect this case and warn.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/vfio/Makefile.objs      |  1 +
> >  hw/vfio/eeh.c              | 64 ++++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/vfio/vfio-eeh.h | 42 ++++++++++++++++++++++++++++++
> >  3 files changed, 107 insertions(+)
> >  create mode 100644 hw/vfio/eeh.c
> >  create mode 100644 include/hw/vfio/vfio-eeh.h
> > 
> > diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs
> > index d540c9d..384c702 100644
> > --- a/hw/vfio/Makefile.objs
> > +++ b/hw/vfio/Makefile.objs
> > @@ -3,4 +3,5 @@ obj-$(CONFIG_SOFTMMU) += common.o
> >  obj-$(CONFIG_PCI) += pci.o
> >  obj-$(CONFIG_SOFTMMU) += platform.o
> >  obj-$(CONFIG_SOFTMMU) += calxeda-xgmac.o
> > +obj-$(CONFIG_PSERIES) += eeh.o
> >  endif
> > diff --git a/hw/vfio/eeh.c b/hw/vfio/eeh.c
> > new file mode 100644
> > index 0000000..35bd06c
> > --- /dev/null
> > +++ b/hw/vfio/eeh.c
> > @@ -0,0 +1,64 @@
> > +/*
> > + * EEH (IBM Enhanced Error Handling) functions for VFIO devices
> > + *
> > + * Copyright Red Hat, Inc. 2015
> > + *
> > + * Authors:
> > + *  David Gibson <dgibson@redhat.com>
> > + *
> > + * 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/>.
> > + *
> > + * Based on earlier EEH implementations:
> > + * Copyright (c) 2011-2014 Alexey Kardashevskiy, IBM Corporation.
> > + */
> > +#include <sys/ioctl.h>
> > +#include <linux/vfio.h>
> > +
> > +#include "qemu/error-report.h"
> > +
> > +#include "hw/vfio/vfio-common.h"
> > +#include "hw/vfio/vfio-eeh.h"
> > +
> > +int vfio_eeh_op(VFIOGroup *group, uint32_t op)
> > +{
> > +    VFIOContainer *container = group->container;
> > +    struct vfio_eeh_pe_op pe_op = {
> > +        .argsz = sizeof(pe_op),
> > +        .op = op,
> > +    };
> > +    int ret;
> > +
> > +    if (!container) {
> > +        error_report("vfio/eeh: EEH_PE_OP 0x%x called on unattached group %d",
> > +                     op, group->groupid);
> > +        return -ENODEV;
> > +    }
> > +
> > +    /* A broken kernel interface means EEH operations can't work
> > +     * correctly if there are multiple groups in a container */
> > +    if ((QLIST_FIRST(&container->group_list) != group)
> > +        || QLIST_NEXT(group, container_next)) {
> > +        error_report("vfio/eeh: EEH_PE_OP 0x%x called on container"
> > +                     " with multiple groups", op);
> > +        return -ENOSPC;
> 
> -EINVAL really seems more appropriate

So, I agree that ENOSPC is a dubious choice, but EINVAL is definitely
wrong here.

Broad as it is, EINVAL should always indicate that the caller has
supplied some sort of bad parameter.  In this case the parameters are
just fine, it's just that the kernel is broken so we can't handle that
case.

Perhaps EBUSY?  Since there isn't an ESHOULDWORKBUTDOESNT or EDEVBROKEN.

> 
> > +    }
> > +
> > +    ret = ioctl(container->fd, VFIO_EEH_PE_OP, &pe_op);
> > +    if (ret < 0) {
> > +        error_report("vfio/eeh: EEH_PE_OP 0x%x failed on group %d: %m",
> > +                     op, group->groupid);
> > +    }
> > +
> > +    return ret;
> 
> Would -errno be more interesting in the failure case?

Oh, yes.  Too much kernel work, I'm used to things returning the error
code.

-- 
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] [RFC PATCH 01/14] vfio: Start adding VFIO/EEH interface

[Alex Williamson]

On Thu, 2015-09-24 at 11:11 +1000, David Gibson wrote:
> On Wed, Sep 23, 2015 at 11:28:29AM -0600, Alex Williamson wrote:
> > On Sat, 2015-09-19 at 17:18 +1000, David Gibson wrote:
> > > At present the code handling IBM's Enhanced Error Handling (EEH) interface
> > > on VFIO devices operates by bypassing the usual VFIO logic with
> > > vfio_container_ioctl().  That's a poorly designed interface with unclear
> > > semantics about exactly what can be operated on.
> > > 
> > > As a first step to cleaning that up, start creating a new VFIO interface
> > > for EEH operations.  Because EEH operates in units of a "Partitionable
> > > Endpoint" (PE) - a group of devices that can't be mutually isolated), it
> > > needs to expose host PEs (that is, IOMMU groups) to the guest.  This means
> > > EEH needs VFIO concepts exposed that other VFIO users don't.
> > > 
> > > At present all EEH ioctl()s in use operate conceptually on a single PE and
> > > take no parameters except the opcode itself.  So, expose a vfio_eeh_op()
> > > function to apply a single no-argument operation to a VFIO group.
> > > 
> > > At present the kernel VFIO/EEH interface is broken, because it assumes
> > > there is only one VFIO group per container, which is no longer always the
> > > case.  So, add logic to detect this case and warn.
> > > 
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  hw/vfio/Makefile.objs      |  1 +
> > >  hw/vfio/eeh.c              | 64 ++++++++++++++++++++++++++++++++++++++++++++++
> > >  include/hw/vfio/vfio-eeh.h | 42 ++++++++++++++++++++++++++++++
> > >  3 files changed, 107 insertions(+)
> > >  create mode 100644 hw/vfio/eeh.c
> > >  create mode 100644 include/hw/vfio/vfio-eeh.h
> > > 
> > > diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs
> > > index d540c9d..384c702 100644
> > > --- a/hw/vfio/Makefile.objs
> > > +++ b/hw/vfio/Makefile.objs
> > > @@ -3,4 +3,5 @@ obj-$(CONFIG_SOFTMMU) += common.o
> > >  obj-$(CONFIG_PCI) += pci.o
> > >  obj-$(CONFIG_SOFTMMU) += platform.o
> > >  obj-$(CONFIG_SOFTMMU) += calxeda-xgmac.o
> > > +obj-$(CONFIG_PSERIES) += eeh.o
> > >  endif
> > > diff --git a/hw/vfio/eeh.c b/hw/vfio/eeh.c
> > > new file mode 100644
> > > index 0000000..35bd06c
> > > --- /dev/null
> > > +++ b/hw/vfio/eeh.c
> > > @@ -0,0 +1,64 @@
> > > +/*
> > > + * EEH (IBM Enhanced Error Handling) functions for VFIO devices
> > > + *
> > > + * Copyright Red Hat, Inc. 2015
> > > + *
> > > + * Authors:
> > > + *  David Gibson <dgibson@redhat.com>
> > > + *
> > > + * 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/>.
> > > + *
> > > + * Based on earlier EEH implementations:
> > > + * Copyright (c) 2011-2014 Alexey Kardashevskiy, IBM Corporation.
> > > + */
> > > +#include <sys/ioctl.h>
> > > +#include <linux/vfio.h>
> > > +
> > > +#include "qemu/error-report.h"
> > > +
> > > +#include "hw/vfio/vfio-common.h"
> > > +#include "hw/vfio/vfio-eeh.h"
> > > +
> > > +int vfio_eeh_op(VFIOGroup *group, uint32_t op)
> > > +{
> > > +    VFIOContainer *container = group->container;
> > > +    struct vfio_eeh_pe_op pe_op = {
> > > +        .argsz = sizeof(pe_op),
> > > +        .op = op,
> > > +    };
> > > +    int ret;
> > > +
> > > +    if (!container) {
> > > +        error_report("vfio/eeh: EEH_PE_OP 0x%x called on unattached group %d",
> > > +                     op, group->groupid);
> > > +        return -ENODEV;
> > > +    }
> > > +
> > > +    /* A broken kernel interface means EEH operations can't work
> > > +     * correctly if there are multiple groups in a container */
> > > +    if ((QLIST_FIRST(&container->group_list) != group)
> > > +        || QLIST_NEXT(group, container_next)) {
> > > +        error_report("vfio/eeh: EEH_PE_OP 0x%x called on container"
> > > +                     " with multiple groups", op);
> > > +        return -ENOSPC;
> > 
> > -EINVAL really seems more appropriate
> 
> So, I agree that ENOSPC is a dubious choice, but EINVAL is definitely
> wrong here.
> 
> Broad as it is, EINVAL should always indicate that the caller has
> supplied some sort of bad parameter.  In this case the parameters are
> just fine, it's just that the kernel is broken so we can't handle that
> case.
> 
> Perhaps EBUSY?  Since there isn't an ESHOULDWORKBUTDOESNT or EDEVBROKEN.

The caller has supplied a bad parameter, a group that doesn't match the
existing group in the container.  If you really object, EPERM?  EACCES?

> > > +    }
> > > +
> > > +    ret = ioctl(container->fd, VFIO_EEH_PE_OP, &pe_op);
> > > +    if (ret < 0) {
> > > +        error_report("vfio/eeh: EEH_PE_OP 0x%x failed on group %d: %m",
> > > +                     op, group->groupid);
> > > +    }
> > > +
> > > +    return ret;
> > 
> > Would -errno be more interesting in the failure case?
> 
> Oh, yes.  Too much kernel work, I'm used to things returning the error
> code.
> 

Re: [Qemu-devel] [RFC PATCH 01/14] vfio: Start adding VFIO/EEH interface

[David Gibson]

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

On Wed, Sep 23, 2015 at 08:12:30PM -0600, Alex Williamson wrote:
> On Thu, 2015-09-24 at 11:11 +1000, David Gibson wrote:
> > On Wed, Sep 23, 2015 at 11:28:29AM -0600, Alex Williamson wrote:
> > > On Sat, 2015-09-19 at 17:18 +1000, David Gibson wrote:
> > > > At present the code handling IBM's Enhanced Error Handling (EEH) interface
> > > > on VFIO devices operates by bypassing the usual VFIO logic with
> > > > vfio_container_ioctl().  That's a poorly designed interface with unclear
> > > > semantics about exactly what can be operated on.
> > > > 
> > > > As a first step to cleaning that up, start creating a new VFIO interface
> > > > for EEH operations.  Because EEH operates in units of a "Partitionable
> > > > Endpoint" (PE) - a group of devices that can't be mutually isolated), it
> > > > needs to expose host PEs (that is, IOMMU groups) to the guest.  This means
> > > > EEH needs VFIO concepts exposed that other VFIO users don't.
> > > > 
> > > > At present all EEH ioctl()s in use operate conceptually on a single PE and
> > > > take no parameters except the opcode itself.  So, expose a vfio_eeh_op()
> > > > function to apply a single no-argument operation to a VFIO group.
> > > > 
> > > > At present the kernel VFIO/EEH interface is broken, because it assumes
> > > > there is only one VFIO group per container, which is no longer always the
> > > > case.  So, add logic to detect this case and warn.
> > > > 
> > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > ---
> > > >  hw/vfio/Makefile.objs      |  1 +
> > > >  hw/vfio/eeh.c              | 64 ++++++++++++++++++++++++++++++++++++++++++++++
> > > >  include/hw/vfio/vfio-eeh.h | 42 ++++++++++++++++++++++++++++++
> > > >  3 files changed, 107 insertions(+)
> > > >  create mode 100644 hw/vfio/eeh.c
> > > >  create mode 100644 include/hw/vfio/vfio-eeh.h
> > > > 
> > > > diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs
> > > > index d540c9d..384c702 100644
> > > > --- a/hw/vfio/Makefile.objs
> > > > +++ b/hw/vfio/Makefile.objs
> > > > @@ -3,4 +3,5 @@ obj-$(CONFIG_SOFTMMU) += common.o
> > > >  obj-$(CONFIG_PCI) += pci.o
> > > >  obj-$(CONFIG_SOFTMMU) += platform.o
> > > >  obj-$(CONFIG_SOFTMMU) += calxeda-xgmac.o
> > > > +obj-$(CONFIG_PSERIES) += eeh.o
> > > >  endif
> > > > diff --git a/hw/vfio/eeh.c b/hw/vfio/eeh.c
> > > > new file mode 100644
> > > > index 0000000..35bd06c
> > > > --- /dev/null
> > > > +++ b/hw/vfio/eeh.c
> > > > @@ -0,0 +1,64 @@
> > > > +/*
> > > > + * EEH (IBM Enhanced Error Handling) functions for VFIO devices
> > > > + *
> > > > + * Copyright Red Hat, Inc. 2015
> > > > + *
> > > > + * Authors:
> > > > + *  David Gibson <dgibson@redhat.com>
> > > > + *
> > > > + * 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/>.
> > > > + *
> > > > + * Based on earlier EEH implementations:
> > > > + * Copyright (c) 2011-2014 Alexey Kardashevskiy, IBM Corporation.
> > > > + */
> > > > +#include <sys/ioctl.h>
> > > > +#include <linux/vfio.h>
> > > > +
> > > > +#include "qemu/error-report.h"
> > > > +
> > > > +#include "hw/vfio/vfio-common.h"
> > > > +#include "hw/vfio/vfio-eeh.h"
> > > > +
> > > > +int vfio_eeh_op(VFIOGroup *group, uint32_t op)
> > > > +{
> > > > +    VFIOContainer *container = group->container;
> > > > +    struct vfio_eeh_pe_op pe_op = {
> > > > +        .argsz = sizeof(pe_op),
> > > > +        .op = op,
> > > > +    };
> > > > +    int ret;
> > > > +
> > > > +    if (!container) {
> > > > +        error_report("vfio/eeh: EEH_PE_OP 0x%x called on unattached group %d",
> > > > +                     op, group->groupid);
> > > > +        return -ENODEV;
> > > > +    }
> > > > +
> > > > +    /* A broken kernel interface means EEH operations can't work
> > > > +     * correctly if there are multiple groups in a container */
> > > > +    if ((QLIST_FIRST(&container->group_list) != group)
> > > > +        || QLIST_NEXT(group, container_next)) {
> > > > +        error_report("vfio/eeh: EEH_PE_OP 0x%x called on container"
> > > > +                     " with multiple groups", op);
> > > > +        return -ENOSPC;
> > > 
> > > -EINVAL really seems more appropriate
> > 
> > So, I agree that ENOSPC is a dubious choice, but EINVAL is definitely
> > wrong here.
> > 
> > Broad as it is, EINVAL should always indicate that the caller has
> > supplied some sort of bad parameter.  In this case the parameters are
> > just fine, it's just that the kernel is broken so we can't handle that
> > case.
> > 
> > Perhaps EBUSY?  Since there isn't an ESHOULDWORKBUTDOESNT or EDEVBROKEN.
> 
> The caller has supplied a bad parameter, a group that doesn't match the
> existing group in the container.  If you really object, EPERM?  EACCES?

Well, I suppose, but the parameter is only bad because the
implementation is flawed, not because the user has requested something
actually impossible.  EACCES is definitely wrong, since that should
refer to a problem with (settable) permissions.  EPERM.. I guess I can
live with, I'll go with that.

-- 
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] [RFC PATCH 01/14] vfio: Start adding VFIO/EEH interface

[Thomas Huth]

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

On 24/09/15 06:09, David Gibson wrote:
> On Wed, Sep 23, 2015 at 08:12:30PM -0600, Alex Williamson wrote:
>> On Thu, 2015-09-24 at 11:11 +1000, David Gibson wrote:
>>> On Wed, Sep 23, 2015 at 11:28:29AM -0600, Alex Williamson wrote:
>>>> On Sat, 2015-09-19 at 17:18 +1000, David Gibson wrote:
>>>>> At present the code handling IBM's Enhanced Error Handling (EEH) interface
>>>>> on VFIO devices operates by bypassing the usual VFIO logic with
>>>>> vfio_container_ioctl().  That's a poorly designed interface with unclear
>>>>> semantics about exactly what can be operated on.
>>>>>
>>>>> As a first step to cleaning that up, start creating a new VFIO interface
>>>>> for EEH operations.  Because EEH operates in units of a "Partitionable
>>>>> Endpoint" (PE) - a group of devices that can't be mutually isolated), it
>>>>> needs to expose host PEs (that is, IOMMU groups) to the guest.  This means
>>>>> EEH needs VFIO concepts exposed that other VFIO users don't.
>>>>>
>>>>> At present all EEH ioctl()s in use operate conceptually on a single PE and
>>>>> take no parameters except the opcode itself.  So, expose a vfio_eeh_op()
>>>>> function to apply a single no-argument operation to a VFIO group.
>>>>>
>>>>> At present the kernel VFIO/EEH interface is broken, because it assumes
>>>>> there is only one VFIO group per container, which is no longer always the
>>>>> case.  So, add logic to detect this case and warn.
>>>>>
>>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>>>> ---
>>>>>  hw/vfio/Makefile.objs      |  1 +
>>>>>  hw/vfio/eeh.c              | 64 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  include/hw/vfio/vfio-eeh.h | 42 ++++++++++++++++++++++++++++++
>>>>>  3 files changed, 107 insertions(+)
>>>>>  create mode 100644 hw/vfio/eeh.c
>>>>>  create mode 100644 include/hw/vfio/vfio-eeh.h
>>>>>
>>>>> diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs
>>>>> index d540c9d..384c702 100644
>>>>> --- a/hw/vfio/Makefile.objs
>>>>> +++ b/hw/vfio/Makefile.objs
>>>>> @@ -3,4 +3,5 @@ obj-$(CONFIG_SOFTMMU) += common.o
>>>>>  obj-$(CONFIG_PCI) += pci.o
>>>>>  obj-$(CONFIG_SOFTMMU) += platform.o
>>>>>  obj-$(CONFIG_SOFTMMU) += calxeda-xgmac.o
>>>>> +obj-$(CONFIG_PSERIES) += eeh.o
>>>>>  endif
>>>>> diff --git a/hw/vfio/eeh.c b/hw/vfio/eeh.c
>>>>> new file mode 100644
>>>>> index 0000000..35bd06c
>>>>> --- /dev/null
>>>>> +++ b/hw/vfio/eeh.c
>>>>> @@ -0,0 +1,64 @@
>>>>> +/*
>>>>> + * EEH (IBM Enhanced Error Handling) functions for VFIO devices
>>>>> + *
>>>>> + * Copyright Red Hat, Inc. 2015
>>>>> + *
>>>>> + * Authors:
>>>>> + *  David Gibson <dgibson@redhat.com>
>>>>> + *
>>>>> + * 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/>.
>>>>> + *
>>>>> + * Based on earlier EEH implementations:
>>>>> + * Copyright (c) 2011-2014 Alexey Kardashevskiy, IBM Corporation.
>>>>> + */
>>>>> +#include <sys/ioctl.h>
>>>>> +#include <linux/vfio.h>
>>>>> +
>>>>> +#include "qemu/error-report.h"
>>>>> +
>>>>> +#include "hw/vfio/vfio-common.h"
>>>>> +#include "hw/vfio/vfio-eeh.h"
>>>>> +
>>>>> +int vfio_eeh_op(VFIOGroup *group, uint32_t op)
>>>>> +{
>>>>> +    VFIOContainer *container = group->container;
>>>>> +    struct vfio_eeh_pe_op pe_op = {
>>>>> +        .argsz = sizeof(pe_op),
>>>>> +        .op = op,
>>>>> +    };
>>>>> +    int ret;
>>>>> +
>>>>> +    if (!container) {
>>>>> +        error_report("vfio/eeh: EEH_PE_OP 0x%x called on unattached group %d",
>>>>> +                     op, group->groupid);
>>>>> +        return -ENODEV;
>>>>> +    }
>>>>> +
>>>>> +    /* A broken kernel interface means EEH operations can't work
>>>>> +     * correctly if there are multiple groups in a container */
>>>>> +    if ((QLIST_FIRST(&container->group_list) != group)
>>>>> +        || QLIST_NEXT(group, container_next)) {
>>>>> +        error_report("vfio/eeh: EEH_PE_OP 0x%x called on container"
>>>>> +                     " with multiple groups", op);
>>>>> +        return -ENOSPC;
>>>>
>>>> -EINVAL really seems more appropriate
>>>
>>> So, I agree that ENOSPC is a dubious choice, but EINVAL is definitely
>>> wrong here.
>>>
>>> Broad as it is, EINVAL should always indicate that the caller has
>>> supplied some sort of bad parameter.  In this case the parameters are
>>> just fine, it's just that the kernel is broken so we can't handle that
>>> case.
>>>
>>> Perhaps EBUSY?  Since there isn't an ESHOULDWORKBUTDOESNT or EDEVBROKEN.
>>
>> The caller has supplied a bad parameter, a group that doesn't match the
>> existing group in the container.  If you really object, EPERM?  EACCES?
> 
> Well, I suppose, but the parameter is only bad because the
> implementation is flawed, not because the user has requested something
> actually impossible.  EACCES is definitely wrong, since that should
> refer to a problem with (settable) permissions.  EPERM.. I guess I can
> live with, I'll go with that.

What about ENOTSUP ?
According to http://www.gnu.org/software/libc/manual/html_node/Error-Codes.html :
"Not supported. A function returns this error when certain parameter values
 are valid, but the functionality they request is not available."

Thomas



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[Qemu-devel] [RFC PATCH 02/14] spapr_pci: Switch EEH to vfio_eeh_op() interface

[David Gibson]

This switches all EEH on VFIO operations in spapr_pci_vfio() from the old
broken vfio_container_ioctl() interface to the new vfio_eeh_op() interface.

In order to obtain the VFIOGroup * handle that vfio_eeh_op() requires, we
add a helper function spapr_phb_check_vfio_group().  It gets the group from
the VFIO PHB's groupid by calling into vfio_get_group().  Using that
outside the VFIO is an abstraction violation, but so was
vfio_container_ioctl() itself, and this is a useful intermediate step to
cleaning it up properly.

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

diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
index a61b418..a2b1093 100644
--- a/hw/ppc/spapr_pci_vfio.c
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -22,12 +22,27 @@
 #include "hw/pci/msix.h"
 #include "linux/vfio.h"
 #include "hw/vfio/vfio.h"
+#include "hw/vfio/vfio-eeh.h"
+
+#include "hw/vfio/vfio-common.h"
 
 static Property spapr_phb_vfio_properties[] = {
     DEFINE_PROP_INT32("iommu", sPAPRPHBVFIOState, iommugroupid, -1),
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static int spapr_phb_check_vfio_group(sPAPRPHBState *phb, VFIOGroup **gpp)
+{
+    VFIOGroup *group;
+
+    /* FIXME: this is an abstraction violation */
+    group = vfio_get_group(SPAPR_PCI_VFIO_HOST_BRIDGE(phb)->iommugroupid,
+                           &phb->iommu_as);
+    if (gpp)
+        *gpp = group;
+    return RTAS_OUT_SUCCESS;
+}
+
 static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp)
 {
     sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
@@ -74,13 +89,14 @@ static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp)
 
 static void spapr_phb_vfio_eeh_reenable(sPAPRPHBVFIOState *svphb)
 {
-    struct vfio_eeh_pe_op op = {
-        .argsz = sizeof(op),
-        .op    = VFIO_EEH_PE_ENABLE
-    };
+    VFIOGroup *group;
+    int ret;
 
-    vfio_container_ioctl(&svphb->phb.iommu_as,
-                         svphb->iommugroupid, VFIO_EEH_PE_OP, &op);
+    ret = spapr_phb_check_vfio_group(SPAPR_PCI_HOST_BRIDGE(svphb), &group);
+    if (ret != RTAS_OUT_SUCCESS) {
+        return;
+    }
+    vfio_eeh_op(group, VFIO_EEH_PE_ENABLE);
 }
 
 static void spapr_phb_vfio_reset(DeviceState *qdev)
@@ -97,13 +113,18 @@ static void spapr_phb_vfio_reset(DeviceState *qdev)
 static int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb,
                                          unsigned int addr, int option)
 {
-    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
-    struct vfio_eeh_pe_op op = { .argsz = sizeof(op) };
+    VFIOGroup *group;
+    uint32_t op;
     int ret;
 
+    ret = spapr_phb_check_vfio_group(sphb, &group);
+    if (ret != RTAS_OUT_SUCCESS) {
+        return ret;
+    }
+
     switch (option) {
     case RTAS_EEH_DISABLE:
-        op.op = VFIO_EEH_PE_DISABLE;
+        op = VFIO_EEH_PE_DISABLE;
         break;
     case RTAS_EEH_ENABLE: {
         PCIHostState *phb;
@@ -121,21 +142,20 @@ static int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb,
             return RTAS_OUT_PARAM_ERROR;
         }
 
-        op.op = VFIO_EEH_PE_ENABLE;
+        op = VFIO_EEH_PE_ENABLE;
         break;
     }
     case RTAS_EEH_THAW_IO:
-        op.op = VFIO_EEH_PE_UNFREEZE_IO;
+        op = VFIO_EEH_PE_UNFREEZE_IO;
         break;
     case RTAS_EEH_THAW_DMA:
-        op.op = VFIO_EEH_PE_UNFREEZE_DMA;
+        op = VFIO_EEH_PE_UNFREEZE_DMA;
         break;
     default:
         return RTAS_OUT_PARAM_ERROR;
     }
 
-    ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
-                               VFIO_EEH_PE_OP, &op);
+    ret = vfio_eeh_op(group, op);
     if (ret < 0) {
         return RTAS_OUT_HW_ERROR;
     }
@@ -145,13 +165,15 @@ static int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb,
 
 static int spapr_phb_vfio_eeh_get_state(sPAPRPHBState *sphb, int *state)
 {
-    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
-    struct vfio_eeh_pe_op op = { .argsz = sizeof(op) };
+    VFIOGroup *group;
     int ret;
 
-    op.op = VFIO_EEH_PE_GET_STATE;
-    ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
-                               VFIO_EEH_PE_OP, &op);
+    ret = spapr_phb_check_vfio_group(sphb, &group);
+    if (ret != RTAS_OUT_SUCCESS) {
+        return ret;
+    }
+
+    ret = vfio_eeh_op(group, VFIO_EEH_PE_GET_STATE);
     if (ret < 0) {
         return RTAS_OUT_PARAM_ERROR;
     }
@@ -205,28 +227,32 @@ static void spapr_phb_vfio_eeh_pre_reset(sPAPRPHBState *sphb)
 
 static int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, int option)
 {
-    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
-    struct vfio_eeh_pe_op op = { .argsz = sizeof(op) };
+    VFIOGroup *group;
+    uint32_t op;
     int ret;
 
+    ret = spapr_phb_check_vfio_group(sphb, &group);
+    if (ret != RTAS_OUT_SUCCESS) {
+        return ret;
+    }
+
     switch (option) {
     case RTAS_SLOT_RESET_DEACTIVATE:
-        op.op = VFIO_EEH_PE_RESET_DEACTIVATE;
+        op = VFIO_EEH_PE_RESET_DEACTIVATE;
         break;
     case RTAS_SLOT_RESET_HOT:
         spapr_phb_vfio_eeh_pre_reset(sphb);
-        op.op = VFIO_EEH_PE_RESET_HOT;
+        op = VFIO_EEH_PE_RESET_HOT;
         break;
     case RTAS_SLOT_RESET_FUNDAMENTAL:
         spapr_phb_vfio_eeh_pre_reset(sphb);
-        op.op = VFIO_EEH_PE_RESET_FUNDAMENTAL;
+        op = VFIO_EEH_PE_RESET_FUNDAMENTAL;
         break;
     default:
         return RTAS_OUT_PARAM_ERROR;
     }
 
-    ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
-                               VFIO_EEH_PE_OP, &op);
+    ret = vfio_eeh_op(group, op);
     if (ret < 0) {
         return RTAS_OUT_HW_ERROR;
     }
@@ -236,13 +262,15 @@ static int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, int option)
 
 static int spapr_phb_vfio_eeh_configure(sPAPRPHBState *sphb)
 {
-    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
-    struct vfio_eeh_pe_op op = { .argsz = sizeof(op) };
+    VFIOGroup *group;
     int ret;
 
-    op.op = VFIO_EEH_PE_CONFIGURE;
-    ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
-                               VFIO_EEH_PE_OP, &op);
+    ret = spapr_phb_check_vfio_group(sphb, &group);
+    if (ret != RTAS_OUT_SUCCESS) {
+        return ret;
+    }
+
+    ret = vfio_eeh_op(group, VFIO_EEH_PE_CONFIGURE);
     if (ret < 0) {
         return RTAS_OUT_PARAM_ERROR;
     }
-- 
2.4.3

Re: [Qemu-devel] [RFC PATCH 02/14] spapr_pci: Switch EEH to vfio_eeh_op() interface

[Alex Williamson]

On Sat, 2015-09-19 at 17:18 +1000, David Gibson wrote:
> This switches all EEH on VFIO operations in spapr_pci_vfio() from the old
> broken vfio_container_ioctl() interface to the new vfio_eeh_op() interface.
> 
> In order to obtain the VFIOGroup * handle that vfio_eeh_op() requires, we
> add a helper function spapr_phb_check_vfio_group().  It gets the group from
> the VFIO PHB's groupid by calling into vfio_get_group().  Using that
> outside the VFIO is an abstraction violation, but so was
> vfio_container_ioctl() itself, and this is a useful intermediate step to
> cleaning it up properly.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_pci_vfio.c | 90 ++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 59 insertions(+), 31 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
> index a61b418..a2b1093 100644
> --- a/hw/ppc/spapr_pci_vfio.c
> +++ b/hw/ppc/spapr_pci_vfio.c
> @@ -22,12 +22,27 @@
>  #include "hw/pci/msix.h"
>  #include "linux/vfio.h"
>  #include "hw/vfio/vfio.h"
> +#include "hw/vfio/vfio-eeh.h"
> +
> +#include "hw/vfio/vfio-common.h"
>  
>  static Property spapr_phb_vfio_properties[] = {
>      DEFINE_PROP_INT32("iommu", sPAPRPHBVFIOState, iommugroupid, -1),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +static int spapr_phb_check_vfio_group(sPAPRPHBState *phb, VFIOGroup **gpp)
> +{
> +    VFIOGroup *group;
> +
> +    /* FIXME: this is an abstraction violation */
> +    group = vfio_get_group(SPAPR_PCI_VFIO_HOST_BRIDGE(phb)->iommugroupid,
> +                           &phb->iommu_as);
> +    if (gpp)
> +        *gpp = group;
> +    return RTAS_OUT_SUCCESS;
> +}

"check" for what?  Why does this have a return value if it can't fail?
This is a rather obscure wrapper of vfio_get_group().  It also seems
like yet another abstraction violation to make such a simply function
return RTAS error codes.

> +
>  static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp)
>  {
>      sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
> @@ -74,13 +89,14 @@ static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp)
>  
>  static void spapr_phb_vfio_eeh_reenable(sPAPRPHBVFIOState *svphb)
>  {
> -    struct vfio_eeh_pe_op op = {
> -        .argsz = sizeof(op),
> -        .op    = VFIO_EEH_PE_ENABLE
> -    };
> +    VFIOGroup *group;
> +    int ret;
>  
> -    vfio_container_ioctl(&svphb->phb.iommu_as,
> -                         svphb->iommugroupid, VFIO_EEH_PE_OP, &op);
> +    ret = spapr_phb_check_vfio_group(SPAPR_PCI_HOST_BRIDGE(svphb), &group);
> +    if (ret != RTAS_OUT_SUCCESS) {
> +        return;
> +    }
> +    vfio_eeh_op(group, VFIO_EEH_PE_ENABLE);

What did we just accomplish?  Is group valid?

>  }
>  
>  static void spapr_phb_vfio_reset(DeviceState *qdev)
> @@ -97,13 +113,18 @@ static void spapr_phb_vfio_reset(DeviceState *qdev)
>  static int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb,
>                                           unsigned int addr, int option)
>  {
> -    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
> -    struct vfio_eeh_pe_op op = { .argsz = sizeof(op) };
> +    VFIOGroup *group;
> +    uint32_t op;
>      int ret;
>  
> +    ret = spapr_phb_check_vfio_group(sphb, &group);
> +    if (ret != RTAS_OUT_SUCCESS) {
> +        return ret;
> +    }
> +
>      switch (option) {
>      case RTAS_EEH_DISABLE:
> -        op.op = VFIO_EEH_PE_DISABLE;
> +        op = VFIO_EEH_PE_DISABLE;
>          break;
>      case RTAS_EEH_ENABLE: {
>          PCIHostState *phb;
> @@ -121,21 +142,20 @@ static int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb,
>              return RTAS_OUT_PARAM_ERROR;
>          }
>  
> -        op.op = VFIO_EEH_PE_ENABLE;
> +        op = VFIO_EEH_PE_ENABLE;
>          break;
>      }
>      case RTAS_EEH_THAW_IO:
> -        op.op = VFIO_EEH_PE_UNFREEZE_IO;
> +        op = VFIO_EEH_PE_UNFREEZE_IO;
>          break;
>      case RTAS_EEH_THAW_DMA:
> -        op.op = VFIO_EEH_PE_UNFREEZE_DMA;
> +        op = VFIO_EEH_PE_UNFREEZE_DMA;
>          break;
>      default:
>          return RTAS_OUT_PARAM_ERROR;
>      }
>  
> -    ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
> -                               VFIO_EEH_PE_OP, &op);
> +    ret = vfio_eeh_op(group, op);
>      if (ret < 0) {
>          return RTAS_OUT_HW_ERROR;
>      }
> @@ -145,13 +165,15 @@ static int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb,
>  
>  static int spapr_phb_vfio_eeh_get_state(sPAPRPHBState *sphb, int *state)
>  {
> -    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
> -    struct vfio_eeh_pe_op op = { .argsz = sizeof(op) };
> +    VFIOGroup *group;
>      int ret;
>  
> -    op.op = VFIO_EEH_PE_GET_STATE;
> -    ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
> -                               VFIO_EEH_PE_OP, &op);
> +    ret = spapr_phb_check_vfio_group(sphb, &group);
> +    if (ret != RTAS_OUT_SUCCESS) {
> +        return ret;
> +    }
> +
> +    ret = vfio_eeh_op(group, VFIO_EEH_PE_GET_STATE);
>      if (ret < 0) {
>          return RTAS_OUT_PARAM_ERROR;
>      }
> @@ -205,28 +227,32 @@ static void spapr_phb_vfio_eeh_pre_reset(sPAPRPHBState *sphb)
>  
>  static int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, int option)
>  {
> -    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
> -    struct vfio_eeh_pe_op op = { .argsz = sizeof(op) };
> +    VFIOGroup *group;
> +    uint32_t op;
>      int ret;
>  
> +    ret = spapr_phb_check_vfio_group(sphb, &group);
> +    if (ret != RTAS_OUT_SUCCESS) {
> +        return ret;
> +    }
> +
>      switch (option) {
>      case RTAS_SLOT_RESET_DEACTIVATE:
> -        op.op = VFIO_EEH_PE_RESET_DEACTIVATE;
> +        op = VFIO_EEH_PE_RESET_DEACTIVATE;
>          break;
>      case RTAS_SLOT_RESET_HOT:
>          spapr_phb_vfio_eeh_pre_reset(sphb);
> -        op.op = VFIO_EEH_PE_RESET_HOT;
> +        op = VFIO_EEH_PE_RESET_HOT;
>          break;
>      case RTAS_SLOT_RESET_FUNDAMENTAL:
>          spapr_phb_vfio_eeh_pre_reset(sphb);
> -        op.op = VFIO_EEH_PE_RESET_FUNDAMENTAL;
> +        op = VFIO_EEH_PE_RESET_FUNDAMENTAL;
>          break;
>      default:
>          return RTAS_OUT_PARAM_ERROR;
>      }
>  
> -    ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
> -                               VFIO_EEH_PE_OP, &op);
> +    ret = vfio_eeh_op(group, op);
>      if (ret < 0) {
>          return RTAS_OUT_HW_ERROR;
>      }
> @@ -236,13 +262,15 @@ static int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, int option)
>  
>  static int spapr_phb_vfio_eeh_configure(sPAPRPHBState *sphb)
>  {
> -    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
> -    struct vfio_eeh_pe_op op = { .argsz = sizeof(op) };
> +    VFIOGroup *group;
>      int ret;
>  
> -    op.op = VFIO_EEH_PE_CONFIGURE;
> -    ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
> -                               VFIO_EEH_PE_OP, &op);
> +    ret = spapr_phb_check_vfio_group(sphb, &group);
> +    if (ret != RTAS_OUT_SUCCESS) {
> +        return ret;
> +    }
> +
> +    ret = vfio_eeh_op(group, VFIO_EEH_PE_CONFIGURE);
>      if (ret < 0) {
>          return RTAS_OUT_PARAM_ERROR;
>      }

[Qemu-devel] [RFC PATCH 03/14] spapr_pci: Expose and generalize spapr_phb_check_vfio_group()

[David Gibson]

At the moment spapr_phb_check_vfio_group() assumes it is always passed an
instance of "spapr-pci-vfio-host-bridge", which it is.  However we want to
unify more of the code with core PAPR PCI, so this changes it to be safe on
any "spapr-pci-host-bridge" instance.  For now we just return an error if
it's not a VFIO host bridge.  We also expose the function, because we'll
be needing it elsewhere.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci_vfio.c     | 6 +++++-
 include/hw/pci-host/spapr.h | 2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
index a2b1093..047fb30 100644
--- a/hw/ppc/spapr_pci_vfio.c
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -31,10 +31,14 @@ static Property spapr_phb_vfio_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
-static int spapr_phb_check_vfio_group(sPAPRPHBState *phb, VFIOGroup **gpp)
+int spapr_phb_check_vfio_group(sPAPRPHBState *phb, VFIOGroup **gpp)
 {
     VFIOGroup *group;
 
+    if (!object_dynamic_cast(OBJECT(phb), "spapr-pci-vfio-host-bridge")) {
+        return RTAS_OUT_PARAM_ERROR;
+    }
+
     /* FIXME: this is an abstraction violation */
     group = vfio_get_group(SPAPR_PCI_VFIO_HOST_BRIDGE(phb)->iommugroupid,
                            &phb->iommu_as);
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index 7de5e02..68cf99b 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -26,6 +26,7 @@
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_host.h"
 #include "hw/ppc/xics.h"
+#include "hw/vfio/vfio-eeh.h"
 
 #define TYPE_SPAPR_PCI_HOST_BRIDGE "spapr-pci-host-bridge"
 #define TYPE_SPAPR_PCI_VFIO_HOST_BRIDGE "spapr-pci-vfio-host-bridge"
@@ -133,6 +134,7 @@ void spapr_pci_msi_init(sPAPRMachineState *spapr, hwaddr addr);
 
 void spapr_pci_rtas_init(void);
 
+int spapr_phb_check_vfio_group(sPAPRPHBState *phb, VFIOGroup **gpp);
 sPAPRPHBState *spapr_pci_find_phb(sPAPRMachineState *spapr, uint64_t buid);
 PCIDevice *spapr_pci_find_dev(sPAPRMachineState *spapr, uint64_t buid,
                               uint32_t config_addr);
-- 
2.4.3

[Qemu-devel] [RFC PATCH 04/14] spapr_pci: Fold spapr_phb_vfio_eeh_configure() into spapr_pci code

[David Gibson]

Because spapr_phb_check_vfio_group() now safely returns an error on a non
VFIO papr host bridge, it becomes safe to call
spapr_phb_vfio_eeh_configure() on any host bridge, not just the special
VFIO host bridges.

So fold its code into rtas_ibm_configure_pe(), instead of only calling it
via a class method.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci.c          | 15 ++++++++++-----
 hw/ppc/spapr_pci_vfio.c     | 19 -------------------
 include/hw/pci-host/spapr.h |  1 -
 3 files changed, 10 insertions(+), 25 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 4b7217d..2da5991 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -604,7 +604,7 @@ static void rtas_ibm_configure_pe(PowerPCCPU *cpu,
                                   target_ulong rets)
 {
     sPAPRPHBState *sphb;
-    sPAPRPHBClass *spc;
+    VFIOGroup *group;
     uint64_t buid;
     int ret;
 
@@ -618,13 +618,18 @@ static void rtas_ibm_configure_pe(PowerPCCPU *cpu,
         goto param_error_exit;
     }
 
-    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
-    if (!spc->eeh_configure) {
+    ret = spapr_phb_check_vfio_group(sphb, &group);
+    if (ret != RTAS_OUT_SUCCESS) {
+        rtas_st(rets, 0, ret);
+        return;
+    }
+
+    ret = vfio_eeh_op(group, VFIO_EEH_PE_CONFIGURE);
+    if (ret < 0) {
         goto param_error_exit;
     }
 
-    ret = spc->eeh_configure(sphb);
-    rtas_st(rets, 0, ret);
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
     return;
 
 param_error_exit:
diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
index 047fb30..a13fb0a 100644
--- a/hw/ppc/spapr_pci_vfio.c
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -264,24 +264,6 @@ static int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, int option)
     return RTAS_OUT_SUCCESS;
 }
 
-static int spapr_phb_vfio_eeh_configure(sPAPRPHBState *sphb)
-{
-    VFIOGroup *group;
-    int ret;
-
-    ret = spapr_phb_check_vfio_group(sphb, &group);
-    if (ret != RTAS_OUT_SUCCESS) {
-        return ret;
-    }
-
-    ret = vfio_eeh_op(group, VFIO_EEH_PE_CONFIGURE);
-    if (ret < 0) {
-        return RTAS_OUT_PARAM_ERROR;
-    }
-
-    return RTAS_OUT_SUCCESS;
-}
-
 static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -293,7 +275,6 @@ static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
     spc->eeh_set_option = spapr_phb_vfio_eeh_set_option;
     spc->eeh_get_state = spapr_phb_vfio_eeh_get_state;
     spc->eeh_reset = spapr_phb_vfio_eeh_reset;
-    spc->eeh_configure = spapr_phb_vfio_eeh_configure;
 }
 
 static const TypeInfo spapr_phb_vfio_info = {
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index 68cf99b..d5d98ec 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -53,7 +53,6 @@ struct sPAPRPHBClass {
     int (*eeh_set_option)(sPAPRPHBState *sphb, unsigned int addr, int option);
     int (*eeh_get_state)(sPAPRPHBState *sphb, int *state);
     int (*eeh_reset)(sPAPRPHBState *sphb, int option);
-    int (*eeh_configure)(sPAPRPHBState *sphb);
 };
 
 typedef struct spapr_pci_msi {
-- 
2.4.3

[Qemu-devel] [RFC PATCH 05/14] spapr_pci: Fold spapr_phb_vfio_eeh_reset() into spapr_pci code

[David Gibson]

Because spapr_phb_check_vfio_group() now safely returns an error on a non
VFIO papr host bridge, it becomes safe to call
spapr_phb_vfio_eeh_reset() on any host bridge, not just the special
VFIO host bridges.

So fold its code into rtas_ibm_set_slot_reset(), instead of only calling it
via a class method.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci.c          | 76 ++++++++++++++++++++++++++++++++++++++++---
 hw/ppc/spapr_pci_vfio.c     | 79 ---------------------------------------------
 include/hw/pci-host/spapr.h |  1 -
 3 files changed, 71 insertions(+), 85 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 2da5991..30a7468 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -561,6 +561,48 @@ param_error_exit:
     rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
 }
 
+static void spapr_phb_eeh_clear_dev_msix(PCIBus *bus, PCIDevice *pdev,
+                                         void *opaque)
+{
+    /* Check if the device is VFIO PCI device */
+    if (!object_dynamic_cast(OBJECT(pdev), "vfio-pci")) {
+        return;
+    }
+
+    /*
+     * The MSIx table will be cleaned out by reset. We need
+     * disable it so that it can be reenabled properly. Also,
+     * the cached MSIx table should be cleared as it's not
+     * reflecting the contents in hardware.
+     */
+    if (msix_enabled(pdev)) {
+        uint16_t flags;
+
+        flags = pci_host_config_read_common(pdev,
+                                            pdev->msix_cap + PCI_MSIX_FLAGS,
+                                            pci_config_size(pdev), 2);
+        flags &= ~PCI_MSIX_FLAGS_ENABLE;
+        pci_host_config_write_common(pdev,
+                                     pdev->msix_cap + PCI_MSIX_FLAGS,
+                                     pci_config_size(pdev), flags, 2);
+    }
+
+    msix_reset(pdev);
+}
+
+static void spapr_phb_eeh_clear_bus_msix(PCIBus *bus, void *opaque)
+{
+       pci_for_each_device(bus, pci_bus_num(bus),
+                           spapr_phb_eeh_clear_dev_msix, NULL);
+}
+
+static void spapr_phb_eeh_pre_reset(sPAPRPHBState *sphb)
+{
+       PCIHostState *phb = PCI_HOST_BRIDGE(sphb);
+
+       pci_for_each_bus(phb->bus, spapr_phb_eeh_clear_bus_msix, NULL);
+}
+
 static void rtas_ibm_set_slot_reset(PowerPCCPU *cpu,
                                     sPAPRMachineState *spapr,
                                     uint32_t token, uint32_t nargs,
@@ -568,9 +610,10 @@ static void rtas_ibm_set_slot_reset(PowerPCCPU *cpu,
                                     target_ulong rets)
 {
     sPAPRPHBState *sphb;
-    sPAPRPHBClass *spc;
+    VFIOGroup *group;
     uint32_t option;
     uint64_t buid;
+    uint32_t op;
     int ret;
 
     if ((nargs != 4) || (nret != 1)) {
@@ -584,13 +627,36 @@ static void rtas_ibm_set_slot_reset(PowerPCCPU *cpu,
         goto param_error_exit;
     }
 
-    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
-    if (!spc->eeh_reset) {
+
+    ret = spapr_phb_check_vfio_group(sphb, &group);
+    if (ret != RTAS_OUT_SUCCESS) {
+        rtas_st(rets, 0, ret);
+        return;
+    }
+
+    switch (option) {
+    case RTAS_SLOT_RESET_DEACTIVATE:
+        op = VFIO_EEH_PE_RESET_DEACTIVATE;
+        break;
+    case RTAS_SLOT_RESET_HOT:
+        spapr_phb_eeh_pre_reset(sphb);
+        op = VFIO_EEH_PE_RESET_HOT;
+        break;
+    case RTAS_SLOT_RESET_FUNDAMENTAL:
+        spapr_phb_eeh_pre_reset(sphb);
+        op = VFIO_EEH_PE_RESET_FUNDAMENTAL;
+        break;
+    default:
         goto param_error_exit;
     }
 
-    ret = spc->eeh_reset(sphb, option);
-    rtas_st(rets, 0, ret);
+    ret = vfio_eeh_op(group, op);
+    if (ret < 0) {
+        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
+        return;
+    }
+
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
     return;
 
 param_error_exit:
diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
index a13fb0a..84186fa 100644
--- a/hw/ppc/spapr_pci_vfio.c
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -186,84 +186,6 @@ static int spapr_phb_vfio_eeh_get_state(sPAPRPHBState *sphb, int *state)
     return RTAS_OUT_SUCCESS;
 }
 
-static void spapr_phb_vfio_eeh_clear_dev_msix(PCIBus *bus,
-                                              PCIDevice *pdev,
-                                              void *opaque)
-{
-    /* Check if the device is VFIO PCI device */
-    if (!object_dynamic_cast(OBJECT(pdev), "vfio-pci")) {
-        return;
-    }
-
-    /*
-     * The MSIx table will be cleaned out by reset. We need
-     * disable it so that it can be reenabled properly. Also,
-     * the cached MSIx table should be cleared as it's not
-     * reflecting the contents in hardware.
-     */
-    if (msix_enabled(pdev)) {
-        uint16_t flags;
-
-        flags = pci_host_config_read_common(pdev,
-                                            pdev->msix_cap + PCI_MSIX_FLAGS,
-                                            pci_config_size(pdev), 2);
-        flags &= ~PCI_MSIX_FLAGS_ENABLE;
-        pci_host_config_write_common(pdev,
-                                     pdev->msix_cap + PCI_MSIX_FLAGS,
-                                     pci_config_size(pdev), flags, 2);
-    }
-
-    msix_reset(pdev);
-}
-
-static void spapr_phb_vfio_eeh_clear_bus_msix(PCIBus *bus, void *opaque)
-{
-       pci_for_each_device(bus, pci_bus_num(bus),
-                           spapr_phb_vfio_eeh_clear_dev_msix, NULL);
-}
-
-static void spapr_phb_vfio_eeh_pre_reset(sPAPRPHBState *sphb)
-{
-       PCIHostState *phb = PCI_HOST_BRIDGE(sphb);
-
-       pci_for_each_bus(phb->bus, spapr_phb_vfio_eeh_clear_bus_msix, NULL);
-}
-
-static int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, int option)
-{
-    VFIOGroup *group;
-    uint32_t op;
-    int ret;
-
-    ret = spapr_phb_check_vfio_group(sphb, &group);
-    if (ret != RTAS_OUT_SUCCESS) {
-        return ret;
-    }
-
-    switch (option) {
-    case RTAS_SLOT_RESET_DEACTIVATE:
-        op = VFIO_EEH_PE_RESET_DEACTIVATE;
-        break;
-    case RTAS_SLOT_RESET_HOT:
-        spapr_phb_vfio_eeh_pre_reset(sphb);
-        op = VFIO_EEH_PE_RESET_HOT;
-        break;
-    case RTAS_SLOT_RESET_FUNDAMENTAL:
-        spapr_phb_vfio_eeh_pre_reset(sphb);
-        op = VFIO_EEH_PE_RESET_FUNDAMENTAL;
-        break;
-    default:
-        return RTAS_OUT_PARAM_ERROR;
-    }
-
-    ret = vfio_eeh_op(group, op);
-    if (ret < 0) {
-        return RTAS_OUT_HW_ERROR;
-    }
-
-    return RTAS_OUT_SUCCESS;
-}
-
 static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -274,7 +196,6 @@ static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
     spc->finish_realize = spapr_phb_vfio_finish_realize;
     spc->eeh_set_option = spapr_phb_vfio_eeh_set_option;
     spc->eeh_get_state = spapr_phb_vfio_eeh_get_state;
-    spc->eeh_reset = spapr_phb_vfio_eeh_reset;
 }
 
 static const TypeInfo spapr_phb_vfio_info = {
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index d5d98ec..f0b9fc3 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -52,7 +52,6 @@ struct sPAPRPHBClass {
     void (*finish_realize)(sPAPRPHBState *sphb, Error **errp);
     int (*eeh_set_option)(sPAPRPHBState *sphb, unsigned int addr, int option);
     int (*eeh_get_state)(sPAPRPHBState *sphb, int *state);
-    int (*eeh_reset)(sPAPRPHBState *sphb, int option);
 };
 
 typedef struct spapr_pci_msi {
-- 
2.4.3

[Qemu-devel] [RFC PATCH 06/14] spapr_pci: Fold spapr_phb_vfio_eeh_get_state() into spapr_pci code

[David Gibson]

Because spapr_phb_check_vfio_group() now safely returns an error on a non
VFIO papr host bridge, it becomes safe to call
spapr_phb_vfio_eeh_get_state() on any host bridge, not just the special
VFIO host bridges.

So fold its code into rtas_ibm_read_slot_reset_state2(), instead of only
calling it via a class method.

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

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 30a7468..4624b90 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -524,9 +524,9 @@ static void rtas_ibm_read_slot_reset_state2(PowerPCCPU *cpu,
                                             target_ulong rets)
 {
     sPAPRPHBState *sphb;
-    sPAPRPHBClass *spc;
+    VFIOGroup *group;
     uint64_t buid;
-    int state, ret;
+    int ret;
 
     if ((nargs != 3) || (nret != 4 && nret != 5)) {
         goto param_error_exit;
@@ -538,18 +538,20 @@ static void rtas_ibm_read_slot_reset_state2(PowerPCCPU *cpu,
         goto param_error_exit;
     }
 
-    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
-    if (!spc->eeh_get_state) {
-        goto param_error_exit;
+    ret = spapr_phb_check_vfio_group(sphb, &group);
+    if (ret != RTAS_OUT_SUCCESS) {
+        rtas_st(rets, 0, ret);
+        return;
     }
 
-    ret = spc->eeh_get_state(sphb, &state);
-    rtas_st(rets, 0, ret);
-    if (ret != RTAS_OUT_SUCCESS) {
+    ret = vfio_eeh_op(group, VFIO_EEH_PE_GET_STATE);
+    if (ret < 0) {
+        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
         return;
     }
 
-    rtas_st(rets, 1, state);
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+    rtas_st(rets, 1, ret);
     rtas_st(rets, 2, RTAS_EEH_SUPPORT);
     rtas_st(rets, 3, RTAS_EEH_PE_UNAVAIL_INFO);
     if (nret >= 5) {
diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
index 84186fa..03b7a10 100644
--- a/hw/ppc/spapr_pci_vfio.c
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -167,25 +167,6 @@ static int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb,
     return RTAS_OUT_SUCCESS;
 }
 
-static int spapr_phb_vfio_eeh_get_state(sPAPRPHBState *sphb, int *state)
-{
-    VFIOGroup *group;
-    int ret;
-
-    ret = spapr_phb_check_vfio_group(sphb, &group);
-    if (ret != RTAS_OUT_SUCCESS) {
-        return ret;
-    }
-
-    ret = vfio_eeh_op(group, VFIO_EEH_PE_GET_STATE);
-    if (ret < 0) {
-        return RTAS_OUT_PARAM_ERROR;
-    }
-
-    *state = ret;
-    return RTAS_OUT_SUCCESS;
-}
-
 static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -195,7 +176,6 @@ static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
     dc->reset = spapr_phb_vfio_reset;
     spc->finish_realize = spapr_phb_vfio_finish_realize;
     spc->eeh_set_option = spapr_phb_vfio_eeh_set_option;
-    spc->eeh_get_state = spapr_phb_vfio_eeh_get_state;
 }
 
 static const TypeInfo spapr_phb_vfio_info = {
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index f0b9fc3..f0b749d 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -51,7 +51,6 @@ struct sPAPRPHBClass {
 
     void (*finish_realize)(sPAPRPHBState *sphb, Error **errp);
     int (*eeh_set_option)(sPAPRPHBState *sphb, unsigned int addr, int option);
-    int (*eeh_get_state)(sPAPRPHBState *sphb, int *state);
 };
 
 typedef struct spapr_pci_msi {
-- 
2.4.3

[Qemu-devel] [RFC PATCH 07/14] spapr_pci: Fold spapr_phb_vfio_eeh_set_option() into spapr_pci code

[David Gibson]

Because spapr_phb_check_vfio_group() now safely returns an error on a non
VFIO papr host bridge, it becomes safe to call
spapr_phb_vfio_eeh_set_option() on any host bridge, not just the special
VFIO host bridges.

So fold its code into rtas_ibm_set_eeh_option(), instead of only calling it
via a class method.

The presence of the eeh_set_option class method was also used to switch
behaviour in rtas_ibm_get_config_addr_info2() and
rtas_ibm_slot_error_detail().  We alter those to directly check if
spapr_phb_check_vfio_group() returns an error.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci.c          | 68 ++++++++++++++++++++++++++++++++++++---------
 hw/ppc/spapr_pci_vfio.c     | 54 -----------------------------------
 include/hw/pci-host/spapr.h |  1 -
 3 files changed, 55 insertions(+), 68 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 4624b90..446770c 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -430,9 +430,10 @@ static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
                                     target_ulong rets)
 {
     sPAPRPHBState *sphb;
-    sPAPRPHBClass *spc;
+    VFIOGroup *group;
     uint32_t addr, option;
     uint64_t buid;
+    uint32_t op;
     int ret;
 
     if ((nargs != 4) || (nret != 1)) {
@@ -448,13 +449,52 @@ static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
         goto param_error_exit;
     }
 
-    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
-    if (!spc->eeh_set_option) {
+    ret = spapr_phb_check_vfio_group(sphb, &group);
+    if (ret != RTAS_OUT_SUCCESS) {
+        rtas_st(rets, 0, ret);
+        return;
+    }
+
+    switch (option) {
+    case RTAS_EEH_DISABLE:
+        op = VFIO_EEH_PE_DISABLE;
+        break;
+    case RTAS_EEH_ENABLE: {
+        PCIHostState *phb;
+        PCIDevice *pdev;
+
+        /*
+         * The EEH functionality is enabled on basis of PCI device,
+         * instead of PE. We need check the validity of the PCI
+         * device address.
+         */
+        phb = PCI_HOST_BRIDGE(sphb);
+        pdev = pci_find_device(phb->bus,
+                               (addr >> 16) & 0xFF, (addr >> 8) & 0xFF);
+        if (!pdev || !object_dynamic_cast(OBJECT(pdev), "vfio-pci")) {
+            goto param_error_exit;
+        }
+
+        op = VFIO_EEH_PE_ENABLE;
+        break;
+    }
+    case RTAS_EEH_THAW_IO:
+        op = VFIO_EEH_PE_UNFREEZE_IO;
+        break;
+    case RTAS_EEH_THAW_DMA:
+        op = VFIO_EEH_PE_UNFREEZE_DMA;
+        break;
+    default:
         goto param_error_exit;
     }
 
-    ret = spc->eeh_set_option(sphb, addr, option);
-    rtas_st(rets, 0, ret);
+    ret = vfio_eeh_op(group, op);
+    if (ret < 0) {
+        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
+        return;
+    }
+
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
     return;
 
 param_error_exit:
@@ -468,10 +508,10 @@ static void rtas_ibm_get_config_addr_info2(PowerPCCPU *cpu,
                                            target_ulong rets)
 {
     sPAPRPHBState *sphb;
-    sPAPRPHBClass *spc;
     PCIDevice *pdev;
     uint32_t addr, option;
     uint64_t buid;
+    int ret;
 
     if ((nargs != 4) || (nret != 2)) {
         goto param_error_exit;
@@ -483,9 +523,10 @@ static void rtas_ibm_get_config_addr_info2(PowerPCCPU *cpu,
         goto param_error_exit;
     }
 
-    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
-    if (!spc->eeh_set_option) {
-        goto param_error_exit;
+    ret = spapr_phb_check_vfio_group(sphb, NULL);
+    if (ret != RTAS_OUT_SUCCESS) {
+        rtas_st(rets, 0, ret);
+        return;
     }
 
     /*
@@ -712,9 +753,9 @@ static void rtas_ibm_slot_error_detail(PowerPCCPU *cpu,
                                        target_ulong rets)
 {
     sPAPRPHBState *sphb;
-    sPAPRPHBClass *spc;
     int option;
     uint64_t buid;
+    int ret;
 
     if ((nargs != 8) || (nret != 1)) {
         goto param_error_exit;
@@ -726,9 +767,10 @@ static void rtas_ibm_slot_error_detail(PowerPCCPU *cpu,
         goto param_error_exit;
     }
 
-    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
-    if (!spc->eeh_set_option) {
-        goto param_error_exit;
+    ret = spapr_phb_check_vfio_group(sphb, NULL);
+    if (ret != RTAS_OUT_SUCCESS) {
+        rtas_st(rets, 0, ret);
+        return;
     }
 
     option = rtas_ld(args, 7);
diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
index 03b7a10..0c34283 100644
--- a/hw/ppc/spapr_pci_vfio.c
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -114,59 +114,6 @@ static void spapr_phb_vfio_reset(DeviceState *qdev)
     spapr_phb_vfio_eeh_reenable(SPAPR_PCI_VFIO_HOST_BRIDGE(qdev));
 }
 
-static int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb,
-                                         unsigned int addr, int option)
-{
-    VFIOGroup *group;
-    uint32_t op;
-    int ret;
-
-    ret = spapr_phb_check_vfio_group(sphb, &group);
-    if (ret != RTAS_OUT_SUCCESS) {
-        return ret;
-    }
-
-    switch (option) {
-    case RTAS_EEH_DISABLE:
-        op = VFIO_EEH_PE_DISABLE;
-        break;
-    case RTAS_EEH_ENABLE: {
-        PCIHostState *phb;
-        PCIDevice *pdev;
-
-        /*
-         * The EEH functionality is enabled on basis of PCI device,
-         * instead of PE. We need check the validity of the PCI
-         * device address.
-         */
-        phb = PCI_HOST_BRIDGE(sphb);
-        pdev = pci_find_device(phb->bus,
-                               (addr >> 16) & 0xFF, (addr >> 8) & 0xFF);
-        if (!pdev || !object_dynamic_cast(OBJECT(pdev), "vfio-pci")) {
-            return RTAS_OUT_PARAM_ERROR;
-        }
-
-        op = VFIO_EEH_PE_ENABLE;
-        break;
-    }
-    case RTAS_EEH_THAW_IO:
-        op = VFIO_EEH_PE_UNFREEZE_IO;
-        break;
-    case RTAS_EEH_THAW_DMA:
-        op = VFIO_EEH_PE_UNFREEZE_DMA;
-        break;
-    default:
-        return RTAS_OUT_PARAM_ERROR;
-    }
-
-    ret = vfio_eeh_op(group, op);
-    if (ret < 0) {
-        return RTAS_OUT_HW_ERROR;
-    }
-
-    return RTAS_OUT_SUCCESS;
-}
-
 static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -175,7 +122,6 @@ static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
     dc->props = spapr_phb_vfio_properties;
     dc->reset = spapr_phb_vfio_reset;
     spc->finish_realize = spapr_phb_vfio_finish_realize;
-    spc->eeh_set_option = spapr_phb_vfio_eeh_set_option;
 }
 
 static const TypeInfo spapr_phb_vfio_info = {
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index f0b749d..741fe65 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -50,7 +50,6 @@ struct sPAPRPHBClass {
     PCIHostBridgeClass parent_class;
 
     void (*finish_realize)(sPAPRPHBState *sphb, Error **errp);
-    int (*eeh_set_option)(sPAPRPHBState *sphb, unsigned int addr, int option);
 };
 
 typedef struct spapr_pci_msi {
-- 
2.4.3

[Qemu-devel] [RFC PATCH 08/14] spapr_pci: Fold spapr_phb_vfio_eeh_configure() into spapr_pci code

[David Gibson]

Because spapr_phb_check_vfio_group() now safely returns an error on a non
VFIO papr host bridge, it becomes safe to call
spapr_phb_vfio_eeh_reenable() on any host bridge, not just the special
VFIO host bridges.  Thus the same is true of spapr_phb_vfio_reset() which
does nothing but call spapr_phb_vfio_eeh_reenable().

So, we can call spapr_phb_vfio_eeh_reenable() from the normal PAPR host
bridge reset hook, instead of overriding the hook in
spapr-pci-vfio-host-bridge to only call it there.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci.c      | 20 ++++++++++++++++++++
 hw/ppc/spapr_pci_vfio.c | 24 ------------------------
 2 files changed, 20 insertions(+), 24 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 446770c..3a1ebea 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1533,10 +1533,30 @@ static int spapr_phb_children_reset(Object *child, void *opaque)
     return 0;
 }
 
+static void spapr_phb_eeh_reenable(sPAPRPHBState *phb)
+{
+    VFIOGroup *group;
+    int ret;
+
+    ret = spapr_phb_check_vfio_group(phb, &group);
+    if (ret != RTAS_OUT_SUCCESS) {
+        return;
+    }
+    vfio_eeh_op(group, VFIO_EEH_PE_ENABLE);
+}
+
 static void spapr_phb_reset(DeviceState *qdev)
 {
     /* Reset the IOMMU state */
     object_child_foreach(OBJECT(qdev), spapr_phb_children_reset, NULL);
+
+    /*
+     * The PE might be in frozen state. To reenable the EEH
+     * functionality on it will clean the frozen state, which
+     * ensures that the contained PCI devices will work properly
+     * after reboot.
+     */
+    spapr_phb_eeh_reenable(SPAPR_PCI_HOST_BRIDGE(qdev));
 }
 
 static Property spapr_phb_properties[] = {
diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
index 0c34283..b61923c 100644
--- a/hw/ppc/spapr_pci_vfio.c
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -91,36 +91,12 @@ static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp)
                                 spapr_tce_get_iommu(tcet));
 }
 
-static void spapr_phb_vfio_eeh_reenable(sPAPRPHBVFIOState *svphb)
-{
-    VFIOGroup *group;
-    int ret;
-
-    ret = spapr_phb_check_vfio_group(SPAPR_PCI_HOST_BRIDGE(svphb), &group);
-    if (ret != RTAS_OUT_SUCCESS) {
-        return;
-    }
-    vfio_eeh_op(group, VFIO_EEH_PE_ENABLE);
-}
-
-static void spapr_phb_vfio_reset(DeviceState *qdev)
-{
-    /*
-     * The PE might be in frozen state. To reenable the EEH
-     * functionality on it will clean the frozen state, which
-     * ensures that the contained PCI devices will work properly
-     * after reboot.
-     */
-    spapr_phb_vfio_eeh_reenable(SPAPR_PCI_VFIO_HOST_BRIDGE(qdev));
-}
-
 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;
 }
 
-- 
2.4.3

[Qemu-devel] [RFC PATCH 09/14] vfio: Expose a VFIO PCI device's group for EEH

[David Gibson]

The Enhanced Error Handling (EEH) interface in PAPR operates on units of a
Partitionable Endpoint (PE).  For VFIO devices, the PE boundaries the guest
sees must match the PE (i.e. IOMMU group) boundaries on the host.  To
implement this VFIO needs to expose to EEH the IOMMU group each VFIO device
belongs to.

Add a vfio_pci_device_group() function to the VFIO/EEH interface for this
purpose.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/vfio/pci.c              | 13 +++++++++++++
 include/hw/vfio/vfio-eeh.h |  1 +
 2 files changed, 14 insertions(+)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 73d34b9..29f9467 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -41,6 +41,7 @@
 #include "trace.h"
 #include "hw/vfio/vfio.h"
 #include "hw/vfio/vfio-common.h"
+#include "hw/vfio/vfio-eeh.h"
 
 struct VFIOPCIDevice;
 
@@ -3351,6 +3352,18 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
     vdev->req_enabled = false;
 }
 
+VFIOGroup *vfio_pci_device_group(PCIDevice *pdev)
+{
+    VFIOPCIDevice *vdev;
+
+    if (!object_dynamic_cast(OBJECT(pdev), "vfio-pci")) {
+        return NULL;
+    }
+
+    vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
+    return vdev->vbasedev.group;
+}
+
 /*
  * AMD Radeon PCI config reset, based on Linux:
  *   drivers/gpu/drm/radeon/ci_smc.c:ci_is_smc_running()
diff --git a/include/hw/vfio/vfio-eeh.h b/include/hw/vfio/vfio-eeh.h
index d7356f2..0ea87e1 100644
--- a/include/hw/vfio/vfio-eeh.h
+++ b/include/hw/vfio/vfio-eeh.h
@@ -38,5 +38,6 @@
 typedef struct VFIOGroup VFIOGroup;
 
 int vfio_eeh_op(VFIOGroup *group, uint32_t op);
+VFIOGroup *vfio_pci_device_group(PCIDevice *pdev);
 
 #endif /* VFIO_EEH_H */
-- 
2.4.3

Re: [Qemu-devel] [RFC PATCH 09/14] vfio: Expose a VFIO PCI device's group for EEH

[Alex Williamson]

On Sat, 2015-09-19 at 17:18 +1000, David Gibson wrote:
> The Enhanced Error Handling (EEH) interface in PAPR operates on units of a
> Partitionable Endpoint (PE).  For VFIO devices, the PE boundaries the guest
> sees must match the PE (i.e. IOMMU group) boundaries on the host.  To
> implement this VFIO needs to expose to EEH the IOMMU group each VFIO device
> belongs to.
> 
> Add a vfio_pci_device_group() function to the VFIO/EEH interface for this
> purpose.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/vfio/pci.c              | 13 +++++++++++++
>  include/hw/vfio/vfio-eeh.h |  1 +
>  2 files changed, 14 insertions(+)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 73d34b9..29f9467 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -41,6 +41,7 @@
>  #include "trace.h"
>  #include "hw/vfio/vfio.h"
>  #include "hw/vfio/vfio-common.h"
> +#include "hw/vfio/vfio-eeh.h"


Why?  EEH may be the consumer, but it's not EEH specific, it should not
have the prototype in an EEH specific header.

>  
>  struct VFIOPCIDevice;
>  
> @@ -3351,6 +3352,18 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
>      vdev->req_enabled = false;
>  }
>  
> +VFIOGroup *vfio_pci_device_group(PCIDevice *pdev)
> +{
> +    VFIOPCIDevice *vdev;
> +
> +    if (!object_dynamic_cast(OBJECT(pdev), "vfio-pci")) {
> +        return NULL;
> +    }
> +
> +    vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> +    return vdev->vbasedev.group;
> +}
> +
>  /*
>   * AMD Radeon PCI config reset, based on Linux:
>   *   drivers/gpu/drm/radeon/ci_smc.c:ci_is_smc_running()
> diff --git a/include/hw/vfio/vfio-eeh.h b/include/hw/vfio/vfio-eeh.h
> index d7356f2..0ea87e1 100644
> --- a/include/hw/vfio/vfio-eeh.h
> +++ b/include/hw/vfio/vfio-eeh.h
> @@ -38,5 +38,6 @@
>  typedef struct VFIOGroup VFIOGroup;
>  
>  int vfio_eeh_op(VFIOGroup *group, uint32_t op);
> +VFIOGroup *vfio_pci_device_group(PCIDevice *pdev);
>  
>  #endif /* VFIO_EEH_H */

Re: [Qemu-devel] [RFC PATCH 09/14] vfio: Expose a VFIO PCI device's group for EEH

[David Gibson]

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

On Wed, Sep 23, 2015 at 11:28:43AM -0600, Alex Williamson wrote:
> On Sat, 2015-09-19 at 17:18 +1000, David Gibson wrote:
> > The Enhanced Error Handling (EEH) interface in PAPR operates on units of a
> > Partitionable Endpoint (PE).  For VFIO devices, the PE boundaries the guest
> > sees must match the PE (i.e. IOMMU group) boundaries on the host.  To
> > implement this VFIO needs to expose to EEH the IOMMU group each VFIO device
> > belongs to.
> > 
> > Add a vfio_pci_device_group() function to the VFIO/EEH interface for this
> > purpose.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/vfio/pci.c              | 13 +++++++++++++
> >  include/hw/vfio/vfio-eeh.h |  1 +
> >  2 files changed, 14 insertions(+)
> > 
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index 73d34b9..29f9467 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -41,6 +41,7 @@
> >  #include "trace.h"
> >  #include "hw/vfio/vfio.h"
> >  #include "hw/vfio/vfio-common.h"
> > +#include "hw/vfio/vfio-eeh.h"
> 
> 
> Why?  EEH may be the consumer, but it's not EEH specific, it should not
> have the prototype in an EEH specific header.

Ok, planning to create a vfio-pci.h in the respin.

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

[Qemu-devel] [RFC PATCH 10/14] spapr_pci: Track guest Partitionable Endpoints

[David Gibson]

PAPR has a concept of Partitionable Endpoints (PEs) on PCI, which are
groups of devices that can't be isolated from each other, which is used in
the Enhanced Error Handling (EEH) interface.

At the moment, we don't model PEs in our PAPR host bridge implementation.
We get away with it because spapr-pci-host-bridge doesn't support the EEH
interfaces, and spapr-pci-vfio-host-bridge, which does, only supports
devices from a single IOMMU group, and therefore a single PE (although
that's not actually properly enforced).

In order to generalize this, this adds tracking of guest PEs to the PAPR
host bridge code.  For now, at least, we only construct PEs for VFIO
devices, since we don't implement EEH for emulated devices anyway
PEs.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci.c          | 39 +++++++++++++++++++++++++++++++++++++++
 include/hw/pci-host/spapr.h |  8 ++++++++
 2 files changed, 47 insertions(+)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 3a1ebea..81ad3ae 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -423,6 +423,29 @@ static void rtas_ibm_query_interrupt_source_number(PowerPCCPU *cpu,
     rtas_st(rets, 2, 1);/* 0 == level; 1 == edge */
 }
 
+static sPAPRPHBGuestPE *spapr_phb_pe_by_device(sPAPRPHBState *phb,
+                                               PCIDevice *pdev)
+{
+    VFIOGroup *group = vfio_pci_device_group(pdev);
+    sPAPRPHBGuestPE *pe;
+
+    if (!group) {
+        return NULL;
+    }
+
+    QLIST_FOREACH(pe, &phb->pe_list, list) {
+        if (pe->group == group) {
+            /* We already have a PE for this group */
+            return pe;
+        }
+    }
+
+    pe = g_malloc0(sizeof(*pe));
+    pe->group = group;
+    QLIST_INSERT_HEAD(&phb->pe_list, pe, list);
+    return pe;
+}
+
 static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
                                     sPAPRMachineState *spapr,
                                     uint32_t token, uint32_t nargs,
@@ -1200,8 +1223,14 @@ static void spapr_phb_add_pci_device(sPAPRDRConnector *drc,
 
     if (object_dynamic_cast(OBJECT(pdev), "vfio-pci")) {
         sPAPRTCETable *tcet = spapr_tce_find_by_liobn(phb->dma_liobn);
+        sPAPRPHBGuestPE *pe;
 
         spapr_tce_need_vfio(tcet);
+
+        pe = spapr_phb_pe_by_device(phb, pdev);
+        if (pe) {
+            pe->num_devices++;
+        }
     }
 
     if (dev->hotplugged) {
@@ -1243,6 +1272,14 @@ static void spapr_phb_remove_pci_device(sPAPRDRConnector *drc,
                                         Error **errp)
 {
     sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
+    sPAPRPHBGuestPE *pe =spapr_phb_pe_by_device(phb, pdev);
+
+    if (pe) {
+        if (--pe->num_devices == 0) {
+            QLIST_REMOVE(pe, list);
+            g_free(pe);
+        }
+    }
 
     drck->detach(drc, DEVICE(pdev), spapr_phb_remove_pci_device_cb, phb, errp);
 }
@@ -1501,6 +1538,8 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
     info->finish_realize(sphb, errp);
 
     sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, g_free);
+
+    QLIST_INIT(&sphb->pe_list);
 }
 
 static void spapr_phb_finish_realize(sPAPRPHBState *sphb, Error **errp)
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index 741fe65..535e5ef 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -62,6 +62,12 @@ typedef struct spapr_pci_msi_mig {
     spapr_pci_msi value;
 } spapr_pci_msi_mig;
 
+typedef struct sPAPRPHBGuestPE {
+    int num_devices;
+    VFIOGroup *group;
+    QLIST_ENTRY(sPAPRPHBGuestPE) list;
+} sPAPRPHBGuestPE;
+
 struct sPAPRPHBState {
     PCIHostState parent_obj;
 
@@ -88,6 +94,8 @@ struct sPAPRPHBState {
     int32_t msi_devs_num;
     spapr_pci_msi_mig *msi_devs;
 
+    QLIST_HEAD(, sPAPRPHBGuestPE) pe_list;
+
     QLIST_ENTRY(sPAPRPHBState) list;
 };
 
-- 
2.4.3

[Qemu-devel] [RFC PATCH 11/14] spapr_pci: Allow EEH on spapr-pci-host-bridge

[David Gibson]

The pseries machine type has two variants of the PCI Host Bridge device:
spapr-pci-host-bridge and spapr-pci-vfio-host-bridge.  Originally, only the
latter could support VFIO devices, but we've now extended the VFIO code so
that they both can.

However, it's still only spapr-pci-vfio-host-bridge which supports the PAPR
Enhanced Error Handling (EEH) interfaces.  The reason is that we don't yet
have a way to determine the correct VFIOGroup for EEH operations on the
"plain" host bridge.

Handling this in general is problematic due to some limitations in the
current code, and some bugs in the kernel EEH interfaces.  However, it's
easy enough to do for the unambiguous case: where there is only one VFIO
group used on the whole host bridge i.e. one Partitionable Endpoint (PE).

This re-implements spapr_phb_check_vfio_group() in terms of the host
bridge's Partitionable Endpoints, allowing EEH on any host bridge in the
unambiguous case.  This is enough to make spapr-pci-host-bridge support
EEH as well as spapr-pci-vfio-host-bridge, since the latter only supported
devices from one IOMMU group anyway (although it didn't properly
enforce that).

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci.c          | 36 ++++++++++++++++++++++++++++++++++++
 hw/ppc/spapr_pci_vfio.c     | 18 ------------------
 include/hw/pci-host/spapr.h |  1 -
 3 files changed, 36 insertions(+), 19 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 81ad3ae..5614b45 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -41,6 +41,8 @@
 #include "hw/ppc/spapr_drc.h"
 #include "sysemu/device_tree.h"
 
+#include "hw/vfio/vfio-common.h"
+
 /* Copied from the kernel arch/powerpc/platforms/pseries/msi.c */
 #define RTAS_QUERY_FN           0
 #define RTAS_CHANGE_FN          1
@@ -446,6 +448,40 @@ static sPAPRPHBGuestPE *spapr_phb_pe_by_device(sPAPRPHBState *phb,
     return pe;
 }
 
+static int spapr_phb_check_vfio_group(sPAPRPHBState *phb, VFIOGroup **gpp)
+{
+    sPAPRPHBGuestPE *pe;
+
+    if (QLIST_EMPTY(&phb->pe_list)) {
+        /* No EEH capable devices on this PHB */
+        return RTAS_OUT_PARAM_ERROR;
+    }
+
+    /* Limitations in both qemu and the kernel mean that, for now, EEH
+     * won't work if there are devices from more than one PE
+     * (i.e. IOMMU group) on the same PHB */
+    pe = QLIST_FIRST(&phb->pe_list);
+    if (QLIST_NEXT(pe, list)) {
+        error_report("spapr-pci-host-bridge: EEH attempted on PHB with multiple"
+                     " IOMMU groups");
+        return RTAS_OUT_HW_ERROR;
+    }
+
+    if (!object_dynamic_cast(OBJECT(phb), "spapr-pci-vfio-host-bridge")) {
+        sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(phb);
+        /* FIXME: this is an abstraction violation */
+        if (pe->group->groupid != svphb->iommugroupid) {
+            error_report("spapr-pci-vfio-host-bridge: Bad IOMMU group");
+            return RTAS_OUT_HW_ERROR;
+        }
+    }
+
+    if (gpp) {
+        *gpp = pe->group;
+    }
+    return RTAS_OUT_SUCCESS;
+}
+
 static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
                                     sPAPRMachineState *spapr,
                                     uint32_t token, uint32_t nargs,
diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
index b61923c..a08292a 100644
--- a/hw/ppc/spapr_pci_vfio.c
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -24,29 +24,11 @@
 #include "hw/vfio/vfio.h"
 #include "hw/vfio/vfio-eeh.h"
 
-#include "hw/vfio/vfio-common.h"
-
 static Property spapr_phb_vfio_properties[] = {
     DEFINE_PROP_INT32("iommu", sPAPRPHBVFIOState, iommugroupid, -1),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-int spapr_phb_check_vfio_group(sPAPRPHBState *phb, VFIOGroup **gpp)
-{
-    VFIOGroup *group;
-
-    if (!object_dynamic_cast(OBJECT(phb), "spapr-pci-vfio-host-bridge")) {
-        return RTAS_OUT_PARAM_ERROR;
-    }
-
-    /* FIXME: this is an abstraction violation */
-    group = vfio_get_group(SPAPR_PCI_VFIO_HOST_BRIDGE(phb)->iommugroupid,
-                           &phb->iommu_as);
-    if (gpp)
-        *gpp = group;
-    return RTAS_OUT_SUCCESS;
-}
-
 static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp)
 {
     sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index 535e5ef..8c8d187 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -138,7 +138,6 @@ void spapr_pci_msi_init(sPAPRMachineState *spapr, hwaddr addr);
 
 void spapr_pci_rtas_init(void);
 
-int spapr_phb_check_vfio_group(sPAPRPHBState *phb, VFIOGroup **gpp);
 sPAPRPHBState *spapr_pci_find_phb(sPAPRMachineState *spapr, uint64_t buid);
 PCIDevice *spapr_pci_find_dev(sPAPRMachineState *spapr, uint64_t buid,
                               uint32_t config_addr);
-- 
2.4.3

Re: [Qemu-devel] [RFC PATCH 11/14] spapr_pci: Allow EEH on spapr-pci-host-bridge

[Alex Williamson]

On Sat, 2015-09-19 at 17:18 +1000, David Gibson wrote:
> The pseries machine type has two variants of the PCI Host Bridge device:
> spapr-pci-host-bridge and spapr-pci-vfio-host-bridge.  Originally, only the
> latter could support VFIO devices, but we've now extended the VFIO code so
> that they both can.
> 
> However, it's still only spapr-pci-vfio-host-bridge which supports the PAPR
> Enhanced Error Handling (EEH) interfaces.  The reason is that we don't yet
> have a way to determine the correct VFIOGroup for EEH operations on the
> "plain" host bridge.
> 
> Handling this in general is problematic due to some limitations in the
> current code, and some bugs in the kernel EEH interfaces.  However, it's
> easy enough to do for the unambiguous case: where there is only one VFIO
> group used on the whole host bridge i.e. one Partitionable Endpoint (PE).
> 
> This re-implements spapr_phb_check_vfio_group() in terms of the host
> bridge's Partitionable Endpoints, allowing EEH on any host bridge in the
> unambiguous case.  This is enough to make spapr-pci-host-bridge support
> EEH as well as spapr-pci-vfio-host-bridge, since the latter only supported
> devices from one IOMMU group anyway (although it didn't properly
> enforce that).
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_pci.c          | 36 ++++++++++++++++++++++++++++++++++++
>  hw/ppc/spapr_pci_vfio.c     | 18 ------------------
>  include/hw/pci-host/spapr.h |  1 -
>  3 files changed, 36 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 81ad3ae..5614b45 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -41,6 +41,8 @@
>  #include "hw/ppc/spapr_drc.h"
>  #include "sysemu/device_tree.h"
>  
> +#include "hw/vfio/vfio-common.h"
> +
>  /* Copied from the kernel arch/powerpc/platforms/pseries/msi.c */
>  #define RTAS_QUERY_FN           0
>  #define RTAS_CHANGE_FN          1
> @@ -446,6 +448,40 @@ static sPAPRPHBGuestPE *spapr_phb_pe_by_device(sPAPRPHBState *phb,
>      return pe;
>  }
>  
> +static int spapr_phb_check_vfio_group(sPAPRPHBState *phb, VFIOGroup **gpp)
> +{
> +    sPAPRPHBGuestPE *pe;
> +
> +    if (QLIST_EMPTY(&phb->pe_list)) {
> +        /* No EEH capable devices on this PHB */
> +        return RTAS_OUT_PARAM_ERROR;
> +    }
> +
> +    /* Limitations in both qemu and the kernel mean that, for now, EEH
> +     * won't work if there are devices from more than one PE
> +     * (i.e. IOMMU group) on the same PHB */
> +    pe = QLIST_FIRST(&phb->pe_list);
> +    if (QLIST_NEXT(pe, list)) {
> +        error_report("spapr-pci-host-bridge: EEH attempted on PHB with multiple"
> +                     " IOMMU groups");

Don't wrap lines with search-able strings

> +        return RTAS_OUT_HW_ERROR;
> +    }
> +
> +    if (!object_dynamic_cast(OBJECT(phb), "spapr-pci-vfio-host-bridge")) {
> +        sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(phb);
> +        /* FIXME: this is an abstraction violation */
> +        if (pe->group->groupid != svphb->iommugroupid) {
> +            error_report("spapr-pci-vfio-host-bridge: Bad IOMMU group");
> +            return RTAS_OUT_HW_ERROR;
> +        }
> +    }
> +
> +    if (gpp) {
> +        *gpp = pe->group;
> +    }
> +    return RTAS_OUT_SUCCESS;


The structure of this function finally makes some sense once we get
here.  I'm still not sure whether RTAS error codes make sense though,
but it's just a nit.  I'd still prefer the function was renamed, "check"
indicates a verification, not a return.  The primary purpose of calling
this function is to get a group, not simply to check the validity.

> +}
> +
>  static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
>                                      sPAPRMachineState *spapr,
>                                      uint32_t token, uint32_t nargs,
> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
> index b61923c..a08292a 100644
> --- a/hw/ppc/spapr_pci_vfio.c
> +++ b/hw/ppc/spapr_pci_vfio.c
> @@ -24,29 +24,11 @@
>  #include "hw/vfio/vfio.h"
>  #include "hw/vfio/vfio-eeh.h"
>  
> -#include "hw/vfio/vfio-common.h"
> -
>  static Property spapr_phb_vfio_properties[] = {
>      DEFINE_PROP_INT32("iommu", sPAPRPHBVFIOState, iommugroupid, -1),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> -int spapr_phb_check_vfio_group(sPAPRPHBState *phb, VFIOGroup **gpp)
> -{
> -    VFIOGroup *group;
> -
> -    if (!object_dynamic_cast(OBJECT(phb), "spapr-pci-vfio-host-bridge")) {
> -        return RTAS_OUT_PARAM_ERROR;
> -    }
> -
> -    /* FIXME: this is an abstraction violation */
> -    group = vfio_get_group(SPAPR_PCI_VFIO_HOST_BRIDGE(phb)->iommugroupid,
> -                           &phb->iommu_as);
> -    if (gpp)
> -        *gpp = group;
> -    return RTAS_OUT_SUCCESS;
> -}
> -
>  static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp)
>  {
>      sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> index 535e5ef..8c8d187 100644
> --- a/include/hw/pci-host/spapr.h
> +++ b/include/hw/pci-host/spapr.h
> @@ -138,7 +138,6 @@ void spapr_pci_msi_init(sPAPRMachineState *spapr, hwaddr addr);
>  
>  void spapr_pci_rtas_init(void);
>  
> -int spapr_phb_check_vfio_group(sPAPRPHBState *phb, VFIOGroup **gpp);
>  sPAPRPHBState *spapr_pci_find_phb(sPAPRMachineState *spapr, uint64_t buid);
>  PCIDevice *spapr_pci_find_dev(sPAPRMachineState *spapr, uint64_t buid,
>                                uint32_t config_addr);

Re: [Qemu-devel] [RFC PATCH 11/14] spapr_pci: Allow EEH on spapr-pci-host-bridge

[David Gibson]

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

On Wed, Sep 23, 2015 at 11:28:01AM -0600, Alex Williamson wrote:
> On Sat, 2015-09-19 at 17:18 +1000, David Gibson wrote:
> > The pseries machine type has two variants of the PCI Host Bridge device:
> > spapr-pci-host-bridge and spapr-pci-vfio-host-bridge.  Originally, only the
> > latter could support VFIO devices, but we've now extended the VFIO code so
> > that they both can.
> > 
> > However, it's still only spapr-pci-vfio-host-bridge which supports the PAPR
> > Enhanced Error Handling (EEH) interfaces.  The reason is that we don't yet
> > have a way to determine the correct VFIOGroup for EEH operations on the
> > "plain" host bridge.
> > 
> > Handling this in general is problematic due to some limitations in the
> > current code, and some bugs in the kernel EEH interfaces.  However, it's
> > easy enough to do for the unambiguous case: where there is only one VFIO
> > group used on the whole host bridge i.e. one Partitionable Endpoint (PE).
> > 
> > This re-implements spapr_phb_check_vfio_group() in terms of the host
> > bridge's Partitionable Endpoints, allowing EEH on any host bridge in the
> > unambiguous case.  This is enough to make spapr-pci-host-bridge support
> > EEH as well as spapr-pci-vfio-host-bridge, since the latter only supported
> > devices from one IOMMU group anyway (although it didn't properly
> > enforce that).
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr_pci.c          | 36 ++++++++++++++++++++++++++++++++++++
> >  hw/ppc/spapr_pci_vfio.c     | 18 ------------------
> >  include/hw/pci-host/spapr.h |  1 -
> >  3 files changed, 36 insertions(+), 19 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 81ad3ae..5614b45 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -41,6 +41,8 @@
> >  #include "hw/ppc/spapr_drc.h"
> >  #include "sysemu/device_tree.h"
> >  
> > +#include "hw/vfio/vfio-common.h"
> > +
> >  /* Copied from the kernel arch/powerpc/platforms/pseries/msi.c */
> >  #define RTAS_QUERY_FN           0
> >  #define RTAS_CHANGE_FN          1
> > @@ -446,6 +448,40 @@ static sPAPRPHBGuestPE *spapr_phb_pe_by_device(sPAPRPHBState *phb,
> >      return pe;
> >  }
> >  
> > +static int spapr_phb_check_vfio_group(sPAPRPHBState *phb, VFIOGroup **gpp)
> > +{
> > +    sPAPRPHBGuestPE *pe;
> > +
> > +    if (QLIST_EMPTY(&phb->pe_list)) {
> > +        /* No EEH capable devices on this PHB */
> > +        return RTAS_OUT_PARAM_ERROR;
> > +    }
> > +
> > +    /* Limitations in both qemu and the kernel mean that, for now, EEH
> > +     * won't work if there are devices from more than one PE
> > +     * (i.e. IOMMU group) on the same PHB */
> > +    pe = QLIST_FIRST(&phb->pe_list);
> > +    if (QLIST_NEXT(pe, list)) {
> > +        error_report("spapr-pci-host-bridge: EEH attempted on PHB with multiple"
> > +                     " IOMMU groups");
> 
> Don't wrap lines with search-able strings

Noted.

> > +        return RTAS_OUT_HW_ERROR;
> > +    }
> > +
> > +    if (!object_dynamic_cast(OBJECT(phb), "spapr-pci-vfio-host-bridge")) {
> > +        sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(phb);
> > +        /* FIXME: this is an abstraction violation */
> > +        if (pe->group->groupid != svphb->iommugroupid) {
> > +            error_report("spapr-pci-vfio-host-bridge: Bad IOMMU group");
> > +            return RTAS_OUT_HW_ERROR;
> > +        }
> > +    }
> > +
> > +    if (gpp) {
> > +        *gpp = pe->group;
> > +    }
> > +    return RTAS_OUT_SUCCESS;
> 
> 
> The structure of this function finally makes some sense once we get
> here.

Yeah, I started off with something more like what you suggested in
earlier comments, but I changed to this rather awkward structure to
avoid churning the callers as this moves towards its final form

> I'm still not sure whether RTAS error codes make sense though,
> but it's just a nit.

Well, (nearly) all the callers are in RTAS, and translating the error
codes there seems pretty ugly too.

> I'd still prefer the function was renamed, "check"
> indicates a verification, not a return.  The primary purpose of calling
> this function is to get a group, not simply to check the validity.

Yeah, it's a crap name.  How about spapr_phb_find_vfio_group().

Fwiw, I plan to make this function go away to as I rework things to
remove the one-group-per-PHB restriction.

> > +}
> > +
> >  static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
> >                                      sPAPRMachineState *spapr,
> >                                      uint32_t token, uint32_t nargs,
> > diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
> > index b61923c..a08292a 100644
> > --- a/hw/ppc/spapr_pci_vfio.c
> > +++ b/hw/ppc/spapr_pci_vfio.c
> > @@ -24,29 +24,11 @@
> >  #include "hw/vfio/vfio.h"
> >  #include "hw/vfio/vfio-eeh.h"
> >  
> > -#include "hw/vfio/vfio-common.h"
> > -
> >  static Property spapr_phb_vfio_properties[] = {
> >      DEFINE_PROP_INT32("iommu", sPAPRPHBVFIOState, iommugroupid, -1),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >  
> > -int spapr_phb_check_vfio_group(sPAPRPHBState *phb, VFIOGroup **gpp)
> > -{
> > -    VFIOGroup *group;
> > -
> > -    if (!object_dynamic_cast(OBJECT(phb), "spapr-pci-vfio-host-bridge")) {
> > -        return RTAS_OUT_PARAM_ERROR;
> > -    }
> > -
> > -    /* FIXME: this is an abstraction violation */
> > -    group = vfio_get_group(SPAPR_PCI_VFIO_HOST_BRIDGE(phb)->iommugroupid,
> > -                           &phb->iommu_as);
> > -    if (gpp)
> > -        *gpp = group;
> > -    return RTAS_OUT_SUCCESS;
> > -}
> > -
> >  static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp)
> >  {
> >      sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
> > diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> > index 535e5ef..8c8d187 100644
> > --- a/include/hw/pci-host/spapr.h
> > +++ b/include/hw/pci-host/spapr.h
> > @@ -138,7 +138,6 @@ void spapr_pci_msi_init(sPAPRMachineState *spapr, hwaddr addr);
> >  
> >  void spapr_pci_rtas_init(void);
> >  
> > -int spapr_phb_check_vfio_group(sPAPRPHBState *phb, VFIOGroup **gpp);
> >  sPAPRPHBState *spapr_pci_find_phb(sPAPRMachineState *spapr, uint64_t buid);
> >  PCIDevice *spapr_pci_find_dev(sPAPRMachineState *spapr, uint64_t buid,
> >                                uint32_t config_addr);
> 
> 
> 

-- 
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] [RFC PATCH 11/14] spapr_pci: Allow EEH on spapr-pci-host-bridge

[Alex Williamson]

On Thu, 2015-09-24 at 11:49 +1000, David Gibson wrote:
> On Wed, Sep 23, 2015 at 11:28:01AM -0600, Alex Williamson wrote:
> > On Sat, 2015-09-19 at 17:18 +1000, David Gibson wrote:
> > > The pseries machine type has two variants of the PCI Host Bridge device:
> > > spapr-pci-host-bridge and spapr-pci-vfio-host-bridge.  Originally, only the
> > > latter could support VFIO devices, but we've now extended the VFIO code so
> > > that they both can.
> > > 
> > > However, it's still only spapr-pci-vfio-host-bridge which supports the PAPR
> > > Enhanced Error Handling (EEH) interfaces.  The reason is that we don't yet
> > > have a way to determine the correct VFIOGroup for EEH operations on the
> > > "plain" host bridge.
> > > 
> > > Handling this in general is problematic due to some limitations in the
> > > current code, and some bugs in the kernel EEH interfaces.  However, it's
> > > easy enough to do for the unambiguous case: where there is only one VFIO
> > > group used on the whole host bridge i.e. one Partitionable Endpoint (PE).
> > > 
> > > This re-implements spapr_phb_check_vfio_group() in terms of the host
> > > bridge's Partitionable Endpoints, allowing EEH on any host bridge in the
> > > unambiguous case.  This is enough to make spapr-pci-host-bridge support
> > > EEH as well as spapr-pci-vfio-host-bridge, since the latter only supported
> > > devices from one IOMMU group anyway (although it didn't properly
> > > enforce that).
> > > 
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  hw/ppc/spapr_pci.c          | 36 ++++++++++++++++++++++++++++++++++++
> > >  hw/ppc/spapr_pci_vfio.c     | 18 ------------------
> > >  include/hw/pci-host/spapr.h |  1 -
> > >  3 files changed, 36 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > > index 81ad3ae..5614b45 100644
> > > --- a/hw/ppc/spapr_pci.c
> > > +++ b/hw/ppc/spapr_pci.c
> > > @@ -41,6 +41,8 @@
> > >  #include "hw/ppc/spapr_drc.h"
> > >  #include "sysemu/device_tree.h"
> > >  
> > > +#include "hw/vfio/vfio-common.h"
> > > +
> > >  /* Copied from the kernel arch/powerpc/platforms/pseries/msi.c */
> > >  #define RTAS_QUERY_FN           0
> > >  #define RTAS_CHANGE_FN          1
> > > @@ -446,6 +448,40 @@ static sPAPRPHBGuestPE *spapr_phb_pe_by_device(sPAPRPHBState *phb,
> > >      return pe;
> > >  }
> > >  
> > > +static int spapr_phb_check_vfio_group(sPAPRPHBState *phb, VFIOGroup **gpp)
> > > +{
> > > +    sPAPRPHBGuestPE *pe;
> > > +
> > > +    if (QLIST_EMPTY(&phb->pe_list)) {
> > > +        /* No EEH capable devices on this PHB */
> > > +        return RTAS_OUT_PARAM_ERROR;
> > > +    }
> > > +
> > > +    /* Limitations in both qemu and the kernel mean that, for now, EEH
> > > +     * won't work if there are devices from more than one PE
> > > +     * (i.e. IOMMU group) on the same PHB */
> > > +    pe = QLIST_FIRST(&phb->pe_list);
> > > +    if (QLIST_NEXT(pe, list)) {
> > > +        error_report("spapr-pci-host-bridge: EEH attempted on PHB with multiple"
> > > +                     " IOMMU groups");
> > 
> > Don't wrap lines with search-able strings
> 
> Noted.
> 
> > > +        return RTAS_OUT_HW_ERROR;
> > > +    }
> > > +
> > > +    if (!object_dynamic_cast(OBJECT(phb), "spapr-pci-vfio-host-bridge")) {
> > > +        sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(phb);
> > > +        /* FIXME: this is an abstraction violation */
> > > +        if (pe->group->groupid != svphb->iommugroupid) {
> > > +            error_report("spapr-pci-vfio-host-bridge: Bad IOMMU group");
> > > +            return RTAS_OUT_HW_ERROR;
> > > +        }
> > > +    }
> > > +
> > > +    if (gpp) {
> > > +        *gpp = pe->group;
> > > +    }
> > > +    return RTAS_OUT_SUCCESS;
> > 
> > 
> > The structure of this function finally makes some sense once we get
> > here.
> 
> Yeah, I started off with something more like what you suggested in
> earlier comments, but I changed to this rather awkward structure to
> avoid churning the callers as this moves towards its final form
> 
> > I'm still not sure whether RTAS error codes make sense though,
> > but it's just a nit.
> 
> Well, (nearly) all the callers are in RTAS, and translating the error
> codes there seems pretty ugly too.
> 
> > I'd still prefer the function was renamed, "check"
> > indicates a verification, not a return.  The primary purpose of calling
> > this function is to get a group, not simply to check the validity.
> 
> Yeah, it's a crap name.  How about spapr_phb_find_vfio_group().

Why not just spapr_phb_get_vfio_group() since this started out as just a
wrapper for vfio_get_group() and that's still what it's doing even
though it's now cached on sPAPRPHBGuestPE rather than retrieved each
time.

> Fwiw, I plan to make this function go away to as I rework things to
> remove the one-group-per-PHB restriction.

I figured, thanks for taking on the challenge of cleaning up this
layering.

Alex

> > > +}
> > > +
> > >  static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
> > >                                      sPAPRMachineState *spapr,
> > >                                      uint32_t token, uint32_t nargs,
> > > diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
> > > index b61923c..a08292a 100644
> > > --- a/hw/ppc/spapr_pci_vfio.c
> > > +++ b/hw/ppc/spapr_pci_vfio.c
> > > @@ -24,29 +24,11 @@
> > >  #include "hw/vfio/vfio.h"
> > >  #include "hw/vfio/vfio-eeh.h"
> > >  
> > > -#include "hw/vfio/vfio-common.h"
> > > -
> > >  static Property spapr_phb_vfio_properties[] = {
> > >      DEFINE_PROP_INT32("iommu", sPAPRPHBVFIOState, iommugroupid, -1),
> > >      DEFINE_PROP_END_OF_LIST(),
> > >  };
> > >  
> > > -int spapr_phb_check_vfio_group(sPAPRPHBState *phb, VFIOGroup **gpp)
> > > -{
> > > -    VFIOGroup *group;
> > > -
> > > -    if (!object_dynamic_cast(OBJECT(phb), "spapr-pci-vfio-host-bridge")) {
> > > -        return RTAS_OUT_PARAM_ERROR;
> > > -    }
> > > -
> > > -    /* FIXME: this is an abstraction violation */
> > > -    group = vfio_get_group(SPAPR_PCI_VFIO_HOST_BRIDGE(phb)->iommugroupid,
> > > -                           &phb->iommu_as);
> > > -    if (gpp)
> > > -        *gpp = group;
> > > -    return RTAS_OUT_SUCCESS;
> > > -}
> > > -
> > >  static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp)
> > >  {
> > >      sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
> > > diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> > > index 535e5ef..8c8d187 100644
> > > --- a/include/hw/pci-host/spapr.h
> > > +++ b/include/hw/pci-host/spapr.h
> > > @@ -138,7 +138,6 @@ void spapr_pci_msi_init(sPAPRMachineState *spapr, hwaddr addr);
> > >  
> > >  void spapr_pci_rtas_init(void);
> > >  
> > > -int spapr_phb_check_vfio_group(sPAPRPHBState *phb, VFIOGroup **gpp);
> > >  sPAPRPHBState *spapr_pci_find_phb(sPAPRMachineState *spapr, uint64_t buid);
> > >  PCIDevice *spapr_pci_find_dev(sPAPRMachineState *spapr, uint64_t buid,
> > >                                uint32_t config_addr);
> > 
> > 
> > 
> 

Re: [Qemu-devel] [RFC PATCH 11/14] spapr_pci: Allow EEH on spapr-pci-host-bridge

[David Gibson]

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

On Wed, Sep 23, 2015 at 08:19:36PM -0600, Alex Williamson wrote:
> On Thu, 2015-09-24 at 11:49 +1000, David Gibson wrote:
> > On Wed, Sep 23, 2015 at 11:28:01AM -0600, Alex Williamson wrote:
> > > On Sat, 2015-09-19 at 17:18 +1000, David Gibson wrote:
> > > > The pseries machine type has two variants of the PCI Host Bridge device:
> > > > spapr-pci-host-bridge and spapr-pci-vfio-host-bridge.  Originally, only the
> > > > latter could support VFIO devices, but we've now extended the VFIO code so
> > > > that they both can.
> > > > 
> > > > However, it's still only spapr-pci-vfio-host-bridge which supports the PAPR
> > > > Enhanced Error Handling (EEH) interfaces.  The reason is that we don't yet
> > > > have a way to determine the correct VFIOGroup for EEH operations on the
> > > > "plain" host bridge.
> > > > 
> > > > Handling this in general is problematic due to some limitations in the
> > > > current code, and some bugs in the kernel EEH interfaces.  However, it's
> > > > easy enough to do for the unambiguous case: where there is only one VFIO
> > > > group used on the whole host bridge i.e. one Partitionable Endpoint (PE).
> > > > 
> > > > This re-implements spapr_phb_check_vfio_group() in terms of the host
> > > > bridge's Partitionable Endpoints, allowing EEH on any host bridge in the
> > > > unambiguous case.  This is enough to make spapr-pci-host-bridge support
> > > > EEH as well as spapr-pci-vfio-host-bridge, since the latter only supported
> > > > devices from one IOMMU group anyway (although it didn't properly
> > > > enforce that).
> > > > 
> > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > ---
> > > >  hw/ppc/spapr_pci.c          | 36 ++++++++++++++++++++++++++++++++++++
> > > >  hw/ppc/spapr_pci_vfio.c     | 18 ------------------
> > > >  include/hw/pci-host/spapr.h |  1 -
> > > >  3 files changed, 36 insertions(+), 19 deletions(-)
> > > > 
> > > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > > > index 81ad3ae..5614b45 100644
> > > > --- a/hw/ppc/spapr_pci.c
> > > > +++ b/hw/ppc/spapr_pci.c
> > > > @@ -41,6 +41,8 @@
> > > >  #include "hw/ppc/spapr_drc.h"
> > > >  #include "sysemu/device_tree.h"
> > > >  
> > > > +#include "hw/vfio/vfio-common.h"
> > > > +
> > > >  /* Copied from the kernel arch/powerpc/platforms/pseries/msi.c */
> > > >  #define RTAS_QUERY_FN           0
> > > >  #define RTAS_CHANGE_FN          1
> > > > @@ -446,6 +448,40 @@ static sPAPRPHBGuestPE *spapr_phb_pe_by_device(sPAPRPHBState *phb,
> > > >      return pe;
> > > >  }
> > > >  
> > > > +static int spapr_phb_check_vfio_group(sPAPRPHBState *phb, VFIOGroup **gpp)
> > > > +{
> > > > +    sPAPRPHBGuestPE *pe;
> > > > +
> > > > +    if (QLIST_EMPTY(&phb->pe_list)) {
> > > > +        /* No EEH capable devices on this PHB */
> > > > +        return RTAS_OUT_PARAM_ERROR;
> > > > +    }
> > > > +
> > > > +    /* Limitations in both qemu and the kernel mean that, for now, EEH
> > > > +     * won't work if there are devices from more than one PE
> > > > +     * (i.e. IOMMU group) on the same PHB */
> > > > +    pe = QLIST_FIRST(&phb->pe_list);
> > > > +    if (QLIST_NEXT(pe, list)) {
> > > > +        error_report("spapr-pci-host-bridge: EEH attempted on PHB with multiple"
> > > > +                     " IOMMU groups");
> > > 
> > > Don't wrap lines with search-able strings
> > 
> > Noted.
> > 
> > > > +        return RTAS_OUT_HW_ERROR;
> > > > +    }
> > > > +
> > > > +    if (!object_dynamic_cast(OBJECT(phb), "spapr-pci-vfio-host-bridge")) {
> > > > +        sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(phb);
> > > > +        /* FIXME: this is an abstraction violation */
> > > > +        if (pe->group->groupid != svphb->iommugroupid) {
> > > > +            error_report("spapr-pci-vfio-host-bridge: Bad IOMMU group");
> > > > +            return RTAS_OUT_HW_ERROR;
> > > > +        }
> > > > +    }
> > > > +
> > > > +    if (gpp) {
> > > > +        *gpp = pe->group;
> > > > +    }
> > > > +    return RTAS_OUT_SUCCESS;
> > > 
> > > 
> > > The structure of this function finally makes some sense once we get
> > > here.
> > 
> > Yeah, I started off with something more like what you suggested in
> > earlier comments, but I changed to this rather awkward structure to
> > avoid churning the callers as this moves towards its final form
> > 
> > > I'm still not sure whether RTAS error codes make sense though,
> > > but it's just a nit.
> > 
> > Well, (nearly) all the callers are in RTAS, and translating the error
> > codes there seems pretty ugly too.
> > 
> > > I'd still prefer the function was renamed, "check"
> > > indicates a verification, not a return.  The primary purpose of calling
> > > this function is to get a group, not simply to check the validity.
> > 
> > Yeah, it's a crap name.  How about spapr_phb_find_vfio_group().
> 
> Why not just spapr_phb_get_vfio_group() since this started out as just a
> wrapper for vfio_get_group() and that's still what it's doing even
> though it's now cached on sPAPRPHBGuestPE rather than retrieved each
> time.

Well, "get" often implies taking a persistent reference in some way,
which is why I avoided it.

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

[Qemu-devel] [RFC PATCH 12/14] spapr_pci: (Mostly) remove spapr-pci-vfio-host-bridge

[David Gibson]

Now that the regular spapr-pci-host-bridge can handle EEH, the only thing
that the spapr-pci-vfio-host-bridge variant does differently is to
automatically size its DMA window based on what the host IOMMU can do.

That's not actually that useful, since the default window used by the
regular host bridge will work with the host IOMMU configuration on all
current systems anyway.

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

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

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci.c          |  9 -------
 hw/ppc/spapr_pci_vfio.c     | 63 +++++++++++----------------------------------
 include/hw/pci-host/spapr.h | 11 --------
 3 files changed, 15 insertions(+), 68 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 5614b45..77223b9 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -467,15 +467,6 @@ static int spapr_phb_check_vfio_group(sPAPRPHBState *phb, VFIOGroup **gpp)
         return RTAS_OUT_HW_ERROR;
     }
 
-    if (!object_dynamic_cast(OBJECT(phb), "spapr-pci-vfio-host-bridge")) {
-        sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(phb);
-        /* FIXME: this is an abstraction violation */
-        if (pe->group->groupid != svphb->iommugroupid) {
-            error_report("spapr-pci-vfio-host-bridge: Bad IOMMU group");
-            return RTAS_OUT_HW_ERROR;
-        }
-    }
-
     if (gpp) {
         *gpp = pe->group;
     }
diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
index a08292a..206a0fb 100644
--- a/hw/ppc/spapr_pci_vfio.c
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -19,75 +19,42 @@
 
 #include "hw/ppc/spapr.h"
 #include "hw/pci-host/spapr.h"
-#include "hw/pci/msix.h"
-#include "linux/vfio.h"
-#include "hw/vfio/vfio.h"
-#include "hw/vfio/vfio-eeh.h"
+#include "qemu/error-report.h"
+
+#define TYPE_SPAPR_PCI_VFIO_HOST_BRIDGE "spapr-pci-vfio-host-bridge"
+
+#define SPAPR_PCI_VFIO_HOST_BRIDGE(obj) \
+    OBJECT_CHECK(sPAPRPHBVFIOState, (obj), TYPE_SPAPR_PCI_VFIO_HOST_BRIDGE)
+
+typedef struct sPAPRPHBVFIOState {
+    sPAPRPHBState phb;
+
+    int32_t iommugroupid;
+} sPAPRPHBVFIOState;
 
 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)
+static void spapr_phb_vfio_instance_init(Object *obj)
 {
-    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));
+    error_report("spapr-pci-vfio-host-bridge is deprecated");
 }
 
 static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
-    sPAPRPHBClass *spc = SPAPR_PCI_HOST_BRIDGE_CLASS(klass);
 
     dc->props = spapr_phb_vfio_properties;
-    spc->finish_realize = spapr_phb_vfio_finish_realize;
 }
 
 static const TypeInfo spapr_phb_vfio_info = {
     .name          = TYPE_SPAPR_PCI_VFIO_HOST_BRIDGE,
     .parent        = TYPE_SPAPR_PCI_HOST_BRIDGE,
     .instance_size = sizeof(sPAPRPHBVFIOState),
+    .instance_init = spapr_phb_vfio_instance_init,
     .class_init    = spapr_phb_vfio_class_init,
-    .class_size    = sizeof(sPAPRPHBClass),
 };
 
 static void spapr_pci_vfio_register_types(void)
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index 8c8d187..fd85a21 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -29,14 +29,10 @@
 #include "hw/vfio/vfio-eeh.h"
 
 #define TYPE_SPAPR_PCI_HOST_BRIDGE "spapr-pci-host-bridge"
-#define TYPE_SPAPR_PCI_VFIO_HOST_BRIDGE "spapr-pci-vfio-host-bridge"
 
 #define SPAPR_PCI_HOST_BRIDGE(obj) \
     OBJECT_CHECK(sPAPRPHBState, (obj), TYPE_SPAPR_PCI_HOST_BRIDGE)
 
-#define SPAPR_PCI_VFIO_HOST_BRIDGE(obj) \
-    OBJECT_CHECK(sPAPRPHBVFIOState, (obj), TYPE_SPAPR_PCI_VFIO_HOST_BRIDGE)
-
 #define SPAPR_PCI_HOST_BRIDGE_CLASS(klass) \
      OBJECT_CLASS_CHECK(sPAPRPHBClass, (klass), TYPE_SPAPR_PCI_HOST_BRIDGE)
 #define SPAPR_PCI_HOST_BRIDGE_GET_CLASS(obj) \
@@ -44,7 +40,6 @@
 
 typedef struct sPAPRPHBClass sPAPRPHBClass;
 typedef struct sPAPRPHBState sPAPRPHBState;
-typedef struct sPAPRPHBVFIOState sPAPRPHBVFIOState;
 
 struct sPAPRPHBClass {
     PCIHostBridgeClass parent_class;
@@ -99,12 +94,6 @@ struct sPAPRPHBState {
     QLIST_ENTRY(sPAPRPHBState) list;
 };
 
-struct sPAPRPHBVFIOState {
-    sPAPRPHBState phb;
-
-    int32_t iommugroupid;
-};
-
 #define SPAPR_PCI_MAX_INDEX          255
 
 #define SPAPR_PCI_BASE_BUID          0x800000020000000ULL
-- 
2.4.3

[Qemu-devel] [RFC PATCH 13/14] spapr_pci: Remove finish_realize hook

[David Gibson]

Now that spapr-pci-vfio-host-bridge is reduced to just a stub, there is
only one implementation of the finish_realize hook in sPAPRPHBClass.  So,
we can fold that implementation into its (single) caller, and remove the
hook.  That's the last thing left in sPAPRPHBClass, so that can go away as
well.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci.c          | 29 +++++++----------------------
 include/hw/pci-host/spapr.h | 12 ------------
 2 files changed, 7 insertions(+), 34 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 77223b9..39c51d8 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1403,11 +1403,12 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
     SysBusDevice *s = SYS_BUS_DEVICE(dev);
     sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(s);
     PCIHostState *phb = PCI_HOST_BRIDGE(s);
-    sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(s);
     char *namebuf;
     int i;
     PCIBus *bus;
     uint64_t msi_window_size = 4096;
+    sPAPRTCETable *tcet;
+    uint32_t nb_table;
 
     if (sphb->index != (uint32_t)-1) {
         hwaddr windows_base;
@@ -1557,35 +1558,22 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
         }
     }
 
-    if (!info->finish_realize) {
-        error_setg(errp, "finish_realize not defined");
-        return;
-    }
-
-    info->finish_realize(sphb, errp);
-
-    sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, g_free);
-
-    QLIST_INIT(&sphb->pe_list);
-}
-
-static void spapr_phb_finish_realize(sPAPRPHBState *sphb, Error **errp)
-{
-    sPAPRTCETable *tcet;
-    uint32_t nb_table;
-
     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) {
         error_setg(errp, "Unable to create TCE table for %s",
                    sphb->dtbusname);
-        return ;
+        return;
     }
 
     /* Register default 32bit DMA window */
     memory_region_add_subregion(&sphb->iommu_root, sphb->dma_win_addr,
                                 spapr_tce_get_iommu(tcet));
+
+    sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, g_free);
+
+    QLIST_INIT(&sphb->pe_list);
 }
 
 static int spapr_phb_children_reset(Object *child, void *opaque)
@@ -1742,7 +1730,6 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
 {
     PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
-    sPAPRPHBClass *spc = SPAPR_PCI_HOST_BRIDGE_CLASS(klass);
     HotplugHandlerClass *hp = HOTPLUG_HANDLER_CLASS(klass);
 
     hc->root_bus_path = spapr_phb_root_bus_path;
@@ -1752,7 +1739,6 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
     dc->vmsd = &vmstate_spapr_pci;
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
     dc->cannot_instantiate_with_device_add_yet = false;
-    spc->finish_realize = spapr_phb_finish_realize;
     hp->plug = spapr_phb_hot_plug_child;
     hp->unplug = spapr_phb_hot_unplug_child;
 }
@@ -1762,7 +1748,6 @@ static const TypeInfo spapr_phb_info = {
     .parent        = TYPE_PCI_HOST_BRIDGE,
     .instance_size = sizeof(sPAPRPHBState),
     .class_init    = spapr_phb_class_init,
-    .class_size    = sizeof(sPAPRPHBClass),
     .interfaces    = (InterfaceInfo[]) {
         { TYPE_HOTPLUG_HANDLER },
         { }
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index fd85a21..2970ad0 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -33,20 +33,8 @@
 #define SPAPR_PCI_HOST_BRIDGE(obj) \
     OBJECT_CHECK(sPAPRPHBState, (obj), TYPE_SPAPR_PCI_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) \
-     OBJECT_GET_CLASS(sPAPRPHBClass, (obj), TYPE_SPAPR_PCI_HOST_BRIDGE)
-
-typedef struct sPAPRPHBClass sPAPRPHBClass;
 typedef struct sPAPRPHBState sPAPRPHBState;
 
-struct sPAPRPHBClass {
-    PCIHostBridgeClass parent_class;
-
-    void (*finish_realize)(sPAPRPHBState *sphb, Error **errp);
-};
-
 typedef struct spapr_pci_msi {
     uint32_t first_irq;
     uint32_t num;
-- 
2.4.3

[Qemu-devel] [RFC PATCH 14/14] vfio: Eliminate vfio_container_ioctl()

[David Gibson]

vfio_container_ioctl() was a bad interface that bypassed abstraction
boundaries, had semantics that sat uneasily with its name, and was unsafe
in many realistic circumstances.  Now that spapr-pci-vfio-host-bridge has
been folded into spapr-pci-host-bridge, there are no more users, so remove
it.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/vfio/common.c       | 45 ---------------------------------------------
 include/hw/vfio/vfio.h |  3 ---
 2 files changed, 48 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 746193f..63b10f9 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -960,48 +960,3 @@ void vfio_put_base_device(VFIODevice *vbasedev)
     trace_vfio_put_base_device(vbasedev->fd);
     close(vbasedev->fd);
 }
-
-static int vfio_container_do_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 %d to container: ret=%d, %s",
-                         _IOC_NR(req) - VFIO_BASE, ret, strerror(errno));
-        }
-    }
-
-    vfio_put_group(group);
-
-    return ret;
-}
-
-int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
-                         int req, void *param)
-{
-    /* We allow only certain ioctls to the container */
-    switch (req) {
-    case VFIO_CHECK_EXTENSION:
-    case VFIO_IOMMU_SPAPR_TCE_GET_INFO:
-    case VFIO_EEH_PE_OP:
-        break;
-    default:
-        /* Return an error on unknown requests */
-        error_report("vfio: unsupported ioctl %X", req);
-        return -1;
-    }
-
-    return vfio_container_do_ioctl(as, groupid, req, param);
-}
diff --git a/include/hw/vfio/vfio.h b/include/hw/vfio/vfio.h
index 0b26cd8..1b46a96 100644
--- a/include/hw/vfio/vfio.h
+++ b/include/hw/vfio/vfio.h
@@ -3,7 +3,4 @@
 
 #include "qemu/typedefs.h"
 
-extern int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
-                                int req, void *param);
-
 #endif
-- 
2.4.3