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

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

[Nikunj A Dadhania]

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 <nikunj@linux.vnet.ibm.com>
---

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);
+    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)
+{
+    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);
+}
+
+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;
+    }
+
+    /* We have a vfio host bridge lets get the path. */
+    if (!spapr_phb_vfio_get_devspec(pdev, &buf)) {
+        return false;
+    }
+
+    snprintf(path, sizeof(path), "/proc/device-tree%s/ibm,loc-code", buf);
+    g_free(buf);
+    if (stat(path, &st) < 0) {
+            return false;
+    }
+
+    /* 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
 
 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);
-- 
1.8.3.1

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

[Alexey Kardashevskiy]

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 <nikunj@linux.vnet.ibm.com>
> ---
>
> 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

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

[David Gibson]

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

On Mon, Mar 30, 2015 at 01:18:01PM +1100, Alexey Kardashevskiy wrote:
> 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 <nikunj@linux.vnet.ibm.com>
[snip]
> >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.

Also, I thought the plan was to remove PCI device enumeration from
SLOF and move it to qemu (since we need to partially do that for
hotplug).  That removes the need for the hcall entirely.

-- 
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 v2] spapr: populate ibm,loc-code

[Alexey Kardashevskiy]

On 03/30/2015 01:25 PM, David Gibson wrote:
> On Mon, Mar 30, 2015 at 01:18:01PM +1100, Alexey Kardashevskiy wrote:
>> 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 <nikunj@linux.vnet.ibm.com>
> [snip]
>>> 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.
>
> Also, I thought the plan was to remove PCI device enumeration from
> SLOF and move it to qemu (since we need to partially do that for
> hotplug).  That removes the need for the hcall entirely.


There was a strong opposition to PCI scan done by QEMU (although it was ok 
if PCI hotplug does some resource assignment in QEMU). Has this changed?

I added Michael in cc: and hope Alexander may enlighten us on this topic...



-- 
Alexey

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

[Nikunj A Dadhania]

Alexey Kardashevskiy <aik@ozlabs.ru> writes:

> 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 <nikunj@linux.vnet.ibm.com>
>> ---
>>
>> 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.

Sure.

>
>
>> +    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/ ?

Yes, should be value now.

>
>> +{
>> +    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.

Did not notive, will change it.

>
>
>> +}
>> +
>> +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.

Yep, will do that.

>
>> +    }
>> +
>> +    /* 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.

Ok

>> +    }
>> +
>> +    /* 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?

They are in stages of review/re-write :(

>
> Also (it is probably a very stupid question but still :) ), why are all 
> these callbacks - hypercalls, not RTAS calls?

No particular reason though. Let me look at doing it as rtas, does not
make much difference 

> 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

Thanks for the review.

Regards,
Nikunj

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

[Nikunj A Dadhania]

David Gibson <david@gibson.dropbear.id.au> writes:

> On Mon, Mar 30, 2015 at 01:18:01PM +1100, Alexey Kardashevskiy wrote:
>> 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 <nikunj@linux.vnet.ibm.com>
> [snip]
>> >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.
>
> Also, I thought the plan was to remove PCI device enumeration from
> SLOF and move it to qemu (since we need to partially do that for
> hotplug).  

For me it was a short term plan. IMHO, this cant be done in short time. 

> That removes the need for the hcall entirely.
>
> -- 
> 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] [Qemu-ppc] [PATCH v2] spapr: populate ibm, loc-code

[Nikunj A Dadhania]

Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes:

> David Gibson <david@gibson.dropbear.id.au> writes:
>
>> On Mon, Mar 30, 2015 at 01:18:01PM +1100, Alexey Kardashevskiy wrote:
>>> 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 <nikunj@linux.vnet.ibm.com>
>> [snip]
>>> >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.
>>
>> Also, I thought the plan was to remove PCI device enumeration from
>> SLOF and move it to qemu (since we need to partially do that for
>> hotplug).  
>
> For me it was a short term plan. 

Sorry, I meant PCI device enumeration removal from SLOF was a long term
plan.


> IMHO, this cant be done in short time.
>
>> That removes the need for the hcall entirely.
>>
>> -- 
>> 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] [Qemu-ppc] [PATCH v2] spapr: populate ibm, loc-code

[Alexey Kardashevskiy]

On 03/30/2015 04:34 PM, Nikunj A Dadhania wrote:
> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes:
>
>> David Gibson <david@gibson.dropbear.id.au> writes:
>>
>>> On Mon, Mar 30, 2015 at 01:18:01PM +1100, Alexey Kardashevskiy wrote:
>>>> 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 <nikunj@linux.vnet.ibm.com>
>>> [snip]
>>>>> 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.
>>>
>>> Also, I thought the plan was to remove PCI device enumeration from
>>> SLOF and move it to qemu (since we need to partially do that for
>>> hotplug).
>>
>> For me it was a short term plan.
>
> Sorry, I meant PCI device enumeration removal from SLOF was a long term
> plan.


It does not have to be removal, rather adding a case if there are already 
devices present (or resources assigned) on a PHB in the device tree, then 
do not do scan, something like that.



>
>
>> IMHO, this cant be done in short time.
>>
>>> That removes the need for the hcall entirely.
>>>
>>> --
>>> 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
>


-- 
Alexey

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

[Nikunj A Dadhania]

Alexey Kardashevskiy <aik@ozlabs.ru> writes:

> On 03/30/2015 04:34 PM, Nikunj A Dadhania wrote:
>> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes:
>>
>>> David Gibson <david@gibson.dropbear.id.au> writes:
>>>
>>>> On Mon, Mar 30, 2015 at 01:18:01PM +1100, Alexey Kardashevskiy wrote:
>>>>> 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 <nikunj@linux.vnet.ibm.com>
>>>> [snip]
>>>>>> 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.
>>>>
>>>> Also, I thought the plan was to remove PCI device enumeration from
>>>> SLOF and move it to qemu (since we need to partially do that for
>>>> hotplug).
>>>
>>> For me it was a short term plan.
>>
>> Sorry, I meant PCI device enumeration removal from SLOF was a long term
>> plan.
>
>
> It does not have to be removal, rather adding a case if there are already 
> devices present (or resources assigned) on a PHB in the device tree, then 
> do not do scan, something like that.

Right, it would require time to analyze impact both on Qemu and SLOF
side. I havent looked at the qemu side of the things how
easy/complicated it would be to do PCI scanning

Regards
Nikunj

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

[Michael Roth]

Quoting Alexey Kardashevskiy (2015-03-29 22:08:17)
> On 03/30/2015 01:25 PM, David Gibson wrote:
> > On Mon, Mar 30, 2015 at 01:18:01PM +1100, Alexey Kardashevskiy wrote:
> >> 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 <nikunj@linux.vnet.ibm.com>
> > [snip]
> >>> 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.
> >
> > Also, I thought the plan was to remove PCI device enumeration from
> > SLOF and move it to qemu (since we need to partially do that for
> > hotplug).  That removes the need for the hcall entirely.
> 
> 
> There was a strong opposition to PCI scan done by QEMU (although it was ok 
> if PCI hotplug does some resource assignment in QEMU). Has this changed?

Was this WRT to hotplug, or was there previous discussion?

As far as hotplug, the main question was handling actual BAR assignments in
QEMU, rather than SLOF, for hotplugged devices. (since RPAPHP hotplug code
expects those assignments to be handled by firmware and encoded in device-tree).

We've worked around that so far by avoiding BAR assignments completely...

To get rpaphp working again, since it was already broken for other reasons,
we'll be adding code that allows it to handle BAR assignments in the kernel
in cases where 'reg'/'assigned-resources' properties don't indicate that
they've already been assigned by QEMU/firmware. Alex seemed okay with this
approach when we discussed it during his IBM visit a few weeks ago.

So, I'd imagine we can take the same approach for offloading PCI DT node
creation to QEMU: handle node creation, but don't do actual BAR assignments.
Just let SLOF inherit whatever DT node we give it, and let it update
'reg'/'assigned-properties' and other properties as needed, but otherwise
pass on what QEMU provides.

> 
> I added Michael in cc: and hope Alexander may enlighten us on this topic...
> 
> 
> 
> -- 
> Alexey

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

[Nikunj A Dadhania]

Michael Roth <mdroth@linux.vnet.ibm.com> writes:

> Quoting Alexey Kardashevskiy (2015-03-29 22:08:17)
>> On 03/30/2015 01:25 PM, David Gibson wrote:
>> > On Mon, Mar 30, 2015 at 01:18:01PM +1100, Alexey Kardashevskiy wrote:
>> >> 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 <nikunj@linux.vnet.ibm.com>
>> > [snip]
>> >>> 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.
>> >
>> > Also, I thought the plan was to remove PCI device enumeration from
>> > SLOF and move it to qemu (since we need to partially do that for
>> > hotplug).  That removes the need for the hcall entirely.
>> 
>> 
>> There was a strong opposition to PCI scan done by QEMU (although it was ok 
>> if PCI hotplug does some resource assignment in QEMU). Has this changed?
>
> Was this WRT to hotplug, or was there previous discussion?
>
> As far as hotplug, the main question was handling actual BAR assignments in
> QEMU, rather than SLOF, for hotplugged devices. (since RPAPHP hotplug code
> expects those assignments to be handled by firmware and encoded in device-tree).
>
> We've worked around that so far by avoiding BAR assignments completely...
>
> To get rpaphp working again, since it was already broken for other reasons,
> we'll be adding code that allows it to handle BAR assignments in the kernel
> in cases where 'reg'/'assigned-resources' properties don't indicate that
> they've already been assigned by QEMU/firmware. Alex seemed okay with this
> approach when we discussed it during his IBM visit a few weeks ago.
>
> So, I'd imagine we can take the same approach for offloading PCI DT node
> creation to QEMU: handle node creation, but don't do actual BAR
> assignments.

Yes, thats what I have been playing with since yesterday. I am using
following of your patches:

  pci: make pci_bar useable outside pci.c
  spapr_pci: enable basic hotplug operations

> Just let SLOF inherit whatever DT node we give it, and let it update
> 'reg'/'assigned-properties' and other properties as needed, but otherwise
> pass on what QEMU provides.

I still need to pass the updated DT blob to SLOF.

Regards
Nikunj