Re: [Qemu-ppc] [PATCH 2/2] spapr: populate ibm,loc-code

[Nikunj A Dadhania]

David Gibson <address@hidden> writes:

> On Fri, Mar 27, 2015 at 10:04:27AM +0530, Nikunj A Dadhania wrote:
>> David Gibson <address@hidden> writes:
>> 
>> > 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 “ibm,loc-code” 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 <address@hidden>
>> >> ---
>> >>  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(PowerPCCPU *cpu_,
>> >>      return H_SUCCESS;
>> >>  }
>> >>  
>> >> +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.
>> 
>> 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.

Yes, you are right.

>
>> 
>> >
>> >> +
>> >>  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_support);
>> >> +    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, hwaddr 
>> >> 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)
>> >
>> > bool as a success/failure indication isn't a normal interface.  Just
>> > get rid of the wrapper and return H_ERROR codes directly.
>> 
>> Ok
>> 
>> >
>> >> +{
>> >> +    sPAPRPHBState *sphb = NULL;
>> >> +    PCIDevice *pdev = NULL;
>> >> +    char *buf, path[PATH_MAX];
>> >> +    struct stat st;
>> >> +
>> >> +    sphb = find_phb(spapr, buid);
>> >> +    if (sphb) {
>> >> +        pdev = 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-code", 
>> >> 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.
>> 
>> Sure, as per your previous comment, will update the return type and
>> return error/success codes directly.
>> 
>> >
>> >> +        }
>> >> +
>> >> +        /* 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.
>> 
>> Yes.
>> 
>> >
>> >> +        }
>> >> +    }
>> >> +
>> >> +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 *spapr,
>> >>                                  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(VFIOPCIDevice *vdev)
>> >>      vdev->req_enabled = false;
>> >>  }
>> >>  
>> >> +bool vfio_get_devspec(PCIDevice *pdev, char **value)
>> >> +{
>> >> +    VFIOPCIDevice *vdev = 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 = 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.
>> 
>> Alexey, has already answered that :-)
>> 
>> 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.

Right. Will I look at this.

Regards
Nikunj