[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 2/2] Add a unique ID in the virt machine to b
From: |
Edgar E. Iglesias |
Subject: |
Re: [Qemu-devel] [PATCH v2 2/2] Add a unique ID in the virt machine to be used as device ID |
Date: |
Mon, 31 Jul 2017 17:39:54 +0200 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
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