qemu-ppc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-ppc] [PATCH v2] spapr: populate ibm,loc-code


From: Alexey Kardashevskiy
Subject: Re: [Qemu-ppc] [PATCH v2] spapr: populate ibm,loc-code
Date: Mon, 30 Mar 2015 13:18:01 +1100
User-agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0

On 03/27/2015 08:49 PM, Nikunj A Dadhania wrote:
Each hardware instance has a platform unique location code.  The OF
device tree that describes a part of a hardware entity must include
the “ibm,loc-code” property with a value that represents the location
code for that hardware entity.

Introduce an hcall to populate ibm,loc-code.
1) PCI passthru devices need to identify with its own ibm,loc-code
    available on the host.
2) Emulated devices encode as following: qemu_<name>:<slot>.<fn>

Signed-off-by: Nikunj A Dadhania <address@hidden>
---

Changelog
v1:
* Dropped is_vfio patch and using TYPE_SPAPR_PCI_VFIO_HOST_BRIDGE
   to recognise vfio devices
* Removed wrapper for hcall
* Added sPAPRPHBClass::get_loc_code

  hw/ppc/spapr_hcall.c        |  1 +
  hw/ppc/spapr_pci.c          | 38 +++++++++++++++++++++++++++++++++
  hw/ppc/spapr_pci_vfio.c     | 51 +++++++++++++++++++++++++++++++++++++++++++++
  include/hw/pci-host/spapr.h |  1 +
  include/hw/ppc/spapr.h      |  8 ++++++-
  5 files changed, 98 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 4f76f1c..b394681 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1010,6 +1010,7 @@ static void hypercall_register_types(void)

      /* ibm,client-architecture-support support */
      spapr_register_hypercall(KVMPPC_H_CAS, h_client_architecture_support);
+    spapr_register_hypercall(KVMPPC_H_GET_LOC_CODE, phb_get_loc_code);
  }

  type_init(hypercall_register_types)
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 05f4fac..c2ee476 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -248,6 +248,44 @@ static void spapr_msi_setmsg(PCIDevice *pdev, hwaddr addr, 
bool msix,
      }
  }

+target_ulong phb_get_loc_code(PowerPCCPU *cpu, sPAPREnvironment *spapr,
+                              target_ulong opcode, target_ulong *args)
+{
+    sPAPRPHBState *sphb = NULL;
+    sPAPRPHBClass *spc = NULL;
+    char *buf = NULL, path[PATH_MAX];
+    PCIDevice *pdev;
+    target_ulong buid = args[0];
+    target_ulong config_addr = args[1];
+    target_ulong loc_code = args[2];
+    target_ulong size = args[3];
+
+    sphb = find_phb(spapr, buid);
+    pdev = find_dev(spapr, buid, config_addr);
+
+    if (!sphb || !pdev) {
+        return H_PARAMETER;
+    }
+
+    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
+    if (spc->get_loc_code && spc->get_loc_code(sphb, pdev, &buf)) {
+        cpu_physical_memory_write(loc_code, buf, strlen(buf));
+        g_free(buf);
+        return H_SUCCESS;
+    }
+
+    /*
+     * For non-vfio devices and failures make up the location code out
+     * of the name, slot and function.
+     *
+     *       qemu_<name>:<slot>.<fn>
+     */
+    snprintf(path, sizeof(path), "qemu_%s:%02d.%1d", pdev->name,
+             PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+    cpu_physical_memory_write(loc_code, path, size);


Move the chunk above to a sPAPRPHBState::get_loc_code. And I'd add PHB's @index in the device name to make it unique across the guest.


+    return H_SUCCESS;
+}
+
  static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                  uint32_t token, uint32_t nargs,
                                  target_ulong args, uint32_t nret,
diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
index 99a1be5..bfdfa67 100644
--- a/hw/ppc/spapr_pci_vfio.c
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -171,6 +171,56 @@ static int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, 
int option)
      return RTAS_OUT_SUCCESS;
  }

+static bool spapr_phb_vfio_get_devspec(PCIDevice *pdev, char **value)

s/value/devspec/ ?

+{
+    char *host;
+    char path[PATH_MAX];
+    struct stat st;
+
+    host = object_property_get_str(OBJECT(pdev), "host", NULL);
+    if (!host) {
+        return false;
+    }
+
+    snprintf(path, sizeof(path), "/sys/bus/pci/devices/%s/devspec", host);
+    g_free(host);
+    if (stat(path, &st) < 0) {
+        return false;
+    }
+
+    return g_file_get_contents(path, value, NULL, NULL);


g_file_get_contents() is expected to return FALSE if the file is missing so stat() seems redundant here.


+}
+
+static int spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb,  PCIDevice *pdev,
+                                        char **loc_code)
+{
+    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
+    char path[PATH_MAX], *buf = NULL;
+    struct stat st;
+
+    /* Non VFIO devices */
+    if (!svphb) {
+        return false;


The function returns int, not bool. s/false/-1/ and s/true/0/ please.

+    }
+
+    /* We have a vfio host bridge lets get the path. */
+    if (!spapr_phb_vfio_get_devspec(pdev, &buf)) {

s/buf/devspec/


+        return false;
+    }
+
+    snprintf(path, sizeof(path), "/proc/device-tree%s/ibm,loc-code", buf);
+    g_free(buf);
+    if (stat(path, &st) < 0) {
+            return false;

Just return what stat() returned.

+    }
+
+    /* A valid file, now read the loc-code */
+    if (g_file_get_contents(path, loc_code, NULL, NULL)) {
+        return true;
+    }
+    return false;
+}
+
  static int spapr_phb_vfio_eeh_configure(sPAPRPHBState *sphb)
  {
      sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
@@ -199,6 +249,7 @@ static void spapr_phb_vfio_class_init(ObjectClass *klass, 
void *data)
      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;
+    spc->get_loc_code = spapr_phb_vfio_get_loc_code;
  }

  static const TypeInfo spapr_phb_vfio_info = {
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index 895d273..1ff50b4 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -53,6 +53,7 @@ struct sPAPRPHBClass {
      int (*eeh_get_state)(sPAPRPHBState *sphb, int *state);
      int (*eeh_reset)(sPAPRPHBState *sphb, int option);
      int (*eeh_configure)(sPAPRPHBState *sphb);
+    int (*get_loc_code)(sPAPRPHBState *sphb,  PCIDevice *pdev, char 
**loc_code);
  };

  typedef struct spapr_pci_msi {
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index af71e8b..95157ac 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -310,7 +310,10 @@ typedef struct sPAPREnvironment {
  #define KVMPPC_H_LOGICAL_MEMOP  (KVMPPC_HCALL_BASE + 0x1)
  /* Client Architecture support */
  #define KVMPPC_H_CAS            (KVMPPC_HCALL_BASE + 0x2)
-#define KVMPPC_HCALL_MAX        KVMPPC_H_CAS
+#define KVMPPC_H_RTAS_UPDATE    (KVMPPC_HCALL_BASE + 0x3)
+#define KVMPPC_H_REPORT_MC_ERR  (KVMPPC_HCALL_BASE + 0x4)
+#define KVMPPC_H_GET_LOC_CODE   (KVMPPC_HCALL_BASE + 0x5)
+#define KVMPPC_HCALL_MAX        KVMPPC_H_GET_LOC_CODE


Please add only relevant codes. And what happened to patches adding H_RTAS_UPDATE and H_REPORT_MC_ERR?

Also (it is probably a very stupid question but still :) ), why are all these callbacks - hypercalls, not RTAS calls? The hypercalls are numbered in sPAPR and we kind of stealing numbers from that space while we are allocating RTAS tokens ourselves and have more freedom.



  extern sPAPREnvironment *spapr;

@@ -522,6 +525,9 @@ int spapr_tcet_dma_dt(void *fdt, int node_off, const char 
*propname,
                        sPAPRTCETable *tcet);
  void spapr_pci_switch_vga(bool big_endian);

+target_ulong phb_get_loc_code(PowerPCCPU *cpu, sPAPREnvironment *spapr,
+                              target_ulong opcode, target_ulong *args);
+
  #define TYPE_SPAPR_RTC "spapr-rtc"

  void spapr_rtc_read(DeviceState *dev, struct tm *tm, uint32_t *ns);



--
Alexey



reply via email to

[Prev in Thread] Current Thread [Next in Thread]