From patchwork Fri Aug 11 14:34:40 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Diana Craciun X-Patchwork-Id: 800600 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=nongnu.org (client-ip=2001:4830:134:3::11; helo=lists.gnu.org; envelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org; receiver=) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=nxp.com header.i=@nxp.com header.b="K0xOCWVp"; dkim-atps=neutral Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3xTSZy1yJ5z9sRq for ; Sat, 12 Aug 2017 00:49:46 +1000 (AEST) Received: from localhost ([::1]:46098 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dgBFw-0006I6-5A for incoming@patchwork.ozlabs.org; Fri, 11 Aug 2017 10:49:44 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37251) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dgB1V-0001fG-5n for qemu-devel@nongnu.org; Fri, 11 Aug 2017 10:34:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dgB1R-0006qy-NY for qemu-devel@nongnu.org; Fri, 11 Aug 2017 10:34:49 -0400 Received: from mail-ve1eur01on0040.outbound.protection.outlook.com ([104.47.1.40]:11074 helo=EUR01-VE1-obe.outbound.protection.outlook.com) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1dgB1R-0006qc-5r; Fri, 11 Aug 2017 10:34:45 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nxp.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=yAGO/E2tf27xkwCb8sZGZV8HdJhN4e9xLg8Fc2iFP4E=; b=K0xOCWVpsc8SKYMZMv0QNQ/BrrlN4DYDgkWCCIv0DJv0D0bLElFSmu5Nn/Saz/GArbHntCYTG/YtGi61r4oXY3qg3oO++H7uU6N3Dl1n62E8dcKqTBTnrxZsApAyEh5G5yVvvU62WWFjBonGxpbibyHN/r3xfecT20U7BY7PsIs= Received: from AM3PR04MB0616.eurprd04.prod.outlook.com (10.255.133.15) by AM3PR04MB0806.eurprd04.prod.outlook.com (10.242.253.144) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.1.1341.17; Fri, 11 Aug 2017 14:34:41 +0000 Received: from AM3PR04MB0616.eurprd04.prod.outlook.com ([fe80::840e:32fc:ecf4:2ffb]) by AM3PR04MB0616.eurprd04.prod.outlook.com ([fe80::840e:32fc:ecf4:2ffb%14]) with mapi id 15.01.1341.019; Fri, 11 Aug 2017 14:34:40 +0000 From: Diana Madalina Craciun To: Auger Eric , "qemu-devel@nongnu.org" Thread-Topic: [PATCH v2 2/2] Add a unique ID in the virt machine to be used as device ID Thread-Index: AQHS07WQhQaf1stp7kus9qldhP1WKg== Date: Fri, 11 Aug 2017 14:34:40 +0000 Message-ID: References: <1495537965-4187-1-git-send-email-diana.craciun@nxp.com> <1495537965-4187-3-git-send-email-diana.craciun@nxp.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=diana.craciun@nxp.com; x-originating-ip: [192.88.146.1] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; AM3PR04MB0806; 6:R6poR8Ym6VXhh6w6iWYZgVBSmDqzmYoYCwpYiT3MdtYifZwTYFzM0UxxSNHs1tOk3IxgSibC1goMKDf2pay9u+cqqTsDom7koh7BeN3L+VBJePSmNAL/KrQHVrKubo3GcqQRwRl3lttAbWJbU5A70yphVnd2iqc3MWW/RdUVV0fMIPuKi43lKpfGV7MqLOV6Y76ol04Zc6I35MgFwtQj/8tifpytHSk0JL/MWlQ82qPZN+FFgagUwX+r4mzD0BXoJNOVQXA/SK+n9y8Dt0SoOTSPG+YWnAvmhusJd53pz/rag/VhYDvCTpKT9gRHexKY8HosBJ9fcPOxkXzc0KR5wA==; 5:Es5VAvJAlSIDgErd0G719F2gac1lU0sQzT229Rd9eMJoaFXotf09V1Zn94LOmfWG1JNSySF6+fbTwYtEdNgf6LT0opTNoe+kAiDse0y68y0xmlgnhdFSElEgxVRMHg554IGHTcTuko+HaMgt/IcZdQ==; 24:qsMS0fyORtzyGg0sPIQ4hWTM2WHCLGjQ1XNAk+zMzeiKDpl2jenf7TN483pn62ZRHHMQ0OcCVN3rVmp8uZAjObu6rkV7Z0ohYQtnP91Jick=; 7:Uz90AmVo703DNnTzxqJDIwuxBHkH/i/PkIFbp9aSG5qkQTktJCQvL8+TWpVz5UBzeHxz1/X4QmxXxPSfVGcTStRAPaz7TmtlWxiQfVmoKa7OHTKWlExVYwrqD+yPQIgkXBhUrCVjlC8yyCfQbuyVyLY5YYLt8lwrPGjcQc7DEzD122xlpjjjAflep/wYV7SYULfyn7Qae4+kzoqhafpZmnKfu61QgVFge38fzMh8qQ4= x-ms-exchange-antispam-srfa-diagnostics: SSOS;SSOR; x-forefront-antispam-report: SFV:SKI; SCL:-1; SFV:NSPM; SFS:(10009020)(39860400002)(199003)(189002)(377454003)(24454002)(2900100001)(54906002)(14454004)(99286003)(105586002)(55016002)(101416001)(106356001)(6436002)(50986999)(76176999)(54356999)(9686003)(2906002)(236005)(54896002)(5250100002)(6506006)(2501003)(7736002)(229853002)(189998001)(66066001)(81156014)(8676002)(81166006)(8936002)(7696004)(3660700001)(33656002)(478600001)(102836003)(3846002)(5660300001)(6116002)(4326008)(97736004)(53546010)(6246003)(25786009)(3280700002)(53936002)(68736007)(74316002)(86362001)(575784001); DIR:OUT; SFP:1101; SCL:1; SRVR:AM3PR04MB0806; H:AM3PR04MB0616.eurprd04.prod.outlook.com; FPR:; SPF:None; PTR:InfoNoRecords; A:1; MX:1; LANG:en; x-ms-office365-filtering-correlation-id: 300d2571-93c0-447a-261a-08d4e0c619d7 x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(300000500095)(300135000095)(300000501095)(300135300095)(300000502095)(300135100095)(22001)(2017030254152)(300000503095)(300135400095)(48565401081)(2017052603031)(201703131423075)(201703031133081)(201702281549075)(300000504095)(300135200095)(300000505095)(300135600095)(300000506095)(300135500095); SRVR:AM3PR04MB0806; x-ms-traffictypediagnostic: AM3PR04MB0806: x-exchange-antispam-report-test: UriScan:(185117386973197); x-microsoft-antispam-prvs: x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(100000700101)(100105000095)(100000701101)(100105300095)(100000702101)(100105100095)(6040450)(601004)(2401047)(5005006)(8121501046)(3002001)(10201501046)(100000703101)(100105400095)(93006095)(93001095)(6055026)(6041248)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123555025)(20161123562025)(20161123564025)(20161123558100)(20161123560025)(6072148)(201708071742011)(100000704101)(100105200095)(100000705101)(100105500095); SRVR:AM3PR04MB0806; BCL:0; PCL:0; RULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095); SRVR:AM3PR04MB0806; x-forefront-prvs: 03965EFC76 received-spf: None (protection.outlook.com: nxp.com does not designate permitted sender hosts) spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM MIME-Version: 1.0 X-OriginatorOrg: nxp.com X-MS-Exchange-CrossTenant-originalarrivaltime: 11 Aug 2017 14:34:40.5807 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635 X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM3PR04MB0806 X-detected-operating-system: by eggs.gnu.org: Windows 7 or 8 [fuzzy] X-Received-From: 104.47.1.40 X-Content-Filtered-By: Mailman/MimeDel 2.1.21 Subject: Re: [Qemu-devel] [PATCH v2 2/2] Add a unique ID in the virt machine to be used as device ID X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "mst@redhat.com" , Mike Caraman , "qemu-arm@nongnu.org" , "marcel@redhat.com" , Bharat Bhushan , "christoffer.dall@linaro.org" , Laurentiu Tudor Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" Hi Eric, Thanks for looking into this. On 07/26/2017 03:22 PM, Auger Eric wrote: Hi Diana, On 23/05/2017 13:12, Diana Craciun wrote: Device IDs are required by the ARM GICv3 ITS for IRQ remapping. Currently, for PCI devices, the requester ID was used as device ID in the virt machine. If the system has multiple masters that if the system has multiple root complex? Well ... root complex is PCI specific terminology. Our device is not a PCI device. However masters is not the best choice either, it should be rephrased. use MSIs a unique ID accross the platform is needed. across A static scheme is used and each master is allocated a range of IDs with the formula: DeviceID = zero_extend( RequesterID[15:0] ) + 0x10000*Constant (as recommended by SBSA). This ID will be configured in the machine creation and if not configured the PCI requester ID will be used insteead. instead Signed-off-by: Diana Craciun --- hw/arm/virt.c | 26 ++++++++++++++++++++++++++ hw/pci-host/gpex.c | 6 ++++++ hw/pci/msi.c | 2 +- hw/pci/pci.c | 25 +++++++++++++++++++++++++ include/hw/arm/virt.h | 1 + include/hw/pci-host/gpex.h | 2 ++ include/hw/pci/pci.h | 8 ++++++++ kvm-all.c | 4 ++-- 8 files changed, 71 insertions(+), 3 deletions(-) OK. Note that when adding other sub-systems you will need to address the ACPI side as the IORT table built by hw/arm/virt-acpi-build.c currently defines an RID mapping for the single root complex. I am not quite familiar with ACPI but for sure I will take a look into this. Thanks for pointing it out. Thanks, Diana diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 5f62a03..a969694 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -110,6 +110,8 @@ static ARMPlatformBusSystemParams platform_bus_params; #define RAMLIMIT_GB 255 #define RAMLIMIT_BYTES (RAMLIMIT_GB * 1024ULL * 1024 * 1024) +#define STREAM_ID_RANGE_SIZE 0x10000 + /* Addresses and sizes of our components. * 0..128MB is space for a flash device so we can run bootrom code such as UEFI. * 128MB..256MB is used for miscellaneous device I/O. @@ -162,6 +164,22 @@ static const int a15irqmap[] = { [VIRT_PLATFORM_BUS] = 112, /* ...to 112 + PLATFORM_BUS_NUM_IRQS -1 */ }; +/* Device IDs are required by the ARM GICV3 ITS for IRQ remapping. Currently + * for PCI devices the requester ID was used as device ID. But if the system has + * multiple masters that use MSIs, the requester ID may cause deviceID clashes. + * So a unique number is needed accross the system. + * We are using the following formula: + * DeviceID = zero_extend( RequesterID[15:0] ) + 0x10000*Constant + * (as recommanded by SBSA). Currently we do not have an SMMU emulation, but the + * same formula can be used for the generation of the streamID as well. + * For each master the device ID will be derrived from the requester ID using + * the abovemntione formula. + */ I think most of this comment should only be in the commit message. typos in derived and above mentioned. OK. stream id is the terminology for the id space at the input of the smmu. device id is the terminology for the id space at the input of the msi controller I think. RID -> deviceID (no IOMMU) RID -> streamID -> deviceID (IOMMU) I would personally get rid of all streamid uses as the smmu is not yet supported and stick to the Documentation/devicetree/bindings/pci/pci-msi.txt terminology? OK. + +static const uint32_t streamidmap[] = { + [VIRT_PCIE] = 0, /* currently only one PCI controller */ +}; + static const char *valid_cpus[] = { "cortex-a15", "cortex-a53", @@ -980,6 +998,7 @@ static void create_pcie(const VirtMachineState *vms, qemu_irq *pic) hwaddr base_ecam = vms->memmap[VIRT_PCIE_ECAM].base; hwaddr size_ecam = vms->memmap[VIRT_PCIE_ECAM].size; hwaddr base = base_mmio; + uint32_t stream_id = vms->streamidmap[VIRT_PCIE] * STREAM_ID_RANGE_SIZE; msi-base? OK, I will get rid of the stream_id naming. STREAM_ID_RANGE_SIZE ~ MSI_MAP_LENGTH? OK. int nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN; int irq = vms->irqmap[VIRT_PCIE]; MemoryRegion *mmio_alias; @@ -992,6 +1011,7 @@ static void create_pcie(const VirtMachineState *vms, qemu_irq *pic) PCIHostState *pci; dev = qdev_create(NULL, TYPE_GPEX_HOST); + qdev_prop_set_uint32(dev, "stream-id-base", stream_id); qdev_init_nofail(dev); /* Map only the first size_ecam bytes of ECAM space */ @@ -1056,6 +1076,11 @@ static void create_pcie(const VirtMachineState *vms, qemu_irq *pic) if (vms->msi_phandle) { qemu_fdt_setprop_cells(vms->fdt, nodename, "msi-parent", vms->msi_phandle); + qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "msi-map", + 1, 0, + 1, vms->msi_phandle, + 1, stream_id, + 1, STREAM_ID_RANGE_SIZE); } qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg", @@ -1609,6 +1634,7 @@ static void virt_2_9_instance_init(Object *obj) vms->memmap = a15memmap; vms->irqmap = a15irqmap; + vms->streamidmap = streamidmap; } static void virt_machine_2_9_options(MachineClass *mc) diff --git a/hw/pci-host/gpex.c b/hw/pci-host/gpex.c index 66055ee..de72408 100644 --- a/hw/pci-host/gpex.c +++ b/hw/pci-host/gpex.c @@ -43,6 +43,11 @@ static void gpex_set_irq(void *opaque, int irq_num, int level) qemu_set_irq(s->irq[irq_num], level); } +static Property gpex_props[] = { + DEFINE_PROP_UINT32("stream-id-base", GPEXHost, stream_id_base, 0), msi_base_base + DEFINE_PROP_END_OF_LIST(), +}; + static void gpex_host_realize(DeviceState *dev, Error **errp) { PCIHostState *pci = PCI_HOST_BRIDGE(dev); @@ -83,6 +88,7 @@ static void gpex_host_class_init(ObjectClass *klass, void *data) hc->root_bus_path = gpex_host_root_bus_path; dc->realize = gpex_host_realize; + dc->props = gpex_props; set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); dc->fw_name = "pci"; } diff --git a/hw/pci/msi.c b/hw/pci/msi.c index 7925851..b60a410 100644 --- a/hw/pci/msi.c +++ b/hw/pci/msi.c @@ -336,7 +336,7 @@ void msi_send_message(PCIDevice *dev, MSIMessage msg) { MemTxAttrs attrs = {}; - attrs.stream_id = pci_requester_id(dev); + attrs.stream_id = pci_stream_id(dev); address_space_stl_le(&dev->bus_master_as, msg.address, msg.data, attrs, NULL); } diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 259483b..92e9a2b 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -951,6 +951,30 @@ uint16_t pci_requester_id(PCIDevice *dev) return pci_req_id_cache_extract(&dev->requester_id_cache); } +static uint32_t pci_get_stream_id_base(PCIDevice *dev) +{ + PCIBus *rootbus = pci_device_root_bus(dev); + PCIHostState *host_bridge = PCI_HOST_BRIDGE(rootbus->qbus.parent); + Error *err = NULL; + int64_t stream_id; + + stream_id = object_property_get_int(OBJECT(host_bridge), "stream-id-base", + &err); + if (stream_id < 0) { + stream_id = 0; + } + + return stream_id; +} + +uint32_t pci_stream_id(PCIDevice *dev) +{ + /* Stream ID = RequesterID[15:0] + stream_id_base. stream_id_base may + * be 0 for devices that are not using any translation between requester_id + * and stream_id */ + return (uint16_t)pci_requester_id(dev) + dev->stream_id_base; +} I think you should split the changes in virt from pci/gpex generic changes. I agree. + /* -1 for devfn means auto assign */ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, const char *name, int devfn, @@ -1000,6 +1024,7 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus, pci_dev->devfn = devfn; pci_dev->requester_id_cache = pci_req_id_cache_get(pci_dev); + pci_dev->stream_id_base = pci_get_stream_id_base(pci_dev); looks strange to me to store the rid base in the end point as this is rather a property of the PCI complex. I acknowledge this is much more simple than reworking pci_requester_id() though. I actually did implemented it in a different way at the beginning: +uint32_t pci_stream_id(PCIDevice *dev) +{ + PCIBus *rootbus = pci_device_root_bus(dev); + PCIHostState *host_bridge = PCI_HOST_BRIDGE(rootbus->qbus.parent); + Error *err = NULL; + int64_t stream_id; + + stream_id = object_property_get_int(OBJECT(host_bridge), "stream-id-base", + &err); + if (stream_id < 0) { + stream_id = 0; + } + /* DeviceID = RequesterID[15:0] + stream_id_base. If the stream-id-base + * property is not found (e.g. for platforms that are not needing a + * global ID) the requester ID will be used instead. */ + stream_id += (uint16_t)pci_requester_id(dev); + + return stream_id; +} The reason I have changed was to avoid traversing the entire hierarchy each time the ID is needed (for example each time when a MSI is sent). memory_region_init(&pci_dev->bus_master_container_region, OBJECT(pci_dev), "bus master container", UINT64_MAX); diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h index 33b0ff3..94c007a 100644 --- a/include/hw/arm/virt.h +++ b/include/hw/arm/virt.h @@ -99,6 +99,7 @@ typedef struct { struct arm_boot_info bootinfo; const MemMapEntry *memmap; const int *irqmap; + const uint32_t *streamidmap; int smp_cpus; void *fdt; int fdt_size; diff --git a/include/hw/pci-host/gpex.h b/include/hw/pci-host/gpex.h index 68c9348..47df01a 100644 --- a/include/hw/pci-host/gpex.h +++ b/include/hw/pci-host/gpex.h @@ -48,6 +48,8 @@ typedef struct GPEXHost { GPEXRootState gpex_root; + uint32_t stream_id_base; + MemoryRegion io_ioport; MemoryRegion io_mmio; qemu_irq irq[GPEX_NUM_IRQS]; diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index a37a2d5..e6e9334 100644 --- a/include/hw/pci/pci.h +++ b/include/hw/pci/pci.h @@ -283,6 +283,12 @@ struct PCIDevice { * MSI). For conventional PCI root complex, this field is * meaningless. */ PCIReqIDCache requester_id_cache; + /* Some platforms need a unique ID for IOMMU source identification + * or MSI source identification. QEMU implements a simple scheme: + * stream_id = stream_id_base + requester_id. The stream_id_base will + * ensure that all the devices in the system have different stream ID + * domains */ + uint32_t stream_id_base; get rid of IOMMU terminology?