[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [Qemu-devel] [PATCH v3 6/6] spapr_pci: populate ibm, loc-
From: |
Nikunj A Dadhania |
Subject: |
Re: [Qemu-ppc] [Qemu-devel] [PATCH v3 6/6] spapr_pci: populate ibm, loc-code |
Date: |
Wed, 06 May 2015 11:44:59 +0530 |
User-agent: |
Notmuch/0.17+27~gae47d61 (http://notmuchmail.org) Emacs/24.3.1 (x86_64-redhat-linux-gnu) |
Thomas Huth <address@hidden> writes:
> On Tue, 5 May 2015 14:23:56 +0530
> Nikunj A Dadhania <address@hidden> 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.
>>
>> 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>:<phb-index>:<slot>.<fn>
>>
>> Signed-off-by: Nikunj A Dadhania <address@hidden>
>> ---
>> hw/ppc/spapr_pci.c | 86
>> +++++++++++++++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 75 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index cbd5661..eacf0bd 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -744,6 +744,70 @@ static AddressSpace *spapr_pci_dma_iommu(PCIBus *bus,
>> void *opaque, int devfn)
>> return &phb->iommu_as;
>> }
>>
>> +static bool spapr_phb_vfio_get_devspec_value(PCIDevice *pdev, char **value)
>> +{
>> + char *host;
>> + char path[PATH_MAX];
>> +
>> + 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);
>> +
>> + return g_file_get_contents(path, value, NULL, NULL);
>> +}
>> +
>> +static char *spapr_phb_vfio_get_loc_code(sPAPRPHBState *sphb, PCIDevice
>> *pdev)
>> +{
>> + char path[PATH_MAX], *buf = NULL;
>> +
>> + /* We have a vfio host bridge lets get the path. */
>> + if (!spapr_phb_vfio_get_devspec_value(pdev, &buf)) {
>> + return NULL;
>> + }
>> +
>> + snprintf(path, sizeof(path), "/proc/device-tree%s/ibm,loc-code", buf);
>> + g_free(buf);
>> +
>> + if (g_file_get_contents(path, &buf, NULL, NULL)) {
>> + return buf;
>> + } else {
>> + return NULL;
>> + }
>
> Minor idea for an optimization: g_file_get_contents() should set buf to
> NULL in case of errors anyway, so you could omit the "if" and
> "return NULLL" and simply always "return buf" here.
Sure
>
>> +}
>> +
>> +static char *spapr_phb_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev)
>> +{
>> + char *path = g_malloc(PATH_MAX);
>> +
>> + if (!path) {
>> + return NULL;
>> + }
>> +
>> + /*
>> + * For non-vfio devices and failures make up the location code out
>
> This comment ("and failures") ...
Oh right, one option is to drop that...
>
>> + * of the name, slot and function.
>> + *
>> + * qemu_<name>:<phb-index>:<slot>.<fn>
>> + */
>> + snprintf(path, PATH_MAX, "qemu_%s:%02d:%02d.%1d", pdev->name,
>> + sphb->index, PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
>> + return path;
>> +}
>> +
>> +
>> +static char *spapr_ibm_get_loc_code(sPAPRPHBState *sphb, PCIDevice *pdev)
>> +{
>> + if (object_dynamic_cast(OBJECT(pdev), "vfio-pci") != NULL) {
>> + return spapr_phb_vfio_get_loc_code(sphb, pdev);
>> + } else {
>> + return spapr_phb_get_loc_code(sphb, pdev);
>> + }
>
> ... does not match quite the behavior here, as far as I can see. I
> guess you also wanted to fall back to spapr_phb_get_loc_code() in case
> spapr_phb_vfio_get_loc_code() did not work as expected (eg. when the
> access to the /sys or /proc filesystem failed)?
... In current case, if getting vfio device loc_code fails, we would not
add any ibm,loc_code info. I thought that would be the proper
behaviour. As "qemu_" would indicate it as an emulated device, which is
not true. Or encode it as follows:
"vfio_<name>:<phb-index>:<slot>.<fn>"
So more checks would be needed here in that case:
if (vfio) {
buf = vfio_get_loc_code()
if (!buf)
buf = cook_vfio_get_loc_code()
return buf;
} else { ... }
>
>> +}
>> +
>> /* Macros to operate with address in OF binding to PCI */
>> #define b_x(x, p, l) (((x) & ((1<<(l))-1)) << (p))
>> #define b_n(x) b_x((x), 31, 1) /* 0 if relocatable */
>> @@ -881,12 +945,12 @@ static void populate_resource_props(PCIDevice *d,
>> ResourceProps *rp)
>> }
>>
>> static int spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int
>> offset,
>> - int phb_index, int drc_index,
>> - const char *drc_name)
>> + sPAPRPHBState *phb, int drc_index)
>
> As David already noted, the sPAPRPHBState related hunks likely rather
> belong to an earlier patch already?
Yes, I will take care, this resuled while juggling with the patches :-)
Regards
Nikunj
- Re: [Qemu-ppc] [Qemu-devel] [PATCH v3 4/6] spapr_pci: enumerate and add PCI device tree, (continued)
[Qemu-ppc] [PATCH v3 0/6] spapr_pci: DT field fixes and PCI DT node creation in QEMU, Nikunj A Dadhania, 2015/05/05
- [Qemu-ppc] [PATCH v3 4/4] spapr: populate ibm,loc-code, Nikunj A Dadhania, 2015/05/05
- Re: [Qemu-ppc] [PATCH v3 0/6] spapr_pci: DT field fixes and PCI DT node creation in QEMU, Nikunj A Dadhania, 2015/05/05
- [Qemu-ppc] [PATCH v3 5/6] spapr_pci: fix boot-time device tree fields for pci hotplug, Nikunj A Dadhania, 2015/05/05
- [Qemu-ppc] [PATCH v3 2/4] spapr_pci: encode class code including Prog IF register, Nikunj A Dadhania, 2015/05/05
- [Qemu-ppc] [PATCH v3 3/6] spapr_pci: encode class code including Prog IF register, Nikunj A Dadhania, 2015/05/05
- [Qemu-ppc] [PATCH v3 3/4] spapr: enumerate and add PCI device tree, Nikunj A Dadhania, 2015/05/05
- [Qemu-ppc] [PATCH v3 4/6] spapr_pci: enumerate and add PCI device tree, Nikunj A Dadhania, 2015/05/05