| Message ID | 20180521140402.23318-15-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iommu: support txattrs, support TCG execution, implement TZ MPC | expand |
On Mon, May 21, 2018 at 03:03:49PM +0100, Peter Maydell wrote: > If an IOMMU supports mappings that care about the memory > transaction attributes, then it no longer has a unique > address -> output mapping, but more than one. We can > represent these using an IOMMU index, analogous to TCG's > mmu indexes. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > include/exec/memory.h | 52 +++++++++++++++++++++++++++++++++++++++++++ > memory.c | 23 +++++++++++++++++++ > 2 files changed, 75 insertions(+) > > diff --git a/include/exec/memory.h b/include/exec/memory.h > index 309fdfb89b..f6226fb263 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -206,6 +206,20 @@ enum IOMMUMemoryRegionAttr { > * to report whenever mappings are changed, by calling > * memory_region_notify_iommu() (or, if necessary, by calling > * memory_region_notify_one() for each registered notifier). > + * > + * Conceptually an IOMMU provides a mapping from input address > + * to an output TLB entry. If the IOMMU is aware of memory transaction > + * attributes and the output TLB entry depends on the transaction > + * attributes, we represent this using IOMMU indexes. Each index Hi, Peter, In what case will an IOMMU translation depend on translation attributes? It seems to me that we should always pass in the translation attributes into the translate() function. The translate() function can omit that parameter if the specific IOMMU does not need that information, but still I am confused about why we need to index IOMMU by translation attributes. > + * selects a particular translation table that the IOMMU has: > + * @attrs_to_index returns the IOMMU index for a set of transaction attributes > + * @translate takes an input address and an IOMMU index > + * and the mapping returned can only depend on the input address and the > + * IOMMU index. > + * > + * Most IOMMUs don't care about the transaction attributes and support > + * only a single IOMMU index. A more complex IOMMU might have one index > + * for secure transactions and one for non-secure transactions. > */ > typedef struct IOMMUMemoryRegionClass { > /* private */ > @@ -290,6 +304,26 @@ typedef struct IOMMUMemoryRegionClass { > */ > int (*get_attr)(IOMMUMemoryRegion *iommu, enum IOMMUMemoryRegionAttr attr, > void *data); > + > + /* Return the IOMMU index to use for a given set of transaction attributes. > + * > + * Optional method: if an IOMMU only supports a single IOMMU index then > + * the default implementation of memory_region_iommu_attrs_to_index() > + * will return 0. > + * > + * The indexes supported by an IOMMU must be contiguous, starting at 0. > + * > + * @iommu: the IOMMUMemoryRegion > + * @attrs: memory transaction attributes > + */ > + int (*attrs_to_index)(IOMMUMemoryRegion *iommu, MemTxAttrs attrs); > + > + /* Return the number of IOMMU indexes this IOMMU supports. > + * > + * Optional method: if this method is not provided, then > + * memory_region_iommu_num_indexes() will return 1, indicating that > + * only a single IOMMU index is supported. > + */ The num_indexes() definition is missing, and I saw that in the next patch. We'll possibly want to move it here. Regards, -- Peter Xu
On 22 May 2018 at 04:03, Peter Xu <peterx@redhat.com> wrote: > On Mon, May 21, 2018 at 03:03:49PM +0100, Peter Maydell wrote: >> If an IOMMU supports mappings that care about the memory >> transaction attributes, then it no longer has a unique >> address -> output mapping, but more than one. We can >> represent these using an IOMMU index, analogous to TCG's >> mmu indexes. >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> --- >> include/exec/memory.h | 52 +++++++++++++++++++++++++++++++++++++++++++ >> memory.c | 23 +++++++++++++++++++ >> 2 files changed, 75 insertions(+) >> >> diff --git a/include/exec/memory.h b/include/exec/memory.h >> index 309fdfb89b..f6226fb263 100644 >> --- a/include/exec/memory.h >> +++ b/include/exec/memory.h >> @@ -206,6 +206,20 @@ enum IOMMUMemoryRegionAttr { >> * to report whenever mappings are changed, by calling >> * memory_region_notify_iommu() (or, if necessary, by calling >> * memory_region_notify_one() for each registered notifier). >> + * >> + * Conceptually an IOMMU provides a mapping from input address >> + * to an output TLB entry. If the IOMMU is aware of memory transaction >> + * attributes and the output TLB entry depends on the transaction >> + * attributes, we represent this using IOMMU indexes. Each index > > Hi, Peter, > > In what case will an IOMMU translation depend on translation > attributes? It seems to me that we should always pass in the > translation attributes into the translate() function. The translate() > function can omit that parameter if the specific IOMMU does not need > that information, but still I am confused about why we need to index > IOMMU by translation attributes. The MPC implementation at the tail end of the patchset is one example -- it needs to look at attrs.secure, because "translation for secure access to address X" differs from that for address Y". The Arm SMMUv3 is the same when it supports TrustZone (the implementation in-tree does not), and it can also give different permissions for transactions with attrs.user = 0 vs 1. The reason for not just passing in the transaction attributes to translate is that (a) the iommu index abstraction makes the notifier setup simpler: rather than having to have some indication in the API of which of the transaction attributes are important and which the notifier cares about, we can just use indexs (b) it means that it's harder to write an iommu with the bug that it looks at parts of the transaction attributes that it didn't claim were important in the notifier API > The num_indexes() definition is missing, and I saw that in the next > patch. We'll possibly want to move it here. Huh. I ran into that and thought I'd fixed it in my local tree -- I must have failed to actually move the change to the right patch. Sorry about that. thanks -- PMM
On Tue, May 22, 2018 at 09:40:44AM +0100, Peter Maydell wrote: > On 22 May 2018 at 04:03, Peter Xu <peterx@redhat.com> wrote: > > On Mon, May 21, 2018 at 03:03:49PM +0100, Peter Maydell wrote: > >> If an IOMMU supports mappings that care about the memory > >> transaction attributes, then it no longer has a unique > >> address -> output mapping, but more than one. We can > >> represent these using an IOMMU index, analogous to TCG's > >> mmu indexes. > >> > >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > >> --- > >> include/exec/memory.h | 52 +++++++++++++++++++++++++++++++++++++++++++ > >> memory.c | 23 +++++++++++++++++++ > >> 2 files changed, 75 insertions(+) > >> > >> diff --git a/include/exec/memory.h b/include/exec/memory.h > >> index 309fdfb89b..f6226fb263 100644 > >> --- a/include/exec/memory.h > >> +++ b/include/exec/memory.h > >> @@ -206,6 +206,20 @@ enum IOMMUMemoryRegionAttr { > >> * to report whenever mappings are changed, by calling > >> * memory_region_notify_iommu() (or, if necessary, by calling > >> * memory_region_notify_one() for each registered notifier). > >> + * > >> + * Conceptually an IOMMU provides a mapping from input address > >> + * to an output TLB entry. If the IOMMU is aware of memory transaction > >> + * attributes and the output TLB entry depends on the transaction > >> + * attributes, we represent this using IOMMU indexes. Each index > > > > Hi, Peter, > > > > In what case will an IOMMU translation depend on translation > > attributes? It seems to me that we should always pass in the > > translation attributes into the translate() function. The translate() > > function can omit that parameter if the specific IOMMU does not need > > that information, but still I am confused about why we need to index > > IOMMU by translation attributes. > > The MPC implementation at the tail end of the patchset is > one example -- it needs to look at attrs.secure, because > "translation for secure access to address X" differs from > that for address Y". The Arm SMMUv3 is the same when it supports > TrustZone (the implementation in-tree does not), and it can also > give different permissions for transactions with attrs.user = 0 vs 1. Thanks for providing more context. > > The reason for not just passing in the transaction attributes to > translate is that > (a) the iommu index abstraction makes the notifier setup simpler: > rather than having to have some indication in the API of which > of the transaction attributes are important and which the notifier > cares about, we can just use indexs Hmm, so here IIUC we'll have a new IOMMU notifier that will only listen to part of the IOMMU notifies, e.g., when attrs.secure=true. Yes I think adding something into IOMMUNotifier might work, but just to mention that in IOMMUTLBEntry we have IOMMUTLBEntry.target_as defined. Until now it's hardly used at least on x86 platform since all of the translations on x86 are targeted to the system RAM. However it seems to be quite tailored in this case since it seems to me that different attrs.secure value for translations should be based on different address spaces too. Then in the IOMMU notifiers that would care about MemTxAttrs, could it be possible to identify that by check against the IOMMUTLBEntry.target_as? > (b) it means that it's harder to write an iommu with the bug that > it looks at parts of the transaction attributes that it didn't > claim were important in the notifier API It is just confusing to me when I looked at current translate() interface (I copied it from some other patch of the series): @@ -252,9 +252,10 @@ typedef struct IOMMUMemoryRegionClass { * @iommu: the IOMMUMemoryRegion * @hwaddr: address to be translated within the memory region * @flag: requested access permissions + * @iommu_idx: IOMMU index for the translation */ IOMMUTLBEntry (*translate)(IOMMUMemoryRegion *iommu, hwaddr addr, - IOMMUAccessFlags flag); + IOMMUAccessFlags flag, int iommu_idx); The "iommu_idx" parameter is really hard to understand here at the first glance. Now I think I understand that it is somehow related to the MemTxAttrs, but still it will take time to understand. And if we see current implementation for this (still, I copied code from other patch in the series to here to ease discussion): @@ -498,8 +498,15 @@ static MemoryRegionSection address_space_translate_iommu(IOMMUMemoryRegion *iomm do { hwaddr addr = *xlat; IOMMUMemoryRegionClass *imrc = memory_region_get_iommu_class_nocheck(iommu_mr); - IOMMUTLBEntry iotlb = imrc->translate(iommu_mr, addr, is_write ? - IOMMU_WO : IOMMU_RO); + int iommu_idx = 0; + IOMMUTLBEntry iotlb; + + if (imrc->attrs_to_index) { + iommu_idx = imrc->attrs_to_index(iommu_mr, attrs); + } + + iotlb = imrc->translate(iommu_mr, addr, is_write ? + IOMMU_WO : IOMMU_RO, iommu_idx); Here what if we pass attrs directly into imrc->translate() and just call imrc->attrs_to_index() inside the arch-dependent translate() function? Will that work too? I had a quick glance at the series, I have no thorough idea on the whole stuff, but I'd say some of the patches are exactly what I wanted if to support MemTxAttrs in VT-d emulation one day (now DMAR of VT-d is bypassing MemTxAttrs and IMHO that's incorrect). If we can somehow pass in the MemTxAttrs then that'll be perfect and I can continue to work on that. If we pass in iommu_idx now instead, it would take some time for me to figure out how to further achieve the same goal for VT-d in the future, e.g., I would still want to pass in MemTxAttrs, but that's obviously duplicated with iommu_idx. (Also CCing David and Alex) Thanks, -- Peter Xu
On 22 May 2018 at 12:02, Peter Xu <peterx@redhat.com> wrote: > On Tue, May 22, 2018 at 09:40:44AM +0100, Peter Maydell wrote: >> On 22 May 2018 at 04:03, Peter Xu <peterx@redhat.com> wrote: >> The reason for not just passing in the transaction attributes to >> translate is that >> (a) the iommu index abstraction makes the notifier setup simpler: >> rather than having to have some indication in the API of which >> of the transaction attributes are important and which the notifier >> cares about, we can just use indexs > > Hmm, so here IIUC we'll have a new IOMMU notifier that will only > listen to part of the IOMMU notifies, e.g., when attrs.secure=true. > Yes I think adding something into IOMMUNotifier might work, but just > to mention that in IOMMUTLBEntry we have IOMMUTLBEntry.target_as > defined. Until now it's hardly used at least on x86 platform since > all of the translations on x86 are targeted to the system RAM. > However it seems to be quite tailored in this case since it seems to > me that different attrs.secure value for translations should be based > on different address spaces too. Then in the IOMMU notifiers that > would care about MemTxAttrs, could it be possible to identify that by > check against the IOMMUTLBEntry.target_as? No, because you can have a single address space that receives transactions with various attributes. (Again, the MPC is an example of this.) >> (b) it means that it's harder to write an iommu with the bug that >> it looks at parts of the transaction attributes that it didn't >> claim were important in the notifier API > > It is just confusing to me when I looked at current translate() > interface (I copied it from some other patch of the series): > > @@ -252,9 +252,10 @@ typedef struct IOMMUMemoryRegionClass { > * @iommu: the IOMMUMemoryRegion > * @hwaddr: address to be translated within the memory region > * @flag: requested access permissions > + * @iommu_idx: IOMMU index for the translation > */ > IOMMUTLBEntry (*translate)(IOMMUMemoryRegion *iommu, hwaddr addr, > - IOMMUAccessFlags flag); > + IOMMUAccessFlags flag, int iommu_idx); > > The "iommu_idx" parameter is really hard to understand here at the > first glance. Now I think I understand that it is somehow related to > the MemTxAttrs, but still it will take time to understand. The part of the documentation where I try to explain the general idea is in this patch, in the comment at the top of the struct: + * Conceptually an IOMMU provides a mapping from input address + * to an output TLB entry. If the IOMMU is aware of memory transaction + * attributes and the output TLB entry depends on the transaction + * attributes, we represent this using IOMMU indexes. Each index + * selects a particular translation table that the IOMMU has: + * @attrs_to_index returns the IOMMU index for a set of transaction attributes + * @translate takes an input address and an IOMMU index + * and the mapping returned can only depend on the input address and the + * IOMMU index. + * + * Most IOMMUs don't care about the transaction attributes and support + * only a single IOMMU index. A more complex IOMMU might have one index + * for secure transactions and one for non-secure transactions. Do you have suggestions for how to improve on this? > And if we see current implementation for this (still, I copied code > from other patch in the series to here to ease discussion): > > @@ -498,8 +498,15 @@ static MemoryRegionSection address_space_translate_iommu(IOMMUMemoryRegion *iomm > do { > hwaddr addr = *xlat; > IOMMUMemoryRegionClass *imrc = memory_region_get_iommu_class_nocheck(iommu_mr); > - IOMMUTLBEntry iotlb = imrc->translate(iommu_mr, addr, is_write ? > - IOMMU_WO : IOMMU_RO); > + int iommu_idx = 0; > + IOMMUTLBEntry iotlb; > + > + if (imrc->attrs_to_index) { > + iommu_idx = imrc->attrs_to_index(iommu_mr, attrs); > + } > + > + iotlb = imrc->translate(iommu_mr, addr, is_write ? > + IOMMU_WO : IOMMU_RO, iommu_idx); > > Here what if we pass attrs directly into imrc->translate() and just > call imrc->attrs_to_index() inside the arch-dependent translate() > function? Will that work too? You don't always have the attributes at the point where you want to call translate. (For instance, memory_region_notify_iommu() doesn't have attributes.) I started off with "pass the tx attrs into the translate method", which is fine for the code flows which are actually doing memory transactions, but breaks down when you try to incorporate notifiers. > I had a quick glance at the series, I have no thorough idea on the > whole stuff, but I'd say some of the patches are exactly what I wanted > if to support MemTxAttrs in VT-d emulation one day (now DMAR of VT-d > is bypassing MemTxAttrs and IMHO that's incorrect). If we can somehow > pass in the MemTxAttrs then that'll be perfect and I can continue to > work on that. If we pass in iommu_idx now instead, it would take some > time for me to figure out how to further achieve the same goal for > VT-d in the future, e.g., I would still want to pass in MemTxAttrs, > but that's obviously duplicated with iommu_idx. The idea is that you should never need to pass in the MemTxAttrs, because everything that the IOMMU might care about in the tx attrs must be encoded into the iommu index. (The point where the IOMMU gets to do that encoding is in its attrs_to_index() method.) thanks -- PMM
Hi Peter, On 05/21/2018 04:03 PM, Peter Maydell wrote: > If an IOMMU supports mappings that care about the memory > transaction attributes, then it no longer has a unique > address -> output mapping, but more than one. We can > represent these using an IOMMU index, analogous to TCG's > mmu indexes. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > include/exec/memory.h | 52 +++++++++++++++++++++++++++++++++++++++++++ > memory.c | 23 +++++++++++++++++++ > 2 files changed, 75 insertions(+) > > diff --git a/include/exec/memory.h b/include/exec/memory.h > index 309fdfb89b..f6226fb263 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -206,6 +206,20 @@ enum IOMMUMemoryRegionAttr { > * to report whenever mappings are changed, by calling > * memory_region_notify_iommu() (or, if necessary, by calling > * memory_region_notify_one() for each registered notifier). > + * > + * Conceptually an IOMMU provides a mapping from input address > + * to an output TLB entry. actually it takes a source identifier too (ARM streamid + substreamID, this latter is not yet supported). At the moment we have 1 IOMMUMRRegion per sid on ARM. For each IOMMUMRRegion we would now have 2 indexes (1 for secure and 1 for unsecure). How do you see the implementation of PASIDs (substreamids). Would that be based on indexes or not? If the IOMMU is aware of memory transaction > + * attributes and the output TLB entry depends on the transaction > + * attributes, we represent this using IOMMU indexes. Each index > + * selects a particular translation table that the IOMMU has: > + * @attrs_to_index returns the IOMMU index for a set of transaction attributes > + * @translate takes an input address and an IOMMU index > + * and the mapping returned can only depend on the input address and the > + * IOMMU index. > + * > + * Most IOMMUs don't care about the transaction attributes and support > + * only a single IOMMU index. A more complex IOMMU might have one index > + * for secure transactions and one for non-secure transactions. > */ > typedef struct IOMMUMemoryRegionClass { > /* private */ > @@ -290,6 +304,26 @@ typedef struct IOMMUMemoryRegionClass { > */ > int (*get_attr)(IOMMUMemoryRegion *iommu, enum IOMMUMemoryRegionAttr attr, > void *data); > + > + /* Return the IOMMU index to use for a given set of transaction attributes. > + * > + * Optional method: if an IOMMU only supports a single IOMMU index then > + * the default implementation of memory_region_iommu_attrs_to_index() > + * will return 0. > + * > + * The indexes supported by an IOMMU must be contiguous, starting at 0. > + * > + * @iommu: the IOMMUMemoryRegion > + * @attrs: memory transaction attributes > + */ > + int (*attrs_to_index)(IOMMUMemoryRegion *iommu, MemTxAttrs attrs); > + > + /* Return the number of IOMMU indexes this IOMMU supports. > + * > + * Optional method: if this method is not provided, then > + * memory_region_iommu_num_indexes() will return 1, indicating that > + * only a single IOMMU index is supported. > + */ > } IOMMUMemoryRegionClass; > > typedef struct CoalescedMemoryRange CoalescedMemoryRange; > @@ -1077,6 +1111,24 @@ int memory_region_iommu_get_attr(IOMMUMemoryRegion *iommu_mr, > enum IOMMUMemoryRegionAttr attr, > void *data); > > +/** > + * memory_region_iommu_attrs_to_index: return the IOMMU index to > + * use for translations with the given memory transaction attributes. > + * > + * @iommu_mr: the memory region > + * @attrs: the memory transaction attributes > + */ > +int memory_region_iommu_attrs_to_index(IOMMUMemoryRegion *iommu_mr, > + MemTxAttrs attrs); > + > +/** > + * memory_region_iommu_num_indexes: return the total number of IOMMU > + * indexes that this IOMMU supports. is it IOMMU wide characteristics or per IOMMUMRRegion (SID)? Thanks Eric > + * > + * @iommu_mr: the memory region > + */ > +int memory_region_iommu_num_indexes(IOMMUMemoryRegion *iommu_mr); > + > /** > * memory_region_name: get a memory region's name > * > diff --git a/memory.c b/memory.c > index 10fa2ddd31..07d5fa7862 100644 > --- a/memory.c > +++ b/memory.c > @@ -1918,6 +1918,29 @@ int memory_region_iommu_get_attr(IOMMUMemoryRegion *iommu_mr, > return imrc->get_attr(iommu_mr, attr, data); > } > > +int memory_region_iommu_attrs_to_index(IOMMUMemoryRegion *iommu_mr, > + MemTxAttrs attrs) > +{ > + IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_GET_CLASS(iommu_mr); > + > + if (!imrc->attrs_to_index) { > + return 0; > + } > + > + return imrc->attrs_to_index(iommu_mr, attrs); > +} > + > +int memory_region_iommu_num_indexes(IOMMUMemoryRegion *iommu_mr) > +{ > + IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_GET_CLASS(iommu_mr); > + > + if (!imrc->num_indexes) { > + return 1; > + } > + > + return imrc->num_indexes(iommu_mr); > +} > + > void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client) > { > uint8_t mask = 1 << client; >
On 22 May 2018 at 13:58, Auger Eric <eric.auger@redhat.com> wrote: > Hi Peter, > On 05/21/2018 04:03 PM, Peter Maydell wrote: >> If an IOMMU supports mappings that care about the memory >> transaction attributes, then it no longer has a unique >> address -> output mapping, but more than one. We can >> represent these using an IOMMU index, analogous to TCG's >> mmu indexes. >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> --- >> include/exec/memory.h | 52 +++++++++++++++++++++++++++++++++++++++++++ >> memory.c | 23 +++++++++++++++++++ >> 2 files changed, 75 insertions(+) >> >> diff --git a/include/exec/memory.h b/include/exec/memory.h >> index 309fdfb89b..f6226fb263 100644 >> --- a/include/exec/memory.h >> +++ b/include/exec/memory.h >> @@ -206,6 +206,20 @@ enum IOMMUMemoryRegionAttr { >> * to report whenever mappings are changed, by calling >> * memory_region_notify_iommu() (or, if necessary, by calling >> * memory_region_notify_one() for each registered notifier). >> + * >> + * Conceptually an IOMMU provides a mapping from input address >> + * to an output TLB entry. > actually it takes a source identifier too (ARM streamid + substreamID, > this latter is not yet supported). > At the moment we have 1 IOMMUMRRegion per sid on ARM. For each > IOMMUMRRegion we would now have 2 indexes (1 for secure and 1 for > unsecure). How do you see the implementation of PASIDs (substreamids). > Would that be based on indexes or not? Good question. I guess we would set that up so that the substream ID is in the memory transaction attributes as the requester_id and then use that as part of the IOMMU index ? Or you could use the indirection between tx attrs and indexes to allow you to map down a potentially large space of substream ID values to a smaller set of actually different configurations. How many substream IDs do we expect to see in practice? >> +/** >> + * memory_region_iommu_attrs_to_index: return the IOMMU index to >> + * use for translations with the given memory transaction attributes. >> + * >> + * @iommu_mr: the memory region >> + * @attrs: the memory transaction attributes >> + */ >> +int memory_region_iommu_attrs_to_index(IOMMUMemoryRegion *iommu_mr, >> + MemTxAttrs attrs); >> + >> +/** >> + * memory_region_iommu_num_indexes: return the total number of IOMMU >> + * indexes that this IOMMU supports. > is it IOMMU wide characteristics or per IOMMUMRRegion (SID)? I generally in this API document have been treating "IOMMU" and "IOMMURegion" as synonymous, since from the perspective of the API we don't care whether there's a bigger underlying thing in common between IOMMURegions. thanks -- PMM
Hi Peter, On 05/22/2018 03:22 PM, Peter Maydell wrote: > On 22 May 2018 at 13:58, Auger Eric <eric.auger@redhat.com> wrote: >> Hi Peter, >> On 05/21/2018 04:03 PM, Peter Maydell wrote: >>> If an IOMMU supports mappings that care about the memory >>> transaction attributes, then it no longer has a unique >>> address -> output mapping, but more than one. We can >>> represent these using an IOMMU index, analogous to TCG's >>> mmu indexes. >>> >>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >>> --- >>> include/exec/memory.h | 52 +++++++++++++++++++++++++++++++++++++++++++ >>> memory.c | 23 +++++++++++++++++++ >>> 2 files changed, 75 insertions(+) >>> >>> diff --git a/include/exec/memory.h b/include/exec/memory.h >>> index 309fdfb89b..f6226fb263 100644 >>> --- a/include/exec/memory.h >>> +++ b/include/exec/memory.h >>> @@ -206,6 +206,20 @@ enum IOMMUMemoryRegionAttr { >>> * to report whenever mappings are changed, by calling >>> * memory_region_notify_iommu() (or, if necessary, by calling >>> * memory_region_notify_one() for each registered notifier). >>> + * >>> + * Conceptually an IOMMU provides a mapping from input address >>> + * to an output TLB entry. >> actually it takes a source identifier too (ARM streamid + substreamID, >> this latter is not yet supported). >> At the moment we have 1 IOMMUMRRegion per sid on ARM. For each >> IOMMUMRRegion we would now have 2 indexes (1 for secure and 1 for >> unsecure). How do you see the implementation of PASIDs (substreamids). >> Would that be based on indexes or not? > > Good question. I guess we would set that up so that the > substream ID is in the memory transaction attributes as the > requester_id and then use that as part of the IOMMU index ? > Or you could use the indirection between tx attrs and indexes > to allow you to map down a potentially large space of substream > ID values to a smaller set of actually different configurations. > > How many substream IDs do we expect to see in practice? Spec says max 20 bits, matching the max size of the PASID Thanks Eric > >>> +/** >>> + * memory_region_iommu_attrs_to_index: return the IOMMU index to >>> + * use for translations with the given memory transaction attributes. >>> + * >>> + * @iommu_mr: the memory region >>> + * @attrs: the memory transaction attributes >>> + */ >>> +int memory_region_iommu_attrs_to_index(IOMMUMemoryRegion *iommu_mr, >>> + MemTxAttrs attrs); >>> + >>> +/** >>> + * memory_region_iommu_num_indexes: return the total number of IOMMU >>> + * indexes that this IOMMU supports. >> is it IOMMU wide characteristics or per IOMMUMRRegion (SID)? > > I generally in this API document have been treating "IOMMU" and > "IOMMURegion" as synonymous, since from the perspective of the API > we don't care whether there's a bigger underlying thing in common > between IOMMURegions. > > thanks > -- PMM >
On 22 May 2018 at 15:11, Auger Eric <eric.auger@redhat.com> wrote: > Hi Peter, > > On 05/22/2018 03:22 PM, Peter Maydell wrote: >> How many substream IDs do we expect to see in practice? > > Spec says max 20 bits, matching the max size of the PASID Right, but do we actually expect to see transactions from devices that use the whole 2^20 possible values, or will they in practice mostly use 1..5, or somethnig else? thanks -- PMM
On 05/22/2018 04:19 PM, Peter Maydell wrote: > On 22 May 2018 at 15:11, Auger Eric <eric.auger@redhat.com> wrote: >> Hi Peter, >> >> On 05/22/2018 03:22 PM, Peter Maydell wrote: >>> How many substream IDs do we expect to see in practice? >> >> Spec says max 20 bits, matching the max size of the PASID > > Right, but do we actually expect to see transactions from > devices that use the whole 2^20 possible values, or will > they in practice mostly use 1..5, or somethnig else? An example given in the spec mentions 8. "An example would be a compute accelerator with 8 contexts that might each map to a different user process, but where the single device has common configuration meaning it must be assigned to a VM whole." Thanks Eric > > thanks > -- PMM >
On 05/21/2018 07:03 AM, Peter Maydell wrote: > + /* Return the IOMMU index to use for a given set of transaction attributes. > + * > + * Optional method: if an IOMMU only supports a single IOMMU index then > + * the default implementation of memory_region_iommu_attrs_to_index() > + * will return 0. > + * > + * The indexes supported by an IOMMU must be contiguous, starting at 0. > + * > + * @iommu: the IOMMUMemoryRegion > + * @attrs: memory transaction attributes > + */ > + int (*attrs_to_index)(IOMMUMemoryRegion *iommu, MemTxAttrs attrs); > + > + /* Return the number of IOMMU indexes this IOMMU supports. > + * > + * Optional method: if this method is not provided, then > + * memory_region_iommu_num_indexes() will return 1, indicating that > + * only a single IOMMU index is supported. > + */ The mispatched callback has been discussed, but would it be equally useful to simply have a variable here instead of a callback? Perhaps max_index instead of num_indexes so that zero-initialization of the structure does the right thing for existing iommu's. r~
On 22 May 2018 at 18:42, Richard Henderson <rth@twiddle.net> wrote: > On 05/21/2018 07:03 AM, Peter Maydell wrote: >> + /* Return the IOMMU index to use for a given set of transaction attributes. >> + * >> + * Optional method: if an IOMMU only supports a single IOMMU index then >> + * the default implementation of memory_region_iommu_attrs_to_index() >> + * will return 0. >> + * >> + * The indexes supported by an IOMMU must be contiguous, starting at 0. >> + * >> + * @iommu: the IOMMUMemoryRegion >> + * @attrs: memory transaction attributes >> + */ >> + int (*attrs_to_index)(IOMMUMemoryRegion *iommu, MemTxAttrs attrs); >> + >> + /* Return the number of IOMMU indexes this IOMMU supports. >> + * >> + * Optional method: if this method is not provided, then >> + * memory_region_iommu_num_indexes() will return 1, indicating that >> + * only a single IOMMU index is supported. >> + */ > > The mispatched callback has been discussed, but would it be equally useful to > simply have a variable here instead of a callback? Perhaps max_index instead > of num_indexes so that zero-initialization of the structure does the right > thing for existing iommu's. That wouldn't allow for multiple instances of the same class where the answer is different (eg "my_iommu->has_trustzone_support ? 2 : 1" where the answer depends on how the instance is configured via QOM properties). thanks -- PMM
On 05/22/2018 10:51 AM, Peter Maydell wrote: > That wouldn't allow for multiple instances of the same class where the > answer is different (eg "my_iommu->has_trustzone_support ? 2 : 1" where > the answer depends on how the instance is configured via QOM properties). Ah. I had somehow been assuming those would be different classes. Ok then, Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On Tue, May 22, 2018 at 12:11:38PM +0100, Peter Maydell wrote: > On 22 May 2018 at 12:02, Peter Xu <peterx@redhat.com> wrote: > > On Tue, May 22, 2018 at 09:40:44AM +0100, Peter Maydell wrote: > >> On 22 May 2018 at 04:03, Peter Xu <peterx@redhat.com> wrote: > >> The reason for not just passing in the transaction attributes to > >> translate is that > >> (a) the iommu index abstraction makes the notifier setup simpler: > >> rather than having to have some indication in the API of which > >> of the transaction attributes are important and which the notifier > >> cares about, we can just use indexs > > > > Hmm, so here IIUC we'll have a new IOMMU notifier that will only > > listen to part of the IOMMU notifies, e.g., when attrs.secure=true. > > Yes I think adding something into IOMMUNotifier might work, but just > > to mention that in IOMMUTLBEntry we have IOMMUTLBEntry.target_as > > defined. Until now it's hardly used at least on x86 platform since > > all of the translations on x86 are targeted to the system RAM. > > However it seems to be quite tailored in this case since it seems to > > me that different attrs.secure value for translations should be based > > on different address spaces too. Then in the IOMMU notifiers that > > would care about MemTxAttrs, could it be possible to identify that by > > check against the IOMMUTLBEntry.target_as? > > No, because you can have a single address space that receives > transactions with various attributes. (Again, the MPC is an > example of this.) > > >> (b) it means that it's harder to write an iommu with the bug that > >> it looks at parts of the transaction attributes that it didn't > >> claim were important in the notifier API > > > > It is just confusing to me when I looked at current translate() > > interface (I copied it from some other patch of the series): > > > > @@ -252,9 +252,10 @@ typedef struct IOMMUMemoryRegionClass { > > * @iommu: the IOMMUMemoryRegion > > * @hwaddr: address to be translated within the memory region > > * @flag: requested access permissions > > + * @iommu_idx: IOMMU index for the translation > > */ > > IOMMUTLBEntry (*translate)(IOMMUMemoryRegion *iommu, hwaddr addr, > > - IOMMUAccessFlags flag); > > + IOMMUAccessFlags flag, int iommu_idx); > > > > The "iommu_idx" parameter is really hard to understand here at the > > first glance. Now I think I understand that it is somehow related to > > the MemTxAttrs, but still it will take time to understand. > > The part of the documentation where I try to explain the general > idea is in this patch, in the comment at the top of the struct: > > + * Conceptually an IOMMU provides a mapping from input address > + * to an output TLB entry. If the IOMMU is aware of memory transaction > + * attributes and the output TLB entry depends on the transaction > + * attributes, we represent this using IOMMU indexes. Each index > + * selects a particular translation table that the IOMMU has: > + * @attrs_to_index returns the IOMMU index for a set of transaction > attributes > + * @translate takes an input address and an IOMMU index > + * and the mapping returned can only depend on the input address and the > + * IOMMU index. > + * > + * Most IOMMUs don't care about the transaction attributes and support > + * only a single IOMMU index. A more complex IOMMU might have one index > + * for secure transactions and one for non-secure transactions. > > Do you have suggestions for how to improve on this? Not yet. But I have some other questions below... > > > And if we see current implementation for this (still, I copied code > > from other patch in the series to here to ease discussion): > > > > @@ -498,8 +498,15 @@ static MemoryRegionSection address_space_translate_iommu(IOMMUMemoryRegion *iomm > > do { > > hwaddr addr = *xlat; > > IOMMUMemoryRegionClass *imrc = memory_region_get_iommu_class_nocheck(iommu_mr); > > - IOMMUTLBEntry iotlb = imrc->translate(iommu_mr, addr, is_write ? > > - IOMMU_WO : IOMMU_RO); > > + int iommu_idx = 0; > > + IOMMUTLBEntry iotlb; > > + > > + if (imrc->attrs_to_index) { > > + iommu_idx = imrc->attrs_to_index(iommu_mr, attrs); > > + } > > + > > + iotlb = imrc->translate(iommu_mr, addr, is_write ? > > + IOMMU_WO : IOMMU_RO, iommu_idx); > > > > Here what if we pass attrs directly into imrc->translate() and just > > call imrc->attrs_to_index() inside the arch-dependent translate() > > function? Will that work too? > > You don't always have the attributes at the point where you want > to call translate. (For instance, memory_region_notify_iommu() > doesn't have attributes.) > > I started off with "pass the tx attrs into the translate method", > which is fine for the code flows which are actually doing > memory transactions, but breaks down when you try to incorporate > notifiers. Could you elaborate a bit more on why IOMMU notifier failed to corporate when passing in MemTxAttrs? I am not sure I caught the idea here, but can we copy the MemTxAttrs into IOMMUTLBEntry when translating, then in IOMMU notifiers we can know the attrs (if that is what MPC wants)? > > > I had a quick glance at the series, I have no thorough idea on the > > whole stuff, but I'd say some of the patches are exactly what I wanted > > if to support MemTxAttrs in VT-d emulation one day (now DMAR of VT-d > > is bypassing MemTxAttrs and IMHO that's incorrect). If we can somehow > > pass in the MemTxAttrs then that'll be perfect and I can continue to > > work on that. If we pass in iommu_idx now instead, it would take some > > time for me to figure out how to further achieve the same goal for > > VT-d in the future, e.g., I would still want to pass in MemTxAttrs, > > but that's obviously duplicated with iommu_idx. > > The idea is that you should never need to pass in the MemTxAttrs, > because everything that the IOMMU might care about in the tx attrs > must be encoded into the iommu index. (The point where the IOMMU > gets to do that encoding is in its attrs_to_index() method.) For the DMAR issue I would care about MemTxAttrs.requester_id. Just to confirm - do you mean I encode the 16bits field into iommu_idx too, or is there any smarter way to do so? Asked since otherwise iommu_idx will gradually grow into another MemTxAttrs to me. Thanks, -- Peter Xu
On 23 May 2018 at 02:06, Peter Xu <peterx@redhat.com> wrote: > On Tue, May 22, 2018 at 12:11:38PM +0100, Peter Maydell wrote: >> On 22 May 2018 at 12:02, Peter Xu <peterx@redhat.com> wrote: >> > On Tue, May 22, 2018 at 09:40:44AM +0100, Peter Maydell wrote: >> > And if we see current implementation for this (still, I copied code >> > from other patch in the series to here to ease discussion): >> > >> > @@ -498,8 +498,15 @@ static MemoryRegionSection address_space_translate_iommu(IOMMUMemoryRegion *iomm >> > do { >> > hwaddr addr = *xlat; >> > IOMMUMemoryRegionClass *imrc = memory_region_get_iommu_class_nocheck(iommu_mr); >> > - IOMMUTLBEntry iotlb = imrc->translate(iommu_mr, addr, is_write ? >> > - IOMMU_WO : IOMMU_RO); >> > + int iommu_idx = 0; >> > + IOMMUTLBEntry iotlb; >> > + >> > + if (imrc->attrs_to_index) { >> > + iommu_idx = imrc->attrs_to_index(iommu_mr, attrs); >> > + } >> > + >> > + iotlb = imrc->translate(iommu_mr, addr, is_write ? >> > + IOMMU_WO : IOMMU_RO, iommu_idx); >> > >> > Here what if we pass attrs directly into imrc->translate() and just >> > call imrc->attrs_to_index() inside the arch-dependent translate() >> > function? Will that work too? >> >> You don't always have the attributes at the point where you want >> to call translate. (For instance, memory_region_notify_iommu() >> doesn't have attributes.) >> >> I started off with "pass the tx attrs into the translate method", >> which is fine for the code flows which are actually doing >> memory transactions, but breaks down when you try to incorporate >> notifiers. > > Could you elaborate a bit more on why IOMMU notifier failed to > corporate when passing in MemTxAttrs? I am not sure I caught the idea > here, but can we copy the MemTxAttrs into IOMMUTLBEntry when > translating, then in IOMMU notifiers we can know the attrs (if that is > what MPC wants)? (1) The notifier API lets you register a notifier before you've called the translate API (2) An IOMMUTLBEntry can be valid for more than just the txattrs that it was passed in (for instance, if an IOMMU doesn't care about txattrs at all, then the resulting TLB entry is valid for any txattrs; or if the IOMMU only cares about attrs.secure the resulting TLB entries are valid for both attrs.user=0 and attrs.user=1). (3) when the IOMMU calls the notifier because the guest config changed it doesn't have tx attributes, so it would have to fabricate some; and the guest config will invalidate transactions with some combinations of tx attributes and not others. As Paolo pointed out you could also implement this by rather of having an iommu_index concept, instead having some kind of "here is a mask of which txattrs fields matter, and here's another parameter with which txattrs fields are affected". That makes it awkward though to implement "txattrs.unspecified acts like txattrs.secure == 1" type behaviour, though, which is easy with an index abstraction layer. It also would be harder to implement the default 'replay' method, I think. Plus I think that handling this the same way TCG does is a reasonable approach -- we know that it's a usefully flexible concept. >> > I had a quick glance at the series, I have no thorough idea on the >> > whole stuff, but I'd say some of the patches are exactly what I wanted >> > if to support MemTxAttrs in VT-d emulation one day (now DMAR of VT-d >> > is bypassing MemTxAttrs and IMHO that's incorrect). If we can somehow >> > pass in the MemTxAttrs then that'll be perfect and I can continue to >> > work on that. If we pass in iommu_idx now instead, it would take some >> > time for me to figure out how to further achieve the same goal for >> > VT-d in the future, e.g., I would still want to pass in MemTxAttrs, >> > but that's obviously duplicated with iommu_idx. >> >> The idea is that you should never need to pass in the MemTxAttrs, >> because everything that the IOMMU might care about in the tx attrs >> must be encoded into the iommu index. (The point where the IOMMU >> gets to do that encoding is in its attrs_to_index() method.) > > For the DMAR issue I would care about MemTxAttrs.requester_id. Just > to confirm - do you mean I encode the 16bits field into iommu_idx too, > or is there any smarter way to do so? Asked since otherwise iommu_idx > will gradually grow into another MemTxAttrs to me. It will only need to do so for IOMMUs that care about that field. (See also the other thread with Eric Auger talking about maybe caring about requester_id like that. Needing to look at requester_id is an area I haven't thought too much about, and it is a bit of an odd one because it's a much larger space than any of the other parts of the txattrs. In some cases it ought to be possible to say "requester_id lets us determine an iommu index, and there are a lot fewer than 2^16 actual iommu indexes because a lot of the requestor_id values indicate the same actual iommu translation", I suspect.) thanks -- PMM
On Wed, May 23, 2018 at 12:47:16PM +0100, Peter Maydell wrote: > On 23 May 2018 at 02:06, Peter Xu <peterx@redhat.com> wrote: > > On Tue, May 22, 2018 at 12:11:38PM +0100, Peter Maydell wrote: > >> On 22 May 2018 at 12:02, Peter Xu <peterx@redhat.com> wrote: > >> > On Tue, May 22, 2018 at 09:40:44AM +0100, Peter Maydell wrote: > > >> > And if we see current implementation for this (still, I copied code > >> > from other patch in the series to here to ease discussion): > >> > > >> > @@ -498,8 +498,15 @@ static MemoryRegionSection address_space_translate_iommu(IOMMUMemoryRegion *iomm > >> > do { > >> > hwaddr addr = *xlat; > >> > IOMMUMemoryRegionClass *imrc = memory_region_get_iommu_class_nocheck(iommu_mr); > >> > - IOMMUTLBEntry iotlb = imrc->translate(iommu_mr, addr, is_write ? > >> > - IOMMU_WO : IOMMU_RO); > >> > + int iommu_idx = 0; > >> > + IOMMUTLBEntry iotlb; > >> > + > >> > + if (imrc->attrs_to_index) { > >> > + iommu_idx = imrc->attrs_to_index(iommu_mr, attrs); > >> > + } > >> > + > >> > + iotlb = imrc->translate(iommu_mr, addr, is_write ? > >> > + IOMMU_WO : IOMMU_RO, iommu_idx); > >> > > >> > Here what if we pass attrs directly into imrc->translate() and just > >> > call imrc->attrs_to_index() inside the arch-dependent translate() > >> > function? Will that work too? > >> > >> You don't always have the attributes at the point where you want > >> to call translate. (For instance, memory_region_notify_iommu() > >> doesn't have attributes.) > >> > >> I started off with "pass the tx attrs into the translate method", > >> which is fine for the code flows which are actually doing > >> memory transactions, but breaks down when you try to incorporate > >> notifiers. > > > > Could you elaborate a bit more on why IOMMU notifier failed to > > corporate when passing in MemTxAttrs? I am not sure I caught the idea > > here, but can we copy the MemTxAttrs into IOMMUTLBEntry when > > translating, then in IOMMU notifiers we can know the attrs (if that is > > what MPC wants)? > > (1) The notifier API lets you register a notifier before you've > called the translate API Yes. > (2) An IOMMUTLBEntry can be valid for more than just the txattrs > that it was passed in (for instance, if an IOMMU doesn't care > about txattrs at all, then the resulting TLB entry is valid for > any txattrs; or if the IOMMU only cares about attrs.secure the > resulting TLB entries are valid for both attrs.user=0 and > attrs.user=1). [1] Yes exactly, that's why I thought copying the txattrs into IOTLB should work. > (3) when the IOMMU calls the notifier because the guest config > changed it doesn't have tx attributes, so it would have to > fabricate some; and the guest config will invalidate transactions > with some combinations of tx attributes and not others. IMHO it doesn't directly matter with what we are discussing now. That IOMMU_NOTIFIER_[UN]MAP flag tells what kind of message would the notifier be interested in from "what kind of mapping it is". IMHO it's not really related to some other attributes when translation happens - in our case, it does not directly related to what txattrs value is. Here as mentioned at [1] above IMHO we can still check this against txattrs in the notifier handler, then we ignore messages that we don't care about. Actually the IOMMU_NOTIFIER_[UN]MAP flags can be removed and we can just do similar things (e.g., we can skip MAP messages if we only care about UNMAP messages), but since it's a general concept and easy to be generalized, so we provided these MAP/UNMAP flags to ease the notifier hooks. In other words, I think we can also add more flags for SECURE or not. However I still don't see a reason (from above three points) on why we can't pass in txattrs directly into translate(), and at the same time we copy the txattrs into IOTLB so that IOMMUTLBEntry can contain some context information. [2] > > As Paolo pointed out you could also implement this by rather > of having an iommu_index concept, instead having some kind > of "here is a mask of which txattrs fields matter, and here's > another parameter with which txattrs fields are affected". > That makes it awkward though to implement "txattrs.unspecified > acts like txattrs.secure == 1" type behaviour, though, which is > easy with an index abstraction layer. It also would be harder > to implement the default 'replay' method, I think. Please refer to my above comment at [2] - I am still confused on why we must use this iommu_idx concept. How about we just introduce IOMMU_NOTIFIER_SECURE (or something similar) and let TCG code register with that? Though for the rest of notifiers we'll need to touch up too to make sure all existing notifiers will still receive all the message, no matter whether it's secure or not. I'd also appreciate if you could paste me the link for Paolo's message, since I cannot find it. > > Plus I think that handling this the same way TCG does is a > reasonable approach -- we know that it's a usefully flexible > concept. > > >> > I had a quick glance at the series, I have no thorough idea on the > >> > whole stuff, but I'd say some of the patches are exactly what I wanted > >> > if to support MemTxAttrs in VT-d emulation one day (now DMAR of VT-d > >> > is bypassing MemTxAttrs and IMHO that's incorrect). If we can somehow > >> > pass in the MemTxAttrs then that'll be perfect and I can continue to > >> > work on that. If we pass in iommu_idx now instead, it would take some > >> > time for me to figure out how to further achieve the same goal for > >> > VT-d in the future, e.g., I would still want to pass in MemTxAttrs, > >> > but that's obviously duplicated with iommu_idx. > >> > >> The idea is that you should never need to pass in the MemTxAttrs, > >> because everything that the IOMMU might care about in the tx attrs > >> must be encoded into the iommu index. (The point where the IOMMU > >> gets to do that encoding is in its attrs_to_index() method.) > > > > For the DMAR issue I would care about MemTxAttrs.requester_id. Just > > to confirm - do you mean I encode the 16bits field into iommu_idx too, > > or is there any smarter way to do so? Asked since otherwise iommu_idx > > will gradually grow into another MemTxAttrs to me. > > It will only need to do so for IOMMUs that care about that field. > > (See also the other thread with Eric Auger talking about > maybe caring about requester_id like that. Needing to look > at requester_id is an area I haven't thought too much about, > and it is a bit of an odd one because it's a much larger > space than any of the other parts of the txattrs. In some > cases it ought to be possible to say "requester_id lets > us determine an iommu index, and there are a lot fewer > than 2^16 actual iommu indexes because a lot of the requestor_id > values indicate the same actual iommu translation", I suspect.) AFAIK requester_id will only be the same in very rare cases, for example, when multiple PCI devices (no matter whether it's PCI or PCIe) are under the same PCIe-to-PCI bridge, then all these devices will use the bridge's requester ID as their own. In most cases, each PCIe device will have their own unique requester ID. So it'll be common that requester ID number can be at least equal to the number of devices. Thanks, -- Peter Xu
On 24 May 2018 at 07:23, Peter Xu <peterx@redhat.com> wrote: > On Wed, May 23, 2018 at 12:47:16PM +0100, Peter Maydell wrote: >> On 23 May 2018 at 02:06, Peter Xu <peterx@redhat.com> wrote: >> > Could you elaborate a bit more on why IOMMU notifier failed to >> > corporate when passing in MemTxAttrs? I am not sure I caught the idea >> > here, but can we copy the MemTxAttrs into IOMMUTLBEntry when >> > translating, then in IOMMU notifiers we can know the attrs (if that is >> > what MPC wants)? >> >> (1) The notifier API lets you register a notifier before you've >> called the translate API > > Yes. > >> (2) An IOMMUTLBEntry can be valid for more than just the txattrs >> that it was passed in (for instance, if an IOMMU doesn't care >> about txattrs at all, then the resulting TLB entry is valid for >> any txattrs; or if the IOMMU only cares about attrs.secure the >> resulting TLB entries are valid for both attrs.user=0 and >> attrs.user=1). > > [1] > > Yes exactly, that's why I thought copying the txattrs into IOTLB > should work. I'm a bit confused about why the IOMMUTLBEntry is relevant here. That's the thing returned from the translate method, so there's no point in copying txattrs into it, because the caller by definition already had them. At the point where the IOMMU notices a guest changed the config, it doesn't have an IOMMUTLBEntry or a set of tx attrs. >> (3) when the IOMMU calls the notifier because the guest config >> changed it doesn't have tx attributes, so it would have to >> fabricate some; and the guest config will invalidate transactions >> with some combinations of tx attributes and not others. > > IMHO it doesn't directly matter with what we are discussing now. That > IOMMU_NOTIFIER_[UN]MAP flag tells what kind of message would the > notifier be interested in from "what kind of mapping it is". IMHO > it's not really related to some other attributes when translation > happens - in our case, it does not directly related to what txattrs > value is. Here as mentioned at [1] above IMHO we can still check this > against txattrs in the notifier handler, then we ignore messages that > we don't care about. Actually the IOMMU_NOTIFIER_[UN]MAP flags can be > removed and we can just do similar things (e.g., we can skip MAP > messages if we only care about UNMAP messages), but since it's a > general concept and easy to be generalized, so we provided these > MAP/UNMAP flags to ease the notifier hooks. > > In other words, I think we can also add more flags for SECURE or not. > However I still don't see a reason (from above three points) on why we > can't pass in txattrs directly into translate(), and at the same time > we copy the txattrs into IOTLB so that IOMMUTLBEntry can contain some > context information. [2] I'm afraid I really don't understand the design you're proposing here. But overall I think the point of divergence is that the mapping from "transaction attributes" to "translation contexts" (ie, effectively different page tables) is not 1:1. So for instance: Our current IOMMUs which don't care about txattrs: [any txattr at all] -> the one and only translation context An IOMMU which cares about attrs.secure, and also treats attrs.unspecified like secure: [any txattr with attrs.secure = 1] \-> 'secure' context' MEMATTRS_UNSPECIFIED / [txattrs with secure = 1] -> 'nonsecure' context An IOMMU which cares about attrs.secure and attrs.user: [secure=1,user=1] -> 'secure user' context [secure=0,user=1] -> 'ns user' context [secure=1,user=0] -> 's priv' context [secure=0,user=0] -> 'ns priv' context The IOMMU index captures this idea that there is not a 1:1 mapping, so we have a way to think about and refer to the actual set of translation contexts that the IOMMU has. >> As Paolo pointed out you could also implement this by rather >> of having an iommu_index concept, instead having some kind >> of "here is a mask of which txattrs fields matter, and here's >> another parameter with which txattrs fields are affected". >> That makes it awkward though to implement "txattrs.unspecified >> acts like txattrs.secure == 1" type behaviour, though, which is >> easy with an index abstraction layer. It also would be harder >> to implement the default 'replay' method, I think. > > Please refer to my above comment at [2] - I am still confused on why > we must use this iommu_idx concept. How about we just introduce > IOMMU_NOTIFIER_SECURE (or something similar) and let TCG code register > with that? Though for the rest of notifiers we'll need to touch up > too to make sure all existing notifiers will still receive all the > message, no matter whether it's secure or not. I think I would definitely prefer not to have the secure/nonsecure specific thing in the API. We've got good experience with TCG where we abstract away the specifics of what an MMU cares about into an mmu_index, and I'd like to keep that approach. Otherwise, you immediately get into "and now we need to change the API again to handle IOMMUs which care about attrs.user"; and then again for attrs.requester_id; and now what about IOMMUs that care about both secure and user... Better to have an abstraction so that we don't need to keep changing the core code. In particular, TCG doesn't know whether it's secure/nonsecure that matters -- that is mostly handled by the target-specific parts, and TCG core code just passes attributes around. > I'd also appreciate if you could paste me the link for Paolo's > message, since I cannot find it. This is the one I had in mind: https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg03522.html >> It will only need to do so for IOMMUs that care about that field. >> >> (See also the other thread with Eric Auger talking about >> maybe caring about requester_id like that. Needing to look >> at requester_id is an area I haven't thought too much about, >> and it is a bit of an odd one because it's a much larger >> space than any of the other parts of the txattrs. In some >> cases it ought to be possible to say "requester_id lets >> us determine an iommu index, and there are a lot fewer >> than 2^16 actual iommu indexes because a lot of the requestor_id >> values indicate the same actual iommu translation", I suspect.) > > AFAIK requester_id will only be the same in very rare cases, for > example, when multiple PCI devices (no matter whether it's PCI or > PCIe) are under the same PCIe-to-PCI bridge, then all these devices > will use the bridge's requester ID as their own. In most cases, each > PCIe device will have their own unique requester ID. So it'll be > common that requester ID number can be at least equal to the number of > devices. I haven't looked at this, but my understanding is that at the moment we do per-device requester_id by having each PCI device get its own IOMMUMemoryRegion mapped into its address space. So we'd only need to look at requester_id for the case of devices with multiple subfunctions(?), and presumably most devices only have a handful of those. thanks -- PMM
On Thu, May 24, 2018 at 11:54:17AM +0100, Peter Maydell wrote: > On 24 May 2018 at 07:23, Peter Xu <peterx@redhat.com> wrote: > > On Wed, May 23, 2018 at 12:47:16PM +0100, Peter Maydell wrote: > >> On 23 May 2018 at 02:06, Peter Xu <peterx@redhat.com> wrote: > >> > Could you elaborate a bit more on why IOMMU notifier failed to > >> > corporate when passing in MemTxAttrs? I am not sure I caught the idea > >> > here, but can we copy the MemTxAttrs into IOMMUTLBEntry when > >> > translating, then in IOMMU notifiers we can know the attrs (if that is > >> > what MPC wants)? > >> > >> (1) The notifier API lets you register a notifier before you've > >> called the translate API > > > > Yes. > > > >> (2) An IOMMUTLBEntry can be valid for more than just the txattrs > >> that it was passed in (for instance, if an IOMMU doesn't care > >> about txattrs at all, then the resulting TLB entry is valid for > >> any txattrs; or if the IOMMU only cares about attrs.secure the > >> resulting TLB entries are valid for both attrs.user=0 and > >> attrs.user=1). > > > > [1] > > > > Yes exactly, that's why I thought copying the txattrs into IOTLB > > should work. > > I'm a bit confused about why the IOMMUTLBEntry is relevant here. > That's the thing returned from the translate method, so there's > no point in copying txattrs into it, because the caller by definition > already had them. At the point where the IOMMU notices a guest > changed the config, it doesn't have an IOMMUTLBEntry or a set of > tx attrs. Yes the txattrs is not for the translate() callers, but for IOMMU notifiers only. Thanks for the pointer below [1], I think it's very similar to what Paolo mentioned there at [1], the major difference is that Paolo suggested to put txattrs into both IOMMUNotify and memory_region_notify_one(), while my above suggestion is that we can directly copy it into IOMMUTLBEntry - note that both IOMMUNotify and memory_region_notify_one will have a IOMMUTLBEntry parameter. Though I am not 100% clear on Paolo's suggestion on why to add two txattrs parameters for each function (since I thought all the values in txattrs to be passed to either IOMMUNotify or memory_region_notify_one should be valid, so I am not sure why we need to "indicate which attributes matter (0 = indifferent, 1 = matter)"). > > >> (3) when the IOMMU calls the notifier because the guest config > >> changed it doesn't have tx attributes, so it would have to > >> fabricate some; and the guest config will invalidate transactions > >> with some combinations of tx attributes and not others. > > > > IMHO it doesn't directly matter with what we are discussing now. That > > IOMMU_NOTIFIER_[UN]MAP flag tells what kind of message would the > > notifier be interested in from "what kind of mapping it is". IMHO > > it's not really related to some other attributes when translation > > happens - in our case, it does not directly related to what txattrs > > value is. Here as mentioned at [1] above IMHO we can still check this > > against txattrs in the notifier handler, then we ignore messages that > > we don't care about. Actually the IOMMU_NOTIFIER_[UN]MAP flags can be > > removed and we can just do similar things (e.g., we can skip MAP > > messages if we only care about UNMAP messages), but since it's a > > general concept and easy to be generalized, so we provided these > > MAP/UNMAP flags to ease the notifier hooks. > > > > In other words, I think we can also add more flags for SECURE or not. > > However I still don't see a reason (from above three points) on why we > > can't pass in txattrs directly into translate(), and at the same time > > we copy the txattrs into IOTLB so that IOMMUTLBEntry can contain some > > context information. [2] > > I'm afraid I really don't understand the design you're proposing > here. But overall I think the point of divergence is that > the mapping from "transaction attributes" to "translation contexts" > (ie, effectively different page tables) is not 1:1. So for instance: > > Our current IOMMUs which don't care about txattrs: > > [any txattr at all] -> the one and only translation context > > An IOMMU which cares about attrs.secure, and also treats > attrs.unspecified like secure: > [any txattr with attrs.secure = 1] \-> 'secure' context' > MEMATTRS_UNSPECIFIED / > > [txattrs with secure = 1] -> 'nonsecure' context (This part is a bit interesting - the "secure" bit is actually flipped between txattrs and the context...) > > An IOMMU which cares about attrs.secure and attrs.user: > [secure=1,user=1] -> 'secure user' context > [secure=0,user=1] -> 'ns user' context > [secure=1,user=0] -> 's priv' context > [secure=0,user=0] -> 'ns priv' context > > The IOMMU index captures this idea that there is not a 1:1 > mapping, so we have a way to think about and refer to the > actual set of translation contexts that the IOMMU has. Yes. I'd say I am not really against this iommu_idx idea. My only worry is that I'm not sure whether that'll be good enough for the future usage of IOMMU, e.g., the requester ID issue to be discussed and solved. My understanding is that the txattrs is already a superset of the iommu_idx concept. Meanwhile, the iommu_idx concept is not easy to understand. So it'll be nice if we can solve the problem with what we already have now (no new concept like iommu_idx), easier to understand, meanwhile it even covers more possibilities in the future (we can easily generate iommu_idx by txattrs, but not other way round). > > >> As Paolo pointed out you could also implement this by rather > >> of having an iommu_index concept, instead having some kind > >> of "here is a mask of which txattrs fields matter, and here's > >> another parameter with which txattrs fields are affected". > >> That makes it awkward though to implement "txattrs.unspecified > >> acts like txattrs.secure == 1" type behaviour, though, which is > >> easy with an index abstraction layer. It also would be harder > >> to implement the default 'replay' method, I think. > > > > Please refer to my above comment at [2] - I am still confused on why > > we must use this iommu_idx concept. How about we just introduce > > IOMMU_NOTIFIER_SECURE (or something similar) and let TCG code register > > with that? Though for the rest of notifiers we'll need to touch up > > too to make sure all existing notifiers will still receive all the > > message, no matter whether it's secure or not. > > I think I would definitely prefer not to have the secure/nonsecure > specific thing in the API. We've got good experience with TCG > where we abstract away the specifics of what an MMU cares about > into an mmu_index, and I'd like to keep that approach. Otherwise, > you immediately get into "and now we need to change the API again > to handle IOMMUs which care about attrs.user"; and then again > for attrs.requester_id; and now what about IOMMUs that care > about both secure and user... Better to have an abstraction so > that we don't need to keep changing the core code. In particular, > TCG doesn't know whether it's secure/nonsecure that matters -- that > is mostly handled by the target-specific parts, and TCG core code > just passes attributes around. IMHO the notifier flags are of course extra - we can introduce new flags, or we can just handle them in the notifier hooks without touching that part (especially if we copy the txattrs into IOMMUTLBEntry we don't even need to touch the notifier API). IMHO the thing that matters more is what we need to pass to the translate() and whether the iommu_idx concept is a must. I am really fine with either way to implement the IOMMU notifiers when we can make sure whether iommu_idx is a must. Again, I am totally not against the iommu_idx concept - I am just afraid one day that'll be not enough for us and we still need to rework on that part. > > > I'd also appreciate if you could paste me the link for Paolo's > > message, since I cannot find it. > > This is the one I had in mind: > https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg03522.html [1] Really sorry to chime in so late no matter what; I didn't noticed the RFC series. These comments are for sure more suitable for RFCs. > > >> It will only need to do so for IOMMUs that care about that field. > >> > >> (See also the other thread with Eric Auger talking about > >> maybe caring about requester_id like that. Needing to look > >> at requester_id is an area I haven't thought too much about, > >> and it is a bit of an odd one because it's a much larger > >> space than any of the other parts of the txattrs. In some > >> cases it ought to be possible to say "requester_id lets > >> us determine an iommu index, and there are a lot fewer > >> than 2^16 actual iommu indexes because a lot of the requestor_id > >> values indicate the same actual iommu translation", I suspect.) > > > > AFAIK requester_id will only be the same in very rare cases, for > > example, when multiple PCI devices (no matter whether it's PCI or > > PCIe) are under the same PCIe-to-PCI bridge, then all these devices > > will use the bridge's requester ID as their own. In most cases, each > > PCIe device will have their own unique requester ID. So it'll be > > common that requester ID number can be at least equal to the number of > > devices. > > I haven't looked at this, but my understanding is that at the moment > we do per-device requester_id by having each PCI device get its own > IOMMUMemoryRegion mapped into its address space. So we'd only need > to look at requester_id for the case of devices with multiple > subfunctions(?), and presumably most devices only have a handful > of those. Not sure I fully understand this, do you mean that requester ID can actually be bound to the DMA address space (and the IOMMU memory regions) for the device? I am not sure, maybe yes... Though ppassing txattrs (and requester ID) from the translate() function still seems more reasonable. I assume that looks more like what the real hardware does - the requester ID should be embeded in the PCIe packet when sending the DMA request (or page translation request). And for me the translate() emulates the page translation requests, that's why passing in txattrs is very easy to understand at least for me (while the iommu_idx is not easy to understand; especially the "index" let me think about "which IOMMU is responsible for this"). Thanks, -- Peter Xu
Hi Peter, On 05/24/2018 12:54 PM, Peter Maydell wrote: > On 24 May 2018 at 07:23, Peter Xu <peterx@redhat.com> wrote: >> On Wed, May 23, 2018 at 12:47:16PM +0100, Peter Maydell wrote: >>> On 23 May 2018 at 02:06, Peter Xu <peterx@redhat.com> wrote: >>>> Could you elaborate a bit more on why IOMMU notifier failed to >>>> corporate when passing in MemTxAttrs? I am not sure I caught the idea >>>> here, but can we copy the MemTxAttrs into IOMMUTLBEntry when >>>> translating, then in IOMMU notifiers we can know the attrs (if that is >>>> what MPC wants)? >>> >>> (1) The notifier API lets you register a notifier before you've >>> called the translate API >> >> Yes. >> >>> (2) An IOMMUTLBEntry can be valid for more than just the txattrs >>> that it was passed in (for instance, if an IOMMU doesn't care >>> about txattrs at all, then the resulting TLB entry is valid for >>> any txattrs; or if the IOMMU only cares about attrs.secure the >>> resulting TLB entries are valid for both attrs.user=0 and >>> attrs.user=1). >> >> [1] >> >> Yes exactly, that's why I thought copying the txattrs into IOTLB >> should work. > > I'm a bit confused about why the IOMMUTLBEntry is relevant here. > That's the thing returned from the translate method, so there's > no point in copying txattrs into it, because the caller by definition > already had them. At the point where the IOMMU notices a guest > changed the config, it doesn't have an IOMMUTLBEntry or a set of > tx attrs. > >>> (3) when the IOMMU calls the notifier because the guest config >>> changed it doesn't have tx attributes, so it would have to >>> fabricate some; and the guest config will invalidate transactions >>> with some combinations of tx attributes and not others. >> >> IMHO it doesn't directly matter with what we are discussing now. That >> IOMMU_NOTIFIER_[UN]MAP flag tells what kind of message would the >> notifier be interested in from "what kind of mapping it is". IMHO >> it's not really related to some other attributes when translation >> happens - in our case, it does not directly related to what txattrs >> value is. Here as mentioned at [1] above IMHO we can still check this >> against txattrs in the notifier handler, then we ignore messages that >> we don't care about. Actually the IOMMU_NOTIFIER_[UN]MAP flags can be >> removed and we can just do similar things (e.g., we can skip MAP >> messages if we only care about UNMAP messages), but since it's a >> general concept and easy to be generalized, so we provided these >> MAP/UNMAP flags to ease the notifier hooks. >> >> In other words, I think we can also add more flags for SECURE or not. >> However I still don't see a reason (from above three points) on why we >> can't pass in txattrs directly into translate(), and at the same time >> we copy the txattrs into IOTLB so that IOMMUTLBEntry can contain some >> context information. [2] > > I'm afraid I really don't understand the design you're proposing > here. But overall I think the point of divergence is that > the mapping from "transaction attributes" to "translation contexts" > (ie, effectively different page tables) is not 1:1. So for instance: > > Our current IOMMUs which don't care about txattrs: > > [any txattr at all] -> the one and only translation context > > An IOMMU which cares about attrs.secure, and also treats > attrs.unspecified like secure: > [any txattr with attrs.secure = 1] \-> 'secure' context' > MEMATTRS_UNSPECIFIED / > > [txattrs with secure = 1] -> 'nonsecure' context > > An IOMMU which cares about attrs.secure and attrs.user: > [secure=1,user=1] -> 'secure user' context > [secure=0,user=1] -> 'ns user' context > [secure=1,user=0] -> 's priv' context > [secure=0,user=0] -> 'ns priv' context I fail to understand the PRIV attribute usage in SMMUv3. My understanding is the STRW (ie. stream world, kind of indication of the exception level the SID is used along) in the STE is used to determine the correct TTB*. Isn't PRIV checked against the page table attributes only? So would be expose 4 indexes for SMMUv3 or only 2 (S and NS)? Thanks Eric > > The IOMMU index captures this idea that there is not a 1:1 > mapping, so we have a way to think about and refer to the > actual set of translation contexts that the IOMMU has. > >>> As Paolo pointed out you could also implement this by rather >>> of having an iommu_index concept, instead having some kind >>> of "here is a mask of which txattrs fields matter, and here's >>> another parameter with which txattrs fields are affected". >>> That makes it awkward though to implement "txattrs.unspecified >>> acts like txattrs.secure == 1" type behaviour, though, which is >>> easy with an index abstraction layer. It also would be harder >>> to implement the default 'replay' method, I think. >> >> Please refer to my above comment at [2] - I am still confused on why >> we must use this iommu_idx concept. How about we just introduce >> IOMMU_NOTIFIER_SECURE (or something similar) and let TCG code register >> with that? Though for the rest of notifiers we'll need to touch up >> too to make sure all existing notifiers will still receive all the >> message, no matter whether it's secure or not. > > I think I would definitely prefer not to have the secure/nonsecure > specific thing in the API. We've got good experience with TCG > where we abstract away the specifics of what an MMU cares about > into an mmu_index, and I'd like to keep that approach. Otherwise, > you immediately get into "and now we need to change the API again > to handle IOMMUs which care about attrs.user"; and then again > for attrs.requester_id; and now what about IOMMUs that care > about both secure and user... Better to have an abstraction so > that we don't need to keep changing the core code. In particular, > TCG doesn't know whether it's secure/nonsecure that matters -- that > is mostly handled by the target-specific parts, and TCG core code > just passes attributes around. > >> I'd also appreciate if you could paste me the link for Paolo's >> message, since I cannot find it. > > This is the one I had in mind: > https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg03522.html > >>> It will only need to do so for IOMMUs that care about that field. >>> >>> (See also the other thread with Eric Auger talking about >>> maybe caring about requester_id like that. Needing to look >>> at requester_id is an area I haven't thought too much about, >>> and it is a bit of an odd one because it's a much larger >>> space than any of the other parts of the txattrs. In some >>> cases it ought to be possible to say "requester_id lets >>> us determine an iommu index, and there are a lot fewer >>> than 2^16 actual iommu indexes because a lot of the requestor_id >>> values indicate the same actual iommu translation", I suspect.) >> >> AFAIK requester_id will only be the same in very rare cases, for >> example, when multiple PCI devices (no matter whether it's PCI or >> PCIe) are under the same PCIe-to-PCI bridge, then all these devices >> will use the bridge's requester ID as their own. In most cases, each >> PCIe device will have their own unique requester ID. So it'll be >> common that requester ID number can be at least equal to the number of >> devices. > > I haven't looked at this, but my understanding is that at the moment > we do per-device requester_id by having each PCI device get its own > IOMMUMemoryRegion mapped into its address space. So we'd only need > to look at requester_id for the case of devices with multiple > subfunctions(?), and presumably most devices only have a handful > of those. > > thanks > -- PMM >
On 25 May 2018 at 10:27, Auger Eric <eric.auger@redhat.com> wrote: > I fail to understand the PRIV attribute usage in SMMUv3. > My understanding is the STRW (ie. stream world, kind of indication of > the exception level the SID is used along) in the STE is used to > determine the correct TTB*. Isn't PRIV checked against the page table > attributes only? I haven't looked too closely at the details for the SMMUv3. But basically if you can return different results for "transaction is priv" vs "transaction is user" then you need separate iommu indexes, even if the only difference might be in the permissions. (This is because an IOMMUTLBEntry can only specify one set of r/w permissions; it can't tell you the permissions separately for priv vs user.) If the SMMUv3 always reports the same permissions and address regardless of the transaction's priv attribute, then it doesn't need to use separate indexes for them. thanks -- PMM
diff --git a/include/exec/memory.h b/include/exec/memory.h index 309fdfb89b..f6226fb263 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -206,6 +206,20 @@ enum IOMMUMemoryRegionAttr { * to report whenever mappings are changed, by calling * memory_region_notify_iommu() (or, if necessary, by calling * memory_region_notify_one() for each registered notifier). + * + * Conceptually an IOMMU provides a mapping from input address + * to an output TLB entry. If the IOMMU is aware of memory transaction + * attributes and the output TLB entry depends on the transaction + * attributes, we represent this using IOMMU indexes. Each index + * selects a particular translation table that the IOMMU has: + * @attrs_to_index returns the IOMMU index for a set of transaction attributes + * @translate takes an input address and an IOMMU index + * and the mapping returned can only depend on the input address and the + * IOMMU index. + * + * Most IOMMUs don't care about the transaction attributes and support + * only a single IOMMU index. A more complex IOMMU might have one index + * for secure transactions and one for non-secure transactions. */ typedef struct IOMMUMemoryRegionClass { /* private */ @@ -290,6 +304,26 @@ typedef struct IOMMUMemoryRegionClass { */ int (*get_attr)(IOMMUMemoryRegion *iommu, enum IOMMUMemoryRegionAttr attr, void *data); + + /* Return the IOMMU index to use for a given set of transaction attributes. + * + * Optional method: if an IOMMU only supports a single IOMMU index then + * the default implementation of memory_region_iommu_attrs_to_index() + * will return 0. + * + * The indexes supported by an IOMMU must be contiguous, starting at 0. + * + * @iommu: the IOMMUMemoryRegion + * @attrs: memory transaction attributes + */ + int (*attrs_to_index)(IOMMUMemoryRegion *iommu, MemTxAttrs attrs); + + /* Return the number of IOMMU indexes this IOMMU supports. + * + * Optional method: if this method is not provided, then + * memory_region_iommu_num_indexes() will return 1, indicating that + * only a single IOMMU index is supported. + */ } IOMMUMemoryRegionClass; typedef struct CoalescedMemoryRange CoalescedMemoryRange; @@ -1077,6 +1111,24 @@ int memory_region_iommu_get_attr(IOMMUMemoryRegion *iommu_mr, enum IOMMUMemoryRegionAttr attr, void *data); +/** + * memory_region_iommu_attrs_to_index: return the IOMMU index to + * use for translations with the given memory transaction attributes. + * + * @iommu_mr: the memory region + * @attrs: the memory transaction attributes + */ +int memory_region_iommu_attrs_to_index(IOMMUMemoryRegion *iommu_mr, + MemTxAttrs attrs); + +/** + * memory_region_iommu_num_indexes: return the total number of IOMMU + * indexes that this IOMMU supports. + * + * @iommu_mr: the memory region + */ +int memory_region_iommu_num_indexes(IOMMUMemoryRegion *iommu_mr); + /** * memory_region_name: get a memory region's name * diff --git a/memory.c b/memory.c index 10fa2ddd31..07d5fa7862 100644 --- a/memory.c +++ b/memory.c @@ -1918,6 +1918,29 @@ int memory_region_iommu_get_attr(IOMMUMemoryRegion *iommu_mr, return imrc->get_attr(iommu_mr, attr, data); } +int memory_region_iommu_attrs_to_index(IOMMUMemoryRegion *iommu_mr, + MemTxAttrs attrs) +{ + IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_GET_CLASS(iommu_mr); + + if (!imrc->attrs_to_index) { + return 0; + } + + return imrc->attrs_to_index(iommu_mr, attrs); +} + +int memory_region_iommu_num_indexes(IOMMUMemoryRegion *iommu_mr) +{ + IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_GET_CLASS(iommu_mr); + + if (!imrc->num_indexes) { + return 1; + } + + return imrc->num_indexes(iommu_mr); +} + void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client) { uint8_t mask = 1 << client;
If an IOMMU supports mappings that care about the memory transaction attributes, then it no longer has a unique address -> output mapping, but more than one. We can represent these using an IOMMU index, analogous to TCG's mmu indexes. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- include/exec/memory.h | 52 +++++++++++++++++++++++++++++++++++++++++++ memory.c | 23 +++++++++++++++++++ 2 files changed, 75 insertions(+) -- 2.17.0