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