| Message ID | 1495537965-4187-2-git-send-email-diana.craciun@nxp.com |
|---|---|
| State | New |
| Headers | show |
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); > }; >
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
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
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); };
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(-)