[Qemu-devel] [PATCH v18 0/2] EEH Support for VFIO Devices

[Qemu-devel] [PATCH v18 0/2] EEH Support for VFIO Devices

[Gavin Shan]

The series of patches adds support EEH for VFIO PCI devices on sPAPR platform.
It requires corresponding host kernel support, which was merged during 3.17
merge window. This patchset has been rebased to Alex Graf's QEMU repository:

   git://github.com/agraf/qemu.git (branch: ppc-next)

The implementations notes are below. Please consider for merging!

* RTAS calls are received in spapr_pci.c, sanity check is done there. RTAS
  handlers handle what they can. If there is something it cannot handle and
  sPAPRPHBClass EEH callback is defined, it is called.
* sPAPRPHBClass EEH callbacks are only implemented for VFIO now. It does ioctl()
  to the IOMMU container fd to complete the call. Error codes from that ioctl()
  are transferred back to the guest.

Changelog
=========
v17 -> v18:
    * Check options in sPAPRPHBClass callbacks for RTAS call "ibm,set-eeh-option"
      and "ibm,set-slot-reset".
    * Return RTAS_OUT_PARAM_ERROR on errors and RTAS_OUT_NO_ERRORS_FOUND on success
      as we don't have error log yet.
v16 -> v17:
    * Split sPAPRPHBClass::eeh_handler() to multiple callbacks according to David
      Gibson's suggestion
    * Make comments for the form of PE address more precise and merge the condition
      checking on the option in rtas_ibm_get_config_addr_info2() to improve code
      readability
    * Return RTAS_OUT_PARAM_ERROR for invalid number of arguments to function
      rtas_ibm_slot_error_detail(), which is inconsistent with PAPR spec
v15 -> v16:
    * Drop rtas_handle_eeh_request() and merge the logic to its callers so that
      more accurate return values can be returned for RTAS calls in the callers
    * Always return 1 ("No error log") for RTAS call "ibm,slot-error-detail" and
      correct wrong return values for other RTAS calls according to David Gibson's
      suggestions.
    * Make fall-through more obvious for case of negative return value from
      sPAPRPHBClass::eeh_handler()
    * Clear the argument buffer passed to ioctl()
    * Rename sPAPRPHBClass variable from "info" to "spc"
v14 -> v15:
    * Dropped unrelated patch making find_phb()/find_dev() public.
    * Checking RTAS parameter number before accessing RTAS parameter buffer for
      more safety.
    * Return hardware error from RTAS call "ibm,set-eeh-option" and "ibm,set-slot-reset"
      for some cases according to PAPR spec.
v13 -> v14:
    * Check if sPAPRPHBState instance is valid before converting it to the
      corresponding class as pointed by Alex Graf.
v12 -> v13:
    * Rebase to Alex Graf's QEMU repository ("ppc-next" branch).
    * Drop the patch for header file (vfio.h) changes, which was merged
      to QEMU repository by commit a9fd1654 ("linux-headers: update to 3.17-rc7").
    * Retested on Emulex adapter and EEH errors are recovered successfully.

Gavin Shan (2):
  sPAPR: Implement EEH RTAS calls
  sPAPR: Implement sPAPRPHBClass EEH callbacks

 hw/ppc/spapr_pci.c          | 281 ++++++++++++++++++++++++++++++++++++++++++++
 hw/ppc/spapr_pci_vfio.c     |  98 +++++++++++++++
 hw/vfio/common.c            |   1 +
 include/hw/pci-host/spapr.h |   4 +
 include/hw/ppc/spapr.h      |  43 ++++++-
 5 files changed, 425 insertions(+), 2 deletions(-)

-- 
1.8.3.2

[Qemu-devel] [PATCH v18 1/2] sPAPR: Implement EEH RTAS calls

[Gavin Shan]

The emulation for EEH RTAS requests from guest isn't covered
by QEMU yet and the patch implements them.

The patch defines constants used by EEH RTAS calls and adds
callbacks sPAPRPHBClass::{eeh_set_option, eeh_get_state, eeh_reset,
eeh_configure}, which are going to be used as follows:

  * RTAS calls are received in spapr_pci.c, sanity check is done
    there.
  * RTAS handlers handle what they can. If there is something it
    cannot handle and the corresponding sPAPRPHBClass callback is
    defined, it is called.
  * Those callbacks are only implemented for VFIO now. They do ioctl()
    to the IOMMU container fd to complete the calls. Error codes from
    that ioctl() are transferred back to the guest.

[aik: defined RTAS tokens for EEH RTAS calls]
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci.c          | 281 ++++++++++++++++++++++++++++++++++++++++++++
 include/hw/pci-host/spapr.h |   4 +
 include/hw/ppc/spapr.h      |  43 ++++++-
 3 files changed, 326 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index cebdeb3..29b071d 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -406,6 +406,268 @@ static void rtas_ibm_query_interrupt_source_number(PowerPCCPU *cpu,
     rtas_st(rets, 2, 1);/* 0 == level; 1 == edge */
 }
 
+static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
+                                    sPAPREnvironment *spapr,
+                                    uint32_t token, uint32_t nargs,
+                                    target_ulong args, uint32_t nret,
+                                    target_ulong rets)
+{
+    sPAPRPHBState *sphb;
+    sPAPRPHBClass *spc;
+    uint32_t addr, option;
+    uint64_t buid;
+    int ret;
+
+    if ((nargs != 4) || (nret != 1)) {
+        goto param_error_exit;
+    }
+
+    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
+    addr = rtas_ld(args, 0);
+    option = rtas_ld(args, 3);
+
+    sphb = find_phb(spapr, buid);
+    if (!sphb) {
+        goto param_error_exit;
+    }
+
+    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
+    if (!spc->eeh_set_option) {
+        goto param_error_exit;
+    }
+
+    /*
+     * The EEH functionality is enabled on basis of PCI device,
+     * instead of PE. We need check the validity of the PCI
+     * device address.
+     */
+    if (option == RTAS_EEH_ENABLE &&
+        !find_dev(spapr, buid, addr)) {
+        goto param_error_exit;
+    }
+
+    ret = spc->eeh_set_option(sphb, option);
+    rtas_st(rets, 0, ret);
+    return;
+
+param_error_exit:
+    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+}
+
+static void rtas_ibm_get_config_addr_info2(PowerPCCPU *cpu,
+                                           sPAPREnvironment *spapr,
+                                           uint32_t token, uint32_t nargs,
+                                           target_ulong args, uint32_t nret,
+                                           target_ulong rets)
+{
+    sPAPRPHBState *sphb;
+    sPAPRPHBClass *spc;
+    PCIDevice *pdev;
+    uint32_t addr, option;
+    uint64_t buid;
+
+    if ((nargs != 4) || (nret != 2)) {
+        goto param_error_exit;
+    }
+
+    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
+    sphb = find_phb(spapr, buid);
+    if (!sphb) {
+        goto param_error_exit;
+    }
+
+    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
+    if (!spc->eeh_set_option) {
+        goto param_error_exit;
+    }
+
+    /*
+     * We always have PE address of form "00BB0001". "BB"
+     * represents the bus number of PE's primary bus.
+     */
+    option = rtas_ld(args, 3);
+    switch (option) {
+    case RTAS_GET_PE_ADDR:
+        addr = rtas_ld(args, 0);
+        pdev = find_dev(spapr, buid, addr);
+        if (!pdev) {
+            goto param_error_exit;
+        }
+
+        rtas_st(rets, 1, (pci_bus_num(pdev->bus) << 16) + 1);
+        break;
+    case RTAS_GET_PE_MODE:
+        rtas_st(rets, 1, RTAS_PE_MODE_SHARED);
+        break;
+    default:
+        goto param_error_exit;
+    }
+
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+    return;
+
+param_error_exit:
+    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+}
+
+static void rtas_ibm_read_slot_reset_state2(PowerPCCPU *cpu,
+                                            sPAPREnvironment *spapr,
+                                            uint32_t token, uint32_t nargs,
+                                            target_ulong args, uint32_t nret,
+                                            target_ulong rets)
+{
+    sPAPRPHBState *sphb;
+    sPAPRPHBClass *spc;
+    uint64_t buid;
+    int state, ret;
+
+    if ((nargs != 3) || (nret != 4 && nret != 5)) {
+        goto param_error_exit;
+    }
+
+    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
+    sphb = find_phb(spapr, buid);
+    if (!sphb) {
+        goto param_error_exit;
+    }
+
+    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
+    if (!spc->eeh_get_state) {
+        goto param_error_exit;
+    }
+
+    ret = spc->eeh_get_state(sphb, &state);
+    rtas_st(rets, 0, ret);
+    if (ret != RTAS_OUT_SUCCESS) {
+        return;
+    }
+
+    rtas_st(rets, 1, state);
+    rtas_st(rets, 2, RTAS_EEH_SUPPORT);
+    rtas_st(rets, 3, RTAS_EEH_PE_UNAVAIL_INFO);
+    if (nret >= 5) {
+        rtas_st(rets, 4, RTAS_EEH_PE_RECOVER_INFO);
+    }
+    return;
+
+param_error_exit:
+    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+}
+
+static void rtas_ibm_set_slot_reset(PowerPCCPU *cpu,
+                                    sPAPREnvironment *spapr,
+                                    uint32_t token, uint32_t nargs,
+                                    target_ulong args, uint32_t nret,
+                                    target_ulong rets)
+{
+    sPAPRPHBState *sphb;
+    sPAPRPHBClass *spc;
+    uint32_t option;
+    uint64_t buid;
+    int ret;
+
+    if ((nargs != 4) || (nret != 1)) {
+        goto param_error_exit;
+    }
+
+    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
+    option = rtas_ld(args, 3);
+    sphb = find_phb(spapr, buid);
+    if (!sphb) {
+        goto param_error_exit;
+    }
+
+    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
+    if (!spc->eeh_reset) {
+        goto param_error_exit;
+    }
+
+    ret = spc->eeh_reset(sphb, option);
+    rtas_st(rets, 0, ret);
+    return;
+
+param_error_exit:
+    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+}
+
+static void rtas_ibm_configure_pe(PowerPCCPU *cpu,
+                                  sPAPREnvironment *spapr,
+                                  uint32_t token, uint32_t nargs,
+                                  target_ulong args, uint32_t nret,
+                                  target_ulong rets)
+{
+    sPAPRPHBState *sphb;
+    sPAPRPHBClass *spc;
+    uint64_t buid;
+    int ret;
+
+    if ((nargs != 3) || (nret != 1)) {
+        goto param_error_exit;
+    }
+
+    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
+    sphb = find_phb(spapr, buid);
+    if (!sphb) {
+        goto param_error_exit;
+    }
+
+    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
+    if (!spc->eeh_configure) {
+        goto param_error_exit;
+    }
+
+    ret = spc->eeh_configure(sphb);
+    rtas_st(rets, 0, ret);
+    return;
+
+param_error_exit:
+    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+}
+
+/* To support it later */
+static void rtas_ibm_slot_error_detail(PowerPCCPU *cpu,
+                                       sPAPREnvironment *spapr,
+                                       uint32_t token, uint32_t nargs,
+                                       target_ulong args, uint32_t nret,
+                                       target_ulong rets)
+{
+    sPAPRPHBState *sphb;
+    sPAPRPHBClass *spc;
+    int option;
+    uint64_t buid;
+
+    if ((nargs != 8) || (nret != 1)) {
+        goto param_error_exit;
+    }
+
+    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
+    sphb = find_phb(spapr, buid);
+    if (!sphb) {
+        goto param_error_exit;
+    }
+
+    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
+    if (!spc->eeh_set_option) {
+        goto param_error_exit;
+    }
+
+    option = rtas_ld(args, 7);
+    switch (option) {
+    case RTAS_SLOT_TEMP_ERR_LOG:
+    case RTAS_SLOT_PERM_ERR_LOG:
+        break;
+    default:
+        goto param_error_exit;
+    }
+
+    /* We don't have error log yet */
+    rtas_st(rets, 0, RTAS_OUT_NO_ERRORS_FOUND);
+    return;
+
+param_error_exit:
+    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
+}
+
 static int pci_spapr_swizzle(int slot, int pin)
 {
     return (slot + pin) % PCI_NUM_PINS;
@@ -974,6 +1236,25 @@ void spapr_pci_rtas_init(void)
         spapr_rtas_register(RTAS_IBM_CHANGE_MSI, "ibm,change-msi",
                             rtas_ibm_change_msi);
     }
+
+    spapr_rtas_register(RTAS_IBM_SET_EEH_OPTION,
+                        "ibm,set-eeh-option",
+                        rtas_ibm_set_eeh_option);
+    spapr_rtas_register(RTAS_IBM_GET_CONFIG_ADDR_INFO2,
+                        "ibm,get-config-addr-info2",
+                        rtas_ibm_get_config_addr_info2);
+    spapr_rtas_register(RTAS_IBM_READ_SLOT_RESET_STATE2,
+                        "ibm,read-slot-reset-state2",
+                        rtas_ibm_read_slot_reset_state2);
+    spapr_rtas_register(RTAS_IBM_SET_SLOT_RESET,
+                        "ibm,set-slot-reset",
+                        rtas_ibm_set_slot_reset);
+    spapr_rtas_register(RTAS_IBM_CONFIGURE_PE,
+                        "ibm,configure-pe",
+                        rtas_ibm_configure_pe);
+    spapr_rtas_register(RTAS_IBM_SLOT_ERROR_DETAIL,
+                        "ibm,slot-error-detail",
+                        rtas_ibm_slot_error_detail);
 }
 
 static void spapr_pci_register_types(void)
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index d725f0e..80af9fd 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -49,6 +49,10 @@ struct sPAPRPHBClass {
     PCIHostBridgeClass parent_class;
 
     void (*finish_realize)(sPAPRPHBState *sphb, Error **errp);
+    int (*eeh_set_option)(sPAPRPHBState *sphb, 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 {
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index a8e6817..81c23e9 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -339,6 +339,39 @@ target_ulong spapr_hypercall(PowerPCCPU *cpu, target_ulong opcode,
 int spapr_allocate_irq(int hint, bool lsi);
 int spapr_allocate_irq_block(int num, bool lsi, bool msi);
 
+/* ibm,set-eeh-option */
+#define RTAS_EEH_DISABLE                 0
+#define RTAS_EEH_ENABLE                  1
+#define RTAS_EEH_THAW_IO                 2
+#define RTAS_EEH_THAW_DMA                3
+
+/* ibm,get-config-addr-info2 */
+#define RTAS_GET_PE_ADDR                 0
+#define RTAS_GET_PE_MODE                 1
+#define RTAS_PE_MODE_NONE                0
+#define RTAS_PE_MODE_NOT_SHARED          1
+#define RTAS_PE_MODE_SHARED              2
+
+/* ibm,read-slot-reset-state2 */
+#define RTAS_EEH_PE_STATE_NORMAL         0
+#define RTAS_EEH_PE_STATE_RESET          1
+#define RTAS_EEH_PE_STATE_STOPPED_IO_DMA 2
+#define RTAS_EEH_PE_STATE_STOPPED_DMA    4
+#define RTAS_EEH_PE_STATE_UNAVAIL        5
+#define RTAS_EEH_NOT_SUPPORT             0
+#define RTAS_EEH_SUPPORT                 1
+#define RTAS_EEH_PE_UNAVAIL_INFO         1000
+#define RTAS_EEH_PE_RECOVER_INFO         0
+
+/* ibm,set-slot-reset */
+#define RTAS_SLOT_RESET_DEACTIVATE       0
+#define RTAS_SLOT_RESET_HOT              1
+#define RTAS_SLOT_RESET_FUNDAMENTAL      3
+
+/* ibm,slot-error-detail */
+#define RTAS_SLOT_TEMP_ERR_LOG           1
+#define RTAS_SLOT_PERM_ERR_LOG           2
+
 /* RTAS return codes */
 #define RTAS_OUT_SUCCESS            0
 #define RTAS_OUT_NO_ERRORS_FOUND    1
@@ -383,8 +416,14 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi);
 #define RTAS_GET_SENSOR_STATE                   (RTAS_TOKEN_BASE + 0x1D)
 #define RTAS_IBM_CONFIGURE_CONNECTOR            (RTAS_TOKEN_BASE + 0x1E)
 #define RTAS_IBM_OS_TERM                        (RTAS_TOKEN_BASE + 0x1F)
-
-#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x20)
+#define RTAS_IBM_SET_EEH_OPTION                 (RTAS_TOKEN_BASE + 0x20)
+#define RTAS_IBM_GET_CONFIG_ADDR_INFO2          (RTAS_TOKEN_BASE + 0x21)
+#define RTAS_IBM_READ_SLOT_RESET_STATE2         (RTAS_TOKEN_BASE + 0x22)
+#define RTAS_IBM_SET_SLOT_RESET                 (RTAS_TOKEN_BASE + 0x23)
+#define RTAS_IBM_CONFIGURE_PE                   (RTAS_TOKEN_BASE + 0x24)
+#define RTAS_IBM_SLOT_ERROR_DETAIL              (RTAS_TOKEN_BASE + 0x25)
+
+#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x26)
 
 /* RTAS ibm,get-system-parameter token values */
 #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20
-- 
1.8.3.2

[Qemu-devel] [PATCH v18 2/2] sPAPR: Implement sPAPRPHBClass EEH callbacks

[Gavin Shan]

The patch implements sPAPRPHBClass EEH callbacks so that the EEH
RTAS requests can be routed to VFIO for further handling.

Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 hw/ppc/spapr_pci_vfio.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++++
 hw/vfio/common.c        |  1 +
 2 files changed, 99 insertions(+)

diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
index 144912b..285d8d5 100644
--- a/hw/ppc/spapr_pci_vfio.c
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -76,6 +76,100 @@ static void spapr_phb_vfio_reset(DeviceState *qdev)
     /* Do nothing */
 }
 
+static int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb, int option)
+{
+    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
+    struct vfio_eeh_pe_op op = { .argsz = sizeof(op) };
+    int ret;
+
+    switch (option) {
+    case RTAS_EEH_DISABLE:
+        op.op = VFIO_EEH_PE_DISABLE;
+        break;
+    case RTAS_EEH_ENABLE:
+        op.op = VFIO_EEH_PE_ENABLE;
+        break;
+    case RTAS_EEH_THAW_IO:
+        op.op = VFIO_EEH_PE_UNFREEZE_IO;
+        break;
+    case RTAS_EEH_THAW_DMA:
+        op.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);
+    if (ret < 0) {
+        return RTAS_OUT_HW_ERROR;
+    }
+
+    return RTAS_OUT_SUCCESS;
+}
+
+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) };
+    int ret;
+
+    op.op = VFIO_EEH_PE_GET_STATE;
+    ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
+                               VFIO_EEH_PE_OP, &op);
+    if (ret < 0) {
+        return RTAS_OUT_PARAM_ERROR;
+    }
+
+    *state = ret;
+    return RTAS_OUT_SUCCESS;
+}
+
+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) };
+    int ret;
+
+    switch (option) {
+    case RTAS_SLOT_RESET_DEACTIVATE:
+        op.op = VFIO_EEH_PE_RESET_DEACTIVATE;
+        break;
+    case RTAS_SLOT_RESET_HOT:
+        op.op = VFIO_EEH_PE_RESET_HOT;
+        break;
+    case RTAS_SLOT_RESET_FUNDAMENTAL:
+        op.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);
+    if (ret < 0) {
+        return RTAS_OUT_HW_ERROR;
+    }
+
+    return RTAS_OUT_SUCCESS;
+}
+
+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) };
+    int ret;
+
+    op.op = VFIO_EEH_PE_CONFIGURE;
+    ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
+                               VFIO_EEH_PE_OP, &op);
+    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);
@@ -84,6 +178,10 @@ 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;
+    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/hw/vfio/common.c b/hw/vfio/common.c
index e71385e..50cd19f 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -948,6 +948,7 @@ int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
     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 */
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH v18 1/2] sPAPR: Implement EEH RTAS calls

[David Gibson]

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

On Mon, Feb 16, 2015 at 10:16:01AM +1100, Gavin Shan wrote:
> The emulation for EEH RTAS requests from guest isn't covered
> by QEMU yet and the patch implements them.
> 
> The patch defines constants used by EEH RTAS calls and adds
> callbacks sPAPRPHBClass::{eeh_set_option, eeh_get_state, eeh_reset,
> eeh_configure}, which are going to be used as follows:
> 
>   * RTAS calls are received in spapr_pci.c, sanity check is done
>     there.
>   * RTAS handlers handle what they can. If there is something it
>     cannot handle and the corresponding sPAPRPHBClass callback is
>     defined, it is called.
>   * Those callbacks are only implemented for VFIO now. They do ioctl()
>     to the IOMMU container fd to complete the calls. Error codes from
>     that ioctl() are transferred back to the guest.
> 
> [aik: defined RTAS tokens for EEH RTAS calls]
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr_pci.c          | 281 ++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/pci-host/spapr.h |   4 +
>  include/hw/ppc/spapr.h      |  43 ++++++-
>  3 files changed, 326 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index cebdeb3..29b071d 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -406,6 +406,268 @@ static void rtas_ibm_query_interrupt_source_number(PowerPCCPU *cpu,
>      rtas_st(rets, 2, 1);/* 0 == level; 1 == edge */
>  }
>  
> +static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
> +                                    sPAPREnvironment *spapr,
> +                                    uint32_t token, uint32_t nargs,
> +                                    target_ulong args, uint32_t nret,
> +                                    target_ulong rets)
> +{
> +    sPAPRPHBState *sphb;
> +    sPAPRPHBClass *spc;
> +    uint32_t addr, option;
> +    uint64_t buid;
> +    int ret;
> +
> +    if ((nargs != 4) || (nret != 1)) {
> +        goto param_error_exit;
> +    }
> +
> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
> +    addr = rtas_ld(args, 0);
> +    option = rtas_ld(args, 3);
> +
> +    sphb = find_phb(spapr, buid);
> +    if (!sphb) {
> +        goto param_error_exit;
> +    }
> +
> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
> +    if (!spc->eeh_set_option) {
> +        goto param_error_exit;
> +    }
> +
> +    /*
> +     * The EEH functionality is enabled on basis of PCI device,
> +     * instead of PE. We need check the validity of the PCI
> +     * device address.
> +     */
> +    if (option == RTAS_EEH_ENABLE &&
> +        !find_dev(spapr, buid, addr)) {
> +        goto param_error_exit;
> +    }

You're still breaking your layering by doing checks dependent on the
specific option both here and in the callback.

What I meant by my comments on the previous version was that this
find_dev() test should also move into the eeh_set_option callback.
Obviously that means adding addr into the parameters - but surely if
the addr has any meaning whatsoever, it must be at least potentially
needed by the callback anyway.

Apart from that,

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v18 2/2] sPAPR: Implement sPAPRPHBClass EEH callbacks

[David Gibson]

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

On Mon, Feb 16, 2015 at 10:16:02AM +1100, Gavin Shan wrote:
> The patch implements sPAPRPHBClass EEH callbacks so that the EEH
> RTAS requests can be routed to VFIO for further handling.
> 
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v18 1/2] sPAPR: Implement EEH RTAS calls

[Gavin Shan]

On Mon, Feb 16, 2015 at 12:52:48PM +1100, David Gibson wrote:
>On Mon, Feb 16, 2015 at 10:16:01AM +1100, Gavin Shan wrote:
>> The emulation for EEH RTAS requests from guest isn't covered
>> by QEMU yet and the patch implements them.
>> 
>> The patch defines constants used by EEH RTAS calls and adds
>> callbacks sPAPRPHBClass::{eeh_set_option, eeh_get_state, eeh_reset,
>> eeh_configure}, which are going to be used as follows:
>> 
>>   * RTAS calls are received in spapr_pci.c, sanity check is done
>>     there.
>>   * RTAS handlers handle what they can. If there is something it
>>     cannot handle and the corresponding sPAPRPHBClass callback is
>>     defined, it is called.
>>   * Those callbacks are only implemented for VFIO now. They do ioctl()
>>     to the IOMMU container fd to complete the calls. Error codes from
>>     that ioctl() are transferred back to the guest.
>> 
>> [aik: defined RTAS tokens for EEH RTAS calls]
>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> ---
>>  hw/ppc/spapr_pci.c          | 281 ++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/pci-host/spapr.h |   4 +
>>  include/hw/ppc/spapr.h      |  43 ++++++-
>>  3 files changed, 326 insertions(+), 2 deletions(-)
>> 
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index cebdeb3..29b071d 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -406,6 +406,268 @@ static void rtas_ibm_query_interrupt_source_number(PowerPCCPU *cpu,
>>      rtas_st(rets, 2, 1);/* 0 == level; 1 == edge */
>>  }
>>  
>> +static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
>> +                                    sPAPREnvironment *spapr,
>> +                                    uint32_t token, uint32_t nargs,
>> +                                    target_ulong args, uint32_t nret,
>> +                                    target_ulong rets)
>> +{
>> +    sPAPRPHBState *sphb;
>> +    sPAPRPHBClass *spc;
>> +    uint32_t addr, option;
>> +    uint64_t buid;
>> +    int ret;
>> +
>> +    if ((nargs != 4) || (nret != 1)) {
>> +        goto param_error_exit;
>> +    }
>> +
>> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
>> +    addr = rtas_ld(args, 0);
>> +    option = rtas_ld(args, 3);
>> +
>> +    sphb = find_phb(spapr, buid);
>> +    if (!sphb) {
>> +        goto param_error_exit;
>> +    }
>> +
>> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
>> +    if (!spc->eeh_set_option) {
>> +        goto param_error_exit;
>> +    }
>> +
>> +    /*
>> +     * The EEH functionality is enabled on basis of PCI device,
>> +     * instead of PE. We need check the validity of the PCI
>> +     * device address.
>> +     */
>> +    if (option == RTAS_EEH_ENABLE &&
>> +        !find_dev(spapr, buid, addr)) {
>> +        goto param_error_exit;
>> +    }
>
>You're still breaking your layering by doing checks dependent on the
>specific option both here and in the callback.
>
>What I meant by my comments on the previous version was that this
>find_dev() test should also move into the eeh_set_option callback.
>Obviously that means adding addr into the parameters - but surely if
>the addr has any meaning whatsoever, it must be at least potentially
>needed by the callback anyway.
>

Ok. Either simply dropping the check here, or moving find_dev() to
sPAPRPHBClass::eeh_set_option() as you suggested. However, there're more
things needed for sPAPRPHBClass::eeh_set_option() to do the check as follows.
David, could you help to confirm which way you prefer?

- Rename find_dev() to spapr_find_pci_dev() and make it public. It will be
  called in spapr_pci_vfio.c
- Add one field sPAPRPHBState::spapr to reference the associated sPAPREnvironment,
  which is required by spapr_find_pci_dev(). Otherwise, we have to pass sPAPREnvironment
  to sPAPRPHBClass::eeh_set_option().

Thanks,
Gavin

>Apart from that,
>
>Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>
>-- 
>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

Re: [Qemu-devel] [PATCH v18 1/2] sPAPR: Implement EEH RTAS calls

[Gavin Shan]

On Mon, Feb 16, 2015 at 04:32:09PM +1100, Gavin Shan wrote:
>On Mon, Feb 16, 2015 at 12:52:48PM +1100, David Gibson wrote:
>>On Mon, Feb 16, 2015 at 10:16:01AM +1100, Gavin Shan wrote:
>>> The emulation for EEH RTAS requests from guest isn't covered
>>> by QEMU yet and the patch implements them.
>>> 
>>> The patch defines constants used by EEH RTAS calls and adds
>>> callbacks sPAPRPHBClass::{eeh_set_option, eeh_get_state, eeh_reset,
>>> eeh_configure}, which are going to be used as follows:
>>> 
>>>   * RTAS calls are received in spapr_pci.c, sanity check is done
>>>     there.
>>>   * RTAS handlers handle what they can. If there is something it
>>>     cannot handle and the corresponding sPAPRPHBClass callback is
>>>     defined, it is called.
>>>   * Those callbacks are only implemented for VFIO now. They do ioctl()
>>>     to the IOMMU container fd to complete the calls. Error codes from
>>>     that ioctl() are transferred back to the guest.
>>> 
>>> [aik: defined RTAS tokens for EEH RTAS calls]
>>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>> ---
>>>  hw/ppc/spapr_pci.c          | 281 ++++++++++++++++++++++++++++++++++++++++++++
>>>  include/hw/pci-host/spapr.h |   4 +
>>>  include/hw/ppc/spapr.h      |  43 ++++++-
>>>  3 files changed, 326 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>>> index cebdeb3..29b071d 100644
>>> --- a/hw/ppc/spapr_pci.c
>>> +++ b/hw/ppc/spapr_pci.c
>>> @@ -406,6 +406,268 @@ static void rtas_ibm_query_interrupt_source_number(PowerPCCPU *cpu,
>>>      rtas_st(rets, 2, 1);/* 0 == level; 1 == edge */
>>>  }
>>>  
>>> +static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
>>> +                                    sPAPREnvironment *spapr,
>>> +                                    uint32_t token, uint32_t nargs,
>>> +                                    target_ulong args, uint32_t nret,
>>> +                                    target_ulong rets)
>>> +{
>>> +    sPAPRPHBState *sphb;
>>> +    sPAPRPHBClass *spc;
>>> +    uint32_t addr, option;
>>> +    uint64_t buid;
>>> +    int ret;
>>> +
>>> +    if ((nargs != 4) || (nret != 1)) {
>>> +        goto param_error_exit;
>>> +    }
>>> +
>>> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
>>> +    addr = rtas_ld(args, 0);
>>> +    option = rtas_ld(args, 3);
>>> +
>>> +    sphb = find_phb(spapr, buid);
>>> +    if (!sphb) {
>>> +        goto param_error_exit;
>>> +    }
>>> +
>>> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
>>> +    if (!spc->eeh_set_option) {
>>> +        goto param_error_exit;
>>> +    }
>>> +
>>> +    /*
>>> +     * The EEH functionality is enabled on basis of PCI device,
>>> +     * instead of PE. We need check the validity of the PCI
>>> +     * device address.
>>> +     */
>>> +    if (option == RTAS_EEH_ENABLE &&
>>> +        !find_dev(spapr, buid, addr)) {
>>> +        goto param_error_exit;
>>> +    }
>>
>>You're still breaking your layering by doing checks dependent on the
>>specific option both here and in the callback.
>>
>>What I meant by my comments on the previous version was that this
>>find_dev() test should also move into the eeh_set_option callback.
>>Obviously that means adding addr into the parameters - but surely if
>>the addr has any meaning whatsoever, it must be at least potentially
>>needed by the callback anyway.
>>
>
>Ok. Either simply dropping the check here, or moving find_dev() to
>sPAPRPHBClass::eeh_set_option() as you suggested. However, there're more
>things needed for sPAPRPHBClass::eeh_set_option() to do the check as follows.
>David, could you help to confirm which way you prefer?
>
>- Rename find_dev() to spapr_find_pci_dev() and make it public. It will be
>  called in spapr_pci_vfio.c
>- Add one field sPAPRPHBState::spapr to reference the associated sPAPREnvironment,
>  which is required by spapr_find_pci_dev(). Otherwise, we have to pass sPAPREnvironment
>  to sPAPRPHBClass::eeh_set_option().
>

Ping, David? If you don't object, I'll drop the check simply in next revision.
Note that I dropped public find_phb()/find_dev() in v15 and I don't want change
the code back.

Thanks,
Gavin

>>Apart from that,
>>
>>Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>>
>>-- 
>>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
>
>

Re: [Qemu-devel] [PATCH v18 1/2] sPAPR: Implement EEH RTAS calls

[Michael Roth]

Quoting Gavin Shan (2015-02-15 23:32:09)
> On Mon, Feb 16, 2015 at 12:52:48PM +1100, David Gibson wrote:
> >On Mon, Feb 16, 2015 at 10:16:01AM +1100, Gavin Shan wrote:
> >> The emulation for EEH RTAS requests from guest isn't covered
> >> by QEMU yet and the patch implements them.
> >> 
> >> The patch defines constants used by EEH RTAS calls and adds
> >> callbacks sPAPRPHBClass::{eeh_set_option, eeh_get_state, eeh_reset,
> >> eeh_configure}, which are going to be used as follows:
> >> 
> >>   * RTAS calls are received in spapr_pci.c, sanity check is done
> >>     there.
> >>   * RTAS handlers handle what they can. If there is something it
> >>     cannot handle and the corresponding sPAPRPHBClass callback is
> >>     defined, it is called.
> >>   * Those callbacks are only implemented for VFIO now. They do ioctl()
> >>     to the IOMMU container fd to complete the calls. Error codes from
> >>     that ioctl() are transferred back to the guest.



> >> 
> >> [aik: defined RTAS tokens for EEH RTAS calls]
> >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> >> ---
> >>  hw/ppc/spapr_pci.c          | 281 ++++++++++++++++++++++++++++++++++++++++++++
> >>  include/hw/pci-host/spapr.h |   4 +
> >>  include/hw/ppc/spapr.h      |  43 ++++++-
> >>  3 files changed, 326 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> >> index cebdeb3..29b071d 100644
> >> --- a/hw/ppc/spapr_pci.c
> >> +++ b/hw/ppc/spapr_pci.c
> >> @@ -406,6 +406,268 @@ static void rtas_ibm_query_interrupt_source_number(PowerPCCPU *cpu,
> >>      rtas_st(rets, 2, 1);/* 0 == level; 1 == edge */
> >>  }
> >>  
> >> +static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
> >> +                                    sPAPREnvironment *spapr,
> >> +                                    uint32_t token, uint32_t nargs,
> >> +                                    target_ulong args, uint32_t nret,
> >> +                                    target_ulong rets)
> >> +{
> >> +    sPAPRPHBState *sphb;
> >> +    sPAPRPHBClass *spc;
> >> +    uint32_t addr, option;
> >> +    uint64_t buid;
> >> +    int ret;
> >> +
> >> +    if ((nargs != 4) || (nret != 1)) {
> >> +        goto param_error_exit;
> >> +    }
> >> +
> >> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
> >> +    addr = rtas_ld(args, 0);
> >> +    option = rtas_ld(args, 3);
> >> +
> >> +    sphb = find_phb(spapr, buid);
> >> +    if (!sphb) {
> >> +        goto param_error_exit;
> >> +    }
> >> +
> >> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
> >> +    if (!spc->eeh_set_option) {
> >> +        goto param_error_exit;
> >> +    }
> >> +
> >> +    /*
> >> +     * The EEH functionality is enabled on basis of PCI device,
> >> +     * instead of PE. We need check the validity of the PCI
> >> +     * device address.
> >> +     */
> >> +    if (option == RTAS_EEH_ENABLE &&
> >> +        !find_dev(spapr, buid, addr)) {
> >> +        goto param_error_exit;
> >> +    }
> >
> >You're still breaking your layering by doing checks dependent on the
> >specific option both here and in the callback.
> >
> >What I meant by my comments on the previous version was that this
> >find_dev() test should also move into the eeh_set_option callback.
> >Obviously that means adding addr into the parameters - but surely if
> >the addr has any meaning whatsoever, it must be at least potentially
> >needed by the callback anyway.
> >
> 
> Ok. Either simply dropping the check here, or moving find_dev() to
> sPAPRPHBClass::eeh_set_option() as you suggested. However, there're more
> things needed for sPAPRPHBClass::eeh_set_option() to do the check as follows.
> David, could you help to confirm which way you prefer?
> 
> - Rename find_dev() to spapr_find_pci_dev() and make it public. It will be
>   called in spapr_pci_vfio.c
> - Add one field sPAPRPHBState::spapr to reference the associated sPAPREnvironment,
>   which is required by spapr_find_pci_dev(). Otherwise, we have to pass sPAPREnvironment
>   to sPAPRPHBClass::eeh_set_option().

AFAICT spapr_pci.c:find_dev() only needs sPAPREnvironment to look up the phb
given a buid, but in your case you already have the phb and pass it on to
eeh_set_option(), so within eeh_set_option() you can call pci_find_device()
just like spapr_pci.c:find_dev() does to do the validation.

The validation seems to assume the addr value is a config_addr for the device
though, isn't it possible we might recieve a pe_addr of the form returned
by rtas_ibm_get_config_addr_info2? That value would happen to correspond to
bus:n,device:0,func:0,reg:1, and find_dev in that case would just mask off
the reg value and verify there's a device in PCI slot 0, instead of whatever
actually needs to be validated in that situation (which isn't clear to me).

> 
> Thanks,
> Gavin
> 
> >Apart from that,
> >
> >Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> >
> >-- 
> >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

Re: [Qemu-devel] [PATCH v18 1/2] sPAPR: Implement EEH RTAS calls

[Gavin Shan]

On Wed, Feb 18, 2015 at 07:28:29PM -0600, Michael Roth wrote:
>Quoting Gavin Shan (2015-02-15 23:32:09)
>> On Mon, Feb 16, 2015 at 12:52:48PM +1100, David Gibson wrote:
>> >On Mon, Feb 16, 2015 at 10:16:01AM +1100, Gavin Shan wrote:
>> >> The emulation for EEH RTAS requests from guest isn't covered
>> >> by QEMU yet and the patch implements them.
>> >> 
>> >> The patch defines constants used by EEH RTAS calls and adds
>> >> callbacks sPAPRPHBClass::{eeh_set_option, eeh_get_state, eeh_reset,
>> >> eeh_configure}, which are going to be used as follows:
>> >> 
>> >>   * RTAS calls are received in spapr_pci.c, sanity check is done
>> >>     there.
>> >>   * RTAS handlers handle what they can. If there is something it
>> >>     cannot handle and the corresponding sPAPRPHBClass callback is
>> >>     defined, it is called.
>> >>   * Those callbacks are only implemented for VFIO now. They do ioctl()
>> >>     to the IOMMU container fd to complete the calls. Error codes from
>> >>     that ioctl() are transferred back to the guest.
>
>
>
>> >> 
>> >> [aik: defined RTAS tokens for EEH RTAS calls]
>> >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> >> ---
>> >>  hw/ppc/spapr_pci.c          | 281 ++++++++++++++++++++++++++++++++++++++++++++
>> >>  include/hw/pci-host/spapr.h |   4 +
>> >>  include/hw/ppc/spapr.h      |  43 ++++++-
>> >>  3 files changed, 326 insertions(+), 2 deletions(-)
>> >> 
>> >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> >> index cebdeb3..29b071d 100644
>> >> --- a/hw/ppc/spapr_pci.c
>> >> +++ b/hw/ppc/spapr_pci.c
>> >> @@ -406,6 +406,268 @@ static void rtas_ibm_query_interrupt_source_number(PowerPCCPU *cpu,
>> >>      rtas_st(rets, 2, 1);/* 0 == level; 1 == edge */
>> >>  }
>> >>  
>> >> +static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
>> >> +                                    sPAPREnvironment *spapr,
>> >> +                                    uint32_t token, uint32_t nargs,
>> >> +                                    target_ulong args, uint32_t nret,
>> >> +                                    target_ulong rets)
>> >> +{
>> >> +    sPAPRPHBState *sphb;
>> >> +    sPAPRPHBClass *spc;
>> >> +    uint32_t addr, option;
>> >> +    uint64_t buid;
>> >> +    int ret;
>> >> +
>> >> +    if ((nargs != 4) || (nret != 1)) {
>> >> +        goto param_error_exit;
>> >> +    }
>> >> +
>> >> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
>> >> +    addr = rtas_ld(args, 0);
>> >> +    option = rtas_ld(args, 3);
>> >> +
>> >> +    sphb = find_phb(spapr, buid);
>> >> +    if (!sphb) {
>> >> +        goto param_error_exit;
>> >> +    }
>> >> +
>> >> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
>> >> +    if (!spc->eeh_set_option) {
>> >> +        goto param_error_exit;
>> >> +    }
>> >> +
>> >> +    /*
>> >> +     * The EEH functionality is enabled on basis of PCI device,
>> >> +     * instead of PE. We need check the validity of the PCI
>> >> +     * device address.
>> >> +     */
>> >> +    if (option == RTAS_EEH_ENABLE &&
>> >> +        !find_dev(spapr, buid, addr)) {
>> >> +        goto param_error_exit;
>> >> +    }
>> >
>> >You're still breaking your layering by doing checks dependent on the
>> >specific option both here and in the callback.
>> >
>> >What I meant by my comments on the previous version was that this
>> >find_dev() test should also move into the eeh_set_option callback.
>> >Obviously that means adding addr into the parameters - but surely if
>> >the addr has any meaning whatsoever, it must be at least potentially
>> >needed by the callback anyway.
>> >
>> 
>> Ok. Either simply dropping the check here, or moving find_dev() to
>> sPAPRPHBClass::eeh_set_option() as you suggested. However, there're more
>> things needed for sPAPRPHBClass::eeh_set_option() to do the check as follows.
>> David, could you help to confirm which way you prefer?
>> 
>> - Rename find_dev() to spapr_find_pci_dev() and make it public. It will be
>>   called in spapr_pci_vfio.c
>> - Add one field sPAPRPHBState::spapr to reference the associated sPAPREnvironment,
>>   which is required by spapr_find_pci_dev(). Otherwise, we have to pass sPAPREnvironment
>>   to sPAPRPHBClass::eeh_set_option().
>
>AFAICT spapr_pci.c:find_dev() only needs sPAPREnvironment to look up the phb
>given a buid, but in your case you already have the phb and pass it on to
>eeh_set_option(), so within eeh_set_option() you can call pci_find_device()
>just like spapr_pci.c:find_dev() does to do the validation.
>

Yeah, it's another option I was thinking about. It would introduce
duplicate code, but it seems it's the best way to go. I'll update
accordingly in next revision. Thanks for your comments.
 
>The validation seems to assume the addr value is a config_addr for the device
>though, isn't it possible we might recieve a pe_addr of the form returned
>by rtas_ibm_get_config_addr_info2? That value would happen to correspond to
>bus:n,device:0,func:0,reg:1, and find_dev in that case would just mask off
>the reg value and verify there's a device in PCI slot 0, instead of whatever
>actually needs to be validated in that situation (which isn't clear to me).
>

Yeah, The address passed to rtas_ibm_set_eeh_option() could be device's
config_addr or PE_addr depending on the options. For option EEH_ENABLE,
it's device's config_addr.

Thanks,
Gavin

>> >Apart from that,
>> >
>> >Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>> >
>> >-- 
>> >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

Re: [Qemu-devel] [PATCH v18 1/2] sPAPR: Implement EEH RTAS calls

[David Gibson]

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

On Fri, Feb 20, 2015 at 09:50:45AM +1100, Gavin Shan wrote:
> On Wed, Feb 18, 2015 at 07:28:29PM -0600, Michael Roth wrote:
> >Quoting Gavin Shan (2015-02-15 23:32:09)
> >> On Mon, Feb 16, 2015 at 12:52:48PM +1100, David Gibson wrote:
> >> >On Mon, Feb 16, 2015 at 10:16:01AM +1100, Gavin Shan wrote:
> >> >> The emulation for EEH RTAS requests from guest isn't covered
> >> >> by QEMU yet and the patch implements them.
> >> >> 
> >> >> The patch defines constants used by EEH RTAS calls and adds
> >> >> callbacks sPAPRPHBClass::{eeh_set_option, eeh_get_state, eeh_reset,
> >> >> eeh_configure}, which are going to be used as follows:
> >> >> 
> >> >>   * RTAS calls are received in spapr_pci.c, sanity check is done
> >> >>     there.
> >> >>   * RTAS handlers handle what they can. If there is something it
> >> >>     cannot handle and the corresponding sPAPRPHBClass callback is
> >> >>     defined, it is called.
> >> >>   * Those callbacks are only implemented for VFIO now. They do ioctl()
> >> >>     to the IOMMU container fd to complete the calls. Error codes from
> >> >>     that ioctl() are transferred back to the guest.
> >
> >
> >
> >> >> 
> >> >> [aik: defined RTAS tokens for EEH RTAS calls]
> >> >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> >> >> ---
> >> >>  hw/ppc/spapr_pci.c          | 281 ++++++++++++++++++++++++++++++++++++++++++++
> >> >>  include/hw/pci-host/spapr.h |   4 +
> >> >>  include/hw/ppc/spapr.h      |  43 ++++++-
> >> >>  3 files changed, 326 insertions(+), 2 deletions(-)
> >> >> 
> >> >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> >> >> index cebdeb3..29b071d 100644
> >> >> --- a/hw/ppc/spapr_pci.c
> >> >> +++ b/hw/ppc/spapr_pci.c
> >> >> @@ -406,6 +406,268 @@ static void rtas_ibm_query_interrupt_source_number(PowerPCCPU *cpu,
> >> >>      rtas_st(rets, 2, 1);/* 0 == level; 1 == edge */
> >> >>  }
> >> >>  
> >> >> +static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
> >> >> +                                    sPAPREnvironment *spapr,
> >> >> +                                    uint32_t token, uint32_t nargs,
> >> >> +                                    target_ulong args, uint32_t nret,
> >> >> +                                    target_ulong rets)
> >> >> +{
> >> >> +    sPAPRPHBState *sphb;
> >> >> +    sPAPRPHBClass *spc;
> >> >> +    uint32_t addr, option;
> >> >> +    uint64_t buid;
> >> >> +    int ret;
> >> >> +
> >> >> +    if ((nargs != 4) || (nret != 1)) {
> >> >> +        goto param_error_exit;
> >> >> +    }
> >> >> +
> >> >> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
> >> >> +    addr = rtas_ld(args, 0);
> >> >> +    option = rtas_ld(args, 3);
> >> >> +
> >> >> +    sphb = find_phb(spapr, buid);
> >> >> +    if (!sphb) {
> >> >> +        goto param_error_exit;
> >> >> +    }
> >> >> +
> >> >> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
> >> >> +    if (!spc->eeh_set_option) {
> >> >> +        goto param_error_exit;
> >> >> +    }
> >> >> +
> >> >> +    /*
> >> >> +     * The EEH functionality is enabled on basis of PCI device,
> >> >> +     * instead of PE. We need check the validity of the PCI
> >> >> +     * device address.
> >> >> +     */
> >> >> +    if (option == RTAS_EEH_ENABLE &&
> >> >> +        !find_dev(spapr, buid, addr)) {
> >> >> +        goto param_error_exit;
> >> >> +    }
> >> >
> >> >You're still breaking your layering by doing checks dependent on the
> >> >specific option both here and in the callback.
> >> >
> >> >What I meant by my comments on the previous version was that this
> >> >find_dev() test should also move into the eeh_set_option callback.
> >> >Obviously that means adding addr into the parameters - but surely if
> >> >the addr has any meaning whatsoever, it must be at least potentially
> >> >needed by the callback anyway.
> >> >
> >> 
> >> Ok. Either simply dropping the check here, or moving find_dev() to
> >> sPAPRPHBClass::eeh_set_option() as you suggested. However, there're more
> >> things needed for sPAPRPHBClass::eeh_set_option() to do the check as follows.
> >> David, could you help to confirm which way you prefer?
> >> 
> >> - Rename find_dev() to spapr_find_pci_dev() and make it public. It will be
> >>   called in spapr_pci_vfio.c
> >> - Add one field sPAPRPHBState::spapr to reference the associated sPAPREnvironment,
> >>   which is required by spapr_find_pci_dev(). Otherwise, we have to pass sPAPREnvironment
> >>   to sPAPRPHBClass::eeh_set_option().
> >
> >AFAICT spapr_pci.c:find_dev() only needs sPAPREnvironment to look up the phb
> >given a buid, but in your case you already have the phb and pass it on to
> >eeh_set_option(), so within eeh_set_option() you can call pci_find_device()
> >just like spapr_pci.c:find_dev() does to do the validation.
> >
> 
> Yeah, it's another option I was thinking about. It would introduce
> duplicate code, but it seems it's the best way to go. I'll update
> accordingly in next revision. Thanks for your comments.

This sounds like the best option to me for now.

> >The validation seems to assume the addr value is a config_addr for the device
> >though, isn't it possible we might recieve a pe_addr of the form returned
> >by rtas_ibm_get_config_addr_info2? That value would happen to correspond to
> >bus:n,device:0,func:0,reg:1, and find_dev in that case would just mask off
> >the reg value and verify there's a device in PCI slot 0, instead of whatever
> >actually needs to be validated in that situation (which isn't clear to me).
> >
> 
> Yeah, The address passed to rtas_ibm_set_eeh_option() could be device's
> config_addr or PE_addr depending on the options. For option EEH_ENABLE,
> it's device's config_addr.

Ah.. ok, that complicates matters.  But that's definitely another
reason to move the validation into the callback that's already
checking the options.

-- 
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] [Qemu-ppc] [PATCH v18 1/2] sPAPR: Implement EEH RTAS calls

[Gavin Shan]

On Fri, Feb 20, 2015 at 10:40:53AM +1100, David Gibson wrote:
>On Fri, Feb 20, 2015 at 09:50:45AM +1100, Gavin Shan wrote:
>> On Wed, Feb 18, 2015 at 07:28:29PM -0600, Michael Roth wrote:
>> >Quoting Gavin Shan (2015-02-15 23:32:09)
>> >> On Mon, Feb 16, 2015 at 12:52:48PM +1100, David Gibson wrote:
>> >> >On Mon, Feb 16, 2015 at 10:16:01AM +1100, Gavin Shan wrote:
>> >> >> The emulation for EEH RTAS requests from guest isn't covered
>> >> >> by QEMU yet and the patch implements them.
>> >> >> 
>> >> >> The patch defines constants used by EEH RTAS calls and adds
>> >> >> callbacks sPAPRPHBClass::{eeh_set_option, eeh_get_state, eeh_reset,
>> >> >> eeh_configure}, which are going to be used as follows:
>> >> >> 
>> >> >>   * RTAS calls are received in spapr_pci.c, sanity check is done
>> >> >>     there.
>> >> >>   * RTAS handlers handle what they can. If there is something it
>> >> >>     cannot handle and the corresponding sPAPRPHBClass callback is
>> >> >>     defined, it is called.
>> >> >>   * Those callbacks are only implemented for VFIO now. They do ioctl()
>> >> >>     to the IOMMU container fd to complete the calls. Error codes from
>> >> >>     that ioctl() are transferred back to the guest.
>> >
>> >
>> >
>> >> >> 
>> >> >> [aik: defined RTAS tokens for EEH RTAS calls]
>> >> >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> >> >> ---
>> >> >>  hw/ppc/spapr_pci.c          | 281 ++++++++++++++++++++++++++++++++++++++++++++
>> >> >>  include/hw/pci-host/spapr.h |   4 +
>> >> >>  include/hw/ppc/spapr.h      |  43 ++++++-
>> >> >>  3 files changed, 326 insertions(+), 2 deletions(-)
>> >> >> 
>> >> >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> >> >> index cebdeb3..29b071d 100644
>> >> >> --- a/hw/ppc/spapr_pci.c
>> >> >> +++ b/hw/ppc/spapr_pci.c
>> >> >> @@ -406,6 +406,268 @@ static void rtas_ibm_query_interrupt_source_number(PowerPCCPU *cpu,
>> >> >>      rtas_st(rets, 2, 1);/* 0 == level; 1 == edge */
>> >> >>  }
>> >> >>  
>> >> >> +static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
>> >> >> +                                    sPAPREnvironment *spapr,
>> >> >> +                                    uint32_t token, uint32_t nargs,
>> >> >> +                                    target_ulong args, uint32_t nret,
>> >> >> +                                    target_ulong rets)
>> >> >> +{
>> >> >> +    sPAPRPHBState *sphb;
>> >> >> +    sPAPRPHBClass *spc;
>> >> >> +    uint32_t addr, option;
>> >> >> +    uint64_t buid;
>> >> >> +    int ret;
>> >> >> +
>> >> >> +    if ((nargs != 4) || (nret != 1)) {
>> >> >> +        goto param_error_exit;
>> >> >> +    }
>> >> >> +
>> >> >> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
>> >> >> +    addr = rtas_ld(args, 0);
>> >> >> +    option = rtas_ld(args, 3);
>> >> >> +
>> >> >> +    sphb = find_phb(spapr, buid);
>> >> >> +    if (!sphb) {
>> >> >> +        goto param_error_exit;
>> >> >> +    }
>> >> >> +
>> >> >> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
>> >> >> +    if (!spc->eeh_set_option) {
>> >> >> +        goto param_error_exit;
>> >> >> +    }
>> >> >> +
>> >> >> +    /*
>> >> >> +     * The EEH functionality is enabled on basis of PCI device,
>> >> >> +     * instead of PE. We need check the validity of the PCI
>> >> >> +     * device address.
>> >> >> +     */
>> >> >> +    if (option == RTAS_EEH_ENABLE &&
>> >> >> +        !find_dev(spapr, buid, addr)) {
>> >> >> +        goto param_error_exit;
>> >> >> +    }
>> >> >
>> >> >You're still breaking your layering by doing checks dependent on the
>> >> >specific option both here and in the callback.
>> >> >
>> >> >What I meant by my comments on the previous version was that this
>> >> >find_dev() test should also move into the eeh_set_option callback.
>> >> >Obviously that means adding addr into the parameters - but surely if
>> >> >the addr has any meaning whatsoever, it must be at least potentially
>> >> >needed by the callback anyway.
>> >> >
>> >> 
>> >> Ok. Either simply dropping the check here, or moving find_dev() to
>> >> sPAPRPHBClass::eeh_set_option() as you suggested. However, there're more
>> >> things needed for sPAPRPHBClass::eeh_set_option() to do the check as follows.
>> >> David, could you help to confirm which way you prefer?
>> >> 
>> >> - Rename find_dev() to spapr_find_pci_dev() and make it public. It will be
>> >>   called in spapr_pci_vfio.c
>> >> - Add one field sPAPRPHBState::spapr to reference the associated sPAPREnvironment,
>> >>   which is required by spapr_find_pci_dev(). Otherwise, we have to pass sPAPREnvironment
>> >>   to sPAPRPHBClass::eeh_set_option().
>> >
>> >AFAICT spapr_pci.c:find_dev() only needs sPAPREnvironment to look up the phb
>> >given a buid, but in your case you already have the phb and pass it on to
>> >eeh_set_option(), so within eeh_set_option() you can call pci_find_device()
>> >just like spapr_pci.c:find_dev() does to do the validation.
>> >
>> 
>> Yeah, it's another option I was thinking about. It would introduce
>> duplicate code, but it seems it's the best way to go. I'll update
>> accordingly in next revision. Thanks for your comments.
>
>This sounds like the best option to me for now.
>

Thanks for the confirm. I'll include the change to the new revision
and send it shortly.

>> >The validation seems to assume the addr value is a config_addr for the device
>> >though, isn't it possible we might recieve a pe_addr of the form returned
>> >by rtas_ibm_get_config_addr_info2? That value would happen to correspond to
>> >bus:n,device:0,func:0,reg:1, and find_dev in that case would just mask off
>> >the reg value and verify there's a device in PCI slot 0, instead of whatever
>> >actually needs to be validated in that situation (which isn't clear to me).
>> >
>> 
>> Yeah, The address passed to rtas_ibm_set_eeh_option() could be device's
>> config_addr or PE_addr depending on the options. For option EEH_ENABLE,
>> it's device's config_addr.
>
>Ah.. ok, that complicates matters.  But that's definitely another
>reason to move the validation into the callback that's already
>checking the options.
>

Indeed :)

Thanks,
Gavin
 
>-- 
>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