| Message ID | 20180501085939.6201-1-peter.maydell@linaro.org |
|---|---|
| Headers | show |
| Series | iommu: add MemTxAttrs argument to IOMMU translate function | expand |
On 01/05/2018 10:59, Peter Maydell wrote: > As well as the SMMU, I'm also thinking about using the IOMMU > infrastructure for the v8M Memory Protection Controller > (though that is a bit trickier as I also need it to support > TCG execution in an IOMMU-controlled region, which is an > orthogonal bit of work to attribute support). > > Based-on: <20180430122404.10741-1-peter.maydell@linaro.org> > ("memory.h: Improve IOMMU related documentation") but > only for textual reasons. > > v1->v2 changes: only adding the patch 1/12 that I forgot > to include in the v1 posting... This is going to conflict with the MemoryRegionCache patches (review welcome :)), as they add a new function address_space_translate_iommu. But apart from this it seems like a no brainer. Paolo
On 2 May 2018 at 10:25, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 01/05/2018 10:59, Peter Maydell wrote: >> As well as the SMMU, I'm also thinking about using the IOMMU >> infrastructure for the v8M Memory Protection Controller >> (though that is a bit trickier as I also need it to support >> TCG execution in an IOMMU-controlled region, which is an >> orthogonal bit of work to attribute support). > This is going to conflict with the MemoryRegionCache patches (review > welcome :)), as they add a new function address_space_translate_iommu. > But apart from this it seems like a no brainer. (We talked about this on IRC, but I don't really have any good ideas about the API changes needed, so I figured I'd write it up here.) While working through this and the MPC changes, I realized that if we pass transaction attributes through to the IOMMU translate method then we also need to change the API of the IOMMU notifier system. This is because a txattrs-aware IOMMU might return more than one mapping for a particular input address (eg "use mapping A if txattrs.secure, but mapping B if not"). So map/unmap events become more complex than just "unmap for address X" and "map for address X" -- you need to distinguish "here's the new map for address X with txattrs XA". Presumably we also want a way for notifier users like vfio to detect "I'm dealing with an IOMMU that is txattrs aware in a way I can't deal with" so they can usefully bail out rather than not working. Unfortunately I don't really know enough about our two current users of notifiers (vhost and vfio) to know what they actually need the iommu notifications for... (For the notifier I need to add for TCG, the requirements are easy: I just want to be able to get unmap events so I can flush the TCG TLB when the info I've cached from translate calls is stale.) thanks -- PMM
On 15/05/2018 18:28, Peter Maydell wrote: > > Presumably we also want a way for notifier users like vfio to > detect "I'm dealing with an IOMMU that is txattrs aware in a > way I can't deal with" so they can usefully bail out rather than > not working. > > Unfortunately I don't really know enough about our two current > users of notifiers (vhost and vfio) to know what they actually > need the iommu notifications for... As you guessed on IRC, they basically establish a shadow IOMMU page table or TLB. My proposal would be to add two MemTxAttrs arguments to the IOMMUNotify typedef and to memory_region_notify_one, respectively to indicate which attributes matter (0 = indifferent, 1 = matter) and their value. So far so good. Perhaps memory_region_register_iommu_notifier can also get an argument with the supported attributes. The function would then fail if there are fewer bits set in that argument than what the IOMMU supports... The only problem with that is that memory_region_register_iommu_notifier is called late from VFIO, and in a place that really cannot fail (a MemoryListener's region_add callback). So I would not be sure about how to deal with failure in the VFIO code. Thanks, Paolo
On 15 May 2018 at 17:50, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 15/05/2018 18:28, Peter Maydell wrote: >> >> Presumably we also want a way for notifier users like vfio to >> detect "I'm dealing with an IOMMU that is txattrs aware in a >> way I can't deal with" so they can usefully bail out rather than >> not working. >> >> Unfortunately I don't really know enough about our two current >> users of notifiers (vhost and vfio) to know what they actually >> need the iommu notifications for... > > As you guessed on IRC, they basically establish a shadow IOMMU page > table or TLB. My proposal would be to add two MemTxAttrs arguments to > the IOMMUNotify typedef and to memory_region_notify_one, respectively to > indicate which attributes matter (0 = indifferent, 1 = matter) and their > value. So far so good. > > Perhaps memory_region_register_iommu_notifier can also get an argument > with the supported attributes. The function would then fail if there > are fewer bits set in that argument than what the IOMMU supports... So I had an idea this morning for a slightly different way to approach this, which is that we add a concept of an iommu_idx, analogous to our existing TCG mmu_idx. Basically an iommu_idx represents a page table (kind of like what Arm calls a "translation regime"), so that for any particular iommu index and input address the output address and permissions are always the same. For the memory protection controller there would be two iommu_idxes, one for secure and one for non-secure. The API changes would be something like: * new method get_num_indexes() which returns the number of iommu indexes this IOMMU implements (default implementation: return 1) * translate method takes an iommu index (and not the txattrs) * new method index_from_attrs() which takes a MemTxAttrs and returns the iommu index that should be used (default implementation: always return 0) * memory_region_register_iommu_notifier() takes an iommu index * the default 'replay' method does "for each supported index, for each address, call @translate" * vfio and vhost can register their notifiers using the index returned by index_from_attrs(MEMTXATTRS_UNSPECIFIED) * maybe they could call get_num_indexes() and bail out if the IOMMU has support for multiple indexes? * maybe they could be enhanced to support tracking multiple page tables if necessary in future I haven't worked through the details yet, but this seems to me more flexible than working directly with txattrs. It also means it's harder to accidentally write an iommu implementation that looks at more fields in the txattrs than its notifier interface claims are significant to it. thanks -- PMM
On 17/05/2018 12:48, Peter Maydell wrote: > So I had an idea this morning for a slightly different way to approach > this, which is that we add a concept of an iommu_idx, analogous to > our existing TCG mmu_idx. Basically an iommu_idx represents a > page table (kind of like what Arm calls a "translation regime"), so > that for any particular iommu index and input address the output > address and permissions are always the same. For the memory > protection controller there would be two iommu_idxes, one for > secure and one for non-secure. > > The API changes would be something like: > * new method get_num_indexes() which returns the number of iommu indexes > this IOMMU implements (default implementation: return 1) > * translate method takes an iommu index (and not the txattrs) > * new method index_from_attrs() which takes a MemTxAttrs and > returns the iommu index that should be used (default implementation: > always return 0) > * memory_region_register_iommu_notifier() takes an iommu index > * the default 'replay' method does "for each supported index, > for each address, call @translate" > * vfio and vhost can register their notifiers using the index > returned by index_from_attrs(MEMTXATTRS_UNSPECIFIED) > * maybe they could call get_num_indexes() and bail out if the > IOMMU has support for multiple indexes? > * maybe they could be enhanced to support tracking multiple > page tables if necessary in future > > I haven't worked through the details yet, but this seems to me > more flexible than working directly with txattrs. It also means > it's harder to accidentally write an iommu implementation that > looks at more fields in the txattrs than its notifier interface > claims are significant to it. Yes, this also sounds good. It does have the same issue for VFIO that get_num_indexes() would be called too late to fail (and again, in a place where it's hard to fail). Maybe the index count and the index-from/to-attrs translation should be static (index-to-attrs could use the same pair of MemTxAttrs for "which bits matter" and "what value should they have"), so that VFIO can inspect it and decide if it's okay to proceed with e.g. the first iommu_idx? Thanks, Paolo
On 17 May 2018 at 13:46, Paolo Bonzini <pbonzini@redhat.com> wrote: > Yes, this also sounds good. It does have the same issue for VFIO that > get_num_indexes() would be called too late to fail (and again, in a > place where it's hard to fail). > > Maybe the index count and the index-from/to-attrs translation should be > static (index-to-attrs could use the same pair of MemTxAttrs for "which > bits matter" and "what value should they have"), so that VFIO can > inspect it and decide if it's okay to proceed with e.g. the first iommu_idx? Well, they need to be static in the sense that the IOMMU can't change its opinion later about what it has. So as long as you have a pointer to the IOMMU you can ask it how many indexes it has. thanks -- PMM
This is an RFC patchset because it's a little bit unmotivated and only lightly tested, but in principle it could be committed, so half-RFC-half-not :-) The Arm SMMU wants to know if the transaction it is handling is secure/nonsecure and user/privileged, because the iommu page tables can be configured by the guest to only allow transactions which satisfy those criteria. At the moment Eric's implementation ignores all that, because we don't provide the IOMMUMemoryRegion translate function with any memory transaction attribute information. This patchset fixes that by plumbing through transaction attributes. Most of the patchset is just starting at the leaves of the calltree rooted at "flatview_do_translate()" and making callsites provide attributes where appropriate or plumbing through existing attribute information where it exists. General principles of when I made a caller pass MEMTXATTRS_UNSPECIFIED and when I had it take an attrs value from further up: * dma_memory_* functions all assume UNSPECIFIED (matching the read/write/rw functions that already do that) * cpu_physical_memory_* also all assume UNSPECIFIED, following the pattern of existing functions in that family * address_space_* take an attributes argument, by analogy with existing functions in that family * endpoints like target-specific code or vhost has to provide attributes, but for all the targets affected here they don't care about attributes and can use UNSPECIFIED As well as the SMMU, I'm also thinking about using the IOMMU infrastructure for the v8M Memory Protection Controller (though that is a bit trickier as I also need it to support TCG execution in an IOMMU-controlled region, which is an orthogonal bit of work to attribute support). Based-on: <20180430122404.10741-1-peter.maydell@linaro.org> ("memory.h: Improve IOMMU related documentation") but only for textual reasons. v1->v2 changes: only adding the patch 1/12 that I forgot to include in the v1 posting... thanks -- PMM Peter Maydell (12): Make tb_invalidate_phys_addr() take a MemTxAttrs argument Make address_space_translate() take a MemTxAttrs argument Make address_space_map() take a MemTxAttrs argument Make address_space_access_valid() take a MemTxAttrs argument Make flatview_extend_translation() take a MemTxAttrs argument Make memory_region_access_valid() take a MemTxAttrs argument Make MemoryRegion valid.accepts callback take a MemTxAttrs argument Make flatview_access_valid() take a MemTxAttrs argument Make flatview_translate() take a MemTxAttrs argument Make address_space_get_iotlb_entry() take a MemTxAttrs argument Make flatview_do_translate() take a MemTxAttrs argument Add MemTxAttrs argument to IOMMU translate function include/exec/exec-all.h | 5 ++- include/exec/memory-internal.h | 3 +- include/exec/memory.h | 26 ++++++++---- include/sysemu/dma.h | 6 ++- accel/tcg/translate-all.c | 4 +- exec.c | 75 ++++++++++++++++++++-------------- hw/alpha/typhoon.c | 3 +- hw/dma/rc4030.c | 3 +- hw/hppa/dino.c | 3 +- hw/i386/amd_iommu.c | 3 +- hw/i386/intel_iommu.c | 3 +- hw/nvram/fw_cfg.c | 12 ++++-- hw/ppc/spapr_iommu.c | 3 +- hw/s390x/s390-pci-bus.c | 3 +- hw/s390x/s390-pci-inst.c | 3 +- hw/scsi/esp.c | 3 +- hw/sparc/sun4m_iommu.c | 3 +- hw/sparc64/sun4u_iommu.c | 3 +- hw/vfio/common.c | 3 +- hw/virtio/vhost.c | 3 +- hw/xen/xen_pt_msi.c | 3 +- memory.c | 15 ++++--- memory_ldst.inc.c | 18 ++++---- target/ppc/mmu-hash64.c | 3 +- target/riscv/helper.c | 2 +- target/s390x/diag.c | 6 ++- target/s390x/excp_helper.c | 3 +- target/s390x/mmu_helper.c | 3 +- target/s390x/sigp.c | 3 +- target/xtensa/op_helper.c | 3 +- 30 files changed, 141 insertions(+), 88 deletions(-) -- 2.17.0