From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:41442)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <nikunj@linux.vnet.ibm.com>) id 1YcRnO-00059k-Ec
	for qemu-devel@nongnu.org; Mon, 30 Mar 2015 00:59:38 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <nikunj@linux.vnet.ibm.com>) id 1YcRnD-0000dm-P0
	for qemu-devel@nongnu.org; Mon, 30 Mar 2015 00:59:30 -0400
Received: from e23smtp09.au.ibm.com ([202.81.31.142]:43009)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <nikunj@linux.vnet.ibm.com>) id 1YcRnC-0000dK-S4
	for qemu-devel@nongnu.org; Mon, 30 Mar 2015 00:59:19 -0400
Received: from /spool/local
	by e23smtp09.au.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>;
	Mon, 30 Mar 2015 14:59:14 +1000
From: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
In-Reply-To: <5518B259.1070609@ozlabs.ru>
References: <1427449798-10345-1-git-send-email-nikunj@linux.vnet.ibm.com>
	<5518B259.1070609@ozlabs.ru>
Date: Mon, 30 Mar 2015 10:28:10 +0530
Message-ID: <87d23rjd3x.fsf@abhimanyu.in.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
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: 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/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 =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-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_suppo=
rt);
>> +    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, hwadd=
r addr, bool msix,
>>       }
>>   }
>>
>> +target_ulong phb_get_loc_code(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>> +                              target_ulong opcode, target_ulong *args)
>> +{
>> +    sPAPRPHBState *sphb =3D NULL;
>> +    sPAPRPHBClass *spc =3D NULL;
>> +    char *buf =3D NULL, path[PATH_MAX];
>> +    PCIDevice *pdev;
>> +    target_ulong buid =3D args[0];
>> +    target_ulong config_addr =3D args[1];
>> +    target_ulong loc_code =3D args[2];
>> +    target_ulong size =3D args[3];
>> +
>> +    sphb =3D find_phb(spapr, buid);
>> +    pdev =3D find_dev(spapr, buid, config_addr);
>> +
>> +    if (!sphb || !pdev) {
>> +        return H_PARAMETER;
>> +    }
>> +
>> +    spc =3D 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=
=20
> @index in the device name to make it unique across the guest.

Sure.

>
>
>> +    return H_SUCCESS;
>> +}
>> +
>>   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/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/ ?

Yes, should be value now.

>
>> +{
>> +    char *host;
>> +    char path[PATH_MAX];
>> +    struct stat st;
>> +
>> +    host =3D object_property_get_str(OBJECT(pdev), "host", NULL);
>> +    if (!host) {
>> +        return false;
>> +    }
>> +
>> +    snprintf(path, sizeof(path), "/sys/bus/pci/devices/%s/devspec", hos=
t);
>> +    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=20
> stat() seems redundant here.

Did not notive, will change it.

>
>
>> +}
>> +
>> +static int spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb,  PCIDevice =
*pdev,
>> +                                        char **loc_code)
>> +{
>> +    sPAPRPHBVFIOState *svphb =3D SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
>> +    char path[PATH_MAX], *buf =3D 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.

Yep, will do that.

>
>> +    }
>> +
>> +    /* 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", bu=
f);
>> +    g_free(buf);
>> +    if (stat(path, &st) < 0) {
>> +            return false;
>
> Just return what stat() returned.

Ok

>> +    }
>> +
>> +    /* 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 =3D SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
>> @@ -199,6 +249,7 @@ static void spapr_phb_vfio_class_init(ObjectClass *k=
lass, void *data)
>>       spc->eeh_get_state =3D spapr_phb_vfio_eeh_get_state;
>>       spc->eeh_reset =3D spapr_phb_vfio_eeh_reset;
>>       spc->eeh_configure =3D spapr_phb_vfio_eeh_configure;
>> +    spc->get_loc_code =3D spapr_phb_vfio_get_loc_code;
>>   }
>>
>>   static const TypeInfo spapr_phb_vfio_info =3D {
>> 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 **l=
oc_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=20
> H_RTAS_UPDATE and H_REPORT_MC_ERR?

They are in stages of review/re-write :(

>
> Also (it is probably a very stupid question but still :) ), why are all=20
> these callbacks - hypercalls, not RTAS calls?

No particular reason though. Let me look at doing it as rtas, does not
make much difference=20

> 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);
>>
>
>
> --=20
> Alexey

Thanks for the review.

Regards,
Nikunj