[Qemu-devel] [PATCH 1/2] vfio-pci: add flag to identify vfio pci device

[Qemu-devel] [PATCH 1/2] vfio-pci: add flag to identify vfio pci device

[Nikunj A Dadhania]

>From PCIDevice there is no way to know if the device has a backing
vfio device.

sPAPR guests inherits the "ibm,loc-code" from the pci pass through
device in hypervisor. This helps in identifying the device if there is
any failures using this "ibm,loc-code" for RAS capabilities.

CC: Alex Williamson <alex.williamson@redhat.com>
CC: Alexander Graf <agraf@suse.de>
Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 hw/vfio/pci.c        | 2 ++
 include/hw/pci/pci.h | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 6b80539..95d666e 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3483,6 +3483,7 @@ static void vfio_instance_finalize(Object *obj)
     VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pci_dev);
     VFIOGroup *group = vdev->vbasedev.group;
 
+    pci_dev->is_vfio = false;
     vfio_unmap_bars(vdev);
     g_free(vdev->emulated_config_bits);
     g_free(vdev->rom);
@@ -3542,6 +3543,7 @@ static void vfio_instance_init(Object *obj)
     PCIDevice *pci_dev = PCI_DEVICE(obj);
     VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, PCI_DEVICE(obj));
 
+    pci_dev->is_vfio = true;
     device_add_bootindex_property(obj, &vdev->bootindex,
                                   "bootindex", NULL,
                                   &pci_dev->qdev, NULL);
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index b97c295..0ddc830 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -291,6 +291,8 @@ struct PCIDevice {
     /* SHPC */
     SHPCDevice *shpc;
 
+    bool is_vfio;
+
     /* Location of option rom */
     char *romfile;
     bool has_rom;
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/2] 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-cde.
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>
---
 hw/ppc/spapr_hcall.c          | 10 +++++++++
 hw/ppc/spapr_pci.c            | 49 +++++++++++++++++++++++++++++++++++++++++++
 hw/vfio/pci.c                 | 18 ++++++++++++++++
 include/hw/ppc/spapr.h        |  8 ++++++-
 include/hw/vfio/vfio-common.h |  1 +
 5 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 4f76f1c..a577395 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -928,6 +928,15 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu_,
     return H_SUCCESS;
 }
 
+static target_ulong h_get_loc_code(PowerPCCPU *cpu, sPAPREnvironment *spapr,
+				   target_ulong opcode, target_ulong *args)
+{
+    if (!spapr_h_get_loc_code(spapr, args[0], args[1], args[2], args[3])) {
+        return H_PARAMETER;
+    }
+    return H_SUCCESS;
+}
+
 static spapr_hcall_fn papr_hypercall_table[(MAX_HCALL_OPCODE / 4) + 1];
 static spapr_hcall_fn kvmppc_hypercall_table[KVMPPC_HCALL_MAX - KVMPPC_HCALL_BASE + 1];
 
@@ -1010,6 +1019,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, h_get_loc_code);
 }
 
 type_init(hypercall_register_types)
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 05f4fac..65cdb91 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -35,6 +35,7 @@
 #include "qemu/error-report.h"
 
 #include "hw/pci/pci_bus.h"
+#include "hw/vfio/vfio-common.h"
 
 /* Copied from the kernel arch/powerpc/platforms/pseries/msi.c */
 #define RTAS_QUERY_FN           0
@@ -248,6 +249,54 @@ static void spapr_msi_setmsg(PCIDevice *pdev, hwaddr addr, bool msix,
     }
 }
 
+bool spapr_h_get_loc_code(sPAPREnvironment *spapr, target_ulong config_addr, target_ulong buid,
+                          target_ulong loc_code, target_ulong size)
+{
+    sPAPRPHBState *sphb = NULL;
+    PCIDevice *pdev = NULL;
+    char *buf, path[PATH_MAX];
+    struct stat st;
+
+    sphb = find_phb(spapr, buid);
+    if (sphb) {
+        pdev = find_dev(spapr, buid, config_addr);
+    }
+
+    if (!sphb || !pdev) {
+        error_report("spapr_h_get_loc_code: Device not found");
+        return false;
+    }
+
+    /* For a VFIO device, get the location in the device tree */
+    if (pdev->is_vfio && vfio_get_devspec(pdev, &buf)) {
+        snprintf(path, sizeof(path), "/proc/device-tree%s/ibm,loc-code", buf);
+        g_free(buf);
+        if (stat(path, &st) < 0) {
+            goto fail;
+        }
+
+        /* A valid file, now read the loc-code */
+        if (g_file_get_contents(path, &buf, NULL, NULL)) {
+            cpu_physical_memory_write(loc_code, buf, strlen(buf));
+            g_free(buf);
+            goto out;
+        }
+    }
+
+fail:
+    /*
+     * For non-vfio devices and failure cases, 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);
+ out:
+    return true;
+}
+
 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/vfio/pci.c b/hw/vfio/pci.c
index 95d666e..dd97258 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3319,6 +3319,24 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
     vdev->req_enabled = false;
 }
 
+bool vfio_get_devspec(PCIDevice *pdev, char **value)
+{
+    VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
+    char path[PATH_MAX];
+    struct stat st;
+
+    snprintf(path, sizeof(path),
+             "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/devspec",
+             vdev->host.domain, vdev->host.bus, vdev->host.slot,
+             vdev->host.function);
+
+    if (stat(path, &st) < 0) {
+        return false;
+    }
+
+    return g_file_get_contents(path, value, NULL, NULL);
+}
+
 static int vfio_initfn(PCIDevice *pdev)
 {
     VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index af71e8b..d3fad67 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);
 
+bool spapr_h_get_loc_code(sPAPREnvironment *spapr, target_ulong config_addr, target_ulong build,
+			  target_ulong loc_code, target_ulong size);
+
 #define TYPE_SPAPR_RTC "spapr-rtc"
 
 void spapr_rtc_read(DeviceState *dev, struct tm *tm, uint32_t *ns);
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 0d1fb80..6dffa8c 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -140,6 +140,7 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace *as);
 void vfio_put_group(VFIOGroup *group);
 int vfio_get_device(VFIOGroup *group, const char *name,
                     VFIODevice *vbasedev);
+bool vfio_get_devspec(PCIDevice *pdev, char **value);
 
 extern const MemoryRegionOps vfio_region_ops;
 extern QLIST_HEAD(vfio_group_head, VFIOGroup) vfio_group_list;
-- 
1.8.3.1

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

[Alexey Kardashevskiy]

On 03/26/2015 05:42 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-cde.
> 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>
> ---
>   hw/ppc/spapr_hcall.c          | 10 +++++++++
>   hw/ppc/spapr_pci.c            | 49 +++++++++++++++++++++++++++++++++++++++++++
>   hw/vfio/pci.c                 | 18 ++++++++++++++++
>   include/hw/ppc/spapr.h        |  8 ++++++-
>   include/hw/vfio/vfio-common.h |  1 +
>   5 files changed, 85 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 4f76f1c..a577395 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -928,6 +928,15 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu_,
>       return H_SUCCESS;
>   }
>
> +static target_ulong h_get_loc_code(PowerPCCPU *cpu, sPAPREnvironment *spapr,
> +				   target_ulong opcode, target_ulong *args)
> +{
> +    if (!spapr_h_get_loc_code(spapr, args[0], args[1], args[2], args[3])) {
> +        return H_PARAMETER;
> +    }
> +    return H_SUCCESS;
> +}
> +
>   static spapr_hcall_fn papr_hypercall_table[(MAX_HCALL_OPCODE / 4) + 1];
>   static spapr_hcall_fn kvmppc_hypercall_table[KVMPPC_HCALL_MAX - KVMPPC_HCALL_BASE + 1];
>
> @@ -1010,6 +1019,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, h_get_loc_code);
>   }
>
>   type_init(hypercall_register_types)
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 05f4fac..65cdb91 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -35,6 +35,7 @@
>   #include "qemu/error-report.h"
>
>   #include "hw/pci/pci_bus.h"
> +#include "hw/vfio/vfio-common.h"
>
>   /* Copied from the kernel arch/powerpc/platforms/pseries/msi.c */
>   #define RTAS_QUERY_FN           0
> @@ -248,6 +249,54 @@ static void spapr_msi_setmsg(PCIDevice *pdev, hwaddr addr, bool msix,
>       }
>   }
>
> +bool spapr_h_get_loc_code(sPAPREnvironment *spapr, target_ulong config_addr, target_ulong buid,
> +                          target_ulong loc_code, target_ulong size)
> +{
> +    sPAPRPHBState *sphb = NULL;
> +    PCIDevice *pdev = NULL;
> +    char *buf, path[PATH_MAX];
> +    struct stat st;
> +
> +    sphb = find_phb(spapr, buid);
> +    if (sphb) {
> +        pdev = find_dev(spapr, buid, config_addr);
> +    }
> +

I would define a sPAPRPHBClass::get_loc_code(sPAPRPHBState *sphb,  uint32_t 
config_addr) and implement it for "spapr-pci-vfio-host-bridge" as you did 
below.


> +    if (!sphb || !pdev) {
> +        error_report("spapr_h_get_loc_code: Device not found");
> +        return false;
> +    }
> +
> +    /* For a VFIO device, get the location in the device tree */
> +    if (pdev->is_vfio && vfio_get_devspec(pdev, &buf)) {
> +        snprintf(path, sizeof(path), "/proc/device-tree%s/ibm,loc-code", buf);
> +        g_free(buf);
> +        if (stat(path, &st) < 0) {
> +            goto fail;
> +        }
> +
> +        /* A valid file, now read the loc-code */
> +        if (g_file_get_contents(path, &buf, NULL, NULL)) {
> +            cpu_physical_memory_write(loc_code, buf, strlen(buf));
> +            g_free(buf);
> +            goto out;
> +        }
> +    }
> +
> +fail:
> +    /*
> +     * For non-vfio devices and failure cases, 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);
> + out:
> +    return true;
> +}
> +
>   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/vfio/pci.c b/hw/vfio/pci.c
> index 95d666e..dd97258 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3319,6 +3319,24 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
>       vdev->req_enabled = false;
>   }
>
> +bool vfio_get_devspec(PCIDevice *pdev, char **value)


This function can live in hw/ppc/spapr_pci_vfio.c. Something like 
object_property_get_str(OBJECT(pdev), "host", NULL) will return the host 
address, and this is all you really want from VFIO here.


> +{
> +    VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> +    char path[PATH_MAX];
> +    struct stat st;
> +
> +    snprintf(path, sizeof(path),
> +             "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/devspec",
> +             vdev->host.domain, vdev->host.bus, vdev->host.slot,
> +             vdev->host.function);
> +
> +    if (stat(path, &st) < 0) {
> +        return false;
> +    }
> +
> +    return g_file_get_contents(path, value, NULL, NULL);
> +}
> +
>   static int vfio_initfn(PCIDevice *pdev)
>   {
>       VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index af71e8b..d3fad67 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)


Are you adding KVMPPC_H_GET_LOC_CODE or all three? :)


> +#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);
>
> +bool spapr_h_get_loc_code(sPAPREnvironment *spapr, target_ulong config_addr, target_ulong build,

s/build/buid/  and please put buid before config_addr in the parameters lice.


> +			  target_ulong loc_code, target_ulong size);
> +
>   #define TYPE_SPAPR_RTC "spapr-rtc"
>
>   void spapr_rtc_read(DeviceState *dev, struct tm *tm, uint32_t *ns);
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 0d1fb80..6dffa8c 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -140,6 +140,7 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace *as);
>   void vfio_put_group(VFIOGroup *group);
>   int vfio_get_device(VFIOGroup *group, const char *name,
>                       VFIODevice *vbasedev);
> +bool vfio_get_devspec(PCIDevice *pdev, char **value);
>
>   extern const MemoryRegionOps vfio_region_ops;
>   extern QLIST_HEAD(vfio_group_head, VFIOGroup) vfio_group_list;
>


-- 
Alexey

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

[Nikunj A Dadhania]

Alexey Kardashevskiy <aik@ozlabs.ru> writes:

> On 03/26/2015 05:42 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-cde.
>> 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>
>> ---
>>   hw/ppc/spapr_hcall.c          | 10 +++++++++
>>   hw/ppc/spapr_pci.c            | 49 +++++++++++++++++++++++++++++++++++++++++++
>>   hw/vfio/pci.c                 | 18 ++++++++++++++++
>>   include/hw/ppc/spapr.h        |  8 ++++++-
>>   include/hw/vfio/vfio-common.h |  1 +
>>   5 files changed, 85 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index 4f76f1c..a577395 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -928,6 +928,15 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu_,
>>       return H_SUCCESS;
>>   }
>>
>> +static target_ulong h_get_loc_code(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>> +				   target_ulong opcode, target_ulong *args)
>> +{
>> +    if (!spapr_h_get_loc_code(spapr, args[0], args[1], args[2], args[3])) {
>> +        return H_PARAMETER;
>> +    }
>> +    return H_SUCCESS;
>> +}
>> +
>>   static spapr_hcall_fn papr_hypercall_table[(MAX_HCALL_OPCODE / 4) + 1];
>>   static spapr_hcall_fn kvmppc_hypercall_table[KVMPPC_HCALL_MAX - KVMPPC_HCALL_BASE + 1];
>>
>> @@ -1010,6 +1019,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, h_get_loc_code);
>>   }
>>
>>   type_init(hypercall_register_types)
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index 05f4fac..65cdb91 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -35,6 +35,7 @@
>>   #include "qemu/error-report.h"
>>
>>   #include "hw/pci/pci_bus.h"
>> +#include "hw/vfio/vfio-common.h"
>>
>>   /* Copied from the kernel arch/powerpc/platforms/pseries/msi.c */
>>   #define RTAS_QUERY_FN           0
>> @@ -248,6 +249,54 @@ static void spapr_msi_setmsg(PCIDevice *pdev, hwaddr addr, bool msix,
>>       }
>>   }
>>
>> +bool spapr_h_get_loc_code(sPAPREnvironment *spapr, target_ulong config_addr, target_ulong buid,
>> +                          target_ulong loc_code, target_ulong size)
>> +{
>> +    sPAPRPHBState *sphb = NULL;
>> +    PCIDevice *pdev = NULL;
>> +    char *buf, path[PATH_MAX];
>> +    struct stat st;
>> +
>> +    sphb = find_phb(spapr, buid);
>> +    if (sphb) {
>> +        pdev = find_dev(spapr, buid, config_addr);
>> +    }
>> +
>
> I would define a sPAPRPHBClass::get_loc_code(sPAPRPHBState *sphb,  uint32_t 
> config_addr) and implement it for "spapr-pci-vfio-host-bridge" as you did 
> below.

Sure.

>
>
>> +    if (!sphb || !pdev) {
>> +        error_report("spapr_h_get_loc_code: Device not found");
>> +        return false;
>> +    }
>> +
>> +    /* For a VFIO device, get the location in the device tree */
>> +    if (pdev->is_vfio && vfio_get_devspec(pdev, &buf)) {
>> +        snprintf(path, sizeof(path), "/proc/device-tree%s/ibm,loc-code", buf);
>> +        g_free(buf);
>> +        if (stat(path, &st) < 0) {
>> +            goto fail;
>> +        }
>> +
>> +        /* A valid file, now read the loc-code */
>> +        if (g_file_get_contents(path, &buf, NULL, NULL)) {
>> +            cpu_physical_memory_write(loc_code, buf, strlen(buf));
>> +            g_free(buf);
>> +            goto out;
>> +        }
>> +    }
>> +
>> +fail:
>> +    /*
>> +     * For non-vfio devices and failure cases, 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);
>> + out:
>> +    return true;
>> +}
>> +
>>   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/vfio/pci.c b/hw/vfio/pci.c
>> index 95d666e..dd97258 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -3319,6 +3319,24 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
>>       vdev->req_enabled = false;
>>   }
>>
>> +bool vfio_get_devspec(PCIDevice *pdev, char **value)
>
>
> This function can live in hw/ppc/spapr_pci_vfio.c. Something like 
> object_property_get_str(OBJECT(pdev), "host", NULL) will return the host 
> address, and this is all you really want from VFIO here.

Yes, host address is what I need. Let me do that there.

>
>> +{
>> +    VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
>> +    char path[PATH_MAX];
>> +    struct stat st;
>> +
>> +    snprintf(path, sizeof(path),
>> +             "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/devspec",
>> +             vdev->host.domain, vdev->host.bus, vdev->host.slot,
>> +             vdev->host.function);
>> +
>> +    if (stat(path, &st) < 0) {
>> +        return false;
>> +    }
>> +
>> +    return g_file_get_contents(path, value, NULL, NULL);
>> +}
>> +
>>   static int vfio_initfn(PCIDevice *pdev)
>>   {
>>       VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index af71e8b..d3fad67 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)
>
>
> Are you adding KVMPPC_H_GET_LOC_CODE or all three? :)

Thats tricky, SLOF has already got code for the above, but corresponding
patches havent made it upstream. We need to maintain same number. For
that I have added.

Let me know what will be the other way to add them?

>
>
>> +#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);
>>
>> +bool spapr_h_get_loc_code(sPAPREnvironment *spapr, target_ulong config_addr, target_ulong build,
>
> s/build/buid/  and please put buid before config_addr in the
> parameters lice.

Yes, will do that.

>
>
>> +			  target_ulong loc_code, target_ulong size);
>> +
>>   #define TYPE_SPAPR_RTC "spapr-rtc"
>>
>>   void spapr_rtc_read(DeviceState *dev, struct tm *tm, uint32_t *ns);
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>> index 0d1fb80..6dffa8c 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -140,6 +140,7 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace *as);
>>   void vfio_put_group(VFIOGroup *group);
>>   int vfio_get_device(VFIOGroup *group, const char *name,
>>                       VFIODevice *vbasedev);
>> +bool vfio_get_devspec(PCIDevice *pdev, char **value);
>>
>>   extern const MemoryRegionOps vfio_region_ops;
>>   extern QLIST_HEAD(vfio_group_head, VFIOGroup) vfio_group_list;
>>
>
>
> -- 
> Alexey

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

[Nikunj A Dadhania]

Alexey Kardashevskiy <aik@ozlabs.ru> writes:

>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 95d666e..dd97258 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -3319,6 +3319,24 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
>>       vdev->req_enabled = false;
>>   }
>>
>> +bool vfio_get_devspec(PCIDevice *pdev, char **value)
>
>
> This function can live in hw/ppc/spapr_pci_vfio.c. Something like 
> object_property_get_str(OBJECT(pdev), "host", NULL) will return the host 
> address, and this is all you really want from VFIO here.

Looking more deeper, "host" is part of VFIOPCIDevice, which is not
available in hw/pci/spapr_pci_vfio.c

This was the reason, I had to move this code to hw/vfio/pci.c

Is there a way to get that?

Regards
Nikunj

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

[Alexey Kardashevskiy]

On 03/27/2015 05:21 PM, Nikunj A Dadhania wrote:
> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>
>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>> index 95d666e..dd97258 100644
>>> --- a/hw/vfio/pci.c
>>> +++ b/hw/vfio/pci.c
>>> @@ -3319,6 +3319,24 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
>>>        vdev->req_enabled = false;
>>>    }
>>>
>>> +bool vfio_get_devspec(PCIDevice *pdev, char **value)
>>
>>
>> This function can live in hw/ppc/spapr_pci_vfio.c. Something like
>> object_property_get_str(OBJECT(pdev), "host", NULL) will return the host
>> address, and this is all you really want from VFIO here.
>
> Looking more deeper, "host" is part of VFIOPCIDevice, which is not
> available in hw/pci/spapr_pci_vfio.c


"host" is also a QOM's property of an VFIO PCI device instance. You do not 
access it via VFIOPCIDevice::host directly but via 
object_property_get_str() indirectly (which will fail if the device does 
not have this property).

>
> This was the reason, I had to move this code to hw/vfio/pci.c
>
> Is there a way to get that?



-- 
Alexey

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

[David Gibson]

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

On Thu, Mar 26, 2015 at 12:12:12PM +0530, 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-cde.
> 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>
> ---
>  hw/ppc/spapr_hcall.c          | 10 +++++++++
>  hw/ppc/spapr_pci.c            | 49 +++++++++++++++++++++++++++++++++++++++++++
>  hw/vfio/pci.c                 | 18 ++++++++++++++++
>  include/hw/ppc/spapr.h        |  8 ++++++-
>  include/hw/vfio/vfio-common.h |  1 +
>  5 files changed, 85 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 4f76f1c..a577395 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -928,6 +928,15 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu_,
>      return H_SUCCESS;
>  }
>  
> +static target_ulong h_get_loc_code(PowerPCCPU *cpu, sPAPREnvironment *spapr,
> +				   target_ulong opcode, target_ulong *args)
> +{
> +    if (!spapr_h_get_loc_code(spapr, args[0], args[1], args[2], args[3])) {
> +        return H_PARAMETER;
> +    }
> +    return H_SUCCESS;
> +}

There's no point to this wrapper.  The hypercalls are defined by PAPR,
so making an "spapr" version of the hypercall function is redundant.

> +
>  static spapr_hcall_fn papr_hypercall_table[(MAX_HCALL_OPCODE / 4) + 1];
>  static spapr_hcall_fn kvmppc_hypercall_table[KVMPPC_HCALL_MAX - KVMPPC_HCALL_BASE + 1];
>  
> @@ -1010,6 +1019,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, h_get_loc_code);
>  }
>  
>  type_init(hypercall_register_types)
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 05f4fac..65cdb91 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -35,6 +35,7 @@
>  #include "qemu/error-report.h"
>  
>  #include "hw/pci/pci_bus.h"
> +#include "hw/vfio/vfio-common.h"
>  
>  /* Copied from the kernel arch/powerpc/platforms/pseries/msi.c */
>  #define RTAS_QUERY_FN           0
> @@ -248,6 +249,54 @@ static void spapr_msi_setmsg(PCIDevice *pdev, hwaddr addr, bool msix,
>      }
>  }
>  
> +bool spapr_h_get_loc_code(sPAPREnvironment *spapr, target_ulong config_addr, target_ulong buid,
> +                          target_ulong loc_code, target_ulong size)

bool as a success/failure indication isn't a normal interface.  Just
get rid of the wrapper and return H_ERROR codes directly.

> +{
> +    sPAPRPHBState *sphb = NULL;
> +    PCIDevice *pdev = NULL;
> +    char *buf, path[PATH_MAX];
> +    struct stat st;
> +
> +    sphb = find_phb(spapr, buid);
> +    if (sphb) {
> +        pdev = find_dev(spapr, buid, config_addr);
> +    }
> +
> +    if (!sphb || !pdev) {
> +        error_report("spapr_h_get_loc_code: Device not found");
> +        return false;
> +    }
> +
> +    /* For a VFIO device, get the location in the device tree */
> +    if (pdev->is_vfio && vfio_get_devspec(pdev, &buf)) {
> +        snprintf(path, sizeof(path), "/proc/device-tree%s/ibm,loc-code", buf);
> +        g_free(buf);
> +        if (stat(path, &st) < 0) {
> +            goto fail;

This isn't really an acceptable use of goto.  And the label is badly
named, because it doesn't fail, just fall back to an alternate method.

> +        }
> +
> +        /* A valid file, now read the loc-code */
> +        if (g_file_get_contents(path, &buf, NULL, NULL)) {
> +            cpu_physical_memory_write(loc_code, buf, strlen(buf));
> +            g_free(buf);
> +            goto out;

This could just be a return.

> +        }
> +    }
> +
> +fail:
> +    /*
> +     * For non-vfio devices and failure cases, 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);
> + out:
> +    return true;
> +}
> +
>  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/vfio/pci.c b/hw/vfio/pci.c
> index 95d666e..dd97258 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3319,6 +3319,24 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
>      vdev->req_enabled = false;
>  }
>  
> +bool vfio_get_devspec(PCIDevice *pdev, char **value)
> +{
> +    VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> +    char path[PATH_MAX];
> +    struct stat st;
> +
> +    snprintf(path, sizeof(path),
> +             "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/devspec",
> +             vdev->host.domain, vdev->host.bus, vdev->host.slot,
> +             vdev->host.function);
> +
> +    if (stat(path, &st) < 0) {
> +        return false;
> +    }
> +
> +    return g_file_get_contents(path, value, NULL, NULL);
> +}
> +
>  static int vfio_initfn(PCIDevice *pdev)
>  {
>      VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index af71e8b..d3fad67 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)

Come to that, I don't even understand why you're defining a new hcall.
Why not just put the loc-code in the initial device tree with the
other information.

> +#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);
>  
> +bool spapr_h_get_loc_code(sPAPREnvironment *spapr, target_ulong config_addr, target_ulong build,
> +			  target_ulong loc_code, target_ulong size);
> +
>  #define TYPE_SPAPR_RTC "spapr-rtc"
>  
>  void spapr_rtc_read(DeviceState *dev, struct tm *tm, uint32_t *ns);
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 0d1fb80..6dffa8c 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -140,6 +140,7 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace *as);
>  void vfio_put_group(VFIOGroup *group);
>  int vfio_get_device(VFIOGroup *group, const char *name,
>                      VFIODevice *vbasedev);
> +bool vfio_get_devspec(PCIDevice *pdev, char **value);
>  
>  extern const MemoryRegionOps vfio_region_ops;
>  extern QLIST_HEAD(vfio_group_head, VFIOGroup) vfio_group_list;

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

[Alexey Kardashevskiy]

On 03/27/2015 02:28 PM, David Gibson wrote:
> On Thu, Mar 26, 2015 at 12:12:12PM +0530, 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-cde.
>> 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>
>> ---
>>   hw/ppc/spapr_hcall.c          | 10 +++++++++
>>   hw/ppc/spapr_pci.c            | 49 +++++++++++++++++++++++++++++++++++++++++++
>>   hw/vfio/pci.c                 | 18 ++++++++++++++++
>>   include/hw/ppc/spapr.h        |  8 ++++++-
>>   include/hw/vfio/vfio-common.h |  1 +
>>   5 files changed, 85 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index 4f76f1c..a577395 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -928,6 +928,15 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu_,
>>       return H_SUCCESS;
>>   }
>>
>> +static target_ulong h_get_loc_code(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>> +				   target_ulong opcode, target_ulong *args)
>> +{
>> +    if (!spapr_h_get_loc_code(spapr, args[0], args[1], args[2], args[3])) {
>> +        return H_PARAMETER;
>> +    }
>> +    return H_SUCCESS;
>> +}
>
> There's no point to this wrapper.  The hypercalls are defined by PAPR,
> so making an "spapr" version of the hypercall function is redundant.
>
>> +
>>   static spapr_hcall_fn papr_hypercall_table[(MAX_HCALL_OPCODE / 4) + 1];
>>   static spapr_hcall_fn kvmppc_hypercall_table[KVMPPC_HCALL_MAX - KVMPPC_HCALL_BASE + 1];
>>
>> @@ -1010,6 +1019,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, h_get_loc_code);
>>   }
>>
>>   type_init(hypercall_register_types)
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index 05f4fac..65cdb91 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -35,6 +35,7 @@
>>   #include "qemu/error-report.h"
>>
>>   #include "hw/pci/pci_bus.h"
>> +#include "hw/vfio/vfio-common.h"
>>
>>   /* Copied from the kernel arch/powerpc/platforms/pseries/msi.c */
>>   #define RTAS_QUERY_FN           0
>> @@ -248,6 +249,54 @@ static void spapr_msi_setmsg(PCIDevice *pdev, hwaddr addr, bool msix,
>>       }
>>   }
>>
>> +bool spapr_h_get_loc_code(sPAPREnvironment *spapr, target_ulong config_addr, target_ulong buid,
>> +                          target_ulong loc_code, target_ulong size)
>
> bool as a success/failure indication isn't a normal interface.  Just
> get rid of the wrapper and return H_ERROR codes directly.
>
>> +{
>> +    sPAPRPHBState *sphb = NULL;
>> +    PCIDevice *pdev = NULL;
>> +    char *buf, path[PATH_MAX];
>> +    struct stat st;
>> +
>> +    sphb = find_phb(spapr, buid);
>> +    if (sphb) {
>> +        pdev = find_dev(spapr, buid, config_addr);
>> +    }
>> +
>> +    if (!sphb || !pdev) {
>> +        error_report("spapr_h_get_loc_code: Device not found");
>> +        return false;
>> +    }
>> +
>> +    /* For a VFIO device, get the location in the device tree */
>> +    if (pdev->is_vfio && vfio_get_devspec(pdev, &buf)) {
>> +        snprintf(path, sizeof(path), "/proc/device-tree%s/ibm,loc-code", buf);
>> +        g_free(buf);
>> +        if (stat(path, &st) < 0) {
>> +            goto fail;
>
> This isn't really an acceptable use of goto.  And the label is badly
> named, because it doesn't fail, just fall back to an alternate method.
>
>> +        }
>> +
>> +        /* A valid file, now read the loc-code */
>> +        if (g_file_get_contents(path, &buf, NULL, NULL)) {
>> +            cpu_physical_memory_write(loc_code, buf, strlen(buf));
>> +            g_free(buf);
>> +            goto out;
>
> This could just be a return.
>
>> +        }
>> +    }
>> +
>> +fail:
>> +    /*
>> +     * For non-vfio devices and failure cases, 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);
>> + out:
>> +    return true;
>> +}
>> +
>>   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/vfio/pci.c b/hw/vfio/pci.c
>> index 95d666e..dd97258 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -3319,6 +3319,24 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
>>       vdev->req_enabled = false;
>>   }
>>
>> +bool vfio_get_devspec(PCIDevice *pdev, char **value)
>> +{
>> +    VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
>> +    char path[PATH_MAX];
>> +    struct stat st;
>> +
>> +    snprintf(path, sizeof(path),
>> +             "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/devspec",
>> +             vdev->host.domain, vdev->host.bus, vdev->host.slot,
>> +             vdev->host.function);
>> +
>> +    if (stat(path, &st) < 0) {
>> +        return false;
>> +    }
>> +
>> +    return g_file_get_contents(path, value, NULL, NULL);
>> +}
>> +
>>   static int vfio_initfn(PCIDevice *pdev)
>>   {
>>       VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index af71e8b..d3fad67 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)
>
> Come to that, I don't even understand why you're defining a new hcall.
> Why not just put the loc-code in the initial device tree with the
> other information.


The initial device tree only has PHBs, not devices, and since we can put 
several groups onto the same VFIO PHB, there is no way to set loc-code in 
QEMU before the guest started and SLOF created nodes for actual PCI devices.


>
>> +#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);
>>
>> +bool spapr_h_get_loc_code(sPAPREnvironment *spapr, target_ulong config_addr, target_ulong build,
>> +			  target_ulong loc_code, target_ulong size);
>> +
>>   #define TYPE_SPAPR_RTC "spapr-rtc"
>>
>>   void spapr_rtc_read(DeviceState *dev, struct tm *tm, uint32_t *ns);
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>> index 0d1fb80..6dffa8c 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -140,6 +140,7 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace *as);
>>   void vfio_put_group(VFIOGroup *group);
>>   int vfio_get_device(VFIOGroup *group, const char *name,
>>                       VFIODevice *vbasedev);
>> +bool vfio_get_devspec(PCIDevice *pdev, char **value);
>>
>>   extern const MemoryRegionOps vfio_region_ops;
>>   extern QLIST_HEAD(vfio_group_head, VFIOGroup) vfio_group_list;
>


-- 
Alexey

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

[Nikunj A Dadhania]

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

> On Thu, Mar 26, 2015 at 12:12:12PM +0530, 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-cde.
>> 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>
>> ---
>>  hw/ppc/spapr_hcall.c          | 10 +++++++++
>>  hw/ppc/spapr_pci.c            | 49 +++++++++++++++++++++++++++++++++++++++++++
>>  hw/vfio/pci.c                 | 18 ++++++++++++++++
>>  include/hw/ppc/spapr.h        |  8 ++++++-
>>  include/hw/vfio/vfio-common.h |  1 +
>>  5 files changed, 85 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index 4f76f1c..a577395 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -928,6 +928,15 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu_,
>>      return H_SUCCESS;
>>  }
>>  
>> +static target_ulong h_get_loc_code(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>> +				   target_ulong opcode, target_ulong *args)
>> +{
>> +    if (!spapr_h_get_loc_code(spapr, args[0], args[1], args[2], args[3])) {
>> +        return H_PARAMETER;
>> +    }
>> +    return H_SUCCESS;
>> +}
>
> There's no point to this wrapper.  The hypercalls are defined by PAPR,
> so making an "spapr" version of the hypercall function is redundant.

I was thinking of new devices like SRIOV, etc, we land here and then
bifurcate accordingly.

>
>> +
>>  static spapr_hcall_fn papr_hypercall_table[(MAX_HCALL_OPCODE / 4) + 1];
>>  static spapr_hcall_fn kvmppc_hypercall_table[KVMPPC_HCALL_MAX - KVMPPC_HCALL_BASE + 1];
>>  
>> @@ -1010,6 +1019,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, h_get_loc_code);
>>  }
>>  
>>  type_init(hypercall_register_types)
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index 05f4fac..65cdb91 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -35,6 +35,7 @@
>>  #include "qemu/error-report.h"
>>  
>>  #include "hw/pci/pci_bus.h"
>> +#include "hw/vfio/vfio-common.h"
>>  
>>  /* Copied from the kernel arch/powerpc/platforms/pseries/msi.c */
>>  #define RTAS_QUERY_FN           0
>> @@ -248,6 +249,54 @@ static void spapr_msi_setmsg(PCIDevice *pdev, hwaddr addr, bool msix,
>>      }
>>  }
>>  
>> +bool spapr_h_get_loc_code(sPAPREnvironment *spapr, target_ulong config_addr, target_ulong buid,
>> +                          target_ulong loc_code, target_ulong size)
>
> bool as a success/failure indication isn't a normal interface.  Just
> get rid of the wrapper and return H_ERROR codes directly.

Ok

>
>> +{
>> +    sPAPRPHBState *sphb = NULL;
>> +    PCIDevice *pdev = NULL;
>> +    char *buf, path[PATH_MAX];
>> +    struct stat st;
>> +
>> +    sphb = find_phb(spapr, buid);
>> +    if (sphb) {
>> +        pdev = find_dev(spapr, buid, config_addr);
>> +    }
>> +
>> +    if (!sphb || !pdev) {
>> +        error_report("spapr_h_get_loc_code: Device not found");
>> +        return false;
>> +    }
>> +
>> +    /* For a VFIO device, get the location in the device tree */
>> +    if (pdev->is_vfio && vfio_get_devspec(pdev, &buf)) {
>> +        snprintf(path, sizeof(path), "/proc/device-tree%s/ibm,loc-code", buf);
>> +        g_free(buf);
>> +        if (stat(path, &st) < 0) {
>> +            goto fail;
>
> This isn't really an acceptable use of goto.  And the label is badly
> named, because it doesn't fail, just fall back to an alternate method.

Sure, as per your previous comment, will update the return type and
return error/success codes directly.

>
>> +        }
>> +
>> +        /* A valid file, now read the loc-code */
>> +        if (g_file_get_contents(path, &buf, NULL, NULL)) {
>> +            cpu_physical_memory_write(loc_code, buf, strlen(buf));
>> +            g_free(buf);
>> +            goto out;
>
> This could just be a return.

Yes.

>
>> +        }
>> +    }
>> +
>> +fail:
>> +    /*
>> +     * For non-vfio devices and failure cases, 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);
>> + out:
>> +    return true;
>> +}
>> +
>>  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/vfio/pci.c b/hw/vfio/pci.c
>> index 95d666e..dd97258 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -3319,6 +3319,24 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
>>      vdev->req_enabled = false;
>>  }
>>  
>> +bool vfio_get_devspec(PCIDevice *pdev, char **value)
>> +{
>> +    VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
>> +    char path[PATH_MAX];
>> +    struct stat st;
>> +
>> +    snprintf(path, sizeof(path),
>> +             "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/devspec",
>> +             vdev->host.domain, vdev->host.bus, vdev->host.slot,
>> +             vdev->host.function);
>> +
>> +    if (stat(path, &st) < 0) {
>> +        return false;
>> +    }
>> +
>> +    return g_file_get_contents(path, value, NULL, NULL);
>> +}
>> +
>>  static int vfio_initfn(PCIDevice *pdev)
>>  {
>>      VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index af71e8b..d3fad67 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)
>
> Come to that, I don't even understand why you're defining a new hcall.
> Why not just put the loc-code in the initial device tree with the
> other information.

Alexey, has already answered that :-)

PCI enumeration happens in SLOF, keep on thinking of moving that to
qemu.

>
>> +#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);
>>  
>> +bool spapr_h_get_loc_code(sPAPREnvironment *spapr, target_ulong config_addr, target_ulong build,
>> +			  target_ulong loc_code, target_ulong size);
>> +
>>  #define TYPE_SPAPR_RTC "spapr-rtc"
>>  
>>  void spapr_rtc_read(DeviceState *dev, struct tm *tm, uint32_t *ns);
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>> index 0d1fb80..6dffa8c 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -140,6 +140,7 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace *as);
>>  void vfio_put_group(VFIOGroup *group);
>>  int vfio_get_device(VFIOGroup *group, const char *name,
>>                      VFIODevice *vbasedev);
>> +bool vfio_get_devspec(PCIDevice *pdev, char **value);
>>  
>>  extern const MemoryRegionOps vfio_region_ops;
>>  extern QLIST_HEAD(vfio_group_head, VFIOGroup) vfio_group_list;
>
> -- 
> 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 2/2] spapr: populate ibm,loc-code

[David Gibson]

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

On Fri, Mar 27, 2015 at 10:04:27AM +0530, Nikunj A Dadhania wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > On Thu, Mar 26, 2015 at 12:12:12PM +0530, 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-cde.
> >> 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>
> >> ---
> >>  hw/ppc/spapr_hcall.c          | 10 +++++++++
> >>  hw/ppc/spapr_pci.c            | 49 +++++++++++++++++++++++++++++++++++++++++++
> >>  hw/vfio/pci.c                 | 18 ++++++++++++++++
> >>  include/hw/ppc/spapr.h        |  8 ++++++-
> >>  include/hw/vfio/vfio-common.h |  1 +
> >>  5 files changed, 85 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> >> index 4f76f1c..a577395 100644
> >> --- a/hw/ppc/spapr_hcall.c
> >> +++ b/hw/ppc/spapr_hcall.c
> >> @@ -928,6 +928,15 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu_,
> >>      return H_SUCCESS;
> >>  }
> >>  
> >> +static target_ulong h_get_loc_code(PowerPCCPU *cpu, sPAPREnvironment *spapr,
> >> +				   target_ulong opcode, target_ulong *args)
> >> +{
> >> +    if (!spapr_h_get_loc_code(spapr, args[0], args[1], args[2], args[3])) {
> >> +        return H_PARAMETER;
> >> +    }
> >> +    return H_SUCCESS;
> >> +}
> >
> > There's no point to this wrapper.  The hypercalls are defined by PAPR,
> > so making an "spapr" version of the hypercall function is redundant.
> 
> I was thinking of new devices like SRIOV, etc, we land here and then
> bifurcate accordingly.

???  They'd still belong under spapr_pci.c one way or another.

> 
> >
> >> +
> >>  static spapr_hcall_fn papr_hypercall_table[(MAX_HCALL_OPCODE / 4) + 1];
> >>  static spapr_hcall_fn kvmppc_hypercall_table[KVMPPC_HCALL_MAX - KVMPPC_HCALL_BASE + 1];
> >>  
> >> @@ -1010,6 +1019,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, h_get_loc_code);
> >>  }
> >>  
> >>  type_init(hypercall_register_types)
> >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> >> index 05f4fac..65cdb91 100644
> >> --- a/hw/ppc/spapr_pci.c
> >> +++ b/hw/ppc/spapr_pci.c
> >> @@ -35,6 +35,7 @@
> >>  #include "qemu/error-report.h"
> >>  
> >>  #include "hw/pci/pci_bus.h"
> >> +#include "hw/vfio/vfio-common.h"
> >>  
> >>  /* Copied from the kernel arch/powerpc/platforms/pseries/msi.c */
> >>  #define RTAS_QUERY_FN           0
> >> @@ -248,6 +249,54 @@ static void spapr_msi_setmsg(PCIDevice *pdev, hwaddr addr, bool msix,
> >>      }
> >>  }
> >>  
> >> +bool spapr_h_get_loc_code(sPAPREnvironment *spapr, target_ulong config_addr, target_ulong buid,
> >> +                          target_ulong loc_code, target_ulong size)
> >
> > bool as a success/failure indication isn't a normal interface.  Just
> > get rid of the wrapper and return H_ERROR codes directly.
> 
> Ok
> 
> >
> >> +{
> >> +    sPAPRPHBState *sphb = NULL;
> >> +    PCIDevice *pdev = NULL;
> >> +    char *buf, path[PATH_MAX];
> >> +    struct stat st;
> >> +
> >> +    sphb = find_phb(spapr, buid);
> >> +    if (sphb) {
> >> +        pdev = find_dev(spapr, buid, config_addr);
> >> +    }
> >> +
> >> +    if (!sphb || !pdev) {
> >> +        error_report("spapr_h_get_loc_code: Device not found");
> >> +        return false;
> >> +    }
> >> +
> >> +    /* For a VFIO device, get the location in the device tree */
> >> +    if (pdev->is_vfio && vfio_get_devspec(pdev, &buf)) {
> >> +        snprintf(path, sizeof(path), "/proc/device-tree%s/ibm,loc-code", buf);
> >> +        g_free(buf);
> >> +        if (stat(path, &st) < 0) {
> >> +            goto fail;
> >
> > This isn't really an acceptable use of goto.  And the label is badly
> > named, because it doesn't fail, just fall back to an alternate method.
> 
> Sure, as per your previous comment, will update the return type and
> return error/success codes directly.
> 
> >
> >> +        }
> >> +
> >> +        /* A valid file, now read the loc-code */
> >> +        if (g_file_get_contents(path, &buf, NULL, NULL)) {
> >> +            cpu_physical_memory_write(loc_code, buf, strlen(buf));
> >> +            g_free(buf);
> >> +            goto out;
> >
> > This could just be a return.
> 
> Yes.
> 
> >
> >> +        }
> >> +    }
> >> +
> >> +fail:
> >> +    /*
> >> +     * For non-vfio devices and failure cases, 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);
> >> + out:
> >> +    return true;
> >> +}
> >> +
> >>  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/vfio/pci.c b/hw/vfio/pci.c
> >> index 95d666e..dd97258 100644
> >> --- a/hw/vfio/pci.c
> >> +++ b/hw/vfio/pci.c
> >> @@ -3319,6 +3319,24 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
> >>      vdev->req_enabled = false;
> >>  }
> >>  
> >> +bool vfio_get_devspec(PCIDevice *pdev, char **value)
> >> +{
> >> +    VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> >> +    char path[PATH_MAX];
> >> +    struct stat st;
> >> +
> >> +    snprintf(path, sizeof(path),
> >> +             "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/devspec",
> >> +             vdev->host.domain, vdev->host.bus, vdev->host.slot,
> >> +             vdev->host.function);
> >> +
> >> +    if (stat(path, &st) < 0) {
> >> +        return false;
> >> +    }
> >> +
> >> +    return g_file_get_contents(path, value, NULL, NULL);
> >> +}
> >> +
> >>  static int vfio_initfn(PCIDevice *pdev)
> >>  {
> >>      VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> >> index af71e8b..d3fad67 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)
> >
> > Come to that, I don't even understand why you're defining a new hcall.
> > Why not just put the loc-code in the initial device tree with the
> > other information.
> 
> Alexey, has already answered that :-)
> 
> PCI enumeration happens in SLOF, keep on thinking of moving that to
> qemu.

I think we should move it to qemu.  We'll shortly need code in qemu to
generate PCI device nodes for hotplug, so we might as well enumerate
the whole tree.

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

[Nikunj A Dadhania]

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

> On Fri, Mar 27, 2015 at 10:04:27AM +0530, Nikunj A Dadhania wrote:
>> David Gibson <david@gibson.dropbear.id.au> writes:
>> 
>> > On Thu, Mar 26, 2015 at 12:12:12PM +0530, 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-cde.
>> >> 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>
>> >> ---
>> >>  hw/ppc/spapr_hcall.c          | 10 +++++++++
>> >>  hw/ppc/spapr_pci.c            | 49 +++++++++++++++++++++++++++++++++++++++++++
>> >>  hw/vfio/pci.c                 | 18 ++++++++++++++++
>> >>  include/hw/ppc/spapr.h        |  8 ++++++-
>> >>  include/hw/vfio/vfio-common.h |  1 +
>> >>  5 files changed, 85 insertions(+), 1 deletion(-)
>> >> 
>> >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> >> index 4f76f1c..a577395 100644
>> >> --- a/hw/ppc/spapr_hcall.c
>> >> +++ b/hw/ppc/spapr_hcall.c
>> >> @@ -928,6 +928,15 @@ static target_ulong h_client_architecture_support(PowerPCCPU *cpu_,
>> >>      return H_SUCCESS;
>> >>  }
>> >>  
>> >> +static target_ulong h_get_loc_code(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>> >> +				   target_ulong opcode, target_ulong *args)
>> >> +{
>> >> +    if (!spapr_h_get_loc_code(spapr, args[0], args[1], args[2], args[3])) {
>> >> +        return H_PARAMETER;
>> >> +    }
>> >> +    return H_SUCCESS;
>> >> +}
>> >
>> > There's no point to this wrapper.  The hypercalls are defined by PAPR,
>> > so making an "spapr" version of the hypercall function is redundant.
>> 
>> I was thinking of new devices like SRIOV, etc, we land here and then
>> bifurcate accordingly.
>
> ???  They'd still belong under spapr_pci.c one way or another.

Yes, you are right.

>
>> 
>> >
>> >> +
>> >>  static spapr_hcall_fn papr_hypercall_table[(MAX_HCALL_OPCODE / 4) + 1];
>> >>  static spapr_hcall_fn kvmppc_hypercall_table[KVMPPC_HCALL_MAX - KVMPPC_HCALL_BASE + 1];
>> >>  
>> >> @@ -1010,6 +1019,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, h_get_loc_code);
>> >>  }
>> >>  
>> >>  type_init(hypercall_register_types)
>> >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> >> index 05f4fac..65cdb91 100644
>> >> --- a/hw/ppc/spapr_pci.c
>> >> +++ b/hw/ppc/spapr_pci.c
>> >> @@ -35,6 +35,7 @@
>> >>  #include "qemu/error-report.h"
>> >>  
>> >>  #include "hw/pci/pci_bus.h"
>> >> +#include "hw/vfio/vfio-common.h"
>> >>  
>> >>  /* Copied from the kernel arch/powerpc/platforms/pseries/msi.c */
>> >>  #define RTAS_QUERY_FN           0
>> >> @@ -248,6 +249,54 @@ static void spapr_msi_setmsg(PCIDevice *pdev, hwaddr addr, bool msix,
>> >>      }
>> >>  }
>> >>  
>> >> +bool spapr_h_get_loc_code(sPAPREnvironment *spapr, target_ulong config_addr, target_ulong buid,
>> >> +                          target_ulong loc_code, target_ulong size)
>> >
>> > bool as a success/failure indication isn't a normal interface.  Just
>> > get rid of the wrapper and return H_ERROR codes directly.
>> 
>> Ok
>> 
>> >
>> >> +{
>> >> +    sPAPRPHBState *sphb = NULL;
>> >> +    PCIDevice *pdev = NULL;
>> >> +    char *buf, path[PATH_MAX];
>> >> +    struct stat st;
>> >> +
>> >> +    sphb = find_phb(spapr, buid);
>> >> +    if (sphb) {
>> >> +        pdev = find_dev(spapr, buid, config_addr);
>> >> +    }
>> >> +
>> >> +    if (!sphb || !pdev) {
>> >> +        error_report("spapr_h_get_loc_code: Device not found");
>> >> +        return false;
>> >> +    }
>> >> +
>> >> +    /* For a VFIO device, get the location in the device tree */
>> >> +    if (pdev->is_vfio && vfio_get_devspec(pdev, &buf)) {
>> >> +        snprintf(path, sizeof(path), "/proc/device-tree%s/ibm,loc-code", buf);
>> >> +        g_free(buf);
>> >> +        if (stat(path, &st) < 0) {
>> >> +            goto fail;
>> >
>> > This isn't really an acceptable use of goto.  And the label is badly
>> > named, because it doesn't fail, just fall back to an alternate method.
>> 
>> Sure, as per your previous comment, will update the return type and
>> return error/success codes directly.
>> 
>> >
>> >> +        }
>> >> +
>> >> +        /* A valid file, now read the loc-code */
>> >> +        if (g_file_get_contents(path, &buf, NULL, NULL)) {
>> >> +            cpu_physical_memory_write(loc_code, buf, strlen(buf));
>> >> +            g_free(buf);
>> >> +            goto out;
>> >
>> > This could just be a return.
>> 
>> Yes.
>> 
>> >
>> >> +        }
>> >> +    }
>> >> +
>> >> +fail:
>> >> +    /*
>> >> +     * For non-vfio devices and failure cases, 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);
>> >> + out:
>> >> +    return true;
>> >> +}
>> >> +
>> >>  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/vfio/pci.c b/hw/vfio/pci.c
>> >> index 95d666e..dd97258 100644
>> >> --- a/hw/vfio/pci.c
>> >> +++ b/hw/vfio/pci.c
>> >> @@ -3319,6 +3319,24 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
>> >>      vdev->req_enabled = false;
>> >>  }
>> >>  
>> >> +bool vfio_get_devspec(PCIDevice *pdev, char **value)
>> >> +{
>> >> +    VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
>> >> +    char path[PATH_MAX];
>> >> +    struct stat st;
>> >> +
>> >> +    snprintf(path, sizeof(path),
>> >> +             "/sys/bus/pci/devices/%04x:%02x:%02x.%01x/devspec",
>> >> +             vdev->host.domain, vdev->host.bus, vdev->host.slot,
>> >> +             vdev->host.function);
>> >> +
>> >> +    if (stat(path, &st) < 0) {
>> >> +        return false;
>> >> +    }
>> >> +
>> >> +    return g_file_get_contents(path, value, NULL, NULL);
>> >> +}
>> >> +
>> >>  static int vfio_initfn(PCIDevice *pdev)
>> >>  {
>> >>      VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
>> >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> >> index af71e8b..d3fad67 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)
>> >
>> > Come to that, I don't even understand why you're defining a new hcall.
>> > Why not just put the loc-code in the initial device tree with the
>> > other information.
>> 
>> Alexey, has already answered that :-)
>> 
>> PCI enumeration happens in SLOF, keep on thinking of moving that to
>> qemu.
>
> I think we should move it to qemu.  We'll shortly need code in qemu to
> generate PCI device nodes for hotplug, so we might as well enumerate
> the whole tree.

Right. Will I look at this.

Regards
Nikunj

Re: [Qemu-devel] [PATCH 1/2] vfio-pci: add flag to identify vfio pci device

[Alexey Kardashevskiy]

On 03/26/2015 05:42 PM, Nikunj A Dadhania wrote:
>  From PCIDevice there is no way to know if the device has a backing
> vfio device.
>
> sPAPR guests inherits the "ibm,loc-code" from the pci pass through
> device in hypervisor. This helps in identifying the device if there is
> any failures using this "ibm,loc-code" for RAS capabilities.

First, you can use object_dynamic_cast(OBJECT(obj), "vfio-pci") to know if 
it is a VFIO device.

Second, rather than adding this flag here, I would fix the second patch to 
check if PHB is of the TYPE_SPAPR_PCI_VFIO_HOST_BRIDGE type 
("spapr-pci-vfio-host-bridge"), using the same QOM mechanism.



> CC: Alex Williamson <alex.williamson@redhat.com>
> CC: Alexander Graf <agraf@suse.de>
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> ---
>   hw/vfio/pci.c        | 2 ++
>   include/hw/pci/pci.h | 2 ++
>   2 files changed, 4 insertions(+)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 6b80539..95d666e 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3483,6 +3483,7 @@ static void vfio_instance_finalize(Object *obj)
>       VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pci_dev);
>       VFIOGroup *group = vdev->vbasedev.group;
>
> +    pci_dev->is_vfio = false;
>       vfio_unmap_bars(vdev);
>       g_free(vdev->emulated_config_bits);
>       g_free(vdev->rom);
> @@ -3542,6 +3543,7 @@ static void vfio_instance_init(Object *obj)
>       PCIDevice *pci_dev = PCI_DEVICE(obj);
>       VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, PCI_DEVICE(obj));
>
> +    pci_dev->is_vfio = true;
>       device_add_bootindex_property(obj, &vdev->bootindex,
>                                     "bootindex", NULL,
>                                     &pci_dev->qdev, NULL);
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index b97c295..0ddc830 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -291,6 +291,8 @@ struct PCIDevice {
>       /* SHPC */
>       SHPCDevice *shpc;
>
> +    bool is_vfio;
> +
>       /* Location of option rom */
>       char *romfile;
>       bool has_rom;
>


-- 
Alexey

Re: [Qemu-devel] [PATCH 1/2] vfio-pci: add flag to identify vfio pci device

[Alex Williamson]

On Thu, 2015-03-26 at 12:12 +0530, Nikunj A Dadhania wrote:
> From PCIDevice there is no way to know if the device has a backing
> vfio device.
> 
> sPAPR guests inherits the "ibm,loc-code" from the pci pass through
> device in hypervisor. This helps in identifying the device if there is
> any failures using this "ibm,loc-code" for RAS capabilities.
> 
> CC: Alex Williamson <alex.williamson@redhat.com>
> CC: Alexander Graf <agraf@suse.de>
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> ---
>  hw/vfio/pci.c        | 2 ++
>  include/hw/pci/pci.h | 2 ++
>  2 files changed, 4 insertions(+)

I agree with the other comments on this series, the vfio information can
be found using existing object and property functions.  Nak to the vfio
related changes.  Also, please us cover letters for multi-patch series.
Thanks,

Alex

Re: [Qemu-devel] [PATCH 1/2] vfio-pci: add flag to identify vfio pci device

[Nikunj A Dadhania]

Alex Williamson <alex.williamson@redhat.com> writes:

> On Thu, 2015-03-26 at 12:12 +0530, Nikunj A Dadhania wrote:
>> From PCIDevice there is no way to know if the device has a backing
>> vfio device.
>> 
>> sPAPR guests inherits the "ibm,loc-code" from the pci pass through
>> device in hypervisor. This helps in identifying the device if there is
>> any failures using this "ibm,loc-code" for RAS capabilities.
>> 
>> CC: Alex Williamson <alex.williamson@redhat.com>
>> CC: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>> ---
>>  hw/vfio/pci.c        | 2 ++
>>  include/hw/pci/pci.h | 2 ++
>>  2 files changed, 4 insertions(+)
>
> I agree with the other comments on this series, the vfio information can
> be found using existing object and property functions.  Nak to the vfio
> related changes. 

Yes, I have dropped this patch for the series.

http://lists.gnu.org/archive/html/qemu-devel/2015-03/msg05940.html

There is discussion around the patch, regarding doing PCI enumeration in
Qemu in place of SLOF.

> Also, please us cover letters for multi-patch series.

Sure, will take care about it.

Regards,
Nikunj