From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:37016)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <nikunj@linux.vnet.ibm.com>) id 1YbLuP-0002ju-6P
	for qemu-devel@nongnu.org; Fri, 27 Mar 2015 00:30:18 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <nikunj@linux.vnet.ibm.com>) id 1YbLuF-0007EU-6j
	for qemu-devel@nongnu.org; Fri, 27 Mar 2015 00:30:13 -0400
Received: from e28smtp06.in.ibm.com ([122.248.162.6]:55621)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <nikunj@linux.vnet.ibm.com>) id 1YbLuE-0007AU-CB
	for qemu-devel@nongnu.org; Fri, 27 Mar 2015 00:30:03 -0400
Received: from /spool/local
	by e28smtp06.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use
	Only! Violators will be prosecuted
	for <qemu-devel@nongnu.org> from <nikunj@linux.vnet.ibm.com>;
	Fri, 27 Mar 2015 09:59:56 +0530
From: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
In-Reply-To: <5514CB9C.4000701@ozlabs.ru>
References: <1427352132-1762-1-git-send-email-nikunj@linux.vnet.ibm.com>
	<1427352132-1762-2-git-send-email-nikunj@linux.vnet.ibm.com>
	<5514CB9C.4000701@ozlabs.ru>
Date: Fri, 27 Mar 2015 09:59:51 +0530
Message-ID: <87egobm5a8.fsf@linux.vnet.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
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: Alexey Kardashevskiy <aik@ozlabs.ru>, qemu-devel@nongnu.org
Cc: qemu-ppc@nongnu.org, agraf@suse.de, david@gibson.dropbear.id.au

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 =E2=80=9Cibm,loc-code=E2=80=9D 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(P=
owerPCCPU *cpu_,
>>       return H_SUCCESS;
>>   }
>>
>> +static target_ulong h_get_loc_code(PowerPCCPU *cpu, sPAPREnvironment *s=
papr,
>> +				   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_suppo=
rt);
>> +    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, hwadd=
r 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 =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);
>> +    }
>> +
>
> I would define a sPAPRPHBClass::get_loc_code(sPAPRPHBState *sphb,  uint32=
_t=20
> config_addr) and implement it for "spapr-pci-vfio-host-bridge" as you did=
=20
> 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 *spa=
pr,
>>                                   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(VFIOPCID=
evice *vdev)
>>       vdev->req_enabled =3D false;
>>   }
>>
>> +bool vfio_get_devspec(PCIDevice *pdev, char **value)
>
>
> This function can live in hw/ppc/spapr_pci_vfio.c. Something like=20
> object_property_get_str(OBJECT(pdev), "host", NULL) will return the host=
=20
> 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 =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)
>
>
> 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;
>>
>
>
> --=20
> Alexey