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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 03/31/2015 04:15 PM, Nikunj A Dadhania wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
>
>> On Tue, Mar 31, 2015 at 01:00:57PM +1100, Alexey Kardashevskiy wrote:
>>> 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.
>>
>> Agreed.
>>
>>> 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...
>>
>> I think that's a good idea.  Introducing a callback to get device
>> information seems pretty odd, when the device tree exists to
>> communicate static device information to the guest.
>>
>> If we're not able to go straight to qemu doing the PCI scan, I think a
>> PHB property listing this information is a better option than an RTAS
>> callback or hcall.
>
> IMHO, RTAS isnt complicated, it might be odd, as we havent done such
> things earlier. We are just changing the place of communicating the
> information. And then pushing the complexity to SLOF to figure out in
> the PHB what is its loc-name.

What kind of "complexity to SLOF" do you mean? You can choose how to store 
loc-code in the device tree, can be a separate subnode under PHB with 
properties where names are slot:fn and content is loc-code.


> Here with RTAS, we are requesting per device and getting the
> information.

... and adding a RTAS token which we will remove later and have a gap in 
the token numbers.


-- 
Alexey