David Gibson <da...@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 <nik...@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
