On Fri, Mar 27, 2015 at 10:04:27AM +0530, Nikunj A Dadhania wrote: > David Gibson <[email protected]> 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 <[email protected]> > >> --- > >> 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
pgpGqAjjGoqBH.pgp
Description: PGP signature
