[Qemu-devel] [PATCH v2 0/2] Add global device ID in virt machine

[Qemu-devel] [PATCH v2 0/2] Add global device ID in virt machine

[Diana Craciun]

The NXP DPAA2 is a hardware architecture designed for high-speeed network
packet processing. The DPAA2 hardware components are managed by a hardware
component called the Management Complex (or MC) which provides an
object-base abstraction for software drivers to use the DPAA2 hardware.
For more details you can see: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/fsl-mc/README.txt?h=v4.10

The interrupts generated by the DPAA2 hardware components are MSIs. We will add
support for direct assigning these DPAA2 components/objects to a virtual 
machine. However, this will add the need to expand the MSI usage in QEMU.

Currently the MSIs in QEMU are pretty much tied to PCI. For ARM the
GIC ITS is using a device ID for interrupt translation. Currently, for
PCI, the requester ID is used as device ID. This will not work when
we add another entity that needs also a device ID which is supposed to
be unique across the system.

My proposal is to add a static allocation in the virt machine. I considered
that this allocation is specific to each machine/platform. Currently only
virt machine has it, but other implementations may use the same mechanism
as well.
So, I used a static allocation with this formula:

DeviceID = zero_extend( RequesterID[15:0] ) + 0x10000 * Constant

This formula was taken from SBSA spec (Appendix I: DeviceID generation and
ITS groups). In case of QEMU the constant will be different for each entity.
In this way a unique DeviceID will be generated and the device ID will be
derived from a requesterID (in case of PCI) or other means in case of other
entities.

The implementation is generic as there might be in the future other non-pci devices
that are using MSIs or IOMMU. Any architecture can use it, though currently
only the ARM architecture is using the function that retrieves the stream ID. I
did not change all the replacements of the pci_requester_id (with pci_stream_id)
in the code (although if the constant is 0, the stream_id is equal with requester_id).
The other architectures (e.g. intel iommu code) assume that the ID is the
requester ID.

Tested on NXP LS2080 platform.

History:

v1->v2
------
- the stream ID was added as a field in the pci device structure in order
not to traverse the PCI hierarchy each time a MSI is sent.


Diana Craciun (2):
  Increased the size of requester_id field from MemTxAttrs
  Add a unique ID in the virt machine to be used as device ID

 hw/arm/virt.c                          | 26 ++++++++++++++++++++++++++
 hw/i386/amd_iommu.c                    |  2 +-
 hw/i386/intel_iommu.c                  |  2 +-
 hw/intc/arm_gicv3_its_common.c         |  2 +-
 hw/intc/arm_gicv3_its_kvm.c            |  2 +-
 hw/pci-host/gpex.c                     |  6 ++++++
 hw/pci/msi.c                           |  2 +-
 hw/pci/pci.c                           | 25 +++++++++++++++++++++++++
 include/exec/memattrs.h                |  4 ++--
 include/hw/arm/virt.h                  |  1 +
 include/hw/intc/arm_gicv3_its_common.h |  2 +-
 include/hw/pci-host/gpex.h             |  2 ++
 include/hw/pci/pci.h                   |  8 ++++++++
 kvm-all.c                              |  4 ++--
 14 files changed, 78 insertions(+), 10 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH v2 1/2] Increased the size of requester_id field from MemTxAttrs

[Diana Craciun]

The PCI requester ID field is 16 bits. The requester_id field
from MemTxAttrs is used for MSIs to specify the device ID for
the platforms where this device ID is needed (e.g virt machine + GICv3
ITS). However, if more entities that uses MSIs in the system are used,
16 bit is no longer enough to represent the device ID. Increased the size
of this field to 24 bits in order to accomodate 256 entities.
Also the name requester_id does no longer reflect the content, so
the name was changed to stream_id.

Signed-off-by: Diana Craciun <diana.craciun@nxp.com>
---
 hw/i386/amd_iommu.c                    | 2 +-
 hw/i386/intel_iommu.c                  | 2 +-
 hw/intc/arm_gicv3_its_common.c         | 2 +-
 hw/intc/arm_gicv3_its_kvm.c            | 2 +-
 hw/pci/msi.c                           | 2 +-
 include/exec/memattrs.h                | 4 ++--
 include/hw/intc/arm_gicv3_its_common.h | 2 +-
 7 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index f86a40a..b61bdd5 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -145,7 +145,7 @@ static void amdvi_generate_msi_interrupt(AMDVIState *s)
 {
     MSIMessage msg = {};
     MemTxAttrs attrs = {
-        .requester_id = pci_requester_id(&s->pci.dev)
+        .stream_id = pci_requester_id(&s->pci.dev)
     };
 
     if (msi_enabled(&s->pci.dev)) {
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 02f047c..9cdcc62 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2597,7 +2597,7 @@ static MemTxResult vtd_mem_ir_write(void *opaque, hwaddr addr,
 
     if (!attrs.unspecified) {
         /* We have explicit Source ID */
-        sid = attrs.requester_id;
+        sid = attrs.stream_id;
     }
 
     ret = vtd_interrupt_remap_msi(opaque, &from, &to, sid);
diff --git a/hw/intc/arm_gicv3_its_common.c b/hw/intc/arm_gicv3_its_common.c
index 9d67c5c..efdb1b3 100644
--- a/hw/intc/arm_gicv3_its_common.c
+++ b/hw/intc/arm_gicv3_its_common.c
@@ -66,7 +66,7 @@ static MemTxResult gicv3_its_trans_write(void *opaque, hwaddr offset,
     if (offset == 0x0040 && ((size == 2) || (size == 4))) {
         GICv3ITSState *s = ARM_GICV3_ITS_COMMON(opaque);
         GICv3ITSCommonClass *c = ARM_GICV3_ITS_COMMON_GET_CLASS(s);
-        int ret = c->send_msi(s, le64_to_cpu(value), attrs.requester_id);
+        int ret = c->send_msi(s, le64_to_cpu(value), attrs.stream_id);
 
         if (ret <= 0) {
             qemu_log_mask(LOG_GUEST_ERROR,
diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
index bd4f3aa..9006907 100644
--- a/hw/intc/arm_gicv3_its_kvm.c
+++ b/hw/intc/arm_gicv3_its_kvm.c
@@ -29,7 +29,7 @@
 #define TYPE_KVM_ARM_ITS "arm-its-kvm"
 #define KVM_ARM_ITS(obj) OBJECT_CHECK(GICv3ITSState, (obj), TYPE_KVM_ARM_ITS)
 
-static int kvm_its_send_msi(GICv3ITSState *s, uint32_t value, uint16_t devid)
+static int kvm_its_send_msi(GICv3ITSState *s, uint32_t value, uint32_t devid)
 {
     struct kvm_msi msi;
 
diff --git a/hw/pci/msi.c b/hw/pci/msi.c
index a87b227..7925851 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.requester_id = pci_requester_id(dev);
+    attrs.stream_id = pci_requester_id(dev);
     address_space_stl_le(&dev->bus_master_as, msg.address, msg.data,
                          attrs, NULL);
 }
diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index e601061..b13e1b8 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -35,8 +35,8 @@ typedef struct MemTxAttrs {
     unsigned int secure:1;
     /* Memory access is usermode (unprivileged) */
     unsigned int user:1;
-    /* Requester ID (for MSI for example) */
-    unsigned int requester_id:16;
+    /* Stream ID (for MSI for example) */
+    unsigned int stream_id:24;
 } MemTxAttrs;
 
 /* Bus masters which don't specify any attributes will get this,
diff --git a/include/hw/intc/arm_gicv3_its_common.h b/include/hw/intc/arm_gicv3_its_common.h
index 1ba1894..6140fc6 100644
--- a/include/hw/intc/arm_gicv3_its_common.h
+++ b/include/hw/intc/arm_gicv3_its_common.h
@@ -68,7 +68,7 @@ struct GICv3ITSCommonClass {
     SysBusDeviceClass parent_class;
     /*< public >*/
 
-    int (*send_msi)(GICv3ITSState *s, uint32_t data, uint16_t devid);
+    int (*send_msi)(GICv3ITSState *s, uint32_t data, uint32_t devid);
     void (*pre_save)(GICv3ITSState *s);
     void (*post_load)(GICv3ITSState *s);
 };
-- 
2.5.5

[Qemu-devel] [PATCH v2 2/2] Add a unique ID in the virt machine to be used as device ID

[Diana Craciun]

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
use MSIs a unique ID accross the platform is needed.
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.

Signed-off-by: Diana Craciun <diana.craciun@nxp.com>
---
 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(-)

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.
+ */
+
+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;
     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),
+    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;
+}
+
 /* -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);
 
     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;
     char name[64];
     PCIIORegion io_regions[PCI_NUM_REGIONS];
     AddressSpace bus_master_as;
@@ -737,6 +743,8 @@ static inline uint16_t pci_get_bdf(PCIDevice *dev)
 
 uint16_t pci_requester_id(PCIDevice *dev);
 
+uint32_t pci_stream_id(PCIDevice *dev);
+
 /* DMA access functions */
 static inline AddressSpace *pci_get_address_space(PCIDevice *dev)
 {
diff --git a/kvm-all.c b/kvm-all.c
index 90b8573..5a508c3 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1280,7 +1280,7 @@ int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
     kroute.u.msi.data = le32_to_cpu(msg.data);
     if (kvm_msi_devid_required()) {
         kroute.flags = KVM_MSI_VALID_DEVID;
-        kroute.u.msi.devid = pci_requester_id(dev);
+        kroute.u.msi.devid = pci_stream_id(dev);
     }
     if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data, dev)) {
         kvm_irqchip_release_virq(s, virq);
@@ -1317,7 +1317,7 @@ int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg,
     kroute.u.msi.data = le32_to_cpu(msg.data);
     if (kvm_msi_devid_required()) {
         kroute.flags = KVM_MSI_VALID_DEVID;
-        kroute.u.msi.devid = pci_requester_id(dev);
+        kroute.u.msi.devid = pci_stream_id(dev);
     }
     if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data, dev)) {
         return -EINVAL;
-- 
2.5.5

Re: [Qemu-devel] [PATCH v2 0/2] Add global device ID in virt machine

[Michael S. Tsirkin]

On Tue, May 23, 2017 at 02:12:43PM +0300, Diana Craciun wrote:
> The NXP DPAA2 is a hardware architecture designed for high-speeed network
> packet processing. The DPAA2 hardware components are managed by a hardware
> component called the Management Complex (or MC) which provides an
> object-base abstraction for software drivers to use the DPAA2 hardware.
> For more details you can see: 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/fsl-mc/README.txt?h=v4.10
> 
> The interrupts generated by the DPAA2 hardware components are MSIs. We will add
> support for direct assigning these DPAA2 components/objects to a virtual 
> machine. However, this will add the need to expand the MSI usage in QEMU.
> 
> Currently the MSIs in QEMU are pretty much tied to PCI. For ARM the
> GIC ITS is using a device ID for interrupt translation. Currently, for
> PCI, the requester ID is used as device ID. This will not work when
> we add another entity that needs also a device ID which is supposed to
> be unique across the system.
> 
> My proposal is to add a static allocation in the virt machine. I considered
> that this allocation is specific to each machine/platform. Currently only
> virt machine has it, but other implementations may use the same mechanism
> as well.
> So, I used a static allocation with this formula:
> 
> DeviceID = zero_extend( RequesterID[15:0] ) + 0x10000 * Constant
> 
> This formula was taken from SBSA spec (Appendix I: DeviceID generation and
> ITS groups). In case of QEMU the constant will be different for each entity.
> In this way a unique DeviceID will be generated and the device ID will be
> derived from a requesterID (in case of PCI) or other means in case of other
> entities.
> 
> The implementation is generic as there might be in the future other non-pci devices
> that are using MSIs or IOMMU. Any architecture can use it, though currently
> only the ARM architecture is using the function that retrieves the stream ID. I
> did not change all the replacements of the pci_requester_id (with pci_stream_id)
> in the code (although if the constant is 0, the stream_id is equal with requester_id).
> The other architectures (e.g. intel iommu code) assume that the ID is the
> requester ID.
> 
> Tested on NXP LS2080 platform.
> 
> History:

I am confused. I get it that non-PCI things want something else
in their requester ID, but why require it for PCI devices?
How about using Constant == 0 for PCI? This way you do
not need to touch PCI at all as DeviceID == RequesterID ...


> v1->v2
> ------
> - the stream ID was added as a field in the pci device structure in order
> not to traverse the PCI hierarchy each time a MSI is sent.
> 
> 
> Diana Craciun (2):
>   Increased the size of requester_id field from MemTxAttrs
>   Add a unique ID in the virt machine to be used as device ID
> 
>  hw/arm/virt.c                          | 26 ++++++++++++++++++++++++++
>  hw/i386/amd_iommu.c                    |  2 +-
>  hw/i386/intel_iommu.c                  |  2 +-
>  hw/intc/arm_gicv3_its_common.c         |  2 +-
>  hw/intc/arm_gicv3_its_kvm.c            |  2 +-
>  hw/pci-host/gpex.c                     |  6 ++++++
>  hw/pci/msi.c                           |  2 +-
>  hw/pci/pci.c                           | 25 +++++++++++++++++++++++++
>  include/exec/memattrs.h                |  4 ++--
>  include/hw/arm/virt.h                  |  1 +
>  include/hw/intc/arm_gicv3_its_common.h |  2 +-
>  include/hw/pci-host/gpex.h             |  2 ++
>  include/hw/pci/pci.h                   |  8 ++++++++
>  kvm-all.c                              |  4 ++--
>  14 files changed, 78 insertions(+), 10 deletions(-)
> 
> -- 
> 2.5.5

Re: [Qemu-devel] [PATCH v2 0/2] Add global device ID in virt machine

[Diana Madalina Craciun]

On 05/25/2017 01:12 AM, Michael S. Tsirkin wrote:
> On Tue, May 23, 2017 at 02:12:43PM +0300, Diana Craciun wrote:
>> The NXP DPAA2 is a hardware architecture designed for high-speeed network
>> packet processing. The DPAA2 hardware components are managed by a hardware
>> component called the Management Complex (or MC) which provides an
>> object-base abstraction for software drivers to use the DPAA2 hardware.
>> For more details you can see: 
>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftree%2Fdrivers%2Fstaging%2Ffsl-mc%2FREADME.txt%3Fh%3Dv4.10&data=01%7C01%7Cdiana.craciun%40nxp.com%7Cce2cc4d066944ce2759308d4a2f1f3f8%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0&sdata=CJAiTF6Qnq4gklSqon7xcRby0O1HQvytTUSdPwaHuSE%3D&reserved=0
>>
>> The interrupts generated by the DPAA2 hardware components are MSIs. We will add
>> support for direct assigning these DPAA2 components/objects to a virtual 
>> machine. However, this will add the need to expand the MSI usage in QEMU.
>>
>> Currently the MSIs in QEMU are pretty much tied to PCI. For ARM the
>> GIC ITS is using a device ID for interrupt translation. Currently, for
>> PCI, the requester ID is used as device ID. This will not work when
>> we add another entity that needs also a device ID which is supposed to
>> be unique across the system.
>>
>> My proposal is to add a static allocation in the virt machine. I considered
>> that this allocation is specific to each machine/platform. Currently only
>> virt machine has it, but other implementations may use the same mechanism
>> as well.
>> So, I used a static allocation with this formula:
>>
>> DeviceID = zero_extend( RequesterID[15:0] ) + 0x10000 * Constant
>>
>> This formula was taken from SBSA spec (Appendix I: DeviceID generation and
>> ITS groups). In case of QEMU the constant will be different for each entity.
>> In this way a unique DeviceID will be generated and the device ID will be
>> derived from a requesterID (in case of PCI) or other means in case of other
>> entities.
>>
>> The implementation is generic as there might be in the future other non-pci devices
>> that are using MSIs or IOMMU. Any architecture can use it, though currently
>> only the ARM architecture is using the function that retrieves the stream ID. I
>> did not change all the replacements of the pci_requester_id (with pci_stream_id)
>> in the code (although if the constant is 0, the stream_id is equal with requester_id).
>> The other architectures (e.g. intel iommu code) assume that the ID is the
>> requester ID.
>>
>> Tested on NXP LS2080 platform.
>>
>> History:
> I am confused. I get it that non-PCI things want something else
> in their requester ID, but why require it for PCI devices?
> How about using Constant == 0 for PCI? This way you do
> not need to touch PCI at all as DeviceID == RequesterID ...

It is not that other devices need something else in the requester ID,
but more about finding a way to provide an unique ID across the system
(more precisely it should be unique for all devices connected to the
same IOMMU/ITS). The DT already offers support to describe the
translation between stream IDs/device IDs to requester ID for PCI
devices (iommu-map for IOMMU ([1]) and msi-map for MSIs ([2]). It will
not change the way the requester ID is used in PCI in general, but only
the places that need a unique ID (which are the MSIs and IOMMU).

If we are to use a value of 0 for the constant in case of PCI devices,
what happens if we have multiple PCI controllers?

[1]
https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/pci-iommu.txt
[2]
https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/pci-msi.txt

Thanks,

Diana

Re: [Qemu-devel] [PATCH v2 0/2] Add global device ID in virt machine

[Diana Madalina Craciun]

ping

On 5/23/2017 2:12 PM, Diana Craciun wrote:
> The NXP DPAA2 is a hardware architecture designed for high-speeed network
> packet processing. The DPAA2 hardware components are managed by a hardware
> component called the Management Complex (or MC) which provides an
> object-base abstraction for software drivers to use the DPAA2 hardware.
> For more details you can see: 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/fsl-mc/README.txt?h=v4.10
>
> The interrupts generated by the DPAA2 hardware components are MSIs. We will add
> support for direct assigning these DPAA2 components/objects to a virtual 
> machine. However, this will add the need to expand the MSI usage in QEMU.
>
> Currently the MSIs in QEMU are pretty much tied to PCI. For ARM the
> GIC ITS is using a device ID for interrupt translation. Currently, for
> PCI, the requester ID is used as device ID. This will not work when
> we add another entity that needs also a device ID which is supposed to
> be unique across the system.
>
> My proposal is to add a static allocation in the virt machine. I considered
> that this allocation is specific to each machine/platform. Currently only
> virt machine has it, but other implementations may use the same mechanism
> as well.
> So, I used a static allocation with this formula:
>
> DeviceID = zero_extend( RequesterID[15:0] ) + 0x10000 * Constant
>
> This formula was taken from SBSA spec (Appendix I: DeviceID generation and
> ITS groups). In case of QEMU the constant will be different for each entity.
> In this way a unique DeviceID will be generated and the device ID will be
> derived from a requesterID (in case of PCI) or other means in case of other
> entities.
>
> The implementation is generic as there might be in the future other non-pci devices
> that are using MSIs or IOMMU. Any architecture can use it, though currently
> only the ARM architecture is using the function that retrieves the stream ID. I
> did not change all the replacements of the pci_requester_id (with pci_stream_id)
> in the code (although if the constant is 0, the stream_id is equal with requester_id).
> The other architectures (e.g. intel iommu code) assume that the ID is the
> requester ID.
>
> Tested on NXP LS2080 platform.
>
> History:
>
> v1->v2
> ------
> - the stream ID was added as a field in the pci device structure in order
> not to traverse the PCI hierarchy each time a MSI is sent.
>
>
> Diana Craciun (2):
>   Increased the size of requester_id field from MemTxAttrs
>   Add a unique ID in the virt machine to be used as device ID
>
>  hw/arm/virt.c                          | 26 ++++++++++++++++++++++++++
>  hw/i386/amd_iommu.c                    |  2 +-
>  hw/i386/intel_iommu.c                  |  2 +-
>  hw/intc/arm_gicv3_its_common.c         |  2 +-
>  hw/intc/arm_gicv3_its_kvm.c            |  2 +-
>  hw/pci-host/gpex.c                     |  6 ++++++
>  hw/pci/msi.c                           |  2 +-
>  hw/pci/pci.c                           | 25 +++++++++++++++++++++++++
>  include/exec/memattrs.h                |  4 ++--
>  include/hw/arm/virt.h                  |  1 +
>  include/hw/intc/arm_gicv3_its_common.h |  2 +-
>  include/hw/pci-host/gpex.h             |  2 ++
>  include/hw/pci/pci.h                   |  8 ++++++++
>  kvm-all.c                              |  4 ++--
>  14 files changed, 78 insertions(+), 10 deletions(-)
>

Re: [Qemu-devel] [PATCH v2 0/2] Add global device ID in virt machine

[Michael S. Tsirkin]

On Wed, May 31, 2017 at 12:02:56PM +0000, Diana Madalina Craciun wrote:
> On 05/25/2017 01:12 AM, Michael S. Tsirkin wrote:
> > On Tue, May 23, 2017 at 02:12:43PM +0300, Diana Craciun wrote:
> >> The NXP DPAA2 is a hardware architecture designed for high-speeed network
> >> packet processing. The DPAA2 hardware components are managed by a hardware
> >> component called the Management Complex (or MC) which provides an
> >> object-base abstraction for software drivers to use the DPAA2 hardware.
> >> For more details you can see: 
> >> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftree%2Fdrivers%2Fstaging%2Ffsl-mc%2FREADME.txt%3Fh%3Dv4.10&data=01%7C01%7Cdiana.craciun%40nxp.com%7Cce2cc4d066944ce2759308d4a2f1f3f8%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0&sdata=CJAiTF6Qnq4gklSqon7xcRby0O1HQvytTUSdPwaHuSE%3D&reserved=0
> >>
> >> The interrupts generated by the DPAA2 hardware components are MSIs. We will add
> >> support for direct assigning these DPAA2 components/objects to a virtual 
> >> machine. However, this will add the need to expand the MSI usage in QEMU.
> >>
> >> Currently the MSIs in QEMU are pretty much tied to PCI. For ARM the
> >> GIC ITS is using a device ID for interrupt translation. Currently, for
> >> PCI, the requester ID is used as device ID. This will not work when
> >> we add another entity that needs also a device ID which is supposed to
> >> be unique across the system.
> >>
> >> My proposal is to add a static allocation in the virt machine. I considered
> >> that this allocation is specific to each machine/platform. Currently only
> >> virt machine has it, but other implementations may use the same mechanism
> >> as well.
> >> So, I used a static allocation with this formula:
> >>
> >> DeviceID = zero_extend( RequesterID[15:0] ) + 0x10000 * Constant
> >>
> >> This formula was taken from SBSA spec (Appendix I: DeviceID generation and
> >> ITS groups). In case of QEMU the constant will be different for each entity.
> >> In this way a unique DeviceID will be generated and the device ID will be
> >> derived from a requesterID (in case of PCI) or other means in case of other
> >> entities.
> >>
> >> The implementation is generic as there might be in the future other non-pci devices
> >> that are using MSIs or IOMMU. Any architecture can use it, though currently
> >> only the ARM architecture is using the function that retrieves the stream ID. I
> >> did not change all the replacements of the pci_requester_id (with pci_stream_id)
> >> in the code (although if the constant is 0, the stream_id is equal with requester_id).
> >> The other architectures (e.g. intel iommu code) assume that the ID is the
> >> requester ID.
> >>
> >> Tested on NXP LS2080 platform.
> >>
> >> History:
> > I am confused. I get it that non-PCI things want something else
> > in their requester ID, but why require it for PCI devices?
> > How about using Constant == 0 for PCI? This way you do
> > not need to touch PCI at all as DeviceID == RequesterID ...
> 
> It is not that other devices need something else in the requester ID,
> but more about finding a way to provide an unique ID across the system
> (more precisely it should be unique for all devices connected to the
> same IOMMU/ITS). The DT already offers support to describe the
> translation between stream IDs/device IDs to requester ID for PCI
> devices (iommu-map for IOMMU ([1]) and msi-map for MSIs ([2]). It will
> not change the way the requester ID is used in PCI in general, but only
> the places that need a unique ID (which are the MSIs and IOMMU).
> 
> If we are to use a value of 0 for the constant in case of PCI devices,
> what happens if we have multiple PCI controllers?

I guess we'd use the PCI Segment number for that?

> [1]
> https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/pci-iommu.txt
> [2]
> https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/pci-msi.txt
> 
> Thanks,
> 
> Diana
> 

Re: [Qemu-devel] [PATCH v2 0/2] Add global device ID in virt machine

[Peter Maydell]

On 23 May 2017 at 12:12, Diana Craciun <diana.craciun@nxp.com> wrote:
> The NXP DPAA2 is a hardware architecture designed for high-speeed network
> packet processing. The DPAA2 hardware components are managed by a hardware
> component called the Management Complex (or MC) which provides an
> object-base abstraction for software drivers to use the DPAA2 hardware.
> For more details you can see:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/fsl-mc/README.txt?h=v4.10
>
> The interrupts generated by the DPAA2 hardware components are MSIs. We will add
> support for direct assigning these DPAA2 components/objects to a virtual
> machine. However, this will add the need to expand the MSI usage in QEMU.
>
> Currently the MSIs in QEMU are pretty much tied to PCI. For ARM the
> GIC ITS is using a device ID for interrupt translation. Currently, for
> PCI, the requester ID is used as device ID. This will not work when
> we add another entity that needs also a device ID which is supposed to
> be unique across the system.
>
> My proposal is to add a static allocation in the virt machine. I considered
> that this allocation is specific to each machine/platform. Currently only
> virt machine has it, but other implementations may use the same mechanism
> as well.
> So, I used a static allocation with this formula:
>
> DeviceID = zero_extend( RequesterID[15:0] ) + 0x10000 * Constant
>
> This formula was taken from SBSA spec (Appendix I: DeviceID generation and
> ITS groups). In case of QEMU the constant will be different for each entity.
> In this way a unique DeviceID will be generated and the device ID will be
> derived from a requesterID (in case of PCI) or other means in case of other
> entities.
>
> The implementation is generic as there might be in the future other non-pci devices
> that are using MSIs or IOMMU. Any architecture can use it, though currently
> only the ARM architecture is using the function that retrieves the stream ID. I
> did not change all the replacements of the pci_requester_id (with pci_stream_id)
> in the code (although if the constant is 0, the stream_id is equal with requester_id).
> The other architectures (e.g. intel iommu code) assume that the ID is the
> requester ID.
>
> Tested on NXP LS2080 platform.

So I'm still a bit confused about what this is for and what the
relationship is with how the equivalent task would be performed
in real hardware. Can you explain how that works, please?
Would there be a system MMU involved somewhere that would be
allocating these IDs somehow?

The patchset also seems to be introducing a fair amount of
mechanism without also implementing the actual use for it -- yes,
if we had two PCI controllers in the virt board we would need to
sort something out I guess, but we don't, we only have one.
Evaluating whether the mechanism makes sense without its major
user is always trickier.

Essentially, I know very little about this use case, but my
default position is that if in doubt we should aim to match
how real hardware handles something. Virtualization-specific
code, and especially something specific to a niche use case like
this, is something I'd really prefer to avoid. The ideal is to
have something like PCI device passthrough, where the guest can
automatically probe for things and QEMU doesn't need to get
involved.

I was hoping Eric would review this, since he's more up to speed
on direct hardware passthrough than I am, but I think he's just
gone off on holiday...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 0/2] Add global device ID in virt machine

[Mike Caraman]

Peter, thanks for feedback. Diana will return from vacation next week,
I am trying clarify some of the questions.

>From: Peter Maydell <peter.maydell@linaro.org>
>Sent: Monday, July 10, 2017 8:10 PM
>To: Diana Madalina Craciun
>Cc: QEMU Developers; Michael S. Tsirkin; Eric Auger; Mike Caraman; qemu-arm; Marcel Apfelbaum; Bharat Bhushan; Christoffer Dall; Laurentiu Tudor
>Subject: Re: [Qemu-devel] [PATCH v2 0/2] Add global device ID in virt machine
>
>On 23 May 2017 at 12:12, Diana Craciun <diana.craciun@nxp.com> wrote:
>> The NXP DPAA2 is a hardware architecture designed for high-speeed network
>> packet processing. The DPAA2 hardware components are managed by a hardware
>> component called the Management Complex (or MC) which provides an
>> object-base abstraction for software drivers to use the DPAA2 hardware.
>> For more details you can see:
>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftree%2Fdrivers%2Fstaging%2Ffsl-mc%2FREADME.txt%3Fh%3Dv4.10&data=01%7C01%7Cmike.caraman%40nxp.com%7Ccdb80ed25c4a4a71d86708d4c7b68cd8%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0&sdata=JfYha1w2rZ12URBU3LHgo7OwtEPEDqcGWjdAUiRCP4k%3D&reserved=0
>>
>> The interrupts generated by the DPAA2 hardware components are MSIs. We will add
>> support for direct assigning these DPAA2 components/objects to a virtual
>> machine. However, this will add the need to expand the MSI usage in QEMU.
>>
>> Currently the MSIs in QEMU are pretty much tied to PCI. For ARM the
>> GIC ITS is using a device ID for interrupt translation. Currently, for
>> PCI, the requester ID is used as device ID. This will not work when
>> we add another entity that needs also a device ID which is supposed to
>> be unique across the system.
>>
>> My proposal is to add a static allocation in the virt machine. I considered
>> that this allocation is specific to each machine/platform. Currently only
>> virt machine has it, but other implementations may use the same mechanism
>> as well.
>> So, I used a static allocation with this formula:
>>
>> DeviceID = zero_extend( RequesterID[15:0] ) + 0x10000 * Constant
>>
>> This formula was taken from SBSA spec (Appendix I: DeviceID generation and
>> ITS groups). In case of QEMU the constant will be different for each entity.
>> In this way a unique DeviceID will be generated and the device ID will be
>> derived from a requesterID (in case of PCI) or other means in case of other
>> entities.
>>
>> The implementation is generic as there might be in the future other non-pci devices
>> that are using MSIs or IOMMU. Any architecture can use it, though currently
>> only the ARM architecture is using the function that retrieves the stream ID. I
>> did not change all the replacements of the pci_requester_id (with pci_stream_id)
>> in the code (although if the constant is 0, the stream_id is equal with requester_id).
>> The other architectures (e.g. intel iommu code) assume that the ID is the
>> requester ID.
>>
>> Tested on NXP LS2080 platform.
>
>So I'm still a bit confused about what this is for and what the
>relationship is with how the equivalent task would be performed
>in real hardware. Can you explain how that works, please?
>Would there be a system MMU involved somewhere that would be
>allocating these IDs somehow?

This is the first step to accommodate device pass-though with MSI support,
required for non-PCI/heterogeneous buses on ARM64 platforms with GIC-ITS.
We reached the point where our mc-bus infrastructure got the ack and
it is ready to get out of staging:
https://lkml.org/lkml/2017/7/18/708

The problem we want to address is related to guest DeviceIDs allocation
for GIC-ITS emulation. On real hardware the boot loader, in our case u-boot,
allocates the DeviceIDs and populates the PCI msi-map device tree binding.
We have a similar binding mechanism for MC devices:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/pci-msi.txt
http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/pci/pcie_layerscape_fixup.c;h=9e6c2f5dfcf62e8b06935aa9ae3c40a734f9571d;hb=HEAD#l118

Qemu does not populate yet the msi-map and the DeviceIDs for PCI devices
defaults to bdf at run-time:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/of/irq.c#n599
http://elixir.free-electrons.com/linux/latest/source/drivers/pci/msi.c#L1488

The proposal is to allocate the DeviceIDs as described in SBSA and to populate the
device tree, as oppose of generating them at runtime, in order to stick to device
binding definitions and to avoid clashes.

>The patchset also seems to be introducing a fair amount of
>mechanism without also implementing the actual use for it -- yes,
>if we had two PCI controllers in the virt board we would need to
>sort something out I guess, but we don't, we only have one.
>Evaluating whether the mechanism makes sense without its major
>user is always trickier.
>
>Essentially, I know very little about this use case, but my
>default position is that if in doubt we should aim to match
>how real hardware handles something. Virtualization-specific
>code, and especially something specific to a niche use case like
>this, is something I'd really prefer to avoid. The ideal is to
>have something like PCI device passthrough, where the guest can
>automatically probe for things and QEMU doesn't need to get
>involved.
>
>I was hoping Eric would review this, since he's more up to speed
>on direct hardware passthrough than I am, but I think he's just
>gone off on holiday...
>
>thanks
>-- PMM
>

Regards,
Mike

Re: [Qemu-devel] [PATCH v2 2/2] Add a unique ID in the virt machine to be used as device ID

[Auger Eric]

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?
> 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 <diana.craciun@nxp.com>
> ---
>  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(-)
> 
> 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.

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?

> +
> +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?
STREAM_ID_RANGE_SIZE ~ MSI_MAP_LENGTH?
>      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.

> +
>  /* -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.
>  
>      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?

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.

Thanks

Eric
>      char name[64];
>      PCIIORegion io_regions[PCI_NUM_REGIONS];
>      AddressSpace bus_master_as;
> @@ -737,6 +743,8 @@ static inline uint16_t pci_get_bdf(PCIDevice *dev)
>  
>  uint16_t pci_requester_id(PCIDevice *dev);
>  
> +uint32_t pci_stream_id(PCIDevice *dev);
> +
>  /* DMA access functions */
>  static inline AddressSpace *pci_get_address_space(PCIDevice *dev)
>  {
> diff --git a/kvm-all.c b/kvm-all.c
> index 90b8573..5a508c3 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1280,7 +1280,7 @@ int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
>      kroute.u.msi.data = le32_to_cpu(msg.data);
>      if (kvm_msi_devid_required()) {
>          kroute.flags = KVM_MSI_VALID_DEVID;
> -        kroute.u.msi.devid = pci_requester_id(dev);
> +        kroute.u.msi.devid = pci_stream_id(dev);
>      }
>      if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data, dev)) {
>          kvm_irqchip_release_virq(s, virq);
> @@ -1317,7 +1317,7 @@ int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg,
>      kroute.u.msi.data = le32_to_cpu(msg.data);
>      if (kvm_msi_devid_required()) {
>          kroute.flags = KVM_MSI_VALID_DEVID;
> -        kroute.u.msi.devid = pci_requester_id(dev);
> +        kroute.u.msi.devid = pci_stream_id(dev);
>      }
>      if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data, dev)) {
>          return -EINVAL;
> 

Re: [Qemu-devel] [PATCH v2 1/2] Increased the size of requester_id field from MemTxAttrs

[Auger Eric]

Hi Diana,
On 23/05/2017 13:12, Diana Craciun wrote:
> The PCI requester ID field is 16 bits. The requester_id field
> from MemTxAttrs is used for MSIs to specify the device ID for
> the platforms where this device ID is needed (e.g virt machine + GICv3
> ITS). However, if more entities that uses MSIs in the system are used,
> 16 bit is no longer enough to represent the device ID. Increased the size
> of this field to 24 bits in order to accomodate 256 entities.
> Also the name requester_id does no longer reflect the content, so
> the name was changed to stream_id.

I think both deviceid and streamid can be up to 32 bits in theory.

With respect to the renaming, stream_id really is ARM specific and
corresponds to the ID space before the SMMU while you mostly address
device id problematics here (ie. space id input to the interrupt
controller). Maybe a more generic terminology such as originator id
could be used or source id (I think this is Intel terminology though).

Maybe the changes in semantics of the field should be
associated/combined with its new computation found in next patch? see
comments in subsequent patch.

Thanks

Eric

> 
> Signed-off-by: Diana Craciun <diana.craciun@nxp.com>
> ---
>  hw/i386/amd_iommu.c                    | 2 +-
>  hw/i386/intel_iommu.c                  | 2 +-
>  hw/intc/arm_gicv3_its_common.c         | 2 +-
>  hw/intc/arm_gicv3_its_kvm.c            | 2 +-
>  hw/pci/msi.c                           | 2 +-
>  include/exec/memattrs.h                | 4 ++--
>  include/hw/intc/arm_gicv3_its_common.h | 2 +-
>  7 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
> index f86a40a..b61bdd5 100644
> --- a/hw/i386/amd_iommu.c
> +++ b/hw/i386/amd_iommu.c
> @@ -145,7 +145,7 @@ static void amdvi_generate_msi_interrupt(AMDVIState *s)
>  {
>      MSIMessage msg = {};
>      MemTxAttrs attrs = {
> -        .requester_id = pci_requester_id(&s->pci.dev)
> +        .stream_id = pci_requester_id(&s->pci.dev)
>      };
>  
>      if (msi_enabled(&s->pci.dev)) {
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 02f047c..9cdcc62 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2597,7 +2597,7 @@ static MemTxResult vtd_mem_ir_write(void *opaque, hwaddr addr,
>  
>      if (!attrs.unspecified) {
>          /* We have explicit Source ID */
> -        sid = attrs.requester_id;
> +        sid = attrs.stream_id;
>      }
>  
>      ret = vtd_interrupt_remap_msi(opaque, &from, &to, sid);
> diff --git a/hw/intc/arm_gicv3_its_common.c b/hw/intc/arm_gicv3_its_common.c
> index 9d67c5c..efdb1b3 100644
> --- a/hw/intc/arm_gicv3_its_common.c
> +++ b/hw/intc/arm_gicv3_its_common.c
> @@ -66,7 +66,7 @@ static MemTxResult gicv3_its_trans_write(void *opaque, hwaddr offset,
>      if (offset == 0x0040 && ((size == 2) || (size == 4))) {
>          GICv3ITSState *s = ARM_GICV3_ITS_COMMON(opaque);
>          GICv3ITSCommonClass *c = ARM_GICV3_ITS_COMMON_GET_CLASS(s);
> -        int ret = c->send_msi(s, le64_to_cpu(value), attrs.requester_id);
> +        int ret = c->send_msi(s, le64_to_cpu(value), attrs.stream_id);
>  
>          if (ret <= 0) {
>              qemu_log_mask(LOG_GUEST_ERROR,
> diff --git a/hw/intc/arm_gicv3_its_kvm.c b/hw/intc/arm_gicv3_its_kvm.c
> index bd4f3aa..9006907 100644
> --- a/hw/intc/arm_gicv3_its_kvm.c
> +++ b/hw/intc/arm_gicv3_its_kvm.c
> @@ -29,7 +29,7 @@
>  #define TYPE_KVM_ARM_ITS "arm-its-kvm"
>  #define KVM_ARM_ITS(obj) OBJECT_CHECK(GICv3ITSState, (obj), TYPE_KVM_ARM_ITS)
>  
> -static int kvm_its_send_msi(GICv3ITSState *s, uint32_t value, uint16_t devid)
> +static int kvm_its_send_msi(GICv3ITSState *s, uint32_t value, uint32_t devid)
>  {
>      struct kvm_msi msi;
>  
> diff --git a/hw/pci/msi.c b/hw/pci/msi.c
> index a87b227..7925851 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.requester_id = pci_requester_id(dev);
> +    attrs.stream_id = pci_requester_id(dev);
>      address_space_stl_le(&dev->bus_master_as, msg.address, msg.data,
>                           attrs, NULL);
>  }
> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
> index e601061..b13e1b8 100644
> --- a/include/exec/memattrs.h
> +++ b/include/exec/memattrs.h
> @@ -35,8 +35,8 @@ typedef struct MemTxAttrs {
>      unsigned int secure:1;
>      /* Memory access is usermode (unprivileged) */
>      unsigned int user:1;
> -    /* Requester ID (for MSI for example) */
> -    unsigned int requester_id:16;
> +    /* Stream ID (for MSI for example) */
> +    unsigned int stream_id:24;
>  } MemTxAttrs;
>  
>  /* Bus masters which don't specify any attributes will get this,
> diff --git a/include/hw/intc/arm_gicv3_its_common.h b/include/hw/intc/arm_gicv3_its_common.h
> index 1ba1894..6140fc6 100644
> --- a/include/hw/intc/arm_gicv3_its_common.h
> +++ b/include/hw/intc/arm_gicv3_its_common.h
> @@ -68,7 +68,7 @@ struct GICv3ITSCommonClass {
>      SysBusDeviceClass parent_class;
>      /*< public >*/
>  
> -    int (*send_msi)(GICv3ITSState *s, uint32_t data, uint16_t devid);
> +    int (*send_msi)(GICv3ITSState *s, uint32_t data, uint32_t devid);
>      void (*pre_save)(GICv3ITSState *s);
>      void (*post_load)(GICv3ITSState *s);
>  };
> 

Re: [Qemu-devel] [PATCH v2 0/2] Add global device ID in virt machine

[Diana Madalina Craciun]

Hi Peter,

On 07/10/2017 08:10 PM, Peter Maydell wrote:

On 23 May 2017 at 12:12, Diana Craciun <diana.craciun@nxp.com><mailto:diana.craciun@nxp.com> wrote:


The NXP DPAA2 is a hardware architecture designed for high-speeed network
packet processing. The DPAA2 hardware components are managed by a hardware
component called the Management Complex (or MC) which provides an
object-base abstraction for software drivers to use the DPAA2 hardware.
For more details you can see:
https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftree%2Fdrivers%2Fstaging%2Ffsl-mc%2FREADME.txt%3Fh%3Dv4.10&data=01%7C01%7Cdiana.craciun%40nxp.com%7C87a43122f7374dd4483008d4c7b68d10%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0&sdata=Rf9Uae7gxCeBSbLbRyAs24J9R6HUT1p4%2B4K46BgxqvM%3D&reserved=0

The interrupts generated by the DPAA2 hardware components are MSIs. We will add
support for direct assigning these DPAA2 components/objects to a virtual
machine. However, this will add the need to expand the MSI usage in QEMU.

Currently the MSIs in QEMU are pretty much tied to PCI. For ARM the
GIC ITS is using a device ID for interrupt translation. Currently, for
PCI, the requester ID is used as device ID. This will not work when
we add another entity that needs also a device ID which is supposed to
be unique across the system.

My proposal is to add a static allocation in the virt machine. I considered
that this allocation is specific to each machine/platform. Currently only
virt machine has it, but other implementations may use the same mechanism
as well.
So, I used a static allocation with this formula:

DeviceID = zero_extend( RequesterID[15:0] ) + 0x10000 * Constant

This formula was taken from SBSA spec (Appendix I: DeviceID generation and
ITS groups). In case of QEMU the constant will be different for each entity.
In this way a unique DeviceID will be generated and the device ID will be
derived from a requesterID (in case of PCI) or other means in case of other
entities.

The implementation is generic as there might be in the future other non-pci devices
that are using MSIs or IOMMU. Any architecture can use it, though currently
only the ARM architecture is using the function that retrieves the stream ID. I
did not change all the replacements of the pci_requester_id (with pci_stream_id)
in the code (although if the constant is 0, the stream_id is equal with requester_id).
The other architectures (e.g. intel iommu code) assume that the ID is the
requester ID.

Tested on NXP LS2080 platform.



So I'm still a bit confused about what this is for and what the
relationship is with how the equivalent task would be performed
in real hardware. Can you explain how that works, please?
Would there be a system MMU involved somewhere that would be
allocating these IDs somehow?

Let's keep the discussion to MSIs for now (assuming that there is no SMMU as QEMU does not have currently an SMMU emulation integrated).
Every device that sends MSIs has a deviceID associated with it. The deviceID is used by the GIC-ITS hardware for interrupt translation. The deviceID is assigned by the "system designer", so there is not an hardware entity that generates/allocates the deviceIDs (the device ID can be programmed into the device by software).

For PCI these IDs are specified in the device tree:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/pci-msi.txt

(msi-map property).



The patchset also seems to be introducing a fair amount of
mechanism without also implementing the actual use for it -- yes,
if we had two PCI controllers in the virt board we would need to
sort something out I guess, but we don't, we only have one.
Evaluating whether the mechanism makes sense without its major
user is always trickier.

Right, we have only one PCI controller in the QEMU for now. We are in the process of implementing device pass-through for our non-PCI bus infrastructure which is using MSIs as well. As Mike pointed out, the mc-bus infrastructure is on the point of getting out of staging:

"
The fsl-mc is a hardware resource manager that manages specialized hardware objects used in network-oriented packet processing applications. After the fsl-mc block is enabled, pools of hardware resources are available, such as queues, buffer pools, I/O interfaces. These resources are building blocks that can be used to create functional hardware objects such as network interfaces, crypto accelerator instances, or L2 switches.

All the fsl-mc managed hardware resources/objects are represented in a physical grouping mechanism called a 'container' or DPRC (data path resource container).

>From the point of view of an OS, a DPRC functions similar to a plug and play bus. Using fsl-mc commands software can enumerate the contents of the DPRC discovering the hardware objects present and binding them to drivers. Hardware objects can be created and removed dynamically, providing hot pluggability of the hardware objects.
"

After adding pass-through support, we will have two entities that are using MSIs and thus that are needing unique deviceIDs.
PCI is making the assumption that the deviceID=requesterID, so, theoretically, we could allocate numbers bigger than the requesterID for our bus infrastructure. But I thought that this is more like an temporary workaround which will not work in the future when: another PCI controller is added or another platform device that is using MSIs is added.




Essentially, I know very little about this use case, but my
default position is that if in doubt we should aim to match
how real hardware handles something. Virtualization-specific
code, and especially something specific to a niche use case like
this, is something I'd really prefer to avoid. The ideal is to
have something like PCI device passthrough, where the guest can
automatically probe for things and QEMU doesn't need to get
involved.


It is not automatically probed for PCI, either. In the current implementation an assumption is made:

if the msi-map is missing from the device tree (property which specifies the translation between requesterID and deviceID) it is asummed that the deviceID is the same with the requesterID.



I was hoping Eric would review this, since he's more up to speed
on direct hardware passthrough than I am, but I think he's just
gone off on holiday...

thanks
-- PMM



Thanks,

Diana

Re: [Qemu-devel] [PATCH v2 0/2] Add global device ID in virt machine

[Diana Madalina Craciun]

On 07/06/2017 02:44 AM, Michael S. Tsirkin wrote:
> On Wed, May 31, 2017 at 12:02:56PM +0000, Diana Madalina Craciun wrote:
>> On 05/25/2017 01:12 AM, Michael S. Tsirkin wrote:
>>> On Tue, May 23, 2017 at 02:12:43PM +0300, Diana Craciun wrote:
>>>> The NXP DPAA2 is a hardware architecture designed for high-speeed network
>>>> packet processing. The DPAA2 hardware components are managed by a hardware
>>>> component called the Management Complex (or MC) which provides an
>>>> object-base abstraction for software drivers to use the DPAA2 hardware.
>>>> For more details you can see: 
>>>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftree%2Fdrivers%2Fstaging%2Ffsl-mc%2FREADME.txt%3Fh%3Dv4.10&data=01%7C01%7Cdiana.craciun%40nxp.com%7Cce2cc4d066944ce2759308d4a2f1f3f8%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0&sdata=CJAiTF6Qnq4gklSqon7xcRby0O1HQvytTUSdPwaHuSE%3D&reserved=0
>>>>
>>>> The interrupts generated by the DPAA2 hardware components are MSIs. We will add
>>>> support for direct assigning these DPAA2 components/objects to a virtual 
>>>> machine. However, this will add the need to expand the MSI usage in QEMU.
>>>>
>>>> Currently the MSIs in QEMU are pretty much tied to PCI. For ARM the
>>>> GIC ITS is using a device ID for interrupt translation. Currently, for
>>>> PCI, the requester ID is used as device ID. This will not work when
>>>> we add another entity that needs also a device ID which is supposed to
>>>> be unique across the system.
>>>>
>>>> My proposal is to add a static allocation in the virt machine. I considered
>>>> that this allocation is specific to each machine/platform. Currently only
>>>> virt machine has it, but other implementations may use the same mechanism
>>>> as well.
>>>> So, I used a static allocation with this formula:
>>>>
>>>> DeviceID = zero_extend( RequesterID[15:0] ) + 0x10000 * Constant
>>>>
>>>> This formula was taken from SBSA spec (Appendix I: DeviceID generation and
>>>> ITS groups). In case of QEMU the constant will be different for each entity.
>>>> In this way a unique DeviceID will be generated and the device ID will be
>>>> derived from a requesterID (in case of PCI) or other means in case of other
>>>> entities.
>>>>
>>>> The implementation is generic as there might be in the future other non-pci devices
>>>> that are using MSIs or IOMMU. Any architecture can use it, though currently
>>>> only the ARM architecture is using the function that retrieves the stream ID. I
>>>> did not change all the replacements of the pci_requester_id (with pci_stream_id)
>>>> in the code (although if the constant is 0, the stream_id is equal with requester_id).
>>>> The other architectures (e.g. intel iommu code) assume that the ID is the
>>>> requester ID.
>>>>
>>>> Tested on NXP LS2080 platform.
>>>>
>>>> History:
>>> I am confused. I get it that non-PCI things want something else
>>> in their requester ID, but why require it for PCI devices?
>>> How about using Constant == 0 for PCI? This way you do
>>> not need to touch PCI at all as DeviceID == RequesterID ...
>> It is not that other devices need something else in the requester ID,
>> but more about finding a way to provide an unique ID across the system
>> (more precisely it should be unique for all devices connected to the
>> same IOMMU/ITS). The DT already offers support to describe the
>> translation between stream IDs/device IDs to requester ID for PCI
>> devices (iommu-map for IOMMU ([1]) and msi-map for MSIs ([2]). It will
>> not change the way the requester ID is used in PCI in general, but only
>> the places that need a unique ID (which are the MSIs and IOMMU).
>>
>> If we are to use a value of 0 for the constant in case of PCI devices,
>> what happens if we have multiple PCI controllers?
> I guess we'd use the PCI Segment number for that?
>
>

Yes, we can use the PCI segment for this scenario. But this would mean
different solutions for the same problem. The main problem is that we
can have multiple entities in the system that are using MSIs (for now
PCI and NXP non-PCI bus infrastructure
(https://lwn.net/Articles/635905/). I guess that we may have other
platform devices that are using MSIs in the future.

Thanks,
Diana

Re: [Qemu-devel] [PATCH v2 0/2] Add global device ID in virt machine

[Michael S. Tsirkin]

On Mon, Jul 31, 2017 at 01:22:45PM +0000, Diana Madalina Craciun wrote:
> >> If we are to use a value of 0 for the constant in case of PCI devices,
> >> what happens if we have multiple PCI controllers?
> > I guess we'd use the PCI Segment number for that?
> >
> >
> 
> Yes, we can use the PCI segment for this scenario. But this would mean
> different solutions for the same problem. The main problem is that we
> can have multiple entities in the system that are using MSIs (for now
> PCI and NXP non-PCI bus infrastructure
> (https://lwn.net/Articles/635905/). I guess that we may have other
> platform devices that are using MSIs in the future.
> 
> Thanks,
> Diana
> 
> 

Don't have the time to explore NXP in depth, sorry - there's
a lot of complexity there.
Could you maybe stick some bits to specify bus type in there?
It just looks very wrong to push low level things like this
that users have no interest in up the stack.

-- 
MST

Re: [Qemu-devel] [PATCH v2 0/2] Add global device ID in virt machine

[Diana Madalina Craciun]

On 07/31/2017 05:06 PM, Michael S. Tsirkin wrote:
> On Mon, Jul 31, 2017 at 01:22:45PM +0000, Diana Madalina Craciun wrote:
>>>> If we are to use a value of 0 for the constant in case of PCI devices,
>>>> what happens if we have multiple PCI controllers?
>>> I guess we'd use the PCI Segment number for that?
>>>
>>>
>> Yes, we can use the PCI segment for this scenario. But this would mean
>> different solutions for the same problem. The main problem is that we
>> can have multiple entities in the system that are using MSIs (for now
>> PCI and NXP non-PCI bus infrastructure
>> (https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flwn.net%2FArticles%2F635905%2F&data=01%7C01%7Cdiana.craciun%40nxp.com%7C6b0c6c879af64718a21908d4d81d534e%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0&sdata=bpYMMqajWzgzdbdQgy%2FUYR7y%2BswyvwE%2BqFzs7wdIkkA%3D&reserved=0). I guess that we may have other
>> platform devices that are using MSIs in the future.
>>
>> Thanks,
>> Diana
>>
>>
> Don't have the time to explore NXP in depth, sorry - there's
> a lot of complexity there.
> Could you maybe stick some bits to specify bus type in there?
> It just looks very wrong to push low level things like this
> that users have no interest in up the stack.
>
Let's generalize the problem a little bit, the NXP details just does not
matter much. The problem we have is the following:

The GIC-ITS, the ARM MSI controller is using deviceIDs in order to remap
the interrupts. Each device which is expected to send MSIs has a
deviceID associated with it. These deviceIDs are configured into devices
by software/firmware. There is support in the device tree to specify the
correlation between requesterID and deviceID:

"msi-map: Maps a Requester ID to an MSI controller and associated
  msi-specifier data. The property is an arbitrary number of tuples of
  (rid-base,msi-controller,msi-base,length)"
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/pci-msi.txt)

Our problem is that we have to allocate these deviceIDs in QEMU as well
and we have to ensure that they are unique. Currently, for PCI, the
assumption requesterID=deviceID is made which will no longer be true in
case other devices are added. So we need a way (preferable a general
one) to allocate these IDs to different devices in the system in a
consistent way which will ensure that two devices do not share the same ID.

The reason I put this ID into the controller itself is because on real
hardware is actually programmed into the controller. It is needed (for
example) when the MSIs are sent.

Thanks,

Diana

Re: [Qemu-devel] [PATCH v2 2/2] Add a unique ID in the virt machine to be used as device ID

[Edgar E. Iglesias]

On Wed, Jul 26, 2017 at 02:22:28PM +0200, 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?
> > 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 <diana.craciun@nxp.com>
> > ---
> >  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(-)
> > 
> > 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.
> 
> 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?
> 
> > +
> > +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?
> STREAM_ID_RANGE_SIZE ~ MSI_MAP_LENGTH?
> >      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.
> 
> > +
> >  /* -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

I agree.

I think that what we need is to add support for allowing PCI RCs
to transform requesterIDs in transactions attributes according to the
implementation specifics.

The way we did it when modelling the ZynqMP is by adding support for
transaction attribute translation in QEMU's IOMMU interface.
In our PCI RC, we have an IOMMU covering the entire AS that PCI devs DMA into.
This IOMMU doesn't do address-translation, only RequesterID -> StreamID
transforms according to how the ZynqMP PCI RC derives StreamIDs from RequesterIDs.

This is useful not only to model PCI RequesterID to AXI Master ID mappings but
also for modelling things like the ARM TZC (or the Xilinx ZynqMP XMPU/XPPUs).

Cheers,
Edgar


> simple than reworking pci_requester_id() though.
> >  
> >      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?
> 
> 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.
> 
> Thanks
> 
> Eric
> >      char name[64];
> >      PCIIORegion io_regions[PCI_NUM_REGIONS];
> >      AddressSpace bus_master_as;
> > @@ -737,6 +743,8 @@ static inline uint16_t pci_get_bdf(PCIDevice *dev)
> >  
> >  uint16_t pci_requester_id(PCIDevice *dev);
> >  
> > +uint32_t pci_stream_id(PCIDevice *dev);
> > +
> >  /* DMA access functions */
> >  static inline AddressSpace *pci_get_address_space(PCIDevice *dev)
> >  {
> > diff --git a/kvm-all.c b/kvm-all.c
> > index 90b8573..5a508c3 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -1280,7 +1280,7 @@ int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
> >      kroute.u.msi.data = le32_to_cpu(msg.data);
> >      if (kvm_msi_devid_required()) {
> >          kroute.flags = KVM_MSI_VALID_DEVID;
> > -        kroute.u.msi.devid = pci_requester_id(dev);
> > +        kroute.u.msi.devid = pci_stream_id(dev);
> >      }
> >      if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data, dev)) {
> >          kvm_irqchip_release_virq(s, virq);
> > @@ -1317,7 +1317,7 @@ int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg,
> >      kroute.u.msi.data = le32_to_cpu(msg.data);
> >      if (kvm_msi_devid_required()) {
> >          kroute.flags = KVM_MSI_VALID_DEVID;
> > -        kroute.u.msi.devid = pci_requester_id(dev);
> > +        kroute.u.msi.devid = pci_stream_id(dev);
> >      }
> >      if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data, dev)) {
> >          return -EINVAL;
> > 
> 

Re: [Qemu-devel] [PATCH v2 2/2] Add a unique ID in the virt machine to be used as device ID

[Edgar E. Iglesias]

On Mon, Jul 31, 2017 at 05:16:02PM +0200, Edgar E. Iglesias wrote:
> On Wed, Jul 26, 2017 at 02:22:28PM +0200, 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?
> > > 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 <diana.craciun@nxp.com>
> > > ---
> > >  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(-)
> > > 
> > > 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.
> > 
> > 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?
> > 
> > > +
> > > +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?
> > STREAM_ID_RANGE_SIZE ~ MSI_MAP_LENGTH?
> > >      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.
> > 
> > > +
> > >  /* -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
> 
> I agree.
> 
> I think that what we need is to add support for allowing PCI RCs
> to transform requesterIDs in transactions attributes according to the
> implementation specifics.
> 
> The way we did it when modelling the ZynqMP is by adding support for
> transaction attribute translation in QEMU's IOMMU interface.
> In our PCI RC, we have an IOMMU covering the entire AS that PCI devs DMA into.
> This IOMMU doesn't do address-translation, only RequesterID -> StreamID
> transforms according to how the ZynqMP PCI RC derives StreamIDs from RequesterIDs.
> 
> This is useful not only to model PCI RequesterID to AXI Master ID mappings but
> also for modelling things like the ARM TZC (or the Xilinx ZynqMP XMPU/XPPUs).
> 


BTW, for AMBA devices, I think upstream is still missing a way for machines
to configure a memory attributes template for DMA devices (e.g with the
MasterID)...

That would also be needed for GICv3 ITS and MSI's originating from non-PCI
devs...

Cheers,
Edgar

Re: [Qemu-devel] [PATCH v2 0/2] Add global device ID in virt machine

[Michael S. Tsirkin]

On Mon, Jul 31, 2017 at 03:13:09PM +0000, Diana Madalina Craciun wrote:
> On 07/31/2017 05:06 PM, Michael S. Tsirkin wrote:
> > On Mon, Jul 31, 2017 at 01:22:45PM +0000, Diana Madalina Craciun wrote:
> >>>> If we are to use a value of 0 for the constant in case of PCI devices,
> >>>> what happens if we have multiple PCI controllers?
> >>> I guess we'd use the PCI Segment number for that?
> >>>
> >>>
> >> Yes, we can use the PCI segment for this scenario. But this would mean
> >> different solutions for the same problem. The main problem is that we
> >> can have multiple entities in the system that are using MSIs (for now
> >> PCI and NXP non-PCI bus infrastructure
> >> (https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flwn.net%2FArticles%2F635905%2F&data=01%7C01%7Cdiana.craciun%40nxp.com%7C6b0c6c879af64718a21908d4d81d534e%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0&sdata=bpYMMqajWzgzdbdQgy%2FUYR7y%2BswyvwE%2BqFzs7wdIkkA%3D&reserved=0). I guess that we may have other
> >> platform devices that are using MSIs in the future.
> >>
> >> Thanks,
> >> Diana
> >>
> >>
> > Don't have the time to explore NXP in depth, sorry - there's
> > a lot of complexity there.
> > Could you maybe stick some bits to specify bus type in there?
> > It just looks very wrong to push low level things like this
> > that users have no interest in up the stack.
> >
> Let's generalize the problem a little bit, the NXP details just does not
> matter much. The problem we have is the following:
> 
> The GIC-ITS, the ARM MSI controller is using deviceIDs in order to remap
> the interrupts. Each device which is expected to send MSIs has a
> deviceID associated with it. These deviceIDs are configured into devices
> by software/firmware. There is support in the device tree to specify the
> correlation between requesterID and deviceID:
> 
> "msi-map: Maps a Requester ID to an MSI controller and associated
>   msi-specifier data. The property is an arbitrary number of tuples of
>   (rid-base,msi-controller,msi-base,length)"
> (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/pci-msi.txt)
> 
> Our problem is that we have to allocate these deviceIDs in QEMU as well
> and we have to ensure that they are unique. Currently, for PCI, the
> assumption requesterID=deviceID is made which will no longer be true in
> case other devices are added. So we need a way (preferable a general
> one) to allocate these IDs to different devices in the system in a
> consistent way which will ensure that two devices do not share the same ID.

My question would be, do other types of devices that are there
right now have some kind of ID like the requester ID?
If so I would say just use that, and set high bits in the device ID
to specify the type (e.g. 00 for pci, etc).


IMHO if possible that is preferable to pushing this up to users.


> The reason I put this ID into the controller itself is because on real
> hardware is actually programmed into the controller. It is needed (for
> example) when the MSIs are sent.
> 
> Thanks,
> 
> Diana
> 

IIUC what happens on real hardware is controller maps each requester ID
(or presumably other source ID in the request) to the device ID,
and the mapping is internal to controller.
If you wanted a lot of flexibility then looks like you could pass this
mapping to controllers, but is it really necessary?
Why don't we build a mapping that's convenient for us?


-- 
MST

Re: [Qemu-devel] [Qemu-arm] [PATCH v2 0/2] Add global device ID in virt machine

[Edgar E. Iglesias]

On Tue, Aug 01, 2017 at 05:05:42AM +0300, Michael S. Tsirkin wrote:
> On Mon, Jul 31, 2017 at 03:13:09PM +0000, Diana Madalina Craciun wrote:
> > On 07/31/2017 05:06 PM, Michael S. Tsirkin wrote:
> > > On Mon, Jul 31, 2017 at 01:22:45PM +0000, Diana Madalina Craciun wrote:
> > >>>> If we are to use a value of 0 for the constant in case of PCI devices,
> > >>>> what happens if we have multiple PCI controllers?
> > >>> I guess we'd use the PCI Segment number for that?
> > >>>
> > >>>
> > >> Yes, we can use the PCI segment for this scenario. But this would mean
> > >> different solutions for the same problem. The main problem is that we
> > >> can have multiple entities in the system that are using MSIs (for now
> > >> PCI and NXP non-PCI bus infrastructure
> > >> (https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flwn.net%2FArticles%2F635905%2F&data=01%7C01%7Cdiana.craciun%40nxp.com%7C6b0c6c879af64718a21908d4d81d534e%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0&sdata=bpYMMqajWzgzdbdQgy%2FUYR7y%2BswyvwE%2BqFzs7wdIkkA%3D&reserved=0). I guess that we may have other
> > >> platform devices that are using MSIs in the future.
> > >>
> > >> Thanks,
> > >> Diana
> > >>
> > >>
> > > Don't have the time to explore NXP in depth, sorry - there's
> > > a lot of complexity there.
> > > Could you maybe stick some bits to specify bus type in there?
> > > It just looks very wrong to push low level things like this
> > > that users have no interest in up the stack.
> > >
> > Let's generalize the problem a little bit, the NXP details just does not
> > matter much. The problem we have is the following:
> > 
> > The GIC-ITS, the ARM MSI controller is using deviceIDs in order to remap
> > the interrupts. Each device which is expected to send MSIs has a
> > deviceID associated with it. These deviceIDs are configured into devices
> > by software/firmware. There is support in the device tree to specify the
> > correlation between requesterID and deviceID:
> > 
> > "msi-map: Maps a Requester ID to an MSI controller and associated
> >   msi-specifier data. The property is an arbitrary number of tuples of
> >   (rid-base,msi-controller,msi-base,length)"
> > (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/pci-msi.txt)
> > 
> > Our problem is that we have to allocate these deviceIDs in QEMU as well
> > and we have to ensure that they are unique. Currently, for PCI, the
> > assumption requesterID=deviceID is made which will no longer be true in
> > case other devices are added. So we need a way (preferable a general
> > one) to allocate these IDs to different devices in the system in a
> > consistent way which will ensure that two devices do not share the same ID.
> 
> My question would be, do other types of devices that are there
> right now have some kind of ID like the requester ID?
> If so I would say just use that, and set high bits in the device ID
> to specify the type (e.g. 00 for pci, etc).
> 
> 
> IMHO if possible that is preferable to pushing this up to users.
> 
> 
> > The reason I put this ID into the controller itself is because on real
> > hardware is actually programmed into the controller. It is needed (for
> > example) when the MSIs are sent.
> > 
> > Thanks,
> > 
> > Diana
> > 
> 
> IIUC what happens on real hardware is controller maps each requester ID
> (or presumably other source ID in the request) to the device ID,
> and the mapping is internal to controller.
> If you wanted a lot of flexibility then looks like you could pass this
> mapping to controllers, but is it really necessary?
> Why don't we build a mapping that's convenient for us?
> 

Hi,

I agree with you for boards that are defined by QEMU (like the ARM virt board).
We can pick a convenient scheme and avoid too much user interaction.

But when we model real SoCs, the IDs and the way bridges convert these IDs
needs to match real HW. Otherwise guest SW will not work unmodified.
To make this work, users should not need to be involved though, it's
the machine and SoC modules that should instantiate the necessary handling
of it, IMO.

Cheers,
Edgar

Re: [Qemu-devel] [PATCH v2 0/2] Add global device ID in virt machine

[Diana Madalina Craciun]

On 08/01/2017 05:05 AM, Michael S. Tsirkin wrote:
> On Mon, Jul 31, 2017 at 03:13:09PM +0000, Diana Madalina Craciun wrote:
>> On 07/31/2017 05:06 PM, Michael S. Tsirkin wrote:
>>> On Mon, Jul 31, 2017 at 01:22:45PM +0000, Diana Madalina Craciun wrote:
>>>>>> If we are to use a value of 0 for the constant in case of PCI devices,
>>>>>> what happens if we have multiple PCI controllers?
>>>>> I guess we'd use the PCI Segment number for that?
>>>>>
>>>>>
>>>> Yes, we can use the PCI segment for this scenario. But this would mean
>>>> different solutions for the same problem. The main problem is that we
>>>> can have multiple entities in the system that are using MSIs (for now
>>>> PCI and NXP non-PCI bus infrastructure
>>>> (https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flwn.net%2FArticles%2F635905%2F&data=01%7C01%7Cdiana.craciun%40nxp.com%7C6b0c6c879af64718a21908d4d81d534e%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0&sdata=bpYMMqajWzgzdbdQgy%2FUYR7y%2BswyvwE%2BqFzs7wdIkkA%3D&reserved=0). I guess that we may have other
>>>> platform devices that are using MSIs in the future.
>>>>
>>>> Thanks,
>>>> Diana
>>>>
>>>>
>>> Don't have the time to explore NXP in depth, sorry - there's
>>> a lot of complexity there.
>>> Could you maybe stick some bits to specify bus type in there?
>>> It just looks very wrong to push low level things like this
>>> that users have no interest in up the stack.
>>>
>> Let's generalize the problem a little bit, the NXP details just does not
>> matter much. The problem we have is the following:
>>
>> The GIC-ITS, the ARM MSI controller is using deviceIDs in order to remap
>> the interrupts. Each device which is expected to send MSIs has a
>> deviceID associated with it. These deviceIDs are configured into devices
>> by software/firmware. There is support in the device tree to specify the
>> correlation between requesterID and deviceID:
>>
>> "msi-map: Maps a Requester ID to an MSI controller and associated
>>   msi-specifier data. The property is an arbitrary number of tuples of
>>   (rid-base,msi-controller,msi-base,length)"
>> (https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2Ftree%2FDocumentation%2Fdevicetree%2Fbindings%2Fpci%2Fpci-msi.txt&data=01%7C01%7Cdiana.craciun%40nxp.com%7C259982f3a83b4160083708d4d881d48a%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0&sdata=QSRwrYcsTk9%2FgekD0YBYKvpWv05%2BXKiH2abPp%2FtHpes%3D&reserved=0)
>>
>> Our problem is that we have to allocate these deviceIDs in QEMU as well
>> and we have to ensure that they are unique. Currently, for PCI, the
>> assumption requesterID=deviceID is made which will no longer be true in
>> case other devices are added. So we need a way (preferable a general
>> one) to allocate these IDs to different devices in the system in a
>> consistent way which will ensure that two devices do not share the same ID.
> My question would be, do other types of devices that are there
> right now have some kind of ID like the requester ID?
> If so I would say just use that, and set high bits in the device ID
> to specify the type (e.g. 00 for pci, etc).

Would this mean extending the PCI requesterID usage as well? Or assume
that it's 00 for PCI and no changes are needed? I understand that with
this approach the allocation is  static: 00 for PCI, 01 for device type
X, etc., so the situation when we have multiple controllers (PCI or
whatever other devices) has to be solved differently. Wouldn't be this
more complex?

>
>
> IMHO if possible that is preferable to pushing this up to users.
>
>
>> The reason I put this ID into the controller itself is because on real
>> hardware is actually programmed into the controller. It is needed (for
>> example) when the MSIs are sent.
>>
>> Thanks,
>>
>> Diana
>>
> IIUC what happens on real hardware is controller maps each requester ID
> (or presumably other source ID in the request) to the device ID,
> and the mapping is internal to controller.
> If you wanted a lot of flexibility then looks like you could pass this
> mapping to controllers, but is it really necessary?
> Why don't we build a mapping that's convenient for us?
>
>
Thanks,

Diana

Re: [Qemu-devel] [PATCH v2 1/2] Increased the size of requester_id field from MemTxAttrs

[Diana Madalina Craciun]

Hi Eric,

On 07/26/2017 03:23 PM, Auger Eric wrote:
> Hi Diana,
> On 23/05/2017 13:12, Diana Craciun wrote:
>> The PCI requester ID field is 16 bits. The requester_id field
>> from MemTxAttrs is used for MSIs to specify the device ID for
>> the platforms where this device ID is needed (e.g virt machine + GICv3
>> ITS). However, if more entities that uses MSIs in the system are used,
>> 16 bit is no longer enough to represent the device ID. Increased the size
>> of this field to 24 bits in order to accomodate 256 entities.
>> Also the name requester_id does no longer reflect the content, so
>> the name was changed to stream_id.
> I think both deviceid and streamid can be up to 32 bits in theory.

Yes, both of them can be up to 32 bit. I can extend the field to 32 bit.

>
> With respect to the renaming, stream_id really is ARM specific and
> corresponds to the ID space before the SMMU while you mostly address
> device id problematics here (ie. space id input to the interrupt
> controller). Maybe a more generic terminology such as originator id
> could be used or source id (I think this is Intel terminology though).

I agree, originator id sounds good and so far it doesn't seem to overlap
to other specific terminology.

>
> Maybe the changes in semantics of the field should be
> associated/combined with its new computation found in next patch? see
> comments in subsequent patch.

I agree.

Thanks,

Diana

Re: [Qemu-devel] [PATCH v2 2/2] Add a unique ID in the virt machine to be used as device ID

[Diana Madalina Craciun]

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 <diana.craciun@nxp.com><mailto:diana.craciun@nxp.com>
---
 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(-)

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?

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

Re: [Qemu-devel] [PATCH v2 2/2] Add a unique ID in the virt machine to be used as device ID

[Diana Madalina Craciun]

Hi Edgar,

On 07/31/2017 06:16 PM, Edgar E. Iglesias wrote:
> On Wed, Jul 26, 2017 at 02:22:28PM +0200, 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?
>>> 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 <diana.craciun@nxp.com>
>>> ---
>>>  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(-)
>>>
>>> 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.
>>
>> 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?
>>
>>> +
>>> +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?
>> STREAM_ID_RANGE_SIZE ~ MSI_MAP_LENGTH?
>>>      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.
>>
>>> +
>>>  /* -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
> I agree.

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).

> I think that what we need is to add support for allowing PCI RCs
> to transform requesterIDs in transactions attributes according to the
> implementation specifics.

Do you mean that you need more than a linear offset between requesterID
and whatever other ID?

>
> The way we did it when modelling the ZynqMP is by adding support for
> transaction attribute translation in QEMU's IOMMU interface.
> In our PCI RC, we have an IOMMU covering the entire AS that PCI devs DMA into.
> This IOMMU doesn't do address-translation, only RequesterID -> StreamID
> transforms according to how the ZynqMP PCI RC derives StreamIDs from RequesterIDs.

Are there any patches for this support in order for me to better understand?

Thanks,

Diana


> This is useful not only to model PCI RequesterID to AXI Master ID mappings but
> also for modelling things like the ARM TZC (or the Xilinx ZynqMP XMPU/XPPUs).
>
> Cheers,
> Edgar
>
>
>> simple than reworking pci_requester_id() though.
>>>  
>>>      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?
>>
>> 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.
>>
>> Thanks
>>
>> Eric
>>>      char name[64];
>>>      PCIIORegion io_regions[PCI_NUM_REGIONS];
>>>      AddressSpace bus_master_as;
>>> @@ -737,6 +743,8 @@ static inline uint16_t pci_get_bdf(PCIDevice *dev)
>>>  
>>>  uint16_t pci_requester_id(PCIDevice *dev);
>>>  
>>> +uint32_t pci_stream_id(PCIDevice *dev);
>>> +
>>>  /* DMA access functions */
>>>  static inline AddressSpace *pci_get_address_space(PCIDevice *dev)
>>>  {
>>> diff --git a/kvm-all.c b/kvm-all.c
>>> index 90b8573..5a508c3 100644
>>> --- a/kvm-all.c
>>> +++ b/kvm-all.c
>>> @@ -1280,7 +1280,7 @@ int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
>>>      kroute.u.msi.data = le32_to_cpu(msg.data);
>>>      if (kvm_msi_devid_required()) {
>>>          kroute.flags = KVM_MSI_VALID_DEVID;
>>> -        kroute.u.msi.devid = pci_requester_id(dev);
>>> +        kroute.u.msi.devid = pci_stream_id(dev);
>>>      }
>>>      if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data, dev)) {
>>>          kvm_irqchip_release_virq(s, virq);
>>> @@ -1317,7 +1317,7 @@ int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg,
>>>      kroute.u.msi.data = le32_to_cpu(msg.data);
>>>      if (kvm_msi_devid_required()) {
>>>          kroute.flags = KVM_MSI_VALID_DEVID;
>>> -        kroute.u.msi.devid = pci_requester_id(dev);
>>> +        kroute.u.msi.devid = pci_stream_id(dev);
>>>      }
>>>      if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data, dev)) {
>>>          return -EINVAL;
>>>

Re: [Qemu-devel] [PATCH v2 2/2] Add a unique ID in the virt machine to be used as device ID

[Edgar E. Iglesias]

On Fri, Aug 11, 2017 at 02:35:28PM +0000, Diana Madalina Craciun wrote:
> Hi Edgar,
> 
> On 07/31/2017 06:16 PM, Edgar E. Iglesias wrote:
> > On Wed, Jul 26, 2017 at 02:22:28PM +0200, 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?
> >>> 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 <diana.craciun@nxp.com>
> >>> ---
> >>>  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(-)
> >>>
> >>> 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.
> >>
> >> 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?
> >>
> >>> +
> >>> +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?
> >> STREAM_ID_RANGE_SIZE ~ MSI_MAP_LENGTH?
> >>>      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.
> >>
> >>> +
> >>>  /* -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
> > I agree.
> 
> 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).
> 
> > I think that what we need is to add support for allowing PCI RCs
> > to transform requesterIDs in transactions attributes according to the
> > implementation specifics.
> 
> Do you mean that you need more than a linear offset between requesterID
> and whatever other ID?


Yes.

This is my understanding for the ARM platforms I'm familiar with:

Since AXI busses don't have a defined way to carry Master IDs, these
are typically carried on the AXI user signals. I'll just refer to
these signals as AXI Master IDs.

1. An endpoint issues an MSI (or any) transaction on the PCI bus.
   In QEMU, these trasactions carry the requester ID in their attributes.

2. The transaction hits the PCI "host" bridge to the SoC internal
   interconnect (typically AXI). This bridge needs to forward the
   PCI transaction onto the AXI bus. Including mapping the PCI
   RequesterID into an AXI MasterID.

3. The AXI transaction hits the IOMMU and the MasterID is mapped
   into a streamID to identify the origin of the transaction
   and apply address translation accordingly. If the SMMU
   allows the transaction to pass, the stream ID is mapped back
   into the transactions MasterID.

4. The AXI transaction continues down the interconnect and hits
   the MSI doorbell and the MasterID is mapped into a DeviceID to
   identify the origin of the MSI and apply possible interrupt translation.

Adding streamID fields to a PCI endpoint doesn't make any sense to me.
The requester ID is already there and is IMO enough.
StreamIDs are a concept of ARM System MMUs, not of PCI endpoints.

When modelling #2, hardcoding a specific linear mapping between
PCI requester IDs and AXI Master IDs may work for one platform
but it won't work for all platforms. There is no one mapping for all.
It can even be run-time programmable in the bridge.

IIRC, the SMMUv3 docs have a section that suggest how these ReqID to AXI Master ID mappings can be done.


> > The way we did it when modelling the ZynqMP is by adding support for
> > transaction attribute translation in QEMU's IOMMU interface.
> > In our PCI RC, we have an IOMMU covering the entire AS that PCI devs DMA into.
> > This IOMMU doesn't do address-translation, only RequesterID -> StreamID
> > transforms according to how the ZynqMP PCI RC derives StreamIDs from RequesterIDs.
> 
> Are there any patches for this support in order for me to better understand?

It's currently on the Xilinx QEMU fork on GitHub.
https://github.com/Xilinx/qemu/blob/master/hw/pci-host/xlnx-nwl-pcie-main.c

In the current ZynqMP, all RequesterIDs map to a single MasterID (it's a HW limitation).
In future versions of the HW, another mapping will be used.
I can't share code for the latter yet though....

Best regards,
Edgar

 

> 
> Thanks,
> 
> Diana
> 
> 
> > This is useful not only to model PCI RequesterID to AXI Master ID mappings but
> > also for modelling things like the ARM TZC (or the Xilinx ZynqMP XMPU/XPPUs).
> >
> > Cheers,
> > Edgar
> >
> >
> >> simple than reworking pci_requester_id() though.
> >>>  
> >>>      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?
> >>
> >> 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.
> >>
> >> Thanks
> >>
> >> Eric
> >>>      char name[64];
> >>>      PCIIORegion io_regions[PCI_NUM_REGIONS];
> >>>      AddressSpace bus_master_as;
> >>> @@ -737,6 +743,8 @@ static inline uint16_t pci_get_bdf(PCIDevice *dev)
> >>>  
> >>>  uint16_t pci_requester_id(PCIDevice *dev);
> >>>  
> >>> +uint32_t pci_stream_id(PCIDevice *dev);
> >>> +
> >>>  /* DMA access functions */
> >>>  static inline AddressSpace *pci_get_address_space(PCIDevice *dev)
> >>>  {
> >>> diff --git a/kvm-all.c b/kvm-all.c
> >>> index 90b8573..5a508c3 100644
> >>> --- a/kvm-all.c
> >>> +++ b/kvm-all.c
> >>> @@ -1280,7 +1280,7 @@ int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
> >>>      kroute.u.msi.data = le32_to_cpu(msg.data);
> >>>      if (kvm_msi_devid_required()) {
> >>>          kroute.flags = KVM_MSI_VALID_DEVID;
> >>> -        kroute.u.msi.devid = pci_requester_id(dev);
> >>> +        kroute.u.msi.devid = pci_stream_id(dev);
> >>>      }
> >>>      if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data, dev)) {
> >>>          kvm_irqchip_release_virq(s, virq);
> >>> @@ -1317,7 +1317,7 @@ int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg,
> >>>      kroute.u.msi.data = le32_to_cpu(msg.data);
> >>>      if (kvm_msi_devid_required()) {
> >>>          kroute.flags = KVM_MSI_VALID_DEVID;
> >>> -        kroute.u.msi.devid = pci_requester_id(dev);
> >>> +        kroute.u.msi.devid = pci_stream_id(dev);
> >>>      }
> >>>      if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data, dev)) {
> >>>          return -EINVAL;
> >>>
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH v2 1/2] Increased the size of requester_id field from MemTxAttrs

[Peter Maydell]

On 11 August 2017 at 15:32, Diana Madalina Craciun
<diana.craciun@nxp.com> wrote:
> On 07/26/2017 03:23 PM, Auger Eric wrote:
>> With respect to the renaming, stream_id really is ARM specific and
>> corresponds to the ID space before the SMMU while you mostly address
>> device id problematics here (ie. space id input to the interrupt
>> controller). Maybe a more generic terminology such as originator id
>> could be used or source id (I think this is Intel terminology though).
>
> I agree, originator id sounds good and so far it doesn't seem to overlap
> to other specific terminology.

That reminds me that the other thing I'd like to use this field
for is the "identify which CPU of a cluster made this memory
transaction" thing (which you can get on ARM AXI etc buses).
So that's another thing that would need to have some part
of the ID space...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 2/2] Add a unique ID in the virt machine to be used as device ID

[Diana Madalina Craciun]

On 08/11/2017 06:50 PM, Edgar E. Iglesias wrote:
> On Fri, Aug 11, 2017 at 02:35:28PM +0000, Diana Madalina Craciun wrote:
>> Hi Edgar,
>>
>> On 07/31/2017 06:16 PM, Edgar E. Iglesias wrote:
>>> On Wed, Jul 26, 2017 at 02:22:28PM +0200, 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?
>>>>> 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 <diana.craciun@nxp.com>
>>>>> ---
>>>>>  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(-)
>>>>>
>>>>> 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.
>>>>
>>>> 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?
>>>>
>>>>> +
>>>>> +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?
>>>> STREAM_ID_RANGE_SIZE ~ MSI_MAP_LENGTH?
>>>>>      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.
>>>>
>>>>> +
>>>>>  /* -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
>>> I agree.
>> 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).
>>
>>> I think that what we need is to add support for allowing PCI RCs
>>> to transform requesterIDs in transactions attributes according to the
>>> implementation specifics.
>> Do you mean that you need more than a linear offset between requesterID
>> and whatever other ID?
>
> Yes.
>
> This is my understanding for the ARM platforms I'm familiar with:
>
> Since AXI busses don't have a defined way to carry Master IDs, these
> are typically carried on the AXI user signals. I'll just refer to
> these signals as AXI Master IDs.
>
> 1. An endpoint issues an MSI (or any) transaction on the PCI bus.
>    In QEMU, these trasactions carry the requester ID in their attributes.
>
> 2. The transaction hits the PCI "host" bridge to the SoC internal
>    interconnect (typically AXI). This bridge needs to forward the
>    PCI transaction onto the AXI bus. Including mapping the PCI
>    RequesterID into an AXI MasterID.
>
> 3. The AXI transaction hits the IOMMU and the MasterID is mapped
>    into a streamID to identify the origin of the transaction
>    and apply address translation accordingly. If the SMMU
>    allows the transaction to pass, the stream ID is mapped back
>    into the transactions MasterID.
>
> 4. The AXI transaction continues down the interconnect and hits
>    the MSI doorbell and the MasterID is mapped into a DeviceID to
>    identify the origin of the MSI and apply possible interrupt translation.
>
> Adding streamID fields to a PCI endpoint doesn't make any sense to me.
> The requester ID is already there and is IMO enough.
> StreamIDs are a concept of ARM System MMUs, not of PCI endpoints.

What I have added into the endpoint is actually the Master ID (in QEMU
it is actually equal with the streamID). I agree that this is a property
of the root complex, the only reason I have put it into the endpoint was
to avoid traversing the PCI hierarchy each time an MSI is sent.

>
> When modelling #2, hardcoding a specific linear mapping between
> PCI requester IDs and AXI Master IDs may work for one platform
> but it won't work for all platforms. There is no one mapping for all.
> It can even be run-time programmable in the bridge.

One solution might be defining a function in the generic host bridge
which by default returns the requesterIDs (assumes that requesterID is
the same with the masterID). This function can be over overridden by
each specific implementation.

>
> IIRC, the SMMUv3 docs have a section that suggest how these ReqID to AXI Master ID mappings can be done.

I did not find the specific section, just that the streamID should be
derived from requesterID.


Thanks,

Diana


>
>
>>> The way we did it when modelling the ZynqMP is by adding support for
>>> transaction attribute translation in QEMU's IOMMU interface.
>>> In our PCI RC, we have an IOMMU covering the entire AS that PCI devs DMA into.
>>> This IOMMU doesn't do address-translation, only RequesterID -> StreamID
>>> transforms according to how the ZynqMP PCI RC derives StreamIDs from RequesterIDs.
>> Are there any patches for this support in order for me to better understand?
> It's currently on the Xilinx QEMU fork on GitHub.
> https://github.com/Xilinx/qemu/blob/master/hw/pci-host/xlnx-nwl-pcie-main.c
>
> In the current ZynqMP, all RequesterIDs map to a single MasterID (it's a HW limitation).
> In future versions of the HW, another mapping will be used.
> I can't share code for the latter yet though....
>
> Best regards,
> Edgar
>
>  
>
>> Thanks,
>>
>> Diana
>>
>>
>>> This is useful not only to model PCI RequesterID to AXI Master ID mappings but
>>> also for modelling things like the ARM TZC (or the Xilinx ZynqMP XMPU/XPPUs).
>>>
>>> Cheers,
>>> Edgar
>>>
>>>
>>>> simple than reworking pci_requester_id() though.
>>>>>  
>>>>>      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?
>>>>
>>>> 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.
>>>>
>>>> Thanks
>>>>
>>>> Eric
>>>>>      char name[64];
>>>>>      PCIIORegion io_regions[PCI_NUM_REGIONS];
>>>>>      AddressSpace bus_master_as;
>>>>> @@ -737,6 +743,8 @@ static inline uint16_t pci_get_bdf(PCIDevice *dev)
>>>>>  
>>>>>  uint16_t pci_requester_id(PCIDevice *dev);
>>>>>  
>>>>> +uint32_t pci_stream_id(PCIDevice *dev);
>>>>> +
>>>>>  /* DMA access functions */
>>>>>  static inline AddressSpace *pci_get_address_space(PCIDevice *dev)
>>>>>  {
>>>>> diff --git a/kvm-all.c b/kvm-all.c
>>>>> index 90b8573..5a508c3 100644
>>>>> --- a/kvm-all.c
>>>>> +++ b/kvm-all.c
>>>>> @@ -1280,7 +1280,7 @@ int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
>>>>>      kroute.u.msi.data = le32_to_cpu(msg.data);
>>>>>      if (kvm_msi_devid_required()) {
>>>>>          kroute.flags = KVM_MSI_VALID_DEVID;
>>>>> -        kroute.u.msi.devid = pci_requester_id(dev);
>>>>> +        kroute.u.msi.devid = pci_stream_id(dev);
>>>>>      }
>>>>>      if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data, dev)) {
>>>>>          kvm_irqchip_release_virq(s, virq);
>>>>> @@ -1317,7 +1317,7 @@ int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg,
>>>>>      kroute.u.msi.data = le32_to_cpu(msg.data);
>>>>>      if (kvm_msi_devid_required()) {
>>>>>          kroute.flags = KVM_MSI_VALID_DEVID;
>>>>> -        kroute.u.msi.devid = pci_requester_id(dev);
>>>>> +        kroute.u.msi.devid = pci_stream_id(dev);
>>>>>      }
>>>>>      if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data, dev)) {
>>>>>          return -EINVAL;
>>>>>

Re: [Qemu-devel] [PATCH v2 2/2] Add a unique ID in the virt machine to be used as device ID

[Michael S. Tsirkin]

On Tue, Aug 22, 2017 at 03:13:57PM +0000, Diana Madalina Craciun wrote:
> On 08/11/2017 06:50 PM, Edgar E. Iglesias wrote:
> > On Fri, Aug 11, 2017 at 02:35:28PM +0000, Diana Madalina Craciun wrote:
> >> Hi Edgar,
> >>
> >> On 07/31/2017 06:16 PM, Edgar E. Iglesias wrote:
> >>> On Wed, Jul 26, 2017 at 02:22:28PM +0200, 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?
> >>>>> 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 <diana.craciun@nxp.com>
> >>>>> ---
> >>>>>  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(-)
> >>>>>
> >>>>> 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.
> >>>>
> >>>> 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?
> >>>>
> >>>>> +
> >>>>> +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?
> >>>> STREAM_ID_RANGE_SIZE ~ MSI_MAP_LENGTH?
> >>>>>      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.
> >>>>
> >>>>> +
> >>>>>  /* -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
> >>> I agree.
> >> 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).
> >>
> >>> I think that what we need is to add support for allowing PCI RCs
> >>> to transform requesterIDs in transactions attributes according to the
> >>> implementation specifics.
> >> Do you mean that you need more than a linear offset between requesterID
> >> and whatever other ID?
> >
> > Yes.
> >
> > This is my understanding for the ARM platforms I'm familiar with:
> >
> > Since AXI busses don't have a defined way to carry Master IDs, these
> > are typically carried on the AXI user signals. I'll just refer to
> > these signals as AXI Master IDs.
> >
> > 1. An endpoint issues an MSI (or any) transaction on the PCI bus.
> >    In QEMU, these trasactions carry the requester ID in their attributes.
> >
> > 2. The transaction hits the PCI "host" bridge to the SoC internal
> >    interconnect (typically AXI). This bridge needs to forward the
> >    PCI transaction onto the AXI bus. Including mapping the PCI
> >    RequesterID into an AXI MasterID.
> >
> > 3. The AXI transaction hits the IOMMU and the MasterID is mapped
> >    into a streamID to identify the origin of the transaction
> >    and apply address translation accordingly. If the SMMU
> >    allows the transaction to pass, the stream ID is mapped back
> >    into the transactions MasterID.
> >
> > 4. The AXI transaction continues down the interconnect and hits
> >    the MSI doorbell and the MasterID is mapped into a DeviceID to
> >    identify the origin of the MSI and apply possible interrupt translation.
> >
> > Adding streamID fields to a PCI endpoint doesn't make any sense to me.
> > The requester ID is already there and is IMO enough.
> > StreamIDs are a concept of ARM System MMUs, not of PCI endpoints.
> 
> What I have added into the endpoint is actually the Master ID (in QEMU
> it is actually equal with the streamID). I agree that this is a property
> of the root complex, the only reason I have put it into the endpoint was
> to avoid traversing the PCI hierarchy each time an MSI is sent.

Can all this be folded into the IOMMU? Then you might be able to get by
with defining an iommu function.  pci_device_iommu_address_space already
walks the hierarchy.



> >
> > When modelling #2, hardcoding a specific linear mapping between
> > PCI requester IDs and AXI Master IDs may work for one platform
> > but it won't work for all platforms. There is no one mapping for all.
> > It can even be run-time programmable in the bridge.

OK but how does it work with the specific bridge that you emulate?
There is no need to model advanced bridges with super flexible
programmable mappings if guests do not need them to run.

> 
> One solution might be defining a function in the generic host bridge
> which by default returns the requesterIDs (assumes that requesterID is
> the same with the masterID). This function can be over overridden by
> each specific implementation.
> 
> >
> > IIRC, the SMMUv3 docs have a section that suggest how these ReqID to AXI Master ID mappings can be done.
> 
> I did not find the specific section, just that the streamID should be
> derived from requesterID.
> 
> 
> Thanks,
> 
> Diana

> >
> >
> >>> The way we did it when modelling the ZynqMP is by adding support for
> >>> transaction attribute translation in QEMU's IOMMU interface.
> >>> In our PCI RC, we have an IOMMU covering the entire AS that PCI devs DMA into.
> >>> This IOMMU doesn't do address-translation, only RequesterID -> StreamID
> >>> transforms according to how the ZynqMP PCI RC derives StreamIDs from RequesterIDs.
> >> Are there any patches for this support in order for me to better understand?
> > It's currently on the Xilinx QEMU fork on GitHub.
> > https://github.com/Xilinx/qemu/blob/master/hw/pci-host/xlnx-nwl-pcie-main.c
> >
> > In the current ZynqMP, all RequesterIDs map to a single MasterID (it's a HW limitation).
> > In future versions of the HW, another mapping will be used.
> > I can't share code for the latter yet though....
> >
> > Best regards,
> > Edgar
> >
> >  
> >
> >> Thanks,
> >>
> >> Diana
> >>
> >>
> >>> This is useful not only to model PCI RequesterID to AXI Master ID mappings but
> >>> also for modelling things like the ARM TZC (or the Xilinx ZynqMP XMPU/XPPUs).
> >>>
> >>> Cheers,
> >>> Edgar
> >>>
> >>>
> >>>> simple than reworking pci_requester_id() though.
> >>>>>  
> >>>>>      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?
> >>>>
> >>>> 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.
> >>>>
> >>>> Thanks
> >>>>
> >>>> Eric
> >>>>>      char name[64];
> >>>>>      PCIIORegion io_regions[PCI_NUM_REGIONS];
> >>>>>      AddressSpace bus_master_as;
> >>>>> @@ -737,6 +743,8 @@ static inline uint16_t pci_get_bdf(PCIDevice *dev)
> >>>>>  
> >>>>>  uint16_t pci_requester_id(PCIDevice *dev);
> >>>>>  
> >>>>> +uint32_t pci_stream_id(PCIDevice *dev);
> >>>>> +
> >>>>>  /* DMA access functions */
> >>>>>  static inline AddressSpace *pci_get_address_space(PCIDevice *dev)
> >>>>>  {
> >>>>> diff --git a/kvm-all.c b/kvm-all.c
> >>>>> index 90b8573..5a508c3 100644
> >>>>> --- a/kvm-all.c
> >>>>> +++ b/kvm-all.c
> >>>>> @@ -1280,7 +1280,7 @@ int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
> >>>>>      kroute.u.msi.data = le32_to_cpu(msg.data);
> >>>>>      if (kvm_msi_devid_required()) {
> >>>>>          kroute.flags = KVM_MSI_VALID_DEVID;
> >>>>> -        kroute.u.msi.devid = pci_requester_id(dev);
> >>>>> +        kroute.u.msi.devid = pci_stream_id(dev);
> >>>>>      }
> >>>>>      if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data, dev)) {
> >>>>>          kvm_irqchip_release_virq(s, virq);
> >>>>> @@ -1317,7 +1317,7 @@ int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg,
> >>>>>      kroute.u.msi.data = le32_to_cpu(msg.data);
> >>>>>      if (kvm_msi_devid_required()) {
> >>>>>          kroute.flags = KVM_MSI_VALID_DEVID;
> >>>>> -        kroute.u.msi.devid = pci_requester_id(dev);
> >>>>> +        kroute.u.msi.devid = pci_stream_id(dev);
> >>>>>      }
> >>>>>      if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data, dev)) {
> >>>>>          return -EINVAL;
> >>>>>
> 

Re: [Qemu-devel] [PATCH v2 2/2] Add a unique ID in the virt machine to be used as device ID

[Edgar E. Iglesias]

On Tue, Aug 22, 2017 at 10:04:25PM +0300, Michael S. Tsirkin wrote:
> On Tue, Aug 22, 2017 at 03:13:57PM +0000, Diana Madalina Craciun wrote:
> > On 08/11/2017 06:50 PM, Edgar E. Iglesias wrote:
> > > On Fri, Aug 11, 2017 at 02:35:28PM +0000, Diana Madalina Craciun wrote:
> > >> Hi Edgar,
> > >>
> > >> On 07/31/2017 06:16 PM, Edgar E. Iglesias wrote:
> > >>> On Wed, Jul 26, 2017 at 02:22:28PM +0200, 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?
> > >>>>> 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 <diana.craciun@nxp.com>
> > >>>>> ---
> > >>>>>  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(-)
> > >>>>>
> > >>>>> 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.
> > >>>>
> > >>>> 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?
> > >>>>
> > >>>>> +
> > >>>>> +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?
> > >>>> STREAM_ID_RANGE_SIZE ~ MSI_MAP_LENGTH?
> > >>>>>      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.
> > >>>>
> > >>>>> +
> > >>>>>  /* -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
> > >>> I agree.
> > >> 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).
> > >>
> > >>> I think that what we need is to add support for allowing PCI RCs
> > >>> to transform requesterIDs in transactions attributes according to the
> > >>> implementation specifics.
> > >> Do you mean that you need more than a linear offset between requesterID
> > >> and whatever other ID?
> > >
> > > Yes.
> > >
> > > This is my understanding for the ARM platforms I'm familiar with:
> > >
> > > Since AXI busses don't have a defined way to carry Master IDs, these
> > > are typically carried on the AXI user signals. I'll just refer to
> > > these signals as AXI Master IDs.
> > >
> > > 1. An endpoint issues an MSI (or any) transaction on the PCI bus.
> > >    In QEMU, these trasactions carry the requester ID in their attributes.
> > >
> > > 2. The transaction hits the PCI "host" bridge to the SoC internal
> > >    interconnect (typically AXI). This bridge needs to forward the
> > >    PCI transaction onto the AXI bus. Including mapping the PCI
> > >    RequesterID into an AXI MasterID.
> > >
> > > 3. The AXI transaction hits the IOMMU and the MasterID is mapped
> > >    into a streamID to identify the origin of the transaction
> > >    and apply address translation accordingly. If the SMMU
> > >    allows the transaction to pass, the stream ID is mapped back
> > >    into the transactions MasterID.
> > >
> > > 4. The AXI transaction continues down the interconnect and hits
> > >    the MSI doorbell and the MasterID is mapped into a DeviceID to
> > >    identify the origin of the MSI and apply possible interrupt translation.
> > >
> > > Adding streamID fields to a PCI endpoint doesn't make any sense to me.
> > > The requester ID is already there and is IMO enough.
> > > StreamIDs are a concept of ARM System MMUs, not of PCI endpoints.
> > 
> > What I have added into the endpoint is actually the Master ID (in QEMU
> > it is actually equal with the streamID). I agree that this is a property
> > of the root complex, the only reason I have put it into the endpoint was
> > to avoid traversing the PCI hierarchy each time an MSI is sent.
> 
> Can all this be folded into the IOMMU? Then you might be able to get by
> with defining an iommu function.  pci_device_iommu_address_space already
> walks the hierarchy.

Hmm, perhaps. I guess iommu_fn's would have to be able to return modified
transaction attributes. That would work, I think,

I was first thinking that if we change the IOMMU translate() method to allow
IOMMUs to modify memory attributes, Diana could register an IOMMU memory-region
with pci_setup_iommu() that modifies the attributes and returns a new attribute
with the AS that originally would have been set with pci_setup_iommu().

Adding support for IOMMU translate() to modify attributes is something we
need to do anyway IMO.


> > >
> > > When modelling #2, hardcoding a specific linear mapping between
> > > PCI requester IDs and AXI Master IDs may work for one platform
> > > but it won't work for all platforms. There is no one mapping for all.
> > > It can even be run-time programmable in the bridge.
> 
> OK but how does it work with the specific bridge that you emulate?
> There is no need to model advanced bridges with super flexible
> programmable mappings if guests do not need them to run.


Diana can answer for the details of the bridge she wants to model.
I was mostly trying to make a point that we should avoid hardcoding
a specific mapping that cannot be overriden by other host bridge models.


> 
> > 
> > One solution might be defining a function in the generic host bridge
> > which by default returns the requesterIDs (assumes that requesterID is
> > the same with the masterID). This function can be over overridden by
> > each specific implementation.
> > 
> > >
> > > IIRC, the SMMUv3 docs have a section that suggest how these ReqID to AXI Master ID mappings can be done.
> > 
> > I did not find the specific section, just that the streamID should be
> > derived from requesterID.


In my version of the spec, it's section 4.2 Stream Numbering.
At the end of that secion there are some recomendations on how to
map PCI ReqIDs into Stream IDs.

Cheers,
Edgar




> > 
> > 
> > Thanks,
> > 
> > Diana
> 
> > >
> > >
> > >>> The way we did it when modelling the ZynqMP is by adding support for
> > >>> transaction attribute translation in QEMU's IOMMU interface.
> > >>> In our PCI RC, we have an IOMMU covering the entire AS that PCI devs DMA into.
> > >>> This IOMMU doesn't do address-translation, only RequesterID -> StreamID
> > >>> transforms according to how the ZynqMP PCI RC derives StreamIDs from RequesterIDs.
> > >> Are there any patches for this support in order for me to better understand?
> > > It's currently on the Xilinx QEMU fork on GitHub.
> > > https://github.com/Xilinx/qemu/blob/master/hw/pci-host/xlnx-nwl-pcie-main.c
> > >
> > > In the current ZynqMP, all RequesterIDs map to a single MasterID (it's a HW limitation).
> > > In future versions of the HW, another mapping will be used.
> > > I can't share code for the latter yet though....
> > >
> > > Best regards,
> > > Edgar
> > >
> > >  
> > >
> > >> Thanks,
> > >>
> > >> Diana
> > >>
> > >>
> > >>> This is useful not only to model PCI RequesterID to AXI Master ID mappings but
> > >>> also for modelling things like the ARM TZC (or the Xilinx ZynqMP XMPU/XPPUs).
> > >>>
> > >>> Cheers,
> > >>> Edgar
> > >>>
> > >>>
> > >>>> simple than reworking pci_requester_id() though.
> > >>>>>  
> > >>>>>      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?
> > >>>>
> > >>>> 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.
> > >>>>
> > >>>> Thanks
> > >>>>
> > >>>> Eric
> > >>>>>      char name[64];
> > >>>>>      PCIIORegion io_regions[PCI_NUM_REGIONS];
> > >>>>>      AddressSpace bus_master_as;
> > >>>>> @@ -737,6 +743,8 @@ static inline uint16_t pci_get_bdf(PCIDevice *dev)
> > >>>>>  
> > >>>>>  uint16_t pci_requester_id(PCIDevice *dev);
> > >>>>>  
> > >>>>> +uint32_t pci_stream_id(PCIDevice *dev);
> > >>>>> +
> > >>>>>  /* DMA access functions */
> > >>>>>  static inline AddressSpace *pci_get_address_space(PCIDevice *dev)
> > >>>>>  {
> > >>>>> diff --git a/kvm-all.c b/kvm-all.c
> > >>>>> index 90b8573..5a508c3 100644
> > >>>>> --- a/kvm-all.c
> > >>>>> +++ b/kvm-all.c
> > >>>>> @@ -1280,7 +1280,7 @@ int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
> > >>>>>      kroute.u.msi.data = le32_to_cpu(msg.data);
> > >>>>>      if (kvm_msi_devid_required()) {
> > >>>>>          kroute.flags = KVM_MSI_VALID_DEVID;
> > >>>>> -        kroute.u.msi.devid = pci_requester_id(dev);
> > >>>>> +        kroute.u.msi.devid = pci_stream_id(dev);
> > >>>>>      }
> > >>>>>      if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data, dev)) {
> > >>>>>          kvm_irqchip_release_virq(s, virq);
> > >>>>> @@ -1317,7 +1317,7 @@ int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg,
> > >>>>>      kroute.u.msi.data = le32_to_cpu(msg.data);
> > >>>>>      if (kvm_msi_devid_required()) {
> > >>>>>          kroute.flags = KVM_MSI_VALID_DEVID;
> > >>>>> -        kroute.u.msi.devid = pci_requester_id(dev);
> > >>>>> +        kroute.u.msi.devid = pci_stream_id(dev);
> > >>>>>      }
> > >>>>>      if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data, dev)) {
> > >>>>>          return -EINVAL;
> > >>>>>
> > 

Re: [Qemu-devel] [PATCH v2 2/2] Add a unique ID in the virt machine to be used as device ID

[Diana Madalina Craciun]

On 08/22/2017 10:04 PM, Michael S. Tsirkin wrote:
> On Tue, Aug 22, 2017 at 03:13:57PM +0000, Diana Madalina Craciun wrote:
>> On 08/11/2017 06:50 PM, Edgar E. Iglesias wrote:
>>> On Fri, Aug 11, 2017 at 02:35:28PM +0000, Diana Madalina Craciun wrote:
>>>> Hi Edgar,
>>>>
>>>> On 07/31/2017 06:16 PM, Edgar E. Iglesias wrote:
>>>>> On Wed, Jul 26, 2017 at 02:22:28PM +0200, 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?
>>>>>>> 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 <diana.craciun@nxp.com>
>>>>>>> ---
>>>>>>>  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(-)
>>>>>>>
>>>>>>> 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.
>>>>>>
>>>>>> 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?
>>>>>>
>>>>>>> +
>>>>>>> +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?
>>>>>> STREAM_ID_RANGE_SIZE ~ MSI_MAP_LENGTH?
>>>>>>>      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.
>>>>>>
>>>>>>> +
>>>>>>>  /* -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
>>>>> I agree.
>>>> 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).
>>>>
>>>>> I think that what we need is to add support for allowing PCI RCs
>>>>> to transform requesterIDs in transactions attributes according to the
>>>>> implementation specifics.
>>>> Do you mean that you need more than a linear offset between requesterID
>>>> and whatever other ID?
>>> Yes.
>>>
>>> This is my understanding for the ARM platforms I'm familiar with:
>>>
>>> Since AXI busses don't have a defined way to carry Master IDs, these
>>> are typically carried on the AXI user signals. I'll just refer to
>>> these signals as AXI Master IDs.
>>>
>>> 1. An endpoint issues an MSI (or any) transaction on the PCI bus.
>>>    In QEMU, these trasactions carry the requester ID in their attributes.
>>>
>>> 2. The transaction hits the PCI "host" bridge to the SoC internal
>>>    interconnect (typically AXI). This bridge needs to forward the
>>>    PCI transaction onto the AXI bus. Including mapping the PCI
>>>    RequesterID into an AXI MasterID.
>>>
>>> 3. The AXI transaction hits the IOMMU and the MasterID is mapped
>>>    into a streamID to identify the origin of the transaction
>>>    and apply address translation accordingly. If the SMMU
>>>    allows the transaction to pass, the stream ID is mapped back
>>>    into the transactions MasterID.
>>>
>>> 4. The AXI transaction continues down the interconnect and hits
>>>    the MSI doorbell and the MasterID is mapped into a DeviceID to
>>>    identify the origin of the MSI and apply possible interrupt translation.
>>>
>>> Adding streamID fields to a PCI endpoint doesn't make any sense to me.
>>> The requester ID is already there and is IMO enough.
>>> StreamIDs are a concept of ARM System MMUs, not of PCI endpoints.
>> What I have added into the endpoint is actually the Master ID (in QEMU
>> it is actually equal with the streamID). I agree that this is a property
>> of the root complex, the only reason I have put it into the endpoint was
>> to avoid traversing the PCI hierarchy each time an MSI is sent.
> Can all this be folded into the IOMMU? Then you might be able to get by
> with defining an iommu function.  pci_device_iommu_address_space already
> walks the hierarchy.

I do not see how to put this into iommu. From what I understand
pci_device_iommu_address_space is used at init time, I need the device
ID each time an MSI is generated in order to provide the stream ID to
the emulated ITS.

>
>
>
>>> When modelling #2, hardcoding a specific linear mapping between
>>> PCI requester IDs and AXI Master IDs may work for one platform
>>> but it won't work for all platforms. There is no one mapping for all.
>>> It can even be run-time programmable in the bridge.
> OK but how does it work with the specific bridge that you emulate?
> There is no need to model advanced bridges with super flexible
> programmable mappings if guests do not need them to run.
>
>> One solution might be defining a function in the generic host bridge
>> which by default returns the requesterIDs (assumes that requesterID is
>> the same with the masterID). This function can be over overridden by
>> each specific implementation.
>>
>>> IIRC, the SMMUv3 docs have a section that suggest how these ReqID to AXI Master ID mappings can be done.
>> I did not find the specific section, just that the streamID should be
>> derived from requesterID.
>>
>>
>> Thanks,
>>
>> Diana
>>>
>>>>> The way we did it when modelling the ZynqMP is by adding support for
>>>>> transaction attribute translation in QEMU's IOMMU interface.
>>>>> In our PCI RC, we have an IOMMU covering the entire AS that PCI devs DMA into.
>>>>> This IOMMU doesn't do address-translation, only RequesterID -> StreamID
>>>>> transforms according to how the ZynqMP PCI RC derives StreamIDs from RequesterIDs.
>>>> Are there any patches for this support in order for me to better understand?
>>> It's currently on the Xilinx QEMU fork on GitHub.
>>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FXilinx%2Fqemu%2Fblob%2Fmaster%2Fhw%2Fpci-host%2Fxlnx-nwl-pcie-main.c&data=01%7C01%7Cdiana.craciun%40nxp.com%7C650a699248ee4247e51c08d4e9909fb5%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0&sdata=ScjctWczcmqF6s9muvPZoBaQhtMNchm9vo5LBw45R9A%3D&reserved=0
>>>
>>> In the current ZynqMP, all RequesterIDs map to a single MasterID (it's a HW limitation).
>>> In future versions of the HW, another mapping will be used.
>>> I can't share code for the latter yet though....
>>>
>>> Best regards,
>>> Edgar
>>>
>>>  
>>>
>>>> Thanks,
>>>>
>>>> Diana
>>>>
>>>>
>>>>> This is useful not only to model PCI RequesterID to AXI Master ID mappings but
>>>>> also for modelling things like the ARM TZC (or the Xilinx ZynqMP XMPU/XPPUs).
>>>>>
>>>>> Cheers,
>>>>> Edgar
>>>>>
>>>>>
>>>>>> simple than reworking pci_requester_id() though.
>>>>>>>  
>>>>>>>      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?
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> Eric
>>>>>>>      char name[64];
>>>>>>>      PCIIORegion io_regions[PCI_NUM_REGIONS];
>>>>>>>      AddressSpace bus_master_as;
>>>>>>> @@ -737,6 +743,8 @@ static inline uint16_t pci_get_bdf(PCIDevice *dev)
>>>>>>>  
>>>>>>>  uint16_t pci_requester_id(PCIDevice *dev);
>>>>>>>  
>>>>>>> +uint32_t pci_stream_id(PCIDevice *dev);
>>>>>>> +
>>>>>>>  /* DMA access functions */
>>>>>>>  static inline AddressSpace *pci_get_address_space(PCIDevice *dev)
>>>>>>>  {
>>>>>>> diff --git a/kvm-all.c b/kvm-all.c
>>>>>>> index 90b8573..5a508c3 100644
>>>>>>> --- a/kvm-all.c
>>>>>>> +++ b/kvm-all.c
>>>>>>> @@ -1280,7 +1280,7 @@ int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
>>>>>>>      kroute.u.msi.data = le32_to_cpu(msg.data);
>>>>>>>      if (kvm_msi_devid_required()) {
>>>>>>>          kroute.flags = KVM_MSI_VALID_DEVID;
>>>>>>> -        kroute.u.msi.devid = pci_requester_id(dev);
>>>>>>> +        kroute.u.msi.devid = pci_stream_id(dev);
>>>>>>>      }
>>>>>>>      if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data, dev)) {
>>>>>>>          kvm_irqchip_release_virq(s, virq);
>>>>>>> @@ -1317,7 +1317,7 @@ int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg,
>>>>>>>      kroute.u.msi.data = le32_to_cpu(msg.data);
>>>>>>>      if (kvm_msi_devid_required()) {
>>>>>>>          kroute.flags = KVM_MSI_VALID_DEVID;
>>>>>>> -        kroute.u.msi.devid = pci_requester_id(dev);
>>>>>>> +        kroute.u.msi.devid = pci_stream_id(dev);
>>>>>>>      }
>>>>>>>      if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data, dev)) {
>>>>>>>          return -EINVAL;
>>>>>>>

Re: [Qemu-devel] [PATCH v2 2/2] Add a unique ID in the virt machine to be used as device ID

[Diana Madalina Craciun]

On 08/23/2017 11:29 PM, Edgar E. Iglesias wrote:
> On Tue, Aug 22, 2017 at 10:04:25PM +0300, Michael S. Tsirkin wrote:
>> On Tue, Aug 22, 2017 at 03:13:57PM +0000, Diana Madalina Craciun wrote:
>>> On 08/11/2017 06:50 PM, Edgar E. Iglesias wrote:
>>>> On Fri, Aug 11, 2017 at 02:35:28PM +0000, Diana Madalina Craciun wrote:
>>>>> Hi Edgar,
>>>>>
>>>>> On 07/31/2017 06:16 PM, Edgar E. Iglesias wrote:
>>>>>> On Wed, Jul 26, 2017 at 02:22:28PM +0200, 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?
>>>>>>>> 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 <diana.craciun@nxp.com>
>>>>>>>> ---
>>>>>>>>  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(-)
>>>>>>>>
>>>>>>>> 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.
>>>>>>>
>>>>>>> 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?
>>>>>>>
>>>>>>>> +
>>>>>>>> +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?
>>>>>>> STREAM_ID_RANGE_SIZE ~ MSI_MAP_LENGTH?
>>>>>>>>      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.
>>>>>>>
>>>>>>>> +
>>>>>>>>  /* -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
>>>>>> I agree.
>>>>> 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).
>>>>>
>>>>>> I think that what we need is to add support for allowing PCI RCs
>>>>>> to transform requesterIDs in transactions attributes according to the
>>>>>> implementation specifics.
>>>>> Do you mean that you need more than a linear offset between requesterID
>>>>> and whatever other ID?
>>>> Yes.
>>>>
>>>> This is my understanding for the ARM platforms I'm familiar with:
>>>>
>>>> Since AXI busses don't have a defined way to carry Master IDs, these
>>>> are typically carried on the AXI user signals. I'll just refer to
>>>> these signals as AXI Master IDs.
>>>>
>>>> 1. An endpoint issues an MSI (or any) transaction on the PCI bus.
>>>>    In QEMU, these trasactions carry the requester ID in their attributes.
>>>>
>>>> 2. The transaction hits the PCI "host" bridge to the SoC internal
>>>>    interconnect (typically AXI). This bridge needs to forward the
>>>>    PCI transaction onto the AXI bus. Including mapping the PCI
>>>>    RequesterID into an AXI MasterID.
>>>>
>>>> 3. The AXI transaction hits the IOMMU and the MasterID is mapped
>>>>    into a streamID to identify the origin of the transaction
>>>>    and apply address translation accordingly. If the SMMU
>>>>    allows the transaction to pass, the stream ID is mapped back
>>>>    into the transactions MasterID.
>>>>
>>>> 4. The AXI transaction continues down the interconnect and hits
>>>>    the MSI doorbell and the MasterID is mapped into a DeviceID to
>>>>    identify the origin of the MSI and apply possible interrupt translation.
>>>>
>>>> Adding streamID fields to a PCI endpoint doesn't make any sense to me.
>>>> The requester ID is already there and is IMO enough.
>>>> StreamIDs are a concept of ARM System MMUs, not of PCI endpoints.
>>> What I have added into the endpoint is actually the Master ID (in QEMU
>>> it is actually equal with the streamID). I agree that this is a property
>>> of the root complex, the only reason I have put it into the endpoint was
>>> to avoid traversing the PCI hierarchy each time an MSI is sent.
>> Can all this be folded into the IOMMU? Then you might be able to get by
>> with defining an iommu function.  pci_device_iommu_address_space already
>> walks the hierarchy.
> Hmm, perhaps. I guess iommu_fn's would have to be able to return modified
> transaction attributes. That would work, I think,
>
> I was first thinking that if we change the IOMMU translate() method to allow
> IOMMUs to modify memory attributes, Diana could register an IOMMU memory-region
> with pci_setup_iommu() that modifies the attributes and returns a new attribute
> with the AS that originally would have been set with pci_setup_iommu().
>
> Adding support for IOMMU translate() to modify attributes is something we
> need to do anyway IMO.
>
>
>>>> When modelling #2, hardcoding a specific linear mapping between
>>>> PCI requester IDs and AXI Master IDs may work for one platform
>>>> but it won't work for all platforms. There is no one mapping for all.
>>>> It can even be run-time programmable in the bridge.
>> OK but how does it work with the specific bridge that you emulate?
>> There is no need to model advanced bridges with super flexible
>> programmable mappings if guests do not need them to run.
>
> Diana can answer for the details of the bridge she wants to model.
> I was mostly trying to make a point that we should avoid hardcoding
> a specific mapping that cannot be overriden by other host bridge models.

I am using the generic host bridge from the virt machine, so nothing
special. The problem I am trying to solve is having non-PCI devices
which are using MSIs as well. So they need also a translation from
whatever ID they are having to device ID. And the translated device ID
must not be in the same range with the PCI ones.


>
>>> One solution might be defining a function in the generic host bridge
>>> which by default returns the requesterIDs (assumes that requesterID is
>>> the same with the masterID). This function can be over overridden by
>>> each specific implementation.
>>>
>>>> IIRC, the SMMUv3 docs have a section that suggest how these ReqID to AXI Master ID mappings can be done.
>>> I did not find the specific section, just that the streamID should be
>>> derived from requesterID.
>
> In my version of the spec, it's section 4.2 Stream Numbering.
> At the end of that secion there are some recomendations on how to
> map PCI ReqIDs into Stream IDs.

OK, I found that section. In that section ARM recommends the streamID to
be generated from PCI requester ID so that StreamID[15:0] ==
RequesterID[15:0]. It also mentions that in the case of more than one
root complex to extend the number of streamID bits to differentiate
between them (e.g. bits [N:16]).

Almost the same idea (a linear mapping) is presented in the SBSA spec:
StreamID = RequesterID[15:0] + 0x10000 * Constant

My proposal is an implementation of the SBSA version.

In this email thread it has been suggested to use Constant=0 for PCI and
leave the PCI code unmodified. It can be done this way, but this will
not work in case of having multiple root complexes because we should
also reserve some bits [N:16] to differentiate between the root
complexes, but in the same time not to overlap with the platform devices
pool. We can agree upon a number of bits reserved for PCIe segment and
used the remaining ones for other platforms.


> Cheers,
> Edgar
>
>
>
>
>>>
>>> Thanks,
>>>
>>> Diana
>>>>
>>>>>> The way we did it when modelling the ZynqMP is by adding support for
>>>>>> transaction attribute translation in QEMU's IOMMU interface.
>>>>>> In our PCI RC, we have an IOMMU covering the entire AS that PCI devs DMA into.
>>>>>> This IOMMU doesn't do address-translation, only RequesterID -> StreamID
>>>>>> transforms according to how the ZynqMP PCI RC derives StreamIDs from RequesterIDs.
>>>>> Are there any patches for this support in order for me to better understand?
>>>> It's currently on the Xilinx QEMU fork on GitHub.
>>>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FXilinx%2Fqemu%2Fblob%2Fmaster%2Fhw%2Fpci-host%2Fxlnx-nwl-pcie-main.c&data=01%7C01%7Cdiana.craciun%40nxp.com%7C8db54b1ee05143ceefbf08d4ea65a585%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0&sdata=h%2F4ZlhJbYkL0dRbxv7MykGuwlRTmV64GsK%2FhH7woBtE%3D&reserved=0
>>>>
>>>> In the current ZynqMP, all RequesterIDs map to a single MasterID (it's a HW limitation).
>>>> In future versions of the HW, another mapping will be used.
>>>> I can't share code for the latter yet though....
>>>>
>>>> Best regards,
>>>> Edgar
>>>>
>>>>  
>>>>
>>>>> Thanks,
>>>>>
>>>>> Diana
>>>>>
>>>>>
>>>>>> This is useful not only to model PCI RequesterID to AXI Master ID mappings but
>>>>>> also for modelling things like the ARM TZC (or the Xilinx ZynqMP XMPU/XPPUs).
>>>>>>
>>>>>> Cheers,
>>>>>> Edgar
>>>>>>
>>>>>>
>>>>>>> simple than reworking pci_requester_id() though.
>>>>>>>>  
>>>>>>>>      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?
>>>>>>>
>>>>>>> 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.
>>>>>>>
>>>>>>> Thanks
>>>>>>>
>>>>>>> Eric
>>>>>>>>      char name[64];
>>>>>>>>      PCIIORegion io_regions[PCI_NUM_REGIONS];
>>>>>>>>      AddressSpace bus_master_as;
>>>>>>>> @@ -737,6 +743,8 @@ static inline uint16_t pci_get_bdf(PCIDevice *dev)
>>>>>>>>  
>>>>>>>>  uint16_t pci_requester_id(PCIDevice *dev);
>>>>>>>>  
>>>>>>>> +uint32_t pci_stream_id(PCIDevice *dev);
>>>>>>>> +
>>>>>>>>  /* DMA access functions */
>>>>>>>>  static inline AddressSpace *pci_get_address_space(PCIDevice *dev)
>>>>>>>>  {
>>>>>>>> diff --git a/kvm-all.c b/kvm-all.c
>>>>>>>> index 90b8573..5a508c3 100644
>>>>>>>> --- a/kvm-all.c
>>>>>>>> +++ b/kvm-all.c
>>>>>>>> @@ -1280,7 +1280,7 @@ int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
>>>>>>>>      kroute.u.msi.data = le32_to_cpu(msg.data);
>>>>>>>>      if (kvm_msi_devid_required()) {
>>>>>>>>          kroute.flags = KVM_MSI_VALID_DEVID;
>>>>>>>> -        kroute.u.msi.devid = pci_requester_id(dev);
>>>>>>>> +        kroute.u.msi.devid = pci_stream_id(dev);
>>>>>>>>      }
>>>>>>>>      if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data, dev)) {
>>>>>>>>          kvm_irqchip_release_virq(s, virq);
>>>>>>>> @@ -1317,7 +1317,7 @@ int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg,
>>>>>>>>      kroute.u.msi.data = le32_to_cpu(msg.data);
>>>>>>>>      if (kvm_msi_devid_required()) {
>>>>>>>>          kroute.flags = KVM_MSI_VALID_DEVID;
>>>>>>>> -        kroute.u.msi.devid = pci_requester_id(dev);
>>>>>>>> +        kroute.u.msi.devid = pci_stream_id(dev);
>>>>>>>>      }
>>>>>>>>      if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data, dev)) {
>>>>>>>>          return -EINVAL;
>>>>>>>>

Re: [Qemu-devel] [PATCH v2 2/2] Add a unique ID in the virt machine to be used as device ID

[Michael S. Tsirkin]

On Fri, Sep 01, 2017 at 02:32:59PM +0000, Diana Madalina Craciun wrote:
> On 08/23/2017 11:29 PM, Edgar E. Iglesias wrote:
> > On Tue, Aug 22, 2017 at 10:04:25PM +0300, Michael S. Tsirkin wrote:
> >> On Tue, Aug 22, 2017 at 03:13:57PM +0000, Diana Madalina Craciun wrote:
> >>> On 08/11/2017 06:50 PM, Edgar E. Iglesias wrote:
> >>>> On Fri, Aug 11, 2017 at 02:35:28PM +0000, Diana Madalina Craciun wrote:
> >>>>> Hi Edgar,
> >>>>>
> >>>>> On 07/31/2017 06:16 PM, Edgar E. Iglesias wrote:
> >>>>>> On Wed, Jul 26, 2017 at 02:22:28PM +0200, 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?
> >>>>>>>> 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 <diana.craciun@nxp.com>
> >>>>>>>> ---
> >>>>>>>>  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(-)
> >>>>>>>>
> >>>>>>>> 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.
> >>>>>>>
> >>>>>>> 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?
> >>>>>>>
> >>>>>>>> +
> >>>>>>>> +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?
> >>>>>>> STREAM_ID_RANGE_SIZE ~ MSI_MAP_LENGTH?
> >>>>>>>>      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.
> >>>>>>>
> >>>>>>>> +
> >>>>>>>>  /* -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
> >>>>>> I agree.
> >>>>> 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).
> >>>>>
> >>>>>> I think that what we need is to add support for allowing PCI RCs
> >>>>>> to transform requesterIDs in transactions attributes according to the
> >>>>>> implementation specifics.
> >>>>> Do you mean that you need more than a linear offset between requesterID
> >>>>> and whatever other ID?
> >>>> Yes.
> >>>>
> >>>> This is my understanding for the ARM platforms I'm familiar with:
> >>>>
> >>>> Since AXI busses don't have a defined way to carry Master IDs, these
> >>>> are typically carried on the AXI user signals. I'll just refer to
> >>>> these signals as AXI Master IDs.
> >>>>
> >>>> 1. An endpoint issues an MSI (or any) transaction on the PCI bus.
> >>>>    In QEMU, these trasactions carry the requester ID in their attributes.
> >>>>
> >>>> 2. The transaction hits the PCI "host" bridge to the SoC internal
> >>>>    interconnect (typically AXI). This bridge needs to forward the
> >>>>    PCI transaction onto the AXI bus. Including mapping the PCI
> >>>>    RequesterID into an AXI MasterID.
> >>>>
> >>>> 3. The AXI transaction hits the IOMMU and the MasterID is mapped
> >>>>    into a streamID to identify the origin of the transaction
> >>>>    and apply address translation accordingly. If the SMMU
> >>>>    allows the transaction to pass, the stream ID is mapped back
> >>>>    into the transactions MasterID.
> >>>>
> >>>> 4. The AXI transaction continues down the interconnect and hits
> >>>>    the MSI doorbell and the MasterID is mapped into a DeviceID to
> >>>>    identify the origin of the MSI and apply possible interrupt translation.
> >>>>
> >>>> Adding streamID fields to a PCI endpoint doesn't make any sense to me.
> >>>> The requester ID is already there and is IMO enough.
> >>>> StreamIDs are a concept of ARM System MMUs, not of PCI endpoints.
> >>> What I have added into the endpoint is actually the Master ID (in QEMU
> >>> it is actually equal with the streamID). I agree that this is a property
> >>> of the root complex, the only reason I have put it into the endpoint was
> >>> to avoid traversing the PCI hierarchy each time an MSI is sent.
> >> Can all this be folded into the IOMMU? Then you might be able to get by
> >> with defining an iommu function.  pci_device_iommu_address_space already
> >> walks the hierarchy.
> > Hmm, perhaps. I guess iommu_fn's would have to be able to return modified
> > transaction attributes. That would work, I think,
> >
> > I was first thinking that if we change the IOMMU translate() method to allow
> > IOMMUs to modify memory attributes, Diana could register an IOMMU memory-region
> > with pci_setup_iommu() that modifies the attributes and returns a new attribute
> > with the AS that originally would have been set with pci_setup_iommu().
> >
> > Adding support for IOMMU translate() to modify attributes is something we
> > need to do anyway IMO.
> >
> >
> >>>> When modelling #2, hardcoding a specific linear mapping between
> >>>> PCI requester IDs and AXI Master IDs may work for one platform
> >>>> but it won't work for all platforms. There is no one mapping for all.
> >>>> It can even be run-time programmable in the bridge.
> >> OK but how does it work with the specific bridge that you emulate?
> >> There is no need to model advanced bridges with super flexible
> >> programmable mappings if guests do not need them to run.
> >
> > Diana can answer for the details of the bridge she wants to model.
> > I was mostly trying to make a point that we should avoid hardcoding
> > a specific mapping that cannot be overriden by other host bridge models.
> 
> I am using the generic host bridge from the virt machine, so nothing
> special. The problem I am trying to solve is having non-PCI devices
> which are using MSIs as well. So they need also a translation from
> whatever ID they are having to device ID. And the translated device ID
> must not be in the same range with the PCI ones.
> 
> 
> >
> >>> One solution might be defining a function in the generic host bridge
> >>> which by default returns the requesterIDs (assumes that requesterID is
> >>> the same with the masterID). This function can be over overridden by
> >>> each specific implementation.
> >>>
> >>>> IIRC, the SMMUv3 docs have a section that suggest how these ReqID to AXI Master ID mappings can be done.
> >>> I did not find the specific section, just that the streamID should be
> >>> derived from requesterID.
> >
> > In my version of the spec, it's section 4.2 Stream Numbering.
> > At the end of that secion there are some recomendations on how to
> > map PCI ReqIDs into Stream IDs.
> 
> OK, I found that section. In that section ARM recommends the streamID to
> be generated from PCI requester ID so that StreamID[15:0] ==
> RequesterID[15:0]. It also mentions that in the case of more than one
> root complex to extend the number of streamID bits to differentiate
> between them (e.g. bits [N:16]).
> 
> Almost the same idea (a linear mapping) is presented in the SBSA spec:
> StreamID = RequesterID[15:0] + 0x10000 * Constant
> 
> My proposal is an implementation of the SBSA version.
> 
> In this email thread it has been suggested to use Constant=0 for PCI and
> leave the PCI code unmodified. It can be done this way, but this will
> not work in case of having multiple root complexes because we should
> also reserve some bits [N:16] to differentiate between the root
> complexes, but in the same time not to overlap with the platform devices
> pool.

Once we have multi-root properly on ARM/x86 yet, when we do specifying
the segment and then caching it on the the device might make sense.

For now just assume segment # is 0, let's not over-engineer.

> We can agree upon a number of bits reserved for PCIe segment and
> used the remaining ones for other platforms.

I'd set all high bits to 1 for non-PCI MSI as a first step.  When there
is more than one kind of non-PCI MSI we'll come up with another mapping.

> 
> > Cheers,
> > Edgar
> >
> >
> >
> >
> >>>
> >>> Thanks,
> >>>
> >>> Diana
> >>>>
> >>>>>> The way we did it when modelling the ZynqMP is by adding support for
> >>>>>> transaction attribute translation in QEMU's IOMMU interface.
> >>>>>> In our PCI RC, we have an IOMMU covering the entire AS that PCI devs DMA into.
> >>>>>> This IOMMU doesn't do address-translation, only RequesterID -> StreamID
> >>>>>> transforms according to how the ZynqMP PCI RC derives StreamIDs from RequesterIDs.
> >>>>> Are there any patches for this support in order for me to better understand?
> >>>> It's currently on the Xilinx QEMU fork on GitHub.
> >>>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FXilinx%2Fqemu%2Fblob%2Fmaster%2Fhw%2Fpci-host%2Fxlnx-nwl-pcie-main.c&data=01%7C01%7Cdiana.craciun%40nxp.com%7C8db54b1ee05143ceefbf08d4ea65a585%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0&sdata=h%2F4ZlhJbYkL0dRbxv7MykGuwlRTmV64GsK%2FhH7woBtE%3D&reserved=0
> >>>>
> >>>> In the current ZynqMP, all RequesterIDs map to a single MasterID (it's a HW limitation).
> >>>> In future versions of the HW, another mapping will be used.
> >>>> I can't share code for the latter yet though....
> >>>>
> >>>> Best regards,
> >>>> Edgar
> >>>>
> >>>>  
> >>>>
> >>>>> Thanks,
> >>>>>
> >>>>> Diana
> >>>>>
> >>>>>
> >>>>>> This is useful not only to model PCI RequesterID to AXI Master ID mappings but
> >>>>>> also for modelling things like the ARM TZC (or the Xilinx ZynqMP XMPU/XPPUs).
> >>>>>>
> >>>>>> Cheers,
> >>>>>> Edgar
> >>>>>>
> >>>>>>
> >>>>>>> simple than reworking pci_requester_id() though.
> >>>>>>>>  
> >>>>>>>>      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?
> >>>>>>>
> >>>>>>> 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.
> >>>>>>>
> >>>>>>> Thanks
> >>>>>>>
> >>>>>>> Eric
> >>>>>>>>      char name[64];
> >>>>>>>>      PCIIORegion io_regions[PCI_NUM_REGIONS];
> >>>>>>>>      AddressSpace bus_master_as;
> >>>>>>>> @@ -737,6 +743,8 @@ static inline uint16_t pci_get_bdf(PCIDevice *dev)
> >>>>>>>>  
> >>>>>>>>  uint16_t pci_requester_id(PCIDevice *dev);
> >>>>>>>>  
> >>>>>>>> +uint32_t pci_stream_id(PCIDevice *dev);
> >>>>>>>> +
> >>>>>>>>  /* DMA access functions */
> >>>>>>>>  static inline AddressSpace *pci_get_address_space(PCIDevice *dev)
> >>>>>>>>  {
> >>>>>>>> diff --git a/kvm-all.c b/kvm-all.c
> >>>>>>>> index 90b8573..5a508c3 100644
> >>>>>>>> --- a/kvm-all.c
> >>>>>>>> +++ b/kvm-all.c
> >>>>>>>> @@ -1280,7 +1280,7 @@ int kvm_irqchip_add_msi_route(KVMState *s, int vector, PCIDevice *dev)
> >>>>>>>>      kroute.u.msi.data = le32_to_cpu(msg.data);
> >>>>>>>>      if (kvm_msi_devid_required()) {
> >>>>>>>>          kroute.flags = KVM_MSI_VALID_DEVID;
> >>>>>>>> -        kroute.u.msi.devid = pci_requester_id(dev);
> >>>>>>>> +        kroute.u.msi.devid = pci_stream_id(dev);
> >>>>>>>>      }
> >>>>>>>>      if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data, dev)) {
> >>>>>>>>          kvm_irqchip_release_virq(s, virq);
> >>>>>>>> @@ -1317,7 +1317,7 @@ int kvm_irqchip_update_msi_route(KVMState *s, int virq, MSIMessage msg,
> >>>>>>>>      kroute.u.msi.data = le32_to_cpu(msg.data);
> >>>>>>>>      if (kvm_msi_devid_required()) {
> >>>>>>>>          kroute.flags = KVM_MSI_VALID_DEVID;
> >>>>>>>> -        kroute.u.msi.devid = pci_requester_id(dev);
> >>>>>>>> +        kroute.u.msi.devid = pci_stream_id(dev);
> >>>>>>>>      }
> >>>>>>>>      if (kvm_arch_fixup_msi_route(&kroute, msg.address, msg.data, dev)) {
> >>>>>>>>          return -EINVAL;
> >>>>>>>>
>