From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:51832)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <dgibson@ozlabs.org>) id 1YbN9g-0004ZP-Vw
	for qemu-devel@nongnu.org; Fri, 27 Mar 2015 01:50:06 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <dgibson@ozlabs.org>) id 1YbN9e-00076t-8Y
	for qemu-devel@nongnu.org; Fri, 27 Mar 2015 01:50:04 -0400
Date: Fri, 27 Mar 2015 15:57:23 +1100
From: David Gibson <david@gibson.dropbear.id.au>
Message-ID: <20150327045723.GE2900@voom.fritz.box>
References: <1427352132-1762-1-git-send-email-nikunj@linux.vnet.ibm.com>
	<1427352132-1762-2-git-send-email-nikunj@linux.vnet.ibm.com>
	<20150327032800.GA2900@voom.fritz.box>
	<87bnjfm52k.fsf@linux.vnet.ibm.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha1;
	protocol="application/pgp-signature"; boundary="ey/N+yb7u/X9mFhi"
Content-Disposition: inline
In-Reply-To: <87bnjfm52k.fsf@linux.vnet.ibm.com>
Subject: Re: [Qemu-devel] [PATCH 2/2] spapr: populate ibm,loc-code
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
Cc: aik@ozlabs.ru, qemu-ppc@nongnu.org, qemu-devel@nongnu.org, agraf@suse.de


--ey/N+yb7u/X9mFhi
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, Mar 27, 2015 at 10:04:27AM +0530, Nikunj A Dadhania wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
>=20
> > 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 =E2=80=9Cibm,loc-code=E2=80=9D property with a value that represen=
ts the location
> >> code for that hardware entity.
> >>=20
> >> 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>
> >>=20
> >> 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(-)
> >>=20
> >> 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;
> >>  }
> >> =20
> >> +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.
>=20
> 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.

>=20
> >
> >> +
> >>  static spapr_hcall_fn papr_hypercall_table[(MAX_HCALL_OPCODE / 4) + 1=
];
> >>  static spapr_hcall_fn kvmppc_hypercall_table[KVMPPC_HCALL_MAX - KVMPP=
C_HCALL_BASE + 1];
> >> =20
> >> @@ -1010,6 +1019,7 @@ static void hypercall_register_types(void)
> >> =20
> >>      /* ibm,client-architecture-support support */
> >>      spapr_register_hypercall(KVMPPC_H_CAS, h_client_architecture_supp=
ort);
> >> +    spapr_register_hypercall(KVMPPC_H_GET_LOC_CODE, h_get_loc_code);
> >>  }
> >> =20
> >>  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"
> >> =20
> >>  #include "hw/pci/pci_bus.h"
> >> +#include "hw/vfio/vfio-common.h"
> >> =20
> >>  /* 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, hwa=
ddr addr, bool msix,
> >>      }
> >>  }
> >> =20
> >> +bool spapr_h_get_loc_code(sPAPREnvironment *spapr, target_ulong confi=
g_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.
>=20
> Ok
>=20
> >
> >> +{
> >> +    sPAPRPHBState *sphb =3D NULL;
> >> +    PCIDevice *pdev =3D NULL;
> >> +    char *buf, path[PATH_MAX];
> >> +    struct stat st;
> >> +
> >> +    sphb =3D find_phb(spapr, buid);
> >> +    if (sphb) {
> >> +        pdev =3D 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-cod=
e", 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.
>=20
> Sure, as per your previous comment, will update the return type and
> return error/success codes directly.
>=20
> >
> >> +        }
> >> +
> >> +        /* 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.
>=20
> Yes.
>=20
> >
> >> +        }
> >> +    }
> >> +
> >> +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 *sp=
apr,
> >>                                  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(VFIOPC=
IDevice *vdev)
> >>      vdev->req_enabled =3D false;
> >>  }
> >> =20
> >> +bool vfio_get_devspec(PCIDevice *pdev, char **value)
> >> +{
> >> +    VFIOPCIDevice *vdev =3D 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 =3D 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.
>=20
> Alexey, has already answered that :-)
>=20
> 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.

--=20
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

--ey/N+yb7u/X9mFhi
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBAgAGBQJVFOMzAAoJEGw4ysog2bOS+WsP/1NOzeC25Q6bFnzGCKUh7zzA
6ZwpxYdu9vWjCBfiYnTVuoAkJD8lG/aiIfkhbRl4K+pxfT7dAoBHxeQUPNSDDrbg
esxrp5XVjGSQiEq8ZkbnNd7Kw6Fg7oN8r/YjiAHGdOK4Quw7ez69z00r/i8lTSzd
JaR5NHyKLRqPfUSTa6NtVBsmAk2bjtrjjtcAhoKppILErdAFdKi0S8y09QiQ3V+E
LjaG9pqiHZx7t3FXJVlU9ndRdZMkeTS+nCRg3i/p9YylNJMMpAEQwy9lMATV+yXN
8s+Pr/E1K6v5b8CFTy0IAOtc8TyqTJHwL2AF2yJnKT7KRocw7Wa3AAbB1l8lRPT2
r0EwVVImgdDkVPTVsgY/Ybio3rnDkGUMLAXaTsTAH5h9UGy0OVxN9P6l3UXFC53W
UPE7I7+EzlBELM93mqiJBnvG4M/xf0jXK6nG0utnu6w4LXOL219/dApnj/eGe4jY
RDMl+zkz495oDnklHv5r4CbvUC/PGMLl5p8NsEAg28sEiIzrtXI3CH7a15SIDJky
8SOAuHdUcUKQFxyJP6hWZWyU0NLhzxy65BcHhBLRvZb5MaFF1+5YINEHgdP4D8Hw
zHNYhDDeyv+tBWu17gqIbqfuiAaZNipHqV23qvI4utYKCPvaGdIJrabNsLMw6q5S
F7zPsKFWo2l2FRZqZke8
=IpuC
-----END PGP SIGNATURE-----

--ey/N+yb7u/X9mFhi--