| Message ID | 20180605131944.14649-4-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | memory: enhance IOMMU notifier to support USER bit | expand |
On 5 June 2018 at 14:19, Peter Xu <peterx@redhat.com> wrote: > Add two more IOMMU notifier flags to selectively choose whether the > notifier would like to listen to an event that with USER bit set/unset. > Note that all existing notifiers should always been registered with both > of the flags set to make sure they'll receive the notification no matter > whether the USER bit is set or unset in the attributes. To simplify > this procedure, some new macros are defined. The old UNMAP-only > notifiers should now be registered with IOMMU_NOTIFIER_UNMAP_ALL > flags (rather than the old IOMMU_NOTIFIER_UNMAP flag), while the old > MAP+UNMAP case can keep to use the IOMMU_NOTIFIER_ALL flag. > > Now if a new notifier would like to register to only UNMAP notifications > with USER bit set, it should register with below flag: > > IOMMU_NOTIFIER_UNMAP | IOMMU_NOTIFIER_USER_SET > > Then when we want to notify a DMA invalidation (we call it IOMMUTLBEntry > in QEMU), we should do this: > > IOMMUTLBEntry entry; > ... (set up the fields) > entry.perm = IOMMU_NONE; > entry.attrs.user = 1; > memory_region_notify_iommu(mr, &entry); I don't think this works, because there is not a single unique MemTxAttrs for a particular invalidation event. Invalidates will affect classes of transaction attributes. For instance in the MPC, changing the lookup table can invalidate cached IOMMU information for any translation that was done either with attrs.secure = 1, or for translations with attrs.unspecified = 1. In general, at the point where the IOMMU calls notifiers it won't have a single MemTxAttrs it can use, so you don't want to put an attrs into the IOMMUTLBEntry. thanks -- PMM
On Tue, Jun 05, 2018 at 02:54:18PM +0100, Peter Maydell wrote: > On 5 June 2018 at 14:19, Peter Xu <peterx@redhat.com> wrote: > > Add two more IOMMU notifier flags to selectively choose whether the > > notifier would like to listen to an event that with USER bit set/unset. > > Note that all existing notifiers should always been registered with both > > of the flags set to make sure they'll receive the notification no matter > > whether the USER bit is set or unset in the attributes. To simplify > > this procedure, some new macros are defined. The old UNMAP-only > > notifiers should now be registered with IOMMU_NOTIFIER_UNMAP_ALL > > flags (rather than the old IOMMU_NOTIFIER_UNMAP flag), while the old > > MAP+UNMAP case can keep to use the IOMMU_NOTIFIER_ALL flag. > > > > Now if a new notifier would like to register to only UNMAP notifications > > with USER bit set, it should register with below flag: > > > > IOMMU_NOTIFIER_UNMAP | IOMMU_NOTIFIER_USER_SET > > > > Then when we want to notify a DMA invalidation (we call it IOMMUTLBEntry > > in QEMU), we should do this: > > > > IOMMUTLBEntry entry; > > ... (set up the fields) > > entry.perm = IOMMU_NONE; > > entry.attrs.user = 1; > > memory_region_notify_iommu(mr, &entry); > > I don't think this works, because there is not a single unique > MemTxAttrs for a particular invalidation event. Invalidates > will affect classes of transaction attributes. For instance > in the MPC, changing the lookup table can invalidate cached > IOMMU information for any translation that was done either > with attrs.secure = 1, or for translations with attrs.unspecified = 1. > > In general, at the point where the IOMMU calls notifiers it > won't have a single MemTxAttrs it can use, so you don't want > to put an attrs into the IOMMUTLBEntry. So we have a requirement that we want to send an invalidation for either (1) unspecified, or (2) secure=1. We can't do that with a single MemTxAttrs. Could we just notify twice? One with unspecified=1 and one with secure=1? Is this (reduce the calls to notify functions) the major reason we introduce this IOMMU index idea into the memory API (besides "avoid possible programming error" you mentioned in IRC discussion)? Again, I think the more critical question would be whether we can pass in MemTxAttrs into translate(), and whether we can avoid introducing this IOMMU index idea into the memory API layer... For example, would it add complexity to other architectures without much benefit? Thanks,
On 5 June 2018 at 15:26, Peter Xu <peterx@redhat.com> wrote: > So we have a requirement that we want to send an invalidation for > either (1) unspecified, or (2) secure=1. We can't do that with a > single MemTxAttrs. > > Could we just notify twice? One with unspecified=1 and one with > secure=1? Is this (reduce the calls to notify functions) the major > reason we introduce this IOMMU index idea into the memory API (besides > "avoid possible programming error" you mentioned in IRC discussion)? How would you handle "this changes mappings for txattrs where unspecified == 0 && secure == 0" ? How does the notifier registering API say what it's interested in? In the exec.c code that deals with sending TCG transactions through IOMMUs, if I have to register a notifier for every possible TCG transaction attribute I ever see, that's a lot of unnecessary notifiers. > Again, I think the more critical question would be whether we can pass > in MemTxAttrs into translate(), and whether we can avoid introducing > this IOMMU index idea into the memory API layer... For example, would > it add complexity to other architectures without much benefit? I think the fundamental difference in our points of view here is that I see the IOMMU index as *reducing* complexity, not adding it. Yes, it is an extra level of indirection, but I think it helps us express a useful concept. The patches you've sent here seem to me to be adding extra complexity to the notifier API and the core code which isn't required in the patch set that I sent. thanks -- PMM
On Tue, Jun 05, 2018 at 03:42:11PM +0100, Peter Maydell wrote: > On 5 June 2018 at 15:26, Peter Xu <peterx@redhat.com> wrote: > > So we have a requirement that we want to send an invalidation for > > either (1) unspecified, or (2) secure=1. We can't do that with a > > single MemTxAttrs. > > > > Could we just notify twice? One with unspecified=1 and one with > > secure=1? Is this (reduce the calls to notify functions) the major > > reason we introduce this IOMMU index idea into the memory API (besides > > "avoid possible programming error" you mentioned in IRC discussion)? > > How would you handle "this changes mappings for txattrs where > unspecified == 0 && secure == 0" ? Let's forget this patch. I was confused about the problem behind when I wrote this up... Now I know a bit more. It's related to how we can send a notification covers multiple contexts. Let's assume we still only support MAP and UNMAP in the notifiers. But let's assume IOMMUTLBEntry has MemTxAttrs field. If so, for "this changes mappings for txattrs where unspecified == 0 && secure == 0", we just send one notify with attrs.unspecified=0 and attrs.secure=0. Meanwhile in the notifier hook you can parse the attrs to see whether that's what you need, and skip if not. Will that work? > > How does the notifier registering API say what it's interested in? > In the exec.c code that deals with sending TCG transactions > through IOMMUs, if I have to register a notifier for every > possible TCG transaction attribute I ever see, that's a lot > of unnecessary notifiers. Again, let's assume this patch does not exist. Then only one notifier is needed to be registered with UNMAP notify, and in the hook function it can detect whether it has (attrs.unspecified==0 && attrs.unsecure==0). > > > Again, I think the more critical question would be whether we can pass > > in MemTxAttrs into translate(), and whether we can avoid introducing > > this IOMMU index idea into the memory API layer... For example, would > > it add complexity to other architectures without much benefit? > > I think the fundamental difference in our points of view here > is that I see the IOMMU index as *reducing* complexity, not > adding it. Yes, it is an extra level of indirection, but I > think it helps us express a useful concept. Yes. And again, I'm also worrying about what we should do when we want to pass requester ID into translate() too if we have this extra layer. > The patches you've > sent here seem to me to be adding extra complexity to the > notifier API and the core code which isn't required in > the patch set that I sent. Again I didn't really understand the problem behind before. Now I don't think it's important on whether we'll need to introduce new notifier flags, I wanted to know whether we can avoid introducing IOMMU index into memory API. Let's just ignore this patch. Sorry for that confusion. Regards,
On Wed, Jun 06, 2018 at 02:56:29PM +0800, Peter Xu wrote: [...] > > The patches you've > > sent here seem to me to be adding extra complexity to the > > notifier API and the core code which isn't required in > > the patch set that I sent. > > Again I didn't really understand the problem behind before. Now I > don't think it's important on whether we'll need to introduce new > notifier flags, I wanted to know whether we can avoid introducing > IOMMU index into memory API. Let's just ignore this patch. Sorry for > that confusion. For this part, I'd say of course this is based on the assumption that the IOMMU index might not help much with non-tcg use cases. If someone could help me understand better on how that could help in general to memory API then I would be much appreciated, since it really confused me even until today (and I can't see a good way at least for VT-d to leverage that idea). Thanks,
diff --git a/include/exec/memory.h b/include/exec/memory.h index 12865a4890..fb9a7059c6 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -82,18 +82,56 @@ struct IOMMUTLBEntry { }; /* - * Bitmap for different IOMMUNotifier capabilities. Each notifier can - * register with one or multiple IOMMU Notifier capability bit(s). + * Bitmap for different IOMMUNotifier capabilities. Please refer to + * comments for each notifier capability to know its usage. Note that, + * a notifier registered with (UNMAP | USER_SET) does not mean that + * it'll notify both MAP notifies and USER_SET notifies, instead it + * means it'll only be notified for the events that are UNMAP + * meanwhile with USER attribute set. */ typedef enum { IOMMU_NOTIFIER_NONE = 0, - /* Notify cache invalidations */ + /* + * When set, will notify deleted entries (cache invalidations). + * When unset, will not notify deleted entries. + */ IOMMU_NOTIFIER_UNMAP = 0x1, - /* Notify entry changes (newly created entries) */ + /* + * When set, will notify newly created entries. When unset, will + * not notify newly created entries. + */ IOMMU_NOTIFIER_MAP = 0x2, + /* + * When set, will notify when the USER bit is set in + * IOMMUTLBEntry.attrs. When unset, will not notify when the USER + * bit is set. + */ + IOMMU_NOTIFIER_USER_SET = 0x4, + /* + * When set, will notify when the USER bit is cleared in + * IOMMUTLBEntry.attrs. When unset, will not notify when the USER + * bit is cleared. + */ + IOMMU_NOTIFIER_USER_UNSET = 0x8, } IOMMUNotifierFlag; -#define IOMMU_NOTIFIER_ALL (IOMMU_NOTIFIER_MAP | IOMMU_NOTIFIER_UNMAP) +/* Use this when the notifier does not care about USER bit */ +#define IOMMU_NOTIFIER_USER_ALL \ + (IOMMU_NOTIFIER_USER_SET | IOMMU_NOTIFIER_USER_UNSET) + +/* Use this when the notifier does not care about any attribute */ +#define IOMMU_NOTIFIER_ATTRS_ALL \ + (IOMMU_NOTIFIER_USER_ALL) + +/* Use this to notify all UNMAP notifications */ +#define IOMMU_NOTIFIER_UNMAP_ALL \ + (IOMMU_NOTIFIER_UNMAP | IOMMU_NOTIFIER_ATTRS_ALL) + +/* Use this to notify all notifications */ +#define IOMMU_NOTIFIER_ALL ( \ + IOMMU_NOTIFIER_MAP | \ + IOMMU_NOTIFIER_UNMAP | \ + IOMMU_NOTIFIER_ATTRS_ALL) struct IOMMUNotifier; typedef void (*IOMMUNotify)(struct IOMMUNotifier *notifier, diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 96175b214d..da6efeadad 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -672,7 +672,7 @@ static void vhost_iommu_region_add(MemoryListener *listener, section->size); end = int128_sub(end, int128_one()); iommu_notifier_init(&iommu->n, vhost_iommu_unmap_notify, - IOMMU_NOTIFIER_UNMAP, + IOMMU_NOTIFIER_UNMAP_ALL, section->offset_within_region, int128_get64(end)); iommu->mr = section->mr; diff --git a/memory.c b/memory.c index 376f72b19c..9e4617df5a 100644 --- a/memory.c +++ b/memory.c @@ -1880,6 +1880,16 @@ void memory_region_notify_one(IOMMUNotifier *notifier, return; } + if (!(notifier->notifier_flags & IOMMU_NOTIFIER_USER_SET) && + entry->attrs.user) { + return; + } + + if (!(notifier->notifier_flags & IOMMU_NOTIFIER_USER_UNSET) && + !entry->attrs.user) { + return; + } + if (entry->perm & IOMMU_RW) { request_flags = IOMMU_NOTIFIER_MAP; } else {
Add two more IOMMU notifier flags to selectively choose whether the notifier would like to listen to an event that with USER bit set/unset. Note that all existing notifiers should always been registered with both of the flags set to make sure they'll receive the notification no matter whether the USER bit is set or unset in the attributes. To simplify this procedure, some new macros are defined. The old UNMAP-only notifiers should now be registered with IOMMU_NOTIFIER_UNMAP_ALL flags (rather than the old IOMMU_NOTIFIER_UNMAP flag), while the old MAP+UNMAP case can keep to use the IOMMU_NOTIFIER_ALL flag. Now if a new notifier would like to register to only UNMAP notifications with USER bit set, it should register with below flag: IOMMU_NOTIFIER_UNMAP | IOMMU_NOTIFIER_USER_SET Then when we want to notify a DMA invalidation (we call it IOMMUTLBEntry in QEMU), we should do this: IOMMUTLBEntry entry; ... (set up the fields) entry.perm = IOMMU_NONE; entry.attrs.user = 1; memory_region_notify_iommu(mr, &entry); Then only the notifiers registered with IOMMU_NOTIFIER_USER_SET will receive this notification. Signed-off-by: Peter Xu <peterx@redhat.com> --- include/exec/memory.h | 48 ++++++++++++++++++++++++++++++++++++++----- hw/virtio/vhost.c | 2 +- memory.c | 10 +++++++++ 3 files changed, 54 insertions(+), 6 deletions(-)