From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:43146)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <aik@ozlabs.ru>) id 1YcPHJ-0006qR-AM
	for qemu-devel@nongnu.org; Sun, 29 Mar 2015 22:18:14 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <aik@ozlabs.ru>) id 1YcPHF-0003pC-81
	for qemu-devel@nongnu.org; Sun, 29 Mar 2015 22:18:13 -0400
Received: from mail-pa0-f51.google.com ([209.85.220.51]:33450)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <aik@ozlabs.ru>) id 1YcPHE-0003ot-Va
	for qemu-devel@nongnu.org; Sun, 29 Mar 2015 22:18:09 -0400
Received: by pacgg7 with SMTP id gg7so23410308pac.0
	for <qemu-devel@nongnu.org>; Sun, 29 Mar 2015 19:18:07 -0700 (PDT)
Message-ID: <5518B259.1070609@ozlabs.ru>
Date: Mon, 30 Mar 2015 13:18:01 +1100
From: Alexey Kardashevskiy <aik@ozlabs.ru>
MIME-Version: 1.0
References: <1427449798-10345-1-git-send-email-nikunj@linux.vnet.ibm.com>
In-Reply-To: <1427449798-10345-1-git-send-email-nikunj@linux.vnet.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Subject: Re: [Qemu-devel] [PATCH v2] 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>, qemu-devel@nongnu.org
Cc: qemu-ppc@nongnu.org, agraf@suse.de, david@gibson.dropbear.id.au

On 03/27/2015 08:49 PM, Nikunj A Dadhania wrote:
> Each hardware instance has a platform unique location code.  The OF
> device tree that describes a part of a hardware entity must include
> the “ibm,loc-code” property with a value that represents the location
> code for that hardware entity.
>
> Introduce an hcall to populate ibm,loc-code.
> 1) PCI passthru devices need to identify with its own ibm,loc-code
>     available on the host.
> 2) Emulated devices encode as following: qemu_<name>:<slot>.<fn>
>
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> ---
>
> Changelog
> v1:
> * Dropped is_vfio patch and using TYPE_SPAPR_PCI_VFIO_HOST_BRIDGE
>    to recognise vfio devices
> * Removed wrapper for hcall
> * Added sPAPRPHBClass::get_loc_code
>
>   hw/ppc/spapr_hcall.c        |  1 +
>   hw/ppc/spapr_pci.c          | 38 +++++++++++++++++++++++++++++++++
>   hw/ppc/spapr_pci_vfio.c     | 51 +++++++++++++++++++++++++++++++++++++++++++++
>   include/hw/pci-host/spapr.h |  1 +
>   include/hw/ppc/spapr.h      |  8 ++++++-
>   5 files changed, 98 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 4f76f1c..b394681 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1010,6 +1010,7 @@ static void hypercall_register_types(void)
>
>       /* ibm,client-architecture-support support */
>       spapr_register_hypercall(KVMPPC_H_CAS, h_client_architecture_support);
> +    spapr_register_hypercall(KVMPPC_H_GET_LOC_CODE, phb_get_loc_code);
>   }
>
>   type_init(hypercall_register_types)
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 05f4fac..c2ee476 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -248,6 +248,44 @@ static void spapr_msi_setmsg(PCIDevice *pdev, hwaddr addr, bool msix,
>       }
>   }
>
> +target_ulong phb_get_loc_code(PowerPCCPU *cpu, sPAPREnvironment *spapr,
> +                              target_ulong opcode, target_ulong *args)
> +{
> +    sPAPRPHBState *sphb = NULL;
> +    sPAPRPHBClass *spc = NULL;
> +    char *buf = NULL, path[PATH_MAX];
> +    PCIDevice *pdev;
> +    target_ulong buid = args[0];
> +    target_ulong config_addr = args[1];
> +    target_ulong loc_code = args[2];
> +    target_ulong size = args[3];
> +
> +    sphb = find_phb(spapr, buid);
> +    pdev = find_dev(spapr, buid, config_addr);
> +
> +    if (!sphb || !pdev) {
> +        return H_PARAMETER;
> +    }
> +
> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
> +    if (spc->get_loc_code && spc->get_loc_code(sphb, pdev, &buf)) {
> +        cpu_physical_memory_write(loc_code, buf, strlen(buf));
> +        g_free(buf);
> +        return H_SUCCESS;
> +    }
> +
> +    /*
> +     * For non-vfio devices and failures make up the location code out
> +     * of the name, slot and function.
> +     *
> +     *       qemu_<name>:<slot>.<fn>
> +     */
> +    snprintf(path, sizeof(path), "qemu_%s:%02d.%1d", pdev->name,
> +             PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> +    cpu_physical_memory_write(loc_code, path, size);


Move the chunk above to a sPAPRPHBState::get_loc_code. And I'd add PHB's 
@index in the device name to make it unique across the guest.


> +    return H_SUCCESS;
> +}
> +
>   static void rtas_ibm_change_msi(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>                                   uint32_t token, uint32_t nargs,
>                                   target_ulong args, uint32_t nret,
> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
> index 99a1be5..bfdfa67 100644
> --- a/hw/ppc/spapr_pci_vfio.c
> +++ b/hw/ppc/spapr_pci_vfio.c
> @@ -171,6 +171,56 @@ static int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, int option)
>       return RTAS_OUT_SUCCESS;
>   }
>
> +static bool spapr_phb_vfio_get_devspec(PCIDevice *pdev, char **value)

s/value/devspec/ ?

> +{
> +    char *host;
> +    char path[PATH_MAX];
> +    struct stat st;
> +
> +    host = object_property_get_str(OBJECT(pdev), "host", NULL);
> +    if (!host) {
> +        return false;
> +    }
> +
> +    snprintf(path, sizeof(path), "/sys/bus/pci/devices/%s/devspec", host);
> +    g_free(host);
> +    if (stat(path, &st) < 0) {
> +        return false;
> +    }
> +
> +    return g_file_get_contents(path, value, NULL, NULL);


g_file_get_contents() is expected to return FALSE if the file is missing so 
stat() seems redundant here.


> +}
> +
> +static int spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb,  PCIDevice *pdev,
> +                                        char **loc_code)
> +{
> +    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
> +    char path[PATH_MAX], *buf = NULL;
> +    struct stat st;
> +
> +    /* Non VFIO devices */
> +    if (!svphb) {
> +        return false;


The function returns int, not bool. s/false/-1/ and s/true/0/ please.

> +    }
> +
> +    /* We have a vfio host bridge lets get the path. */
> +    if (!spapr_phb_vfio_get_devspec(pdev, &buf)) {

s/buf/devspec/


> +        return false;
> +    }
> +
> +    snprintf(path, sizeof(path), "/proc/device-tree%s/ibm,loc-code", buf);
> +    g_free(buf);
> +    if (stat(path, &st) < 0) {
> +            return false;

Just return what stat() returned.

> +    }
> +
> +    /* A valid file, now read the loc-code */
> +    if (g_file_get_contents(path, loc_code, NULL, NULL)) {
> +        return true;
> +    }
> +    return false;
> +}
> +
>   static int spapr_phb_vfio_eeh_configure(sPAPRPHBState *sphb)
>   {
>       sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
> @@ -199,6 +249,7 @@ static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
>       spc->eeh_get_state = spapr_phb_vfio_eeh_get_state;
>       spc->eeh_reset = spapr_phb_vfio_eeh_reset;
>       spc->eeh_configure = spapr_phb_vfio_eeh_configure;
> +    spc->get_loc_code = spapr_phb_vfio_get_loc_code;
>   }
>
>   static const TypeInfo spapr_phb_vfio_info = {
> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> index 895d273..1ff50b4 100644
> --- a/include/hw/pci-host/spapr.h
> +++ b/include/hw/pci-host/spapr.h
> @@ -53,6 +53,7 @@ struct sPAPRPHBClass {
>       int (*eeh_get_state)(sPAPRPHBState *sphb, int *state);
>       int (*eeh_reset)(sPAPRPHBState *sphb, int option);
>       int (*eeh_configure)(sPAPRPHBState *sphb);
> +    int (*get_loc_code)(sPAPRPHBState *sphb,  PCIDevice *pdev, char **loc_code);
>   };
>
>   typedef struct spapr_pci_msi {
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index af71e8b..95157ac 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -310,7 +310,10 @@ typedef struct sPAPREnvironment {
>   #define KVMPPC_H_LOGICAL_MEMOP  (KVMPPC_HCALL_BASE + 0x1)
>   /* Client Architecture support */
>   #define KVMPPC_H_CAS            (KVMPPC_HCALL_BASE + 0x2)
> -#define KVMPPC_HCALL_MAX        KVMPPC_H_CAS
> +#define KVMPPC_H_RTAS_UPDATE    (KVMPPC_HCALL_BASE + 0x3)
> +#define KVMPPC_H_REPORT_MC_ERR  (KVMPPC_HCALL_BASE + 0x4)
> +#define KVMPPC_H_GET_LOC_CODE   (KVMPPC_HCALL_BASE + 0x5)
> +#define KVMPPC_HCALL_MAX        KVMPPC_H_GET_LOC_CODE


Please add only relevant codes. And what happened to patches adding 
H_RTAS_UPDATE and H_REPORT_MC_ERR?

Also (it is probably a very stupid question but still :) ), why are all 
these callbacks - hypercalls, not RTAS calls? The hypercalls are numbered 
in sPAPR and we kind of stealing numbers from that space while we are 
allocating RTAS tokens ourselves and have more freedom.


>
>   extern sPAPREnvironment *spapr;
>
> @@ -522,6 +525,9 @@ int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
>                         sPAPRTCETable *tcet);
>   void spapr_pci_switch_vga(bool big_endian);
>
> +target_ulong phb_get_loc_code(PowerPCCPU *cpu, sPAPREnvironment *spapr,
> +                              target_ulong opcode, target_ulong *args);
> +
>   #define TYPE_SPAPR_RTC "spapr-rtc"
>
>   void spapr_rtc_read(DeviceState *dev, struct tm *tm, uint32_t *ns);
>


-- 
Alexey