Re: [Qemu-arm] [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 <address@hidden>
>>>>>>> ---
>>>>>>>  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;
>>>>>>>