From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36395) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YcqPb-0007u7-U3 for qemu-devel@nongnu.org; Tue, 31 Mar 2015 03:16:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YcqPS-0007rm-VO for qemu-devel@nongnu.org; Tue, 31 Mar 2015 03:16:35 -0400 Received: from mail-pa0-f47.google.com ([209.85.220.47]:36285) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YcqPS-0007rS-Mt for qemu-devel@nongnu.org; Tue, 31 Mar 2015 03:16:26 -0400 Received: by padcy3 with SMTP id cy3so11148443pad.3 for ; Tue, 31 Mar 2015 00:16:25 -0700 (PDT) Message-ID: <551A49C2.7020703@ozlabs.ru> Date: Tue, 31 Mar 2015 18:16:18 +1100 From: Alexey Kardashevskiy MIME-Version: 1.0 References: <1427713337-4295-1-git-send-email-nikunj@linux.vnet.ibm.com> <5519FFD9.2040209@ozlabs.ru> <20150331022954.GV9908@voom.fritz.box> <87384lagsw.fsf@linux.vnet.ibm.com> In-Reply-To: <87384lagsw.fsf@linux.vnet.ibm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v3] spapr: populate ibm,loc-code List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikunj A Dadhania , David Gibson Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org, agraf@suse.de On 03/31/2015 04:15 PM, Nikunj A Dadhania wrote: > David Gibson 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_::. >>>> >>>> Signed-off-by: Nikunj A Dadhania >>>> --- >>>> >>>> >>>> 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_::. >>>> + */ >>>> + 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