Re: [Qemu-devel] [PATCH v3] spapr: populate ibm,loc-code

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 03/30/2015 10:02 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 rtas call 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>:<phb-index>:<slot>.<fn>
>
> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> ---
>
>
> Changelog
> v2:
> * Using rtas call for getting ibm,loc-code
> * Added sPAPRPHBState::get_loc_code
> * Refactored the return type of get_loc_code
> * Drop stat(), and rely on g_file_get_contents
>    return type for file existence
>
> 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_pci.c          | 70 +++++++++++++++++++++++++++++++++++++++++++++
>   hw/ppc/spapr_pci_vfio.c     | 40 ++++++++++++++++++++++++++
>   include/hw/pci-host/spapr.h |  1 +
>   include/hw/ppc/spapr.h      |  3 +-
>   4 files changed, 113 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 05f4fac..fe6dfd5 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -580,6 +580,50 @@ param_error_exit:
>       rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
>   }
>
> +static void rtas_ibm_get_loc_code(PowerPCCPU *cpu,
> +                                    sPAPREnvironment *spapr,
> +                                    uint32_t token, uint32_t nargs,
> +                                    target_ulong args, uint32_t nret,
> +                                    target_ulong rets)
> +{
> +    sPAPRPHBState *sphb = NULL;
> +    sPAPRPHBClass *spc = NULL;
> +    char *buf = NULL;
> +    PCIDevice *pdev;
> +    uint64_t buid;
> +    uint32_t config_addr, loc_code, size;
> +
> +    if ((nargs != 5) || (nret != 1)) {
> +        goto param_error_exit;
> +    }
> +
> +    config_addr = rtas_ld(args, 0);
> +    buid = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 2);
> +    loc_code = rtas_ld(args, 3);
> +    size = rtas_ld(args, 4);
> +
> +    sphb = find_phb(spapr, buid);
> +    pdev = find_dev(spapr, buid, config_addr);
> +
> +    if (!sphb || !pdev) {
> +        goto param_error_exit;
> +    }
> +
> +    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
> +    if (spc->get_loc_code && spc->get_loc_code(sphb, pdev, &buf)) {
> +        uint32_t loc_len = strlen(buf);
> +
> +        loc_len = (loc_len > size) ? size : loc_len;
> +        cpu_physical_memory_write(loc_code, buf, loc_len);
> +        g_free(buf);
> +        rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> +        return;
> +    }
> +
> +param_error_exit:
> +    rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> +}
> +
>   static void rtas_ibm_configure_pe(PowerPCCPU *cpu,
>                                     sPAPREnvironment *spapr,
>                                     uint32_t token, uint32_t nargs,
> @@ -909,6 +953,27 @@ static void spapr_phb_finish_realize(sPAPRPHBState *sphb, Error **errp)
>                                   spapr_tce_get_iommu(tcet));
>   }
>
> +static bool spapr_phb_get_loc_code(sPAPRPHBState *sphb,  PCIDevice *pdev,
> +                                  char **loc_code)
> +{
> +    char *path = g_malloc(PATH_MAX);
> +
> +    if (!path) {
> +        return false;
> +    }
> +
> +    /*
> +     * For non-vfio devices and failures make up the location code out
> +     * 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));
> +    *loc_code = path;
> +    return true;
> +}
> +
>   static int spapr_phb_children_reset(Object *child, void *opaque)
>   {
>       DeviceState *dev = (DeviceState *) object_dynamic_cast(child, TYPE_DEVICE);
> @@ -1058,6 +1123,7 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
>       set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>       dc->cannot_instantiate_with_device_add_yet = false;
>       spc->finish_realize = spapr_phb_finish_realize;
> +    spc->get_loc_code = spapr_phb_get_loc_code;
>   }
>
>   static const TypeInfo spapr_phb_info = {
> @@ -1245,6 +1311,10 @@ void spapr_pci_rtas_init(void)
>       spapr_rtas_register(RTAS_IBM_SLOT_ERROR_DETAIL,
>                           "ibm,slot-error-detail",
>                           rtas_ibm_slot_error_detail);
> +    spapr_rtas_register(RTAS_IBM_GET_LOC_CODE,
> +                        "ibm,get-loc-code",


s/ibm,get-loc-code/qemu,get-loc-code/ as it is not from sPAPR.

btw we could make it even simpler and just put a property under PHB which 
would contain a map slot:function<->loc-code, the map would be per PHB and 
SLOF would just parse it and not call RTAS or do a hypercall. When we get 
PCI scan in QEMU, we will just remove this chunk from the device tree (and 
put loc-code from it to device nodes), and we won't have polluted RTAS 
token namespace. The current thing looks too complicated for such a simple 
function. Dunno...



> +                        rtas_ibm_get_loc_code);
> +

Extra empty line is not needed here.


How is PCI-scan-in-QEMU assessment going on? :)



>   }
>
>   static void spapr_pci_register_types(void)
> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
> index 99a1be5..6217cc5 100644
> --- a/hw/ppc/spapr_pci_vfio.c
> +++ b/hw/ppc/spapr_pci_vfio.c
> @@ -171,6 +171,45 @@ static int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, int option)
>       return RTAS_OUT_SUCCESS;
>   }
>
> +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 bool 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;
> +
> +    /* Non VFIO devices */
> +    if (!svphb) {
> +        return false;
> +    }
> +
> +    /* We have a vfio host bridge lets get the path. */
> +    if (!spapr_phb_vfio_get_devspec_value(pdev, &buf)) {
> +        return false;
> +    }
> +
> +    snprintf(path, sizeof(path), "/proc/device-tree%s/ibm,loc-code", buf);
> +    g_free(buf);
> +
> +    /* Read the loc-code and return */
> +    return g_file_get_contents(path, loc_code, NULL, NULL);
> +}
> +
>   static int spapr_phb_vfio_eeh_configure(sPAPRPHBState *sphb)
>   {
>       sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
> @@ -199,6 +238,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..bdf4677 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);
> +    bool (*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..fab1364 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -422,8 +422,9 @@ int spapr_allocate_irq_block(int num, bool lsi, bool msi);
>   #define RTAS_IBM_SET_SLOT_RESET                 (RTAS_TOKEN_BASE + 0x23)
>   #define RTAS_IBM_CONFIGURE_PE                   (RTAS_TOKEN_BASE + 0x24)
>   #define RTAS_IBM_SLOT_ERROR_DETAIL              (RTAS_TOKEN_BASE + 0x25)
> +#define RTAS_IBM_GET_LOC_CODE                   (RTAS_TOKEN_BASE + 0x26)
>
> -#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x26)
> +#define RTAS_TOKEN_MAX                          (RTAS_TOKEN_BASE + 0x27)
>
>   /* RTAS ibm,get-system-parameter token values */
>   #define RTAS_SYSPARM_SPLPAR_CHARACTERISTICS      20
>


-- 
Alexey