| Message ID | 20200901142608.24481-3-eperezma@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | memory: Delete assertion in memory_region_unregister_iommu_notifier | expand |
On Tue, Sep 01, 2020 at 04:26:05PM +0200, Eugenio Pérez wrote: > This way we can tell between regulars IOMMURLBEntries (entry of IOMMU > hardware) and notifications. s/regulars IOMMURLBEntries/regular IOMMUTLBEntry/ > > In the notifications, we set explicitly if it is a MAPs or an UNMAP, > instead of trusting in entry permissions to differenciate them. > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> [...] > struct IOMMUTLBEntry { > - AddressSpace *target_as; > - hwaddr iova; > - hwaddr translated_addr; > - hwaddr addr_mask; /* 0xfff = 4k translation */ > - IOMMUAccessFlags perm; > + AddressSpace *target_as; > + hwaddr iova; > + hwaddr translated_addr; > + hwaddr addr_mask; /* 0xfff = 4k translation */ > + IOMMUAccessFlags perm; > }; If these lines are identical, then we can avoid touching the spaces. With above changes, please feel free to add: Reviewed-by: Peter Xu <peterx@redhat.com>
Eugenio Pérez <eperezma@redhat.com> wrote: > This way we can tell between regulars IOMMURLBEntries (entry of IOMMU > hardware) and notifications. > > In the notifications, we set explicitly if it is a MAPs or an UNMAP, > instead of trusting in entry permissions to differenciate them. > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com> if you have to respin for whatever other reasons, two suggestions. > @@ -1430,10 +1435,10 @@ static int vtd_dev_to_context_entry(IntelIOMMUState *s, uint8_t bus_num, > return 0; > } > > -static int vtd_sync_shadow_page_hook(IOMMUTLBEntry *entry, > +static int vtd_sync_shadow_page_hook(IOMMUTLBEvent *event, > void *private) > { > - memory_region_notify_iommu((IOMMUMemoryRegion *)private, 0, *entry); > + memory_region_notify_iommu((IOMMUMemoryRegion *)private, 0, *event); I know that it already was there, but if you respin, you can remove the cast. > @@ -1281,12 +1286,12 @@ void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, > * notifies a specific notifier, not all of them. > * > * @notifier: the notifier to be notified > - * @entry: the new entry in the IOMMU translation table. The entry > - * replaces all old entries for the same virtual I/O address range. > - * Deleted entries have .@perm == 0. > + * @event: TLB event with the new entry in the IOMMU translation table. > + * The entry replaces all old entries for the same virtual I/O address > + * range. > */ > void memory_region_notify_iommu_one(IOMMUNotifier *notifier, > - IOMMUTLBEntry *entry); > + IOMMUTLBEvent *event); I didn't catch the missing of indentation on the previous patch .... O:-)
On Tue, Sep 1, 2020 at 10:55 PM Peter Xu <peterx@redhat.com> wrote: > > On Tue, Sep 01, 2020 at 04:26:05PM +0200, Eugenio Pérez wrote: > > This way we can tell between regulars IOMMURLBEntries (entry of IOMMU > > hardware) and notifications. > > s/regulars IOMMURLBEntries/regular IOMMUTLBEntry/ > > > > > In the notifications, we set explicitly if it is a MAPs or an UNMAP, > > instead of trusting in entry permissions to differenciate them. > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > [...] > > > struct IOMMUTLBEntry { > > - AddressSpace *target_as; > > - hwaddr iova; > > - hwaddr translated_addr; > > - hwaddr addr_mask; /* 0xfff = 4k translation */ > > - IOMMUAccessFlags perm; > > + AddressSpace *target_as; > > + hwaddr iova; > > + hwaddr translated_addr; > > + hwaddr addr_mask; /* 0xfff = 4k translation */ > > + IOMMUAccessFlags perm; > > }; > > If these lines are identical, then we can avoid touching the spaces. > Thanks for the catches! Didn't notice them rebasing. > With above changes, please feel free to add: > > Reviewed-by: Peter Xu <peterx@redhat.com> > > -- > Peter Xu >
Applying both, thanks! On Wed, Sep 2, 2020 at 9:54 AM Juan Quintela <quintela@redhat.com> wrote: > > Eugenio Pérez <eperezma@redhat.com> wrote: > > This way we can tell between regulars IOMMURLBEntries (entry of IOMMU > > hardware) and notifications. > > > > In the notifications, we set explicitly if it is a MAPs or an UNMAP, > > instead of trusting in entry permissions to differenciate them. > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > Reviewed-by: Juan Quintela <quintela@redhat.com> > > if you have to respin for whatever other reasons, two suggestions. > > > > @@ -1430,10 +1435,10 @@ static int vtd_dev_to_context_entry(IntelIOMMUState *s, uint8_t bus_num, > > return 0; > > } > > > > -static int vtd_sync_shadow_page_hook(IOMMUTLBEntry *entry, > > +static int vtd_sync_shadow_page_hook(IOMMUTLBEvent *event, > > void *private) > > { > > - memory_region_notify_iommu((IOMMUMemoryRegion *)private, 0, *entry); > > + memory_region_notify_iommu((IOMMUMemoryRegion *)private, 0, *event); > > I know that it already was there, but if you respin, you can remove the cast. > > > > > @@ -1281,12 +1286,12 @@ void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, > > * notifies a specific notifier, not all of them. > > * > > * @notifier: the notifier to be notified > > - * @entry: the new entry in the IOMMU translation table. The entry > > - * replaces all old entries for the same virtual I/O address range. > > - * Deleted entries have .@perm == 0. > > + * @event: TLB event with the new entry in the IOMMU translation table. > > + * The entry replaces all old entries for the same virtual I/O address > > + * range. > > */ > > void memory_region_notify_iommu_one(IOMMUNotifier *notifier, > > - IOMMUTLBEntry *entry); > > + IOMMUTLBEvent *event); > > I didn't catch the missing of indentation on the previous patch .... > O:-) >
Hi Eugenio, On 9/1/20 4:26 PM, Eugenio Pérez wrote: > This way we can tell between regulars IOMMURLBEntries (entry of IOMMU > hardware) and notifications.> > In the notifications, we set explicitly if it is a MAPs or an UNMAP, > instead of trusting in entry permissions to differenciate them. differentiate Is it the real justification or is the rationale behind adding this type to be able to add new types of events? > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > --- > hw/arm/smmu-common.c | 13 ++++--- > hw/arm/smmuv3.c | 13 ++++--- > hw/i386/intel_iommu.c | 88 ++++++++++++++++++++++++------------------- > hw/misc/tz-mpc.c | 32 +++++++++------- > hw/ppc/spapr_iommu.c | 15 ++++---- > include/exec/memory.h | 31 ++++++++------- > softmmu/memory.c | 16 +++----- > 7 files changed, 112 insertions(+), 96 deletions(-) you may add "orderFile = scripts/git.orderfile" in your .git/config diff section to get headers first [diff] orderFile = scripts/git.orderfile > > diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c > index 88d2c454f0..405d5c5325 100644 > --- a/hw/arm/smmu-common.c > +++ b/hw/arm/smmu-common.c > @@ -465,14 +465,15 @@ IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid) > /* Unmap the whole notifier's range */ > static void smmu_unmap_notifier_range(IOMMUNotifier *n) > { > - IOMMUTLBEntry entry; > + IOMMUTLBEvent event; > > - entry.target_as = &address_space_memory; > - entry.iova = n->start; > - entry.perm = IOMMU_NONE; > - entry.addr_mask = n->end - n->start; > + event.type = IOMMU_NOTIFIER_UNMAP; > + event.entry.target_as = &address_space_memory; > + event.entry.iova = n->start; > + event.entry.perm = IOMMU_NONE; > + event.entry.addr_mask = n->end - n->start; > > - memory_region_notify_iommu_one(n, &entry); > + memory_region_notify_iommu_one(n, &event); > } > > /* Unmap all notifiers attached to @mr */ > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c > index 0a893ae918..62b0e289ca 100644 > --- a/hw/arm/smmuv3.c > +++ b/hw/arm/smmuv3.c > @@ -799,7 +799,7 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr, > uint8_t tg, uint64_t num_pages) > { > SMMUDevice *sdev = container_of(mr, SMMUDevice, iommu); > - IOMMUTLBEntry entry; > + IOMMUTLBEvent event; > uint8_t granule = tg; > > if (!tg) { > @@ -822,12 +822,13 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr, > granule = tt->granule_sz; > } > > - entry.target_as = &address_space_memory; > - entry.iova = iova; > - entry.addr_mask = num_pages * (1 << granule) - 1; > - entry.perm = IOMMU_NONE; > + event.type = IOMMU_NOTIFIER_UNMAP; > + event.entry.target_as = &address_space_memory; > + event.entry.iova = iova; > + event.entry.addr_mask = num_pages * (1 << granule) - 1; > + event.entry.perm = IOMMU_NONE; > > - memory_region_notify_iommu_one(n, &entry); > + memory_region_notify_iommu_one(n, &event); > } > > /* invalidate an asid/iova range tuple in all mr's */ > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index 2ad6b9d796..0c4aef5cb5 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -1073,7 +1073,7 @@ static int vtd_iova_to_slpte(IntelIOMMUState *s, VTDContextEntry *ce, > } > } > > -typedef int (*vtd_page_walk_hook)(IOMMUTLBEntry *entry, void *private); > +typedef int (*vtd_page_walk_hook)(IOMMUTLBEvent *event, void *private); > > /** > * Constant information used during page walking > @@ -1094,11 +1094,12 @@ typedef struct { > uint16_t domain_id; > } vtd_page_walk_info; > > -static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) > +static int vtd_page_walk_one(IOMMUTLBEvent *event, vtd_page_walk_info *info) > { > VTDAddressSpace *as = info->as; > vtd_page_walk_hook hook_fn = info->hook_fn; > void *private = info->private; > + IOMMUTLBEntry *entry = &event->entry; > DMAMap target = { > .iova = entry->iova, > .size = entry->addr_mask, > @@ -1107,7 +1108,7 @@ static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) > }; > DMAMap *mapped = iova_tree_find(as->iova_tree, &target); > > - if (entry->perm == IOMMU_NONE && !info->notify_unmap) { > + if (event->type == IOMMU_NOTIFIER_UNMAP && !info->notify_unmap) { > trace_vtd_page_walk_one_skip_unmap(entry->iova, entry->addr_mask); > return 0; > } > @@ -1115,7 +1116,7 @@ static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) > assert(hook_fn); > > /* Update local IOVA mapped ranges */ > - if (entry->perm) { > + if (event->type == IOMMU_NOTIFIER_MAP) { > if (mapped) { > /* If it's exactly the same translation, skip */ > if (!memcmp(mapped, &target, sizeof(target))) { > @@ -1141,19 +1142,21 @@ static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) > int ret; > > /* Emulate an UNMAP */ > + event->type = IOMMU_NOTIFIER_UNMAP; > entry->perm = IOMMU_NONE; > trace_vtd_page_walk_one(info->domain_id, > entry->iova, > entry->translated_addr, > entry->addr_mask, > entry->perm); > - ret = hook_fn(entry, private); > + ret = hook_fn(event, private); > if (ret) { > return ret; > } > /* Drop any existing mapping */ > iova_tree_remove(as->iova_tree, &target); > - /* Recover the correct permission */ > + /* Recover the correct type */ > + event->type = IOMMU_NOTIFIER_MAP; > entry->perm = cache_perm; > } > } > @@ -1170,7 +1173,7 @@ static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) > trace_vtd_page_walk_one(info->domain_id, entry->iova, > entry->translated_addr, entry->addr_mask, > entry->perm); > - return hook_fn(entry, private); > + return hook_fn(event, private); > } > > /** > @@ -1191,7 +1194,7 @@ static int vtd_page_walk_level(dma_addr_t addr, uint64_t start, > uint32_t offset; > uint64_t slpte; > uint64_t subpage_size, subpage_mask; > - IOMMUTLBEntry entry; > + IOMMUTLBEvent event; > uint64_t iova = start; > uint64_t iova_next; > int ret = 0; > @@ -1245,13 +1248,15 @@ static int vtd_page_walk_level(dma_addr_t addr, uint64_t start, > * > * In either case, we send an IOTLB notification down. > */ > - entry.target_as = &address_space_memory; > - entry.iova = iova & subpage_mask; > - entry.perm = IOMMU_ACCESS_FLAG(read_cur, write_cur); > - entry.addr_mask = ~subpage_mask; > + event.entry.target_as = &address_space_memory; > + event.entry.iova = iova & subpage_mask; > + event.entry.perm = IOMMU_ACCESS_FLAG(read_cur, write_cur); > + event.entry.addr_mask = ~subpage_mask; > /* NOTE: this is only meaningful if entry_valid == true */ > - entry.translated_addr = vtd_get_slpte_addr(slpte, info->aw); > - ret = vtd_page_walk_one(&entry, info); > + event.entry.translated_addr = vtd_get_slpte_addr(slpte, info->aw); > + event.type = event.entry.perm ? IOMMU_NOTIFIER_MAP : > + IOMMU_NOTIFIER_UNMAP; > + ret = vtd_page_walk_one(&event, info); > } > > if (ret < 0) { > @@ -1430,10 +1435,10 @@ static int vtd_dev_to_context_entry(IntelIOMMUState *s, uint8_t bus_num, > return 0; > } > > -static int vtd_sync_shadow_page_hook(IOMMUTLBEntry *entry, > +static int vtd_sync_shadow_page_hook(IOMMUTLBEvent *event, > void *private) > { > - memory_region_notify_iommu((IOMMUMemoryRegion *)private, 0, *entry); > + memory_region_notify_iommu((IOMMUMemoryRegion *)private, 0, *event); > return 0; > } > > @@ -1993,14 +1998,17 @@ static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s, > * page tables. We just deliver the PSI down to > * invalidate caches. > */ > - IOMMUTLBEntry entry = { > - .target_as = &address_space_memory, > - .iova = addr, > - .translated_addr = 0, > - .addr_mask = size - 1, > - .perm = IOMMU_NONE, > + IOMMUTLBEvent event = { > + .type = IOMMU_NOTIFIER_UNMAP, > + .entry = { > + .target_as = &address_space_memory, > + .iova = addr, > + .translated_addr = 0, > + .addr_mask = size - 1, > + .perm = IOMMU_NONE, > + }, > }; > - memory_region_notify_iommu(&vtd_as->iommu, 0, entry); > + memory_region_notify_iommu(&vtd_as->iommu, 0, event); > } > } > } > @@ -2412,7 +2420,7 @@ static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s, > VTDInvDesc *inv_desc) > { > VTDAddressSpace *vtd_dev_as; > - IOMMUTLBEntry entry; > + IOMMUTLBEvent event; > struct VTDBus *vtd_bus; > hwaddr addr; > uint64_t sz; > @@ -2460,12 +2468,13 @@ static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s, > sz = VTD_PAGE_SIZE; > } > > - entry.target_as = &vtd_dev_as->as; > - entry.addr_mask = sz - 1; > - entry.iova = addr; > - entry.perm = IOMMU_NONE; > - entry.translated_addr = 0; > - memory_region_notify_iommu(&vtd_dev_as->iommu, 0, entry); > + event.type = IOMMU_NOTIFIER_UNMAP; > + event.entry.target_as = &vtd_dev_as->as; > + event.entry.addr_mask = sz - 1; > + event.entry.iova = addr; > + event.entry.perm = IOMMU_NONE; > + event.entry.translated_addr = 0; > + memory_region_notify_iommu(&vtd_dev_as->iommu, 0, event); > > done: > return true; > @@ -3486,19 +3495,20 @@ static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n) > size = remain = end - start + 1; > > while (remain >= VTD_PAGE_SIZE) { > - IOMMUTLBEntry entry; > + IOMMUTLBEvent event; > uint64_t mask = get_naturally_aligned_size(start, remain, s->aw_bits); > > assert(mask); > > - entry.iova = start; > - entry.addr_mask = mask - 1; > - entry.target_as = &address_space_memory; > - entry.perm = IOMMU_NONE; > + event.type = IOMMU_NOTIFIER_UNMAP; > + event.entry.iova = start; > + event.entry.addr_mask = mask - 1; > + event.entry.target_as = &address_space_memory; > + event.entry.perm = IOMMU_NONE; > /* This field is meaningless for unmap */ > - entry.translated_addr = 0; > + event.entry.translated_addr = 0; > > - memory_region_notify_iommu_one(n, &entry); > + memory_region_notify_iommu_one(n, &event); > > start += mask; > remain -= mask; > @@ -3534,9 +3544,9 @@ static void vtd_address_space_refresh_all(IntelIOMMUState *s) > vtd_switch_address_space_all(s); > } > > -static int vtd_replay_hook(IOMMUTLBEntry *entry, void *private) > +static int vtd_replay_hook(IOMMUTLBEvent *event, void *private) > { > - memory_region_notify_iommu_one((IOMMUNotifier *)private, entry); > + memory_region_notify_iommu_one((IOMMUNotifier *)private, event); > return 0; > } > > diff --git a/hw/misc/tz-mpc.c b/hw/misc/tz-mpc.c > index 98f151237f..30481e1c90 100644 > --- a/hw/misc/tz-mpc.c > +++ b/hw/misc/tz-mpc.c > @@ -82,8 +82,10 @@ static void tz_mpc_iommu_notify(TZMPC *s, uint32_t lutidx, > /* Called when the LUT word at lutidx has changed from oldlut to newlut; > * must call the IOMMU notifiers for the changed blocks. > */ > - IOMMUTLBEntry entry = { > - .addr_mask = s->blocksize - 1, > + IOMMUTLBEvent event = { > + .entry = { > + .addr_mask = s->blocksize - 1, > + } > }; > hwaddr addr = lutidx * s->blocksize * 32; > int i; > @@ -100,26 +102,28 @@ static void tz_mpc_iommu_notify(TZMPC *s, uint32_t lutidx, > block_is_ns = newlut & (1 << i); > > trace_tz_mpc_iommu_notify(addr); > - entry.iova = addr; > - entry.translated_addr = addr; > + event.entry.iova = addr; > + event.entry.translated_addr = addr; > > - entry.perm = IOMMU_NONE; > - memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, entry); > - memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, entry); > + event.type = IOMMU_NOTIFIER_UNMAP; > + event.entry.perm = IOMMU_NONE; > + memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, event); > + memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, event); > > - entry.perm = IOMMU_RW; > + event.type = IOMMU_NOTIFIER_MAP; > + event.entry.perm = IOMMU_RW; > if (block_is_ns) { > - entry.target_as = &s->blocked_io_as; > + event.entry.target_as = &s->blocked_io_as; > } else { > - entry.target_as = &s->downstream_as; > + event.entry.target_as = &s->downstream_as; > } > - memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, entry); > + memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, event); > if (block_is_ns) { > - entry.target_as = &s->downstream_as; > + event.entry.target_as = &s->downstream_as; > } else { > - entry.target_as = &s->blocked_io_as; > + event.entry.target_as = &s->blocked_io_as; > } > - memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, entry); > + memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, event); > } > } > > diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c > index 0fecabc135..8bc3cff05d 100644 > --- a/hw/ppc/spapr_iommu.c > +++ b/hw/ppc/spapr_iommu.c > @@ -445,7 +445,7 @@ static void spapr_tce_reset(DeviceState *dev) > static target_ulong put_tce_emu(SpaprTceTable *tcet, target_ulong ioba, > target_ulong tce) > { > - IOMMUTLBEntry entry; > + IOMMUTLBEvent event; > hwaddr page_mask = IOMMU_PAGE_MASK(tcet->page_shift); > unsigned long index = (ioba - tcet->bus_offset) >> tcet->page_shift; > > @@ -457,12 +457,13 @@ static target_ulong put_tce_emu(SpaprTceTable *tcet, target_ulong ioba, > > tcet->table[index] = tce; > > - entry.target_as = &address_space_memory, > - entry.iova = (ioba - tcet->bus_offset) & page_mask; > - entry.translated_addr = tce & page_mask; > - entry.addr_mask = ~page_mask; > - entry.perm = spapr_tce_iommu_access_flags(tce); > - memory_region_notify_iommu(&tcet->iommu, 0, entry); > + event.entry.target_as = &address_space_memory, > + event.entry.iova = (ioba - tcet->bus_offset) & page_mask; > + event.entry.translated_addr = tce & page_mask; > + event.entry.addr_mask = ~page_mask; > + event.entry.perm = spapr_tce_iommu_access_flags(tce); > + event.type = entry.perm ? IOMMU_NOTIFIER_MAP : IOMMU_NOTIFIER_UNMAP; > + memory_region_notify_iommu(&tcet->iommu, 0, event); > > return H_SUCCESS; > } > diff --git a/include/exec/memory.h b/include/exec/memory.h > index 22c5f564d1..8a56707169 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -70,11 +70,11 @@ typedef enum { > #define IOMMU_ACCESS_FLAG(r, w) (((r) ? IOMMU_RO : 0) | ((w) ? IOMMU_WO : 0)) > > struct IOMMUTLBEntry { > - AddressSpace *target_as; > - hwaddr iova; > - hwaddr translated_addr; > - hwaddr addr_mask; /* 0xfff = 4k translation */ > - IOMMUAccessFlags perm; > + AddressSpace *target_as; > + hwaddr iova; > + hwaddr translated_addr; > + hwaddr addr_mask; /* 0xfff = 4k translation */ > + IOMMUAccessFlags perm; > }; > > /* > @@ -106,6 +106,11 @@ struct IOMMUNotifier { > }; > typedef struct IOMMUNotifier IOMMUNotifier; > > +typedef struct IOMMUTLBEvent { > + IOMMUTLBEntry entry; > + IOMMUNotifierFlag type; nit: I would put the type field before the entry > +} IOMMUTLBEvent; > + > /* RAM is pre-allocated and passed into qemu_ram_alloc_from_ptr */ > #define RAM_PREALLOC (1 << 0) > > @@ -1265,13 +1270,13 @@ uint64_t memory_region_iommu_get_min_page_size(IOMMUMemoryRegion *iommu_mr); > * Above there is: * The notification type will be decided by entry.perm bits: * * - For UNMAP (cache invalidation) notifies: set entry.perm to IOMMU_NONE. * - For MAP (newly added entry) notifies: set entry.perm to the * permission of the page (which is definitely !IOMMU_NONE). I think this should be updated as the main selector now is the type. I am a little bit concerned by the fact nothing really checks perm = IOMMU_NONE is consistent with UNMAP type. New call sites may forget this? shouldn't we check this in memory_region_notify_(one)_iommu? > * @iommu_mr: the memory region that was changed > * @iommu_idx: the IOMMU index for the translation table which has changed > - * @entry: the new entry in the IOMMU translation table. The entry > - * replaces all old entries for the same virtual I/O address range. > - * Deleted entries have .@perm == 0. > + * @event: TLB event with the new entry in the IOMMU translation table. > + * The entry replaces all old entries for the same virtual I/O address > + * range. > */ > void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, > int iommu_idx, > - IOMMUTLBEntry entry); > + IOMMUTLBEvent event); > > /** > * memory_region_notify_iommu_one: notify a change in an IOMMU translation > @@ -1281,12 +1286,12 @@ void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, > * notifies a specific notifier, not all of them. > * > * @notifier: the notifier to be notified > - * @entry: the new entry in the IOMMU translation table. The entry > - * replaces all old entries for the same virtual I/O address range. > - * Deleted entries have .@perm == 0. > + * @event: TLB event with the new entry in the IOMMU translation table. > + * The entry replaces all old entries for the same virtual I/O address > + * range. > */ > void memory_region_notify_iommu_one(IOMMUNotifier *notifier, > - IOMMUTLBEntry *entry); > + IOMMUTLBEvent *event); > > /** > * memory_region_register_iommu_notifier: register a notifier for changes to > diff --git a/softmmu/memory.c b/softmmu/memory.c > index 961c25b42f..09b3443eac 100644 > --- a/softmmu/memory.c > +++ b/softmmu/memory.c > @@ -1891,9 +1891,9 @@ void memory_region_unregister_iommu_notifier(MemoryRegion *mr, > } > > void memory_region_notify_iommu_one(IOMMUNotifier *notifier, > - IOMMUTLBEntry *entry) > + IOMMUTLBEvent *event) > { > - IOMMUNotifierFlag request_flags; > + IOMMUTLBEntry *entry = &event->entry; > hwaddr entry_end = entry->iova + entry->addr_mask; > > /* > @@ -1906,20 +1906,14 @@ void memory_region_notify_iommu_one(IOMMUNotifier *notifier, > > assert(entry->iova >= notifier->start && entry_end <= notifier->end); > > - if (entry->perm & IOMMU_RW) { > - request_flags = IOMMU_NOTIFIER_MAP; > - } else { > - request_flags = IOMMU_NOTIFIER_UNMAP; > - } > - > - if (notifier->notifier_flags & request_flags) { > + if (event->type & notifier->notifier_flags) { > notifier->notify(notifier, entry); > } > } > > void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, > int iommu_idx, > - IOMMUTLBEntry entry) > + IOMMUTLBEvent event) > { > IOMMUNotifier *iommu_notifier; > > @@ -1927,7 +1921,7 @@ void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, > > IOMMU_NOTIFIER_FOREACH(iommu_notifier, iommu_mr) { > if (iommu_notifier->iommu_idx == iommu_idx) { > - memory_region_notify_iommu_one(iommu_notifier, &entry); > + memory_region_notify_iommu_one(iommu_notifier, &event); > } > } > } > Thanks Eric
On Wed, Sep 2, 2020 at 12:17 PM Auger Eric <eric.auger@redhat.com> wrote: > > Hi Eugenio, > > On 9/1/20 4:26 PM, Eugenio Pérez wrote: > > This way we can tell between regulars IOMMURLBEntries (entry of IOMMU > > hardware) and notifications.> > > In the notifications, we set explicitly if it is a MAPs or an UNMAP, > > instead of trusting in entry permissions to differenciate them. > differentiate > > Is it the real justification or is the rationale behind adding this type > to be able to add new types of events? Hi Eric, Actually, the flag is to be able to tell the difference between pre-existing events that were mixed. No new type of event involved, just making it explicit. > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > --- > > hw/arm/smmu-common.c | 13 ++++--- > > hw/arm/smmuv3.c | 13 ++++--- > > hw/i386/intel_iommu.c | 88 ++++++++++++++++++++++++------------------- > > hw/misc/tz-mpc.c | 32 +++++++++------- > > hw/ppc/spapr_iommu.c | 15 ++++---- > > include/exec/memory.h | 31 ++++++++------- > > softmmu/memory.c | 16 +++----- > > 7 files changed, 112 insertions(+), 96 deletions(-) > > you may add "orderFile = scripts/git.orderfile" in your .git/config diff > section to get headers first > > [diff] > orderFile = scripts/git.orderfile > Didn't realize that, thanks! > > > > diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c > > index 88d2c454f0..405d5c5325 100644 > > --- a/hw/arm/smmu-common.c > > +++ b/hw/arm/smmu-common.c > > @@ -465,14 +465,15 @@ IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid) > > /* Unmap the whole notifier's range */ > > static void smmu_unmap_notifier_range(IOMMUNotifier *n) > > { > > - IOMMUTLBEntry entry; > > + IOMMUTLBEvent event; > > > > - entry.target_as = &address_space_memory; > > - entry.iova = n->start; > > - entry.perm = IOMMU_NONE; > > - entry.addr_mask = n->end - n->start; > > + event.type = IOMMU_NOTIFIER_UNMAP; > > + event.entry.target_as = &address_space_memory; > > + event.entry.iova = n->start; > > + event.entry.perm = IOMMU_NONE; > > + event.entry.addr_mask = n->end - n->start; > > > > - memory_region_notify_iommu_one(n, &entry); > > + memory_region_notify_iommu_one(n, &event); > > } > > > > /* Unmap all notifiers attached to @mr */ > > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c > > index 0a893ae918..62b0e289ca 100644 > > --- a/hw/arm/smmuv3.c > > +++ b/hw/arm/smmuv3.c > > @@ -799,7 +799,7 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr, > > uint8_t tg, uint64_t num_pages) > > { > > SMMUDevice *sdev = container_of(mr, SMMUDevice, iommu); > > - IOMMUTLBEntry entry; > > + IOMMUTLBEvent event; > > uint8_t granule = tg; > > > > if (!tg) { > > @@ -822,12 +822,13 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr, > > granule = tt->granule_sz; > > } > > > > - entry.target_as = &address_space_memory; > > - entry.iova = iova; > > - entry.addr_mask = num_pages * (1 << granule) - 1; > > - entry.perm = IOMMU_NONE; > > + event.type = IOMMU_NOTIFIER_UNMAP; > > + event.entry.target_as = &address_space_memory; > > + event.entry.iova = iova; > > + event.entry.addr_mask = num_pages * (1 << granule) - 1; > > + event.entry.perm = IOMMU_NONE; > > > > - memory_region_notify_iommu_one(n, &entry); > > + memory_region_notify_iommu_one(n, &event); > > } > > > > /* invalidate an asid/iova range tuple in all mr's */ > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > index 2ad6b9d796..0c4aef5cb5 100644 > > --- a/hw/i386/intel_iommu.c > > +++ b/hw/i386/intel_iommu.c > > @@ -1073,7 +1073,7 @@ static int vtd_iova_to_slpte(IntelIOMMUState *s, VTDContextEntry *ce, > > } > > } > > > > -typedef int (*vtd_page_walk_hook)(IOMMUTLBEntry *entry, void *private); > > +typedef int (*vtd_page_walk_hook)(IOMMUTLBEvent *event, void *private); > > > > /** > > * Constant information used during page walking > > @@ -1094,11 +1094,12 @@ typedef struct { > > uint16_t domain_id; > > } vtd_page_walk_info; > > > > -static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) > > +static int vtd_page_walk_one(IOMMUTLBEvent *event, vtd_page_walk_info *info) > > { > > VTDAddressSpace *as = info->as; > > vtd_page_walk_hook hook_fn = info->hook_fn; > > void *private = info->private; > > + IOMMUTLBEntry *entry = &event->entry; > > DMAMap target = { > > .iova = entry->iova, > > .size = entry->addr_mask, > > @@ -1107,7 +1108,7 @@ static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) > > }; > > DMAMap *mapped = iova_tree_find(as->iova_tree, &target); > > > > - if (entry->perm == IOMMU_NONE && !info->notify_unmap) { > > + if (event->type == IOMMU_NOTIFIER_UNMAP && !info->notify_unmap) { > > trace_vtd_page_walk_one_skip_unmap(entry->iova, entry->addr_mask); > > return 0; > > } > > @@ -1115,7 +1116,7 @@ static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) > > assert(hook_fn); > > > > /* Update local IOVA mapped ranges */ > > - if (entry->perm) { > > + if (event->type == IOMMU_NOTIFIER_MAP) { > > if (mapped) { > > /* If it's exactly the same translation, skip */ > > if (!memcmp(mapped, &target, sizeof(target))) { > > @@ -1141,19 +1142,21 @@ static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) > > int ret; > > > > /* Emulate an UNMAP */ > > + event->type = IOMMU_NOTIFIER_UNMAP; > > entry->perm = IOMMU_NONE; > > trace_vtd_page_walk_one(info->domain_id, > > entry->iova, > > entry->translated_addr, > > entry->addr_mask, > > entry->perm); > > - ret = hook_fn(entry, private); > > + ret = hook_fn(event, private); > > if (ret) { > > return ret; > > } > > /* Drop any existing mapping */ > > iova_tree_remove(as->iova_tree, &target); > > - /* Recover the correct permission */ > > + /* Recover the correct type */ > > + event->type = IOMMU_NOTIFIER_MAP; > > entry->perm = cache_perm; > > } > > } > > @@ -1170,7 +1173,7 @@ static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) > > trace_vtd_page_walk_one(info->domain_id, entry->iova, > > entry->translated_addr, entry->addr_mask, > > entry->perm); > > - return hook_fn(entry, private); > > + return hook_fn(event, private); > > } > > > > /** > > @@ -1191,7 +1194,7 @@ static int vtd_page_walk_level(dma_addr_t addr, uint64_t start, > > uint32_t offset; > > uint64_t slpte; > > uint64_t subpage_size, subpage_mask; > > - IOMMUTLBEntry entry; > > + IOMMUTLBEvent event; > > uint64_t iova = start; > > uint64_t iova_next; > > int ret = 0; > > @@ -1245,13 +1248,15 @@ static int vtd_page_walk_level(dma_addr_t addr, uint64_t start, > > * > > * In either case, we send an IOTLB notification down. > > */ > > - entry.target_as = &address_space_memory; > > - entry.iova = iova & subpage_mask; > > - entry.perm = IOMMU_ACCESS_FLAG(read_cur, write_cur); > > - entry.addr_mask = ~subpage_mask; > > + event.entry.target_as = &address_space_memory; > > + event.entry.iova = iova & subpage_mask; > > + event.entry.perm = IOMMU_ACCESS_FLAG(read_cur, write_cur); > > + event.entry.addr_mask = ~subpage_mask; > > /* NOTE: this is only meaningful if entry_valid == true */ > > - entry.translated_addr = vtd_get_slpte_addr(slpte, info->aw); > > - ret = vtd_page_walk_one(&entry, info); > > + event.entry.translated_addr = vtd_get_slpte_addr(slpte, info->aw); > > + event.type = event.entry.perm ? IOMMU_NOTIFIER_MAP : > > + IOMMU_NOTIFIER_UNMAP; > > + ret = vtd_page_walk_one(&event, info); > > } > > > > if (ret < 0) { > > @@ -1430,10 +1435,10 @@ static int vtd_dev_to_context_entry(IntelIOMMUState *s, uint8_t bus_num, > > return 0; > > } > > > > -static int vtd_sync_shadow_page_hook(IOMMUTLBEntry *entry, > > +static int vtd_sync_shadow_page_hook(IOMMUTLBEvent *event, > > void *private) > > { > > - memory_region_notify_iommu((IOMMUMemoryRegion *)private, 0, *entry); > > + memory_region_notify_iommu((IOMMUMemoryRegion *)private, 0, *event); > > return 0; > > } > > > > @@ -1993,14 +1998,17 @@ static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s, > > * page tables. We just deliver the PSI down to > > * invalidate caches. > > */ > > - IOMMUTLBEntry entry = { > > - .target_as = &address_space_memory, > > - .iova = addr, > > - .translated_addr = 0, > > - .addr_mask = size - 1, > > - .perm = IOMMU_NONE, > > + IOMMUTLBEvent event = { > > + .type = IOMMU_NOTIFIER_UNMAP, > > + .entry = { > > + .target_as = &address_space_memory, > > + .iova = addr, > > + .translated_addr = 0, > > + .addr_mask = size - 1, > > + .perm = IOMMU_NONE, > > + }, > > }; > > - memory_region_notify_iommu(&vtd_as->iommu, 0, entry); > > + memory_region_notify_iommu(&vtd_as->iommu, 0, event); > > } > > } > > } > > @@ -2412,7 +2420,7 @@ static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s, > > VTDInvDesc *inv_desc) > > { > > VTDAddressSpace *vtd_dev_as; > > - IOMMUTLBEntry entry; > > + IOMMUTLBEvent event; > > struct VTDBus *vtd_bus; > > hwaddr addr; > > uint64_t sz; > > @@ -2460,12 +2468,13 @@ static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s, > > sz = VTD_PAGE_SIZE; > > } > > > > - entry.target_as = &vtd_dev_as->as; > > - entry.addr_mask = sz - 1; > > - entry.iova = addr; > > - entry.perm = IOMMU_NONE; > > - entry.translated_addr = 0; > > - memory_region_notify_iommu(&vtd_dev_as->iommu, 0, entry); > > + event.type = IOMMU_NOTIFIER_UNMAP; > > + event.entry.target_as = &vtd_dev_as->as; > > + event.entry.addr_mask = sz - 1; > > + event.entry.iova = addr; > > + event.entry.perm = IOMMU_NONE; > > + event.entry.translated_addr = 0; > > + memory_region_notify_iommu(&vtd_dev_as->iommu, 0, event); > > > > done: > > return true; > > @@ -3486,19 +3495,20 @@ static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n) > > size = remain = end - start + 1; > > > > while (remain >= VTD_PAGE_SIZE) { > > - IOMMUTLBEntry entry; > > + IOMMUTLBEvent event; > > uint64_t mask = get_naturally_aligned_size(start, remain, s->aw_bits); > > > > assert(mask); > > > > - entry.iova = start; > > - entry.addr_mask = mask - 1; > > - entry.target_as = &address_space_memory; > > - entry.perm = IOMMU_NONE; > > + event.type = IOMMU_NOTIFIER_UNMAP; > > + event.entry.iova = start; > > + event.entry.addr_mask = mask - 1; > > + event.entry.target_as = &address_space_memory; > > + event.entry.perm = IOMMU_NONE; > > /* This field is meaningless for unmap */ > > - entry.translated_addr = 0; > > + event.entry.translated_addr = 0; > > > > - memory_region_notify_iommu_one(n, &entry); > > + memory_region_notify_iommu_one(n, &event); > > > > start += mask; > > remain -= mask; > > @@ -3534,9 +3544,9 @@ static void vtd_address_space_refresh_all(IntelIOMMUState *s) > > vtd_switch_address_space_all(s); > > } > > > > -static int vtd_replay_hook(IOMMUTLBEntry *entry, void *private) > > +static int vtd_replay_hook(IOMMUTLBEvent *event, void *private) > > { > > - memory_region_notify_iommu_one((IOMMUNotifier *)private, entry); > > + memory_region_notify_iommu_one((IOMMUNotifier *)private, event); > > return 0; > > } > > > > diff --git a/hw/misc/tz-mpc.c b/hw/misc/tz-mpc.c > > index 98f151237f..30481e1c90 100644 > > --- a/hw/misc/tz-mpc.c > > +++ b/hw/misc/tz-mpc.c > > @@ -82,8 +82,10 @@ static void tz_mpc_iommu_notify(TZMPC *s, uint32_t lutidx, > > /* Called when the LUT word at lutidx has changed from oldlut to newlut; > > * must call the IOMMU notifiers for the changed blocks. > > */ > > - IOMMUTLBEntry entry = { > > - .addr_mask = s->blocksize - 1, > > + IOMMUTLBEvent event = { > > + .entry = { > > + .addr_mask = s->blocksize - 1, > > + } > > }; > > hwaddr addr = lutidx * s->blocksize * 32; > > int i; > > @@ -100,26 +102,28 @@ static void tz_mpc_iommu_notify(TZMPC *s, uint32_t lutidx, > > block_is_ns = newlut & (1 << i); > > > > trace_tz_mpc_iommu_notify(addr); > > - entry.iova = addr; > > - entry.translated_addr = addr; > > + event.entry.iova = addr; > > + event.entry.translated_addr = addr; > > > > - entry.perm = IOMMU_NONE; > > - memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, entry); > > - memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, entry); > > + event.type = IOMMU_NOTIFIER_UNMAP; > > + event.entry.perm = IOMMU_NONE; > > + memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, event); > > + memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, event); > > > > - entry.perm = IOMMU_RW; > > + event.type = IOMMU_NOTIFIER_MAP; > > + event.entry.perm = IOMMU_RW; > > if (block_is_ns) { > > - entry.target_as = &s->blocked_io_as; > > + event.entry.target_as = &s->blocked_io_as; > > } else { > > - entry.target_as = &s->downstream_as; > > + event.entry.target_as = &s->downstream_as; > > } > > - memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, entry); > > + memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, event); > > if (block_is_ns) { > > - entry.target_as = &s->downstream_as; > > + event.entry.target_as = &s->downstream_as; > > } else { > > - entry.target_as = &s->blocked_io_as; > > + event.entry.target_as = &s->blocked_io_as; > > } > > - memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, entry); > > + memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, event); > > } > > } > > > > diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c > > index 0fecabc135..8bc3cff05d 100644 > > --- a/hw/ppc/spapr_iommu.c > > +++ b/hw/ppc/spapr_iommu.c > > @@ -445,7 +445,7 @@ static void spapr_tce_reset(DeviceState *dev) > > static target_ulong put_tce_emu(SpaprTceTable *tcet, target_ulong ioba, > > target_ulong tce) > > { > > - IOMMUTLBEntry entry; > > + IOMMUTLBEvent event; > > hwaddr page_mask = IOMMU_PAGE_MASK(tcet->page_shift); > > unsigned long index = (ioba - tcet->bus_offset) >> tcet->page_shift; > > > > @@ -457,12 +457,13 @@ static target_ulong put_tce_emu(SpaprTceTable *tcet, target_ulong ioba, > > > > tcet->table[index] = tce; > > > > - entry.target_as = &address_space_memory, > > - entry.iova = (ioba - tcet->bus_offset) & page_mask; > > - entry.translated_addr = tce & page_mask; > > - entry.addr_mask = ~page_mask; > > - entry.perm = spapr_tce_iommu_access_flags(tce); > > - memory_region_notify_iommu(&tcet->iommu, 0, entry); > > + event.entry.target_as = &address_space_memory, > > + event.entry.iova = (ioba - tcet->bus_offset) & page_mask; > > + event.entry.translated_addr = tce & page_mask; > > + event.entry.addr_mask = ~page_mask; > > + event.entry.perm = spapr_tce_iommu_access_flags(tce); > > + event.type = entry.perm ? IOMMU_NOTIFIER_MAP : IOMMU_NOTIFIER_UNMAP; > > + memory_region_notify_iommu(&tcet->iommu, 0, event); > > > > return H_SUCCESS; > > } > > diff --git a/include/exec/memory.h b/include/exec/memory.h > > index 22c5f564d1..8a56707169 100644 > > --- a/include/exec/memory.h > > +++ b/include/exec/memory.h > > @@ -70,11 +70,11 @@ typedef enum { > > #define IOMMU_ACCESS_FLAG(r, w) (((r) ? IOMMU_RO : 0) | ((w) ? IOMMU_WO : 0)) > > > > struct IOMMUTLBEntry { > > - AddressSpace *target_as; > > - hwaddr iova; > > - hwaddr translated_addr; > > - hwaddr addr_mask; /* 0xfff = 4k translation */ > > - IOMMUAccessFlags perm; > > + AddressSpace *target_as; > > + hwaddr iova; > > + hwaddr translated_addr; > > + hwaddr addr_mask; /* 0xfff = 4k translation */ > > + IOMMUAccessFlags perm; > > }; > > > > /* > > @@ -106,6 +106,11 @@ struct IOMMUNotifier { > > }; > > typedef struct IOMMUNotifier IOMMUNotifier; > > > > +typedef struct IOMMUTLBEvent { > > + IOMMUTLBEntry entry; > > + IOMMUNotifierFlag type; > nit: I would put the type field before the entry > > +} IOMMUTLBEvent; > > + > > /* RAM is pre-allocated and passed into qemu_ram_alloc_from_ptr */ > > #define RAM_PREALLOC (1 << 0) > > > > @@ -1265,13 +1270,13 @@ uint64_t memory_region_iommu_get_min_page_size(IOMMUMemoryRegion *iommu_mr); > > * > > Above there is: > > * The notification type will be decided by entry.perm bits: > * > * - For UNMAP (cache invalidation) notifies: set entry.perm to IOMMU_NONE. > * - For MAP (newly added entry) notifies: set entry.perm to the > * permission of the page (which is definitely !IOMMU_NONE). > > I think this should be updated as the main selector now is the type. > Definitely it needs to be updated, thanks! > I am a little bit concerned by the fact nothing really checks perm = > IOMMU_NONE is consistent with UNMAP type. New call sites may forget > this? shouldn't we check this in memory_region_notify_(one)_iommu? > So it should be not checked that way from now on, but by the use of `IOMMUTLBEvent.type`. So the caller should be responsible for set it properly. Another way of making it explicit is by making that type a parameter of memory_region_notify_(one)_iommu, and to keep IOMMUTLBEvent private tomemory.c, but I'm not sure if the notification will need more and more parameters, and I felt it scale better this way. Thanks! > > * @iommu_mr: the memory region that was changed > > * @iommu_idx: the IOMMU index for the translation table which has changed > > - * @entry: the new entry in the IOMMU translation table. The entry > > - * replaces all old entries for the same virtual I/O address range. > > - * Deleted entries have .@perm == 0. > > + * @event: TLB event with the new entry in the IOMMU translation table. > > + * The entry replaces all old entries for the same virtual I/O address > > + * range. > > */ > > void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, > > int iommu_idx, > > - IOMMUTLBEntry entry); > > + IOMMUTLBEvent event); > > > > /** > > * memory_region_notify_iommu_one: notify a change in an IOMMU translation > > @@ -1281,12 +1286,12 @@ void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, > > * notifies a specific notifier, not all of them. > > * > > * @notifier: the notifier to be notified > > - * @entry: the new entry in the IOMMU translation table. The entry > > - * replaces all old entries for the same virtual I/O address range. > > - * Deleted entries have .@perm == 0. > > + * @event: TLB event with the new entry in the IOMMU translation table. > > + * The entry replaces all old entries for the same virtual I/O address > > + * range. > > */ > > void memory_region_notify_iommu_one(IOMMUNotifier *notifier, > > - IOMMUTLBEntry *entry); > > + IOMMUTLBEvent *event); > > > > /** > > * memory_region_register_iommu_notifier: register a notifier for changes to > > diff --git a/softmmu/memory.c b/softmmu/memory.c > > index 961c25b42f..09b3443eac 100644 > > --- a/softmmu/memory.c > > +++ b/softmmu/memory.c > > @@ -1891,9 +1891,9 @@ void memory_region_unregister_iommu_notifier(MemoryRegion *mr, > > } > > > > void memory_region_notify_iommu_one(IOMMUNotifier *notifier, > > - IOMMUTLBEntry *entry) > > + IOMMUTLBEvent *event) > > { > > - IOMMUNotifierFlag request_flags; > > + IOMMUTLBEntry *entry = &event->entry; > > hwaddr entry_end = entry->iova + entry->addr_mask; > > > > /* > > @@ -1906,20 +1906,14 @@ void memory_region_notify_iommu_one(IOMMUNotifier *notifier, > > > > assert(entry->iova >= notifier->start && entry_end <= notifier->end); > > > > - if (entry->perm & IOMMU_RW) { > > - request_flags = IOMMU_NOTIFIER_MAP; > > - } else { > > - request_flags = IOMMU_NOTIFIER_UNMAP; > > - } > > - > > - if (notifier->notifier_flags & request_flags) { > > + if (event->type & notifier->notifier_flags) { > > notifier->notify(notifier, entry); > > } > > } > > > > void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, > > int iommu_idx, > > - IOMMUTLBEntry entry) > > + IOMMUTLBEvent event) > > { > > IOMMUNotifier *iommu_notifier; > > > > @@ -1927,7 +1921,7 @@ void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, > > > > IOMMU_NOTIFIER_FOREACH(iommu_notifier, iommu_mr) { > > if (iommu_notifier->iommu_idx == iommu_idx) { > > - memory_region_notify_iommu_one(iommu_notifier, &entry); > > + memory_region_notify_iommu_one(iommu_notifier, &event); > > } > > } > > } > > > Thanks > > Eric >
Hi Eugenio, On 9/2/20 3:18 PM, Eugenio Perez Martin wrote: > On Wed, Sep 2, 2020 at 12:17 PM Auger Eric <eric.auger@redhat.com> wrote: >> >> Hi Eugenio, >> >> On 9/1/20 4:26 PM, Eugenio Pérez wrote: >>> This way we can tell between regulars IOMMURLBEntries (entry of IOMMU >>> hardware) and notifications.> >>> In the notifications, we set explicitly if it is a MAPs or an UNMAP, >>> instead of trusting in entry permissions to differenciate them. >> differentiate >> >> Is it the real justification or is the rationale behind adding this type >> to be able to add new types of events? > > Hi Eric, > > Actually, the flag is to be able to tell the difference between > pre-existing events that were mixed. No new type of event involved, > just making it explicit. Well IOMMU_NOTIFIER_DEVIOTLB, introduced in subsequent patch is a new type of event. By the way in, [RFC v8 3/5] memory: Add IOMMU_DEVIOTLB_UNMAP IOMMUTLBNotificationType you should fix the commit msg (IOMMU_DEVIOTLB_UNMAP) or effectively use IOMMU_DEVIOTLB_UNMAP as suggested in a previous email. Hopefully you did not get the advise to rename before ;-) Sorry I joined the review too late. > >>> >>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com> >>> --- >>> hw/arm/smmu-common.c | 13 ++++--- >>> hw/arm/smmuv3.c | 13 ++++--- >>> hw/i386/intel_iommu.c | 88 ++++++++++++++++++++++++------------------- >>> hw/misc/tz-mpc.c | 32 +++++++++------- >>> hw/ppc/spapr_iommu.c | 15 ++++---- >>> include/exec/memory.h | 31 ++++++++------- >>> softmmu/memory.c | 16 +++----- >>> 7 files changed, 112 insertions(+), 96 deletions(-) >> >> you may add "orderFile = scripts/git.orderfile" in your .git/config diff >> section to get headers first >> >> [diff] >> orderFile = scripts/git.orderfile >> > > Didn't realize that, thanks! > >>> >>> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c >>> index 88d2c454f0..405d5c5325 100644 >>> --- a/hw/arm/smmu-common.c >>> +++ b/hw/arm/smmu-common.c >>> @@ -465,14 +465,15 @@ IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid) >>> /* Unmap the whole notifier's range */ >>> static void smmu_unmap_notifier_range(IOMMUNotifier *n) >>> { >>> - IOMMUTLBEntry entry; >>> + IOMMUTLBEvent event; >>> >>> - entry.target_as = &address_space_memory; >>> - entry.iova = n->start; >>> - entry.perm = IOMMU_NONE; >>> - entry.addr_mask = n->end - n->start; >>> + event.type = IOMMU_NOTIFIER_UNMAP; >>> + event.entry.target_as = &address_space_memory; >>> + event.entry.iova = n->start; >>> + event.entry.perm = IOMMU_NONE; >>> + event.entry.addr_mask = n->end - n->start; >>> >>> - memory_region_notify_iommu_one(n, &entry); >>> + memory_region_notify_iommu_one(n, &event); >>> } >>> >>> /* Unmap all notifiers attached to @mr */ >>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c >>> index 0a893ae918..62b0e289ca 100644 >>> --- a/hw/arm/smmuv3.c >>> +++ b/hw/arm/smmuv3.c >>> @@ -799,7 +799,7 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr, >>> uint8_t tg, uint64_t num_pages) >>> { >>> SMMUDevice *sdev = container_of(mr, SMMUDevice, iommu); >>> - IOMMUTLBEntry entry; >>> + IOMMUTLBEvent event; >>> uint8_t granule = tg; >>> >>> if (!tg) { >>> @@ -822,12 +822,13 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr, >>> granule = tt->granule_sz; >>> } >>> >>> - entry.target_as = &address_space_memory; >>> - entry.iova = iova; >>> - entry.addr_mask = num_pages * (1 << granule) - 1; >>> - entry.perm = IOMMU_NONE; >>> + event.type = IOMMU_NOTIFIER_UNMAP; >>> + event.entry.target_as = &address_space_memory; >>> + event.entry.iova = iova; >>> + event.entry.addr_mask = num_pages * (1 << granule) - 1; >>> + event.entry.perm = IOMMU_NONE; >>> >>> - memory_region_notify_iommu_one(n, &entry); >>> + memory_region_notify_iommu_one(n, &event); >>> } >>> >>> /* invalidate an asid/iova range tuple in all mr's */ >>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >>> index 2ad6b9d796..0c4aef5cb5 100644 >>> --- a/hw/i386/intel_iommu.c >>> +++ b/hw/i386/intel_iommu.c >>> @@ -1073,7 +1073,7 @@ static int vtd_iova_to_slpte(IntelIOMMUState *s, VTDContextEntry *ce, >>> } >>> } >>> >>> -typedef int (*vtd_page_walk_hook)(IOMMUTLBEntry *entry, void *private); >>> +typedef int (*vtd_page_walk_hook)(IOMMUTLBEvent *event, void *private); >>> >>> /** >>> * Constant information used during page walking >>> @@ -1094,11 +1094,12 @@ typedef struct { >>> uint16_t domain_id; >>> } vtd_page_walk_info; >>> >>> -static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) >>> +static int vtd_page_walk_one(IOMMUTLBEvent *event, vtd_page_walk_info *info) >>> { >>> VTDAddressSpace *as = info->as; >>> vtd_page_walk_hook hook_fn = info->hook_fn; >>> void *private = info->private; >>> + IOMMUTLBEntry *entry = &event->entry; >>> DMAMap target = { >>> .iova = entry->iova, >>> .size = entry->addr_mask, >>> @@ -1107,7 +1108,7 @@ static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) >>> }; >>> DMAMap *mapped = iova_tree_find(as->iova_tree, &target); >>> >>> - if (entry->perm == IOMMU_NONE && !info->notify_unmap) { >>> + if (event->type == IOMMU_NOTIFIER_UNMAP && !info->notify_unmap) { >>> trace_vtd_page_walk_one_skip_unmap(entry->iova, entry->addr_mask); >>> return 0; >>> } >>> @@ -1115,7 +1116,7 @@ static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) >>> assert(hook_fn); >>> >>> /* Update local IOVA mapped ranges */ >>> - if (entry->perm) { >>> + if (event->type == IOMMU_NOTIFIER_MAP) { >>> if (mapped) { >>> /* If it's exactly the same translation, skip */ >>> if (!memcmp(mapped, &target, sizeof(target))) { >>> @@ -1141,19 +1142,21 @@ static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) >>> int ret; >>> >>> /* Emulate an UNMAP */ >>> + event->type = IOMMU_NOTIFIER_UNMAP; >>> entry->perm = IOMMU_NONE; >>> trace_vtd_page_walk_one(info->domain_id, >>> entry->iova, >>> entry->translated_addr, >>> entry->addr_mask, >>> entry->perm); >>> - ret = hook_fn(entry, private); >>> + ret = hook_fn(event, private); >>> if (ret) { >>> return ret; >>> } >>> /* Drop any existing mapping */ >>> iova_tree_remove(as->iova_tree, &target); >>> - /* Recover the correct permission */ >>> + /* Recover the correct type */ >>> + event->type = IOMMU_NOTIFIER_MAP; >>> entry->perm = cache_perm; >>> } >>> } >>> @@ -1170,7 +1173,7 @@ static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) >>> trace_vtd_page_walk_one(info->domain_id, entry->iova, >>> entry->translated_addr, entry->addr_mask, >>> entry->perm); >>> - return hook_fn(entry, private); >>> + return hook_fn(event, private); >>> } >>> >>> /** >>> @@ -1191,7 +1194,7 @@ static int vtd_page_walk_level(dma_addr_t addr, uint64_t start, >>> uint32_t offset; >>> uint64_t slpte; >>> uint64_t subpage_size, subpage_mask; >>> - IOMMUTLBEntry entry; >>> + IOMMUTLBEvent event; >>> uint64_t iova = start; >>> uint64_t iova_next; >>> int ret = 0; >>> @@ -1245,13 +1248,15 @@ static int vtd_page_walk_level(dma_addr_t addr, uint64_t start, >>> * >>> * In either case, we send an IOTLB notification down. >>> */ >>> - entry.target_as = &address_space_memory; >>> - entry.iova = iova & subpage_mask; >>> - entry.perm = IOMMU_ACCESS_FLAG(read_cur, write_cur); >>> - entry.addr_mask = ~subpage_mask; >>> + event.entry.target_as = &address_space_memory; >>> + event.entry.iova = iova & subpage_mask; >>> + event.entry.perm = IOMMU_ACCESS_FLAG(read_cur, write_cur); >>> + event.entry.addr_mask = ~subpage_mask; >>> /* NOTE: this is only meaningful if entry_valid == true */ >>> - entry.translated_addr = vtd_get_slpte_addr(slpte, info->aw); >>> - ret = vtd_page_walk_one(&entry, info); >>> + event.entry.translated_addr = vtd_get_slpte_addr(slpte, info->aw); >>> + event.type = event.entry.perm ? IOMMU_NOTIFIER_MAP : >>> + IOMMU_NOTIFIER_UNMAP; >>> + ret = vtd_page_walk_one(&event, info); >>> } >>> >>> if (ret < 0) { >>> @@ -1430,10 +1435,10 @@ static int vtd_dev_to_context_entry(IntelIOMMUState *s, uint8_t bus_num, >>> return 0; >>> } >>> >>> -static int vtd_sync_shadow_page_hook(IOMMUTLBEntry *entry, >>> +static int vtd_sync_shadow_page_hook(IOMMUTLBEvent *event, >>> void *private) >>> { >>> - memory_region_notify_iommu((IOMMUMemoryRegion *)private, 0, *entry); >>> + memory_region_notify_iommu((IOMMUMemoryRegion *)private, 0, *event); >>> return 0; >>> } >>> >>> @@ -1993,14 +1998,17 @@ static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s, >>> * page tables. We just deliver the PSI down to >>> * invalidate caches. >>> */ >>> - IOMMUTLBEntry entry = { >>> - .target_as = &address_space_memory, >>> - .iova = addr, >>> - .translated_addr = 0, >>> - .addr_mask = size - 1, >>> - .perm = IOMMU_NONE, >>> + IOMMUTLBEvent event = { >>> + .type = IOMMU_NOTIFIER_UNMAP, >>> + .entry = { >>> + .target_as = &address_space_memory, >>> + .iova = addr, >>> + .translated_addr = 0, >>> + .addr_mask = size - 1, >>> + .perm = IOMMU_NONE, >>> + }, >>> }; >>> - memory_region_notify_iommu(&vtd_as->iommu, 0, entry); >>> + memory_region_notify_iommu(&vtd_as->iommu, 0, event); >>> } >>> } >>> } >>> @@ -2412,7 +2420,7 @@ static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s, >>> VTDInvDesc *inv_desc) >>> { >>> VTDAddressSpace *vtd_dev_as; >>> - IOMMUTLBEntry entry; >>> + IOMMUTLBEvent event; >>> struct VTDBus *vtd_bus; >>> hwaddr addr; >>> uint64_t sz; >>> @@ -2460,12 +2468,13 @@ static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s, >>> sz = VTD_PAGE_SIZE; >>> } >>> >>> - entry.target_as = &vtd_dev_as->as; >>> - entry.addr_mask = sz - 1; >>> - entry.iova = addr; >>> - entry.perm = IOMMU_NONE; >>> - entry.translated_addr = 0; >>> - memory_region_notify_iommu(&vtd_dev_as->iommu, 0, entry); >>> + event.type = IOMMU_NOTIFIER_UNMAP; >>> + event.entry.target_as = &vtd_dev_as->as; >>> + event.entry.addr_mask = sz - 1; >>> + event.entry.iova = addr; >>> + event.entry.perm = IOMMU_NONE; >>> + event.entry.translated_addr = 0; >>> + memory_region_notify_iommu(&vtd_dev_as->iommu, 0, event); >>> >>> done: >>> return true; >>> @@ -3486,19 +3495,20 @@ static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n) >>> size = remain = end - start + 1; >>> >>> while (remain >= VTD_PAGE_SIZE) { >>> - IOMMUTLBEntry entry; >>> + IOMMUTLBEvent event; >>> uint64_t mask = get_naturally_aligned_size(start, remain, s->aw_bits); >>> >>> assert(mask); >>> >>> - entry.iova = start; >>> - entry.addr_mask = mask - 1; >>> - entry.target_as = &address_space_memory; >>> - entry.perm = IOMMU_NONE; >>> + event.type = IOMMU_NOTIFIER_UNMAP; >>> + event.entry.iova = start; >>> + event.entry.addr_mask = mask - 1; >>> + event.entry.target_as = &address_space_memory; >>> + event.entry.perm = IOMMU_NONE; >>> /* This field is meaningless for unmap */ >>> - entry.translated_addr = 0; >>> + event.entry.translated_addr = 0; >>> >>> - memory_region_notify_iommu_one(n, &entry); >>> + memory_region_notify_iommu_one(n, &event); >>> >>> start += mask; >>> remain -= mask; >>> @@ -3534,9 +3544,9 @@ static void vtd_address_space_refresh_all(IntelIOMMUState *s) >>> vtd_switch_address_space_all(s); >>> } >>> >>> -static int vtd_replay_hook(IOMMUTLBEntry *entry, void *private) >>> +static int vtd_replay_hook(IOMMUTLBEvent *event, void *private) >>> { >>> - memory_region_notify_iommu_one((IOMMUNotifier *)private, entry); >>> + memory_region_notify_iommu_one((IOMMUNotifier *)private, event); >>> return 0; >>> } >>> >>> diff --git a/hw/misc/tz-mpc.c b/hw/misc/tz-mpc.c >>> index 98f151237f..30481e1c90 100644 >>> --- a/hw/misc/tz-mpc.c >>> +++ b/hw/misc/tz-mpc.c >>> @@ -82,8 +82,10 @@ static void tz_mpc_iommu_notify(TZMPC *s, uint32_t lutidx, >>> /* Called when the LUT word at lutidx has changed from oldlut to newlut; >>> * must call the IOMMU notifiers for the changed blocks. >>> */ >>> - IOMMUTLBEntry entry = { >>> - .addr_mask = s->blocksize - 1, >>> + IOMMUTLBEvent event = { >>> + .entry = { >>> + .addr_mask = s->blocksize - 1, >>> + } >>> }; >>> hwaddr addr = lutidx * s->blocksize * 32; >>> int i; >>> @@ -100,26 +102,28 @@ static void tz_mpc_iommu_notify(TZMPC *s, uint32_t lutidx, >>> block_is_ns = newlut & (1 << i); >>> >>> trace_tz_mpc_iommu_notify(addr); >>> - entry.iova = addr; >>> - entry.translated_addr = addr; >>> + event.entry.iova = addr; >>> + event.entry.translated_addr = addr; >>> >>> - entry.perm = IOMMU_NONE; >>> - memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, entry); >>> - memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, entry); >>> + event.type = IOMMU_NOTIFIER_UNMAP; >>> + event.entry.perm = IOMMU_NONE; >>> + memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, event); >>> + memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, event); >>> >>> - entry.perm = IOMMU_RW; >>> + event.type = IOMMU_NOTIFIER_MAP; >>> + event.entry.perm = IOMMU_RW; >>> if (block_is_ns) { >>> - entry.target_as = &s->blocked_io_as; >>> + event.entry.target_as = &s->blocked_io_as; >>> } else { >>> - entry.target_as = &s->downstream_as; >>> + event.entry.target_as = &s->downstream_as; >>> } >>> - memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, entry); >>> + memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, event); >>> if (block_is_ns) { >>> - entry.target_as = &s->downstream_as; >>> + event.entry.target_as = &s->downstream_as; >>> } else { >>> - entry.target_as = &s->blocked_io_as; >>> + event.entry.target_as = &s->blocked_io_as; >>> } >>> - memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, entry); >>> + memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, event); >>> } >>> } >>> >>> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c >>> index 0fecabc135..8bc3cff05d 100644 >>> --- a/hw/ppc/spapr_iommu.c >>> +++ b/hw/ppc/spapr_iommu.c >>> @@ -445,7 +445,7 @@ static void spapr_tce_reset(DeviceState *dev) >>> static target_ulong put_tce_emu(SpaprTceTable *tcet, target_ulong ioba, >>> target_ulong tce) >>> { >>> - IOMMUTLBEntry entry; >>> + IOMMUTLBEvent event; >>> hwaddr page_mask = IOMMU_PAGE_MASK(tcet->page_shift); >>> unsigned long index = (ioba - tcet->bus_offset) >> tcet->page_shift; >>> >>> @@ -457,12 +457,13 @@ static target_ulong put_tce_emu(SpaprTceTable *tcet, target_ulong ioba, >>> >>> tcet->table[index] = tce; >>> >>> - entry.target_as = &address_space_memory, >>> - entry.iova = (ioba - tcet->bus_offset) & page_mask; >>> - entry.translated_addr = tce & page_mask; >>> - entry.addr_mask = ~page_mask; >>> - entry.perm = spapr_tce_iommu_access_flags(tce); >>> - memory_region_notify_iommu(&tcet->iommu, 0, entry); >>> + event.entry.target_as = &address_space_memory, >>> + event.entry.iova = (ioba - tcet->bus_offset) & page_mask; >>> + event.entry.translated_addr = tce & page_mask; >>> + event.entry.addr_mask = ~page_mask; >>> + event.entry.perm = spapr_tce_iommu_access_flags(tce); >>> + event.type = entry.perm ? IOMMU_NOTIFIER_MAP : IOMMU_NOTIFIER_UNMAP; >>> + memory_region_notify_iommu(&tcet->iommu, 0, event); >>> >>> return H_SUCCESS; >>> } >>> diff --git a/include/exec/memory.h b/include/exec/memory.h >>> index 22c5f564d1..8a56707169 100644 >>> --- a/include/exec/memory.h >>> +++ b/include/exec/memory.h >>> @@ -70,11 +70,11 @@ typedef enum { >>> #define IOMMU_ACCESS_FLAG(r, w) (((r) ? IOMMU_RO : 0) | ((w) ? IOMMU_WO : 0)) >>> >>> struct IOMMUTLBEntry { >>> - AddressSpace *target_as; >>> - hwaddr iova; >>> - hwaddr translated_addr; >>> - hwaddr addr_mask; /* 0xfff = 4k translation */ >>> - IOMMUAccessFlags perm; >>> + AddressSpace *target_as; >>> + hwaddr iova; >>> + hwaddr translated_addr; >>> + hwaddr addr_mask; /* 0xfff = 4k translation */ >>> + IOMMUAccessFlags perm; >>> }; >>> >>> /* >>> @@ -106,6 +106,11 @@ struct IOMMUNotifier { >>> }; >>> typedef struct IOMMUNotifier IOMMUNotifier; >>> >>> +typedef struct IOMMUTLBEvent { >>> + IOMMUTLBEntry entry; >>> + IOMMUNotifierFlag type; >> nit: I would put the type field before the entry >>> +} IOMMUTLBEvent; >>> + >>> /* RAM is pre-allocated and passed into qemu_ram_alloc_from_ptr */ >>> #define RAM_PREALLOC (1 << 0) >>> >>> @@ -1265,13 +1270,13 @@ uint64_t memory_region_iommu_get_min_page_size(IOMMUMemoryRegion *iommu_mr); >>> * >> >> Above there is: >> >> * The notification type will be decided by entry.perm bits: >> * >> * - For UNMAP (cache invalidation) notifies: set entry.perm to IOMMU_NONE. >> * - For MAP (newly added entry) notifies: set entry.perm to the >> * permission of the page (which is definitely !IOMMU_NONE). >> >> I think this should be updated as the main selector now is the type. >> > > Definitely it needs to be updated, thanks! > >> I am a little bit concerned by the fact nothing really checks perm = >> IOMMU_NONE is consistent with UNMAP type. New call sites may forget >> this? shouldn't we check this in memory_region_notify_(one)_iommu? >> > > So it should be not checked that way from now on, but by the use of > `IOMMUTLBEvent.type`. So the caller should be responsible for set it > properly. > > Another way of making it explicit is by making that type a parameter > of memory_region_notify_(one)_iommu, and to keep IOMMUTLBEvent private > tomemory.c, but I'm not sure if the notification will need more and > more parameters, and I felt it scale better this way. Yep just wanted to share my concern. UNMAP necessary means perm=IOMMU_NONE. New callers may forget this ... an assert may enforce it but well. Thanks Eric > > Thanks! > >>> * @iommu_mr: the memory region that was changed >>> * @iommu_idx: the IOMMU index for the translation table which has changed >>> - * @entry: the new entry in the IOMMU translation table. The entry >>> - * replaces all old entries for the same virtual I/O address range. >>> - * Deleted entries have .@perm == 0. >>> + * @event: TLB event with the new entry in the IOMMU translation table. >>> + * The entry replaces all old entries for the same virtual I/O address >>> + * range. >>> */ >>> void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, >>> int iommu_idx, >>> - IOMMUTLBEntry entry); >>> + IOMMUTLBEvent event); >>> >>> /** >>> * memory_region_notify_iommu_one: notify a change in an IOMMU translation >>> @@ -1281,12 +1286,12 @@ void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, >>> * notifies a specific notifier, not all of them. >>> * >>> * @notifier: the notifier to be notified >>> - * @entry: the new entry in the IOMMU translation table. The entry >>> - * replaces all old entries for the same virtual I/O address range. >>> - * Deleted entries have .@perm == 0. >>> + * @event: TLB event with the new entry in the IOMMU translation table. >>> + * The entry replaces all old entries for the same virtual I/O address >>> + * range. >>> */ >>> void memory_region_notify_iommu_one(IOMMUNotifier *notifier, >>> - IOMMUTLBEntry *entry); >>> + IOMMUTLBEvent *event); >>> >>> /** >>> * memory_region_register_iommu_notifier: register a notifier for changes to >>> diff --git a/softmmu/memory.c b/softmmu/memory.c >>> index 961c25b42f..09b3443eac 100644 >>> --- a/softmmu/memory.c >>> +++ b/softmmu/memory.c >>> @@ -1891,9 +1891,9 @@ void memory_region_unregister_iommu_notifier(MemoryRegion *mr, >>> } >>> >>> void memory_region_notify_iommu_one(IOMMUNotifier *notifier, >>> - IOMMUTLBEntry *entry) >>> + IOMMUTLBEvent *event) >>> { >>> - IOMMUNotifierFlag request_flags; >>> + IOMMUTLBEntry *entry = &event->entry; >>> hwaddr entry_end = entry->iova + entry->addr_mask; >>> >>> /* >>> @@ -1906,20 +1906,14 @@ void memory_region_notify_iommu_one(IOMMUNotifier *notifier, >>> >>> assert(entry->iova >= notifier->start && entry_end <= notifier->end); >>> >>> - if (entry->perm & IOMMU_RW) { >>> - request_flags = IOMMU_NOTIFIER_MAP; >>> - } else { >>> - request_flags = IOMMU_NOTIFIER_UNMAP; >>> - } >>> - >>> - if (notifier->notifier_flags & request_flags) { >>> + if (event->type & notifier->notifier_flags) { >>> notifier->notify(notifier, entry); >>> } >>> } >>> >>> void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, >>> int iommu_idx, >>> - IOMMUTLBEntry entry) >>> + IOMMUTLBEvent event) >>> { >>> IOMMUNotifier *iommu_notifier; >>> >>> @@ -1927,7 +1921,7 @@ void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, >>> >>> IOMMU_NOTIFIER_FOREACH(iommu_notifier, iommu_mr) { >>> if (iommu_notifier->iommu_idx == iommu_idx) { >>> - memory_region_notify_iommu_one(iommu_notifier, &entry); >>> + memory_region_notify_iommu_one(iommu_notifier, &event); >>> } >>> } >>> } >>> >> Thanks >> >> Eric >> >
On Wed, Sep 2, 2020 at 3:40 PM Auger Eric <eric.auger@redhat.com> wrote: > > Hi Eugenio, > > On 9/2/20 3:18 PM, Eugenio Perez Martin wrote: > > On Wed, Sep 2, 2020 at 12:17 PM Auger Eric <eric.auger@redhat.com> wrote: > >> > >> Hi Eugenio, > >> > >> On 9/1/20 4:26 PM, Eugenio Pérez wrote: > >>> This way we can tell between regulars IOMMURLBEntries (entry of IOMMU > >>> hardware) and notifications.> > >>> In the notifications, we set explicitly if it is a MAPs or an UNMAP, > >>> instead of trusting in entry permissions to differenciate them. > >> differentiate PS: Thanks for the correction :). > >> > >> Is it the real justification or is the rationale behind adding this type > >> to be able to add new types of events? > > > > Hi Eric, > > > > Actually, the flag is to be able to tell the difference between > > pre-existing events that were mixed. No new type of event involved, > > just making it explicit. > > Well IOMMU_NOTIFIER_DEVIOTLB, introduced in subsequent patch is a new > type of event. > Yes, in that sense :). What I meant is that is only making it explicit so part of the code is able to process it better or, at least, differently. Is not like we enable new functionality or hardware with the addition of the flag. The commit already has a reason without the need of IOMMU_NOTIFIER_DEVIOTLB type, although a function could have done the job better in that case IMO. But I can add it to the commit message if you think it is needed, of course. > By the way in, > [RFC v8 3/5] memory: Add IOMMU_DEVIOTLB_UNMAP IOMMUTLBNotificationType > you should fix the commit msg (IOMMU_DEVIOTLB_UNMAP) or effectively use > IOMMU_DEVIOTLB_UNMAP as suggested in a previous email. Hopefully you did > not get the advise to rename before ;-) Sorry I joined the review too late. > Ouch, thank you for the catch :). > > > > >>> > >>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > >>> --- > >>> hw/arm/smmu-common.c | 13 ++++--- > >>> hw/arm/smmuv3.c | 13 ++++--- > >>> hw/i386/intel_iommu.c | 88 ++++++++++++++++++++++++------------------- > >>> hw/misc/tz-mpc.c | 32 +++++++++------- > >>> hw/ppc/spapr_iommu.c | 15 ++++---- > >>> include/exec/memory.h | 31 ++++++++------- > >>> softmmu/memory.c | 16 +++----- > >>> 7 files changed, 112 insertions(+), 96 deletions(-) > >> > >> you may add "orderFile = scripts/git.orderfile" in your .git/config diff > >> section to get headers first > >> > >> [diff] > >> orderFile = scripts/git.orderfile > >> > > > > Didn't realize that, thanks! > > > >>> > >>> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c > >>> index 88d2c454f0..405d5c5325 100644 > >>> --- a/hw/arm/smmu-common.c > >>> +++ b/hw/arm/smmu-common.c > >>> @@ -465,14 +465,15 @@ IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid) > >>> /* Unmap the whole notifier's range */ > >>> static void smmu_unmap_notifier_range(IOMMUNotifier *n) > >>> { > >>> - IOMMUTLBEntry entry; > >>> + IOMMUTLBEvent event; > >>> > >>> - entry.target_as = &address_space_memory; > >>> - entry.iova = n->start; > >>> - entry.perm = IOMMU_NONE; > >>> - entry.addr_mask = n->end - n->start; > >>> + event.type = IOMMU_NOTIFIER_UNMAP; > >>> + event.entry.target_as = &address_space_memory; > >>> + event.entry.iova = n->start; > >>> + event.entry.perm = IOMMU_NONE; > >>> + event.entry.addr_mask = n->end - n->start; > >>> > >>> - memory_region_notify_iommu_one(n, &entry); > >>> + memory_region_notify_iommu_one(n, &event); > >>> } > >>> > >>> /* Unmap all notifiers attached to @mr */ > >>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c > >>> index 0a893ae918..62b0e289ca 100644 > >>> --- a/hw/arm/smmuv3.c > >>> +++ b/hw/arm/smmuv3.c > >>> @@ -799,7 +799,7 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr, > >>> uint8_t tg, uint64_t num_pages) > >>> { > >>> SMMUDevice *sdev = container_of(mr, SMMUDevice, iommu); > >>> - IOMMUTLBEntry entry; > >>> + IOMMUTLBEvent event; > >>> uint8_t granule = tg; > >>> > >>> if (!tg) { > >>> @@ -822,12 +822,13 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr, > >>> granule = tt->granule_sz; > >>> } > >>> > >>> - entry.target_as = &address_space_memory; > >>> - entry.iova = iova; > >>> - entry.addr_mask = num_pages * (1 << granule) - 1; > >>> - entry.perm = IOMMU_NONE; > >>> + event.type = IOMMU_NOTIFIER_UNMAP; > >>> + event.entry.target_as = &address_space_memory; > >>> + event.entry.iova = iova; > >>> + event.entry.addr_mask = num_pages * (1 << granule) - 1; > >>> + event.entry.perm = IOMMU_NONE; > >>> > >>> - memory_region_notify_iommu_one(n, &entry); > >>> + memory_region_notify_iommu_one(n, &event); > >>> } > >>> > >>> /* invalidate an asid/iova range tuple in all mr's */ > >>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > >>> index 2ad6b9d796..0c4aef5cb5 100644 > >>> --- a/hw/i386/intel_iommu.c > >>> +++ b/hw/i386/intel_iommu.c > >>> @@ -1073,7 +1073,7 @@ static int vtd_iova_to_slpte(IntelIOMMUState *s, VTDContextEntry *ce, > >>> } > >>> } > >>> > >>> -typedef int (*vtd_page_walk_hook)(IOMMUTLBEntry *entry, void *private); > >>> +typedef int (*vtd_page_walk_hook)(IOMMUTLBEvent *event, void *private); > >>> > >>> /** > >>> * Constant information used during page walking > >>> @@ -1094,11 +1094,12 @@ typedef struct { > >>> uint16_t domain_id; > >>> } vtd_page_walk_info; > >>> > >>> -static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) > >>> +static int vtd_page_walk_one(IOMMUTLBEvent *event, vtd_page_walk_info *info) > >>> { > >>> VTDAddressSpace *as = info->as; > >>> vtd_page_walk_hook hook_fn = info->hook_fn; > >>> void *private = info->private; > >>> + IOMMUTLBEntry *entry = &event->entry; > >>> DMAMap target = { > >>> .iova = entry->iova, > >>> .size = entry->addr_mask, > >>> @@ -1107,7 +1108,7 @@ static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) > >>> }; > >>> DMAMap *mapped = iova_tree_find(as->iova_tree, &target); > >>> > >>> - if (entry->perm == IOMMU_NONE && !info->notify_unmap) { > >>> + if (event->type == IOMMU_NOTIFIER_UNMAP && !info->notify_unmap) { > >>> trace_vtd_page_walk_one_skip_unmap(entry->iova, entry->addr_mask); > >>> return 0; > >>> } > >>> @@ -1115,7 +1116,7 @@ static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) > >>> assert(hook_fn); > >>> > >>> /* Update local IOVA mapped ranges */ > >>> - if (entry->perm) { > >>> + if (event->type == IOMMU_NOTIFIER_MAP) { > >>> if (mapped) { > >>> /* If it's exactly the same translation, skip */ > >>> if (!memcmp(mapped, &target, sizeof(target))) { > >>> @@ -1141,19 +1142,21 @@ static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) > >>> int ret; > >>> > >>> /* Emulate an UNMAP */ > >>> + event->type = IOMMU_NOTIFIER_UNMAP; > >>> entry->perm = IOMMU_NONE; > >>> trace_vtd_page_walk_one(info->domain_id, > >>> entry->iova, > >>> entry->translated_addr, > >>> entry->addr_mask, > >>> entry->perm); > >>> - ret = hook_fn(entry, private); > >>> + ret = hook_fn(event, private); > >>> if (ret) { > >>> return ret; > >>> } > >>> /* Drop any existing mapping */ > >>> iova_tree_remove(as->iova_tree, &target); > >>> - /* Recover the correct permission */ > >>> + /* Recover the correct type */ > >>> + event->type = IOMMU_NOTIFIER_MAP; > >>> entry->perm = cache_perm; > >>> } > >>> } > >>> @@ -1170,7 +1173,7 @@ static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) > >>> trace_vtd_page_walk_one(info->domain_id, entry->iova, > >>> entry->translated_addr, entry->addr_mask, > >>> entry->perm); > >>> - return hook_fn(entry, private); > >>> + return hook_fn(event, private); > >>> } > >>> > >>> /** > >>> @@ -1191,7 +1194,7 @@ static int vtd_page_walk_level(dma_addr_t addr, uint64_t start, > >>> uint32_t offset; > >>> uint64_t slpte; > >>> uint64_t subpage_size, subpage_mask; > >>> - IOMMUTLBEntry entry; > >>> + IOMMUTLBEvent event; > >>> uint64_t iova = start; > >>> uint64_t iova_next; > >>> int ret = 0; > >>> @@ -1245,13 +1248,15 @@ static int vtd_page_walk_level(dma_addr_t addr, uint64_t start, > >>> * > >>> * In either case, we send an IOTLB notification down. > >>> */ > >>> - entry.target_as = &address_space_memory; > >>> - entry.iova = iova & subpage_mask; > >>> - entry.perm = IOMMU_ACCESS_FLAG(read_cur, write_cur); > >>> - entry.addr_mask = ~subpage_mask; > >>> + event.entry.target_as = &address_space_memory; > >>> + event.entry.iova = iova & subpage_mask; > >>> + event.entry.perm = IOMMU_ACCESS_FLAG(read_cur, write_cur); > >>> + event.entry.addr_mask = ~subpage_mask; > >>> /* NOTE: this is only meaningful if entry_valid == true */ > >>> - entry.translated_addr = vtd_get_slpte_addr(slpte, info->aw); > >>> - ret = vtd_page_walk_one(&entry, info); > >>> + event.entry.translated_addr = vtd_get_slpte_addr(slpte, info->aw); > >>> + event.type = event.entry.perm ? IOMMU_NOTIFIER_MAP : > >>> + IOMMU_NOTIFIER_UNMAP; > >>> + ret = vtd_page_walk_one(&event, info); > >>> } > >>> > >>> if (ret < 0) { > >>> @@ -1430,10 +1435,10 @@ static int vtd_dev_to_context_entry(IntelIOMMUState *s, uint8_t bus_num, > >>> return 0; > >>> } > >>> > >>> -static int vtd_sync_shadow_page_hook(IOMMUTLBEntry *entry, > >>> +static int vtd_sync_shadow_page_hook(IOMMUTLBEvent *event, > >>> void *private) > >>> { > >>> - memory_region_notify_iommu((IOMMUMemoryRegion *)private, 0, *entry); > >>> + memory_region_notify_iommu((IOMMUMemoryRegion *)private, 0, *event); > >>> return 0; > >>> } > >>> > >>> @@ -1993,14 +1998,17 @@ static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s, > >>> * page tables. We just deliver the PSI down to > >>> * invalidate caches. > >>> */ > >>> - IOMMUTLBEntry entry = { > >>> - .target_as = &address_space_memory, > >>> - .iova = addr, > >>> - .translated_addr = 0, > >>> - .addr_mask = size - 1, > >>> - .perm = IOMMU_NONE, > >>> + IOMMUTLBEvent event = { > >>> + .type = IOMMU_NOTIFIER_UNMAP, > >>> + .entry = { > >>> + .target_as = &address_space_memory, > >>> + .iova = addr, > >>> + .translated_addr = 0, > >>> + .addr_mask = size - 1, > >>> + .perm = IOMMU_NONE, > >>> + }, > >>> }; > >>> - memory_region_notify_iommu(&vtd_as->iommu, 0, entry); > >>> + memory_region_notify_iommu(&vtd_as->iommu, 0, event); > >>> } > >>> } > >>> } > >>> @@ -2412,7 +2420,7 @@ static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s, > >>> VTDInvDesc *inv_desc) > >>> { > >>> VTDAddressSpace *vtd_dev_as; > >>> - IOMMUTLBEntry entry; > >>> + IOMMUTLBEvent event; > >>> struct VTDBus *vtd_bus; > >>> hwaddr addr; > >>> uint64_t sz; > >>> @@ -2460,12 +2468,13 @@ static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s, > >>> sz = VTD_PAGE_SIZE; > >>> } > >>> > >>> - entry.target_as = &vtd_dev_as->as; > >>> - entry.addr_mask = sz - 1; > >>> - entry.iova = addr; > >>> - entry.perm = IOMMU_NONE; > >>> - entry.translated_addr = 0; > >>> - memory_region_notify_iommu(&vtd_dev_as->iommu, 0, entry); > >>> + event.type = IOMMU_NOTIFIER_UNMAP; > >>> + event.entry.target_as = &vtd_dev_as->as; > >>> + event.entry.addr_mask = sz - 1; > >>> + event.entry.iova = addr; > >>> + event.entry.perm = IOMMU_NONE; > >>> + event.entry.translated_addr = 0; > >>> + memory_region_notify_iommu(&vtd_dev_as->iommu, 0, event); > >>> > >>> done: > >>> return true; > >>> @@ -3486,19 +3495,20 @@ static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n) > >>> size = remain = end - start + 1; > >>> > >>> while (remain >= VTD_PAGE_SIZE) { > >>> - IOMMUTLBEntry entry; > >>> + IOMMUTLBEvent event; > >>> uint64_t mask = get_naturally_aligned_size(start, remain, s->aw_bits); > >>> > >>> assert(mask); > >>> > >>> - entry.iova = start; > >>> - entry.addr_mask = mask - 1; > >>> - entry.target_as = &address_space_memory; > >>> - entry.perm = IOMMU_NONE; > >>> + event.type = IOMMU_NOTIFIER_UNMAP; > >>> + event.entry.iova = start; > >>> + event.entry.addr_mask = mask - 1; > >>> + event.entry.target_as = &address_space_memory; > >>> + event.entry.perm = IOMMU_NONE; > >>> /* This field is meaningless for unmap */ > >>> - entry.translated_addr = 0; > >>> + event.entry.translated_addr = 0; > >>> > >>> - memory_region_notify_iommu_one(n, &entry); > >>> + memory_region_notify_iommu_one(n, &event); > >>> > >>> start += mask; > >>> remain -= mask; > >>> @@ -3534,9 +3544,9 @@ static void vtd_address_space_refresh_all(IntelIOMMUState *s) > >>> vtd_switch_address_space_all(s); > >>> } > >>> > >>> -static int vtd_replay_hook(IOMMUTLBEntry *entry, void *private) > >>> +static int vtd_replay_hook(IOMMUTLBEvent *event, void *private) > >>> { > >>> - memory_region_notify_iommu_one((IOMMUNotifier *)private, entry); > >>> + memory_region_notify_iommu_one((IOMMUNotifier *)private, event); > >>> return 0; > >>> } > >>> > >>> diff --git a/hw/misc/tz-mpc.c b/hw/misc/tz-mpc.c > >>> index 98f151237f..30481e1c90 100644 > >>> --- a/hw/misc/tz-mpc.c > >>> +++ b/hw/misc/tz-mpc.c > >>> @@ -82,8 +82,10 @@ static void tz_mpc_iommu_notify(TZMPC *s, uint32_t lutidx, > >>> /* Called when the LUT word at lutidx has changed from oldlut to newlut; > >>> * must call the IOMMU notifiers for the changed blocks. > >>> */ > >>> - IOMMUTLBEntry entry = { > >>> - .addr_mask = s->blocksize - 1, > >>> + IOMMUTLBEvent event = { > >>> + .entry = { > >>> + .addr_mask = s->blocksize - 1, > >>> + } > >>> }; > >>> hwaddr addr = lutidx * s->blocksize * 32; > >>> int i; > >>> @@ -100,26 +102,28 @@ static void tz_mpc_iommu_notify(TZMPC *s, uint32_t lutidx, > >>> block_is_ns = newlut & (1 << i); > >>> > >>> trace_tz_mpc_iommu_notify(addr); > >>> - entry.iova = addr; > >>> - entry.translated_addr = addr; > >>> + event.entry.iova = addr; > >>> + event.entry.translated_addr = addr; > >>> > >>> - entry.perm = IOMMU_NONE; > >>> - memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, entry); > >>> - memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, entry); > >>> + event.type = IOMMU_NOTIFIER_UNMAP; > >>> + event.entry.perm = IOMMU_NONE; > >>> + memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, event); > >>> + memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, event); > >>> > >>> - entry.perm = IOMMU_RW; > >>> + event.type = IOMMU_NOTIFIER_MAP; > >>> + event.entry.perm = IOMMU_RW; > >>> if (block_is_ns) { > >>> - entry.target_as = &s->blocked_io_as; > >>> + event.entry.target_as = &s->blocked_io_as; > >>> } else { > >>> - entry.target_as = &s->downstream_as; > >>> + event.entry.target_as = &s->downstream_as; > >>> } > >>> - memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, entry); > >>> + memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, event); > >>> if (block_is_ns) { > >>> - entry.target_as = &s->downstream_as; > >>> + event.entry.target_as = &s->downstream_as; > >>> } else { > >>> - entry.target_as = &s->blocked_io_as; > >>> + event.entry.target_as = &s->blocked_io_as; > >>> } > >>> - memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, entry); > >>> + memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, event); > >>> } > >>> } > >>> > >>> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c > >>> index 0fecabc135..8bc3cff05d 100644 > >>> --- a/hw/ppc/spapr_iommu.c > >>> +++ b/hw/ppc/spapr_iommu.c > >>> @@ -445,7 +445,7 @@ static void spapr_tce_reset(DeviceState *dev) > >>> static target_ulong put_tce_emu(SpaprTceTable *tcet, target_ulong ioba, > >>> target_ulong tce) > >>> { > >>> - IOMMUTLBEntry entry; > >>> + IOMMUTLBEvent event; > >>> hwaddr page_mask = IOMMU_PAGE_MASK(tcet->page_shift); > >>> unsigned long index = (ioba - tcet->bus_offset) >> tcet->page_shift; > >>> > >>> @@ -457,12 +457,13 @@ static target_ulong put_tce_emu(SpaprTceTable *tcet, target_ulong ioba, > >>> > >>> tcet->table[index] = tce; > >>> > >>> - entry.target_as = &address_space_memory, > >>> - entry.iova = (ioba - tcet->bus_offset) & page_mask; > >>> - entry.translated_addr = tce & page_mask; > >>> - entry.addr_mask = ~page_mask; > >>> - entry.perm = spapr_tce_iommu_access_flags(tce); > >>> - memory_region_notify_iommu(&tcet->iommu, 0, entry); > >>> + event.entry.target_as = &address_space_memory, > >>> + event.entry.iova = (ioba - tcet->bus_offset) & page_mask; > >>> + event.entry.translated_addr = tce & page_mask; > >>> + event.entry.addr_mask = ~page_mask; > >>> + event.entry.perm = spapr_tce_iommu_access_flags(tce); > >>> + event.type = entry.perm ? IOMMU_NOTIFIER_MAP : IOMMU_NOTIFIER_UNMAP; > >>> + memory_region_notify_iommu(&tcet->iommu, 0, event); > >>> > >>> return H_SUCCESS; > >>> } > >>> diff --git a/include/exec/memory.h b/include/exec/memory.h > >>> index 22c5f564d1..8a56707169 100644 > >>> --- a/include/exec/memory.h > >>> +++ b/include/exec/memory.h > >>> @@ -70,11 +70,11 @@ typedef enum { > >>> #define IOMMU_ACCESS_FLAG(r, w) (((r) ? IOMMU_RO : 0) | ((w) ? IOMMU_WO : 0)) > >>> > >>> struct IOMMUTLBEntry { > >>> - AddressSpace *target_as; > >>> - hwaddr iova; > >>> - hwaddr translated_addr; > >>> - hwaddr addr_mask; /* 0xfff = 4k translation */ > >>> - IOMMUAccessFlags perm; > >>> + AddressSpace *target_as; > >>> + hwaddr iova; > >>> + hwaddr translated_addr; > >>> + hwaddr addr_mask; /* 0xfff = 4k translation */ > >>> + IOMMUAccessFlags perm; > >>> }; > >>> > >>> /* > >>> @@ -106,6 +106,11 @@ struct IOMMUNotifier { > >>> }; > >>> typedef struct IOMMUNotifier IOMMUNotifier; > >>> > >>> +typedef struct IOMMUTLBEvent { > >>> + IOMMUTLBEntry entry; > >>> + IOMMUNotifierFlag type; > >> nit: I would put the type field before the entry > >>> +} IOMMUTLBEvent; > >>> + > >>> /* RAM is pre-allocated and passed into qemu_ram_alloc_from_ptr */ > >>> #define RAM_PREALLOC (1 << 0) > >>> > >>> @@ -1265,13 +1270,13 @@ uint64_t memory_region_iommu_get_min_page_size(IOMMUMemoryRegion *iommu_mr); > >>> * > >> > >> Above there is: > >> > >> * The notification type will be decided by entry.perm bits: > >> * > >> * - For UNMAP (cache invalidation) notifies: set entry.perm to IOMMU_NONE. > >> * - For MAP (newly added entry) notifies: set entry.perm to the > >> * permission of the page (which is definitely !IOMMU_NONE). > >> > >> I think this should be updated as the main selector now is the type. > >> > > > > Definitely it needs to be updated, thanks! > > > >> I am a little bit concerned by the fact nothing really checks perm = > >> IOMMU_NONE is consistent with UNMAP type. New call sites may forget > >> this? shouldn't we check this in memory_region_notify_(one)_iommu? > >> > > > > So it should be not checked that way from now on, but by the use of > > `IOMMUTLBEvent.type`. So the caller should be responsible for set it > > properly. > > > > Another way of making it explicit is by making that type a parameter > > of memory_region_notify_(one)_iommu, and to keep IOMMUTLBEvent private > > tomemory.c, but I'm not sure if the notification will need more and > > more parameters, and I felt it scale better this way. > Yep just wanted to share my concern. UNMAP necessary means > perm=IOMMU_NONE. New callers may forget this ... an assert may enforce > it but well. > > Thanks > > Eric > > > > Thanks! > > > >>> * @iommu_mr: the memory region that was changed > >>> * @iommu_idx: the IOMMU index for the translation table which has changed > >>> - * @entry: the new entry in the IOMMU translation table. The entry > >>> - * replaces all old entries for the same virtual I/O address range. > >>> - * Deleted entries have .@perm == 0. > >>> + * @event: TLB event with the new entry in the IOMMU translation table. > >>> + * The entry replaces all old entries for the same virtual I/O address > >>> + * range. > >>> */ > >>> void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, > >>> int iommu_idx, > >>> - IOMMUTLBEntry entry); > >>> + IOMMUTLBEvent event); > >>> > >>> /** > >>> * memory_region_notify_iommu_one: notify a change in an IOMMU translation > >>> @@ -1281,12 +1286,12 @@ void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, > >>> * notifies a specific notifier, not all of them. > >>> * > >>> * @notifier: the notifier to be notified > >>> - * @entry: the new entry in the IOMMU translation table. The entry > >>> - * replaces all old entries for the same virtual I/O address range. > >>> - * Deleted entries have .@perm == 0. > >>> + * @event: TLB event with the new entry in the IOMMU translation table. > >>> + * The entry replaces all old entries for the same virtual I/O address > >>> + * range. > >>> */ > >>> void memory_region_notify_iommu_one(IOMMUNotifier *notifier, > >>> - IOMMUTLBEntry *entry); > >>> + IOMMUTLBEvent *event); > >>> > >>> /** > >>> * memory_region_register_iommu_notifier: register a notifier for changes to > >>> diff --git a/softmmu/memory.c b/softmmu/memory.c > >>> index 961c25b42f..09b3443eac 100644 > >>> --- a/softmmu/memory.c > >>> +++ b/softmmu/memory.c > >>> @@ -1891,9 +1891,9 @@ void memory_region_unregister_iommu_notifier(MemoryRegion *mr, > >>> } > >>> > >>> void memory_region_notify_iommu_one(IOMMUNotifier *notifier, > >>> - IOMMUTLBEntry *entry) > >>> + IOMMUTLBEvent *event) > >>> { > >>> - IOMMUNotifierFlag request_flags; > >>> + IOMMUTLBEntry *entry = &event->entry; > >>> hwaddr entry_end = entry->iova + entry->addr_mask; > >>> > >>> /* > >>> @@ -1906,20 +1906,14 @@ void memory_region_notify_iommu_one(IOMMUNotifier *notifier, > >>> > >>> assert(entry->iova >= notifier->start && entry_end <= notifier->end); > >>> > >>> - if (entry->perm & IOMMU_RW) { > >>> - request_flags = IOMMU_NOTIFIER_MAP; > >>> - } else { > >>> - request_flags = IOMMU_NOTIFIER_UNMAP; > >>> - } > >>> - > >>> - if (notifier->notifier_flags & request_flags) { > >>> + if (event->type & notifier->notifier_flags) { > >>> notifier->notify(notifier, entry); > >>> } > >>> } > >>> > >>> void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, > >>> int iommu_idx, > >>> - IOMMUTLBEntry entry) > >>> + IOMMUTLBEvent event) > >>> { > >>> IOMMUNotifier *iommu_notifier; > >>> > >>> @@ -1927,7 +1921,7 @@ void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, > >>> > >>> IOMMU_NOTIFIER_FOREACH(iommu_notifier, iommu_mr) { > >>> if (iommu_notifier->iommu_idx == iommu_idx) { > >>> - memory_region_notify_iommu_one(iommu_notifier, &entry); > >>> + memory_region_notify_iommu_one(iommu_notifier, &event); > >>> } > >>> } > >>> } > >>> > >> Thanks > >> > >> Eric > >> > > >
On Wed, Sep 2, 2020 at 3:58 PM Eugenio Perez Martin <eperezma@redhat.com> wrote: > > On Wed, Sep 2, 2020 at 3:40 PM Auger Eric <eric.auger@redhat.com> wrote: > > > > Hi Eugenio, > > > > On 9/2/20 3:18 PM, Eugenio Perez Martin wrote: > > > On Wed, Sep 2, 2020 at 12:17 PM Auger Eric <eric.auger@redhat.com> wrote: > > >> > > >> Hi Eugenio, > > >> > > >> On 9/1/20 4:26 PM, Eugenio Pérez wrote: > > >>> This way we can tell between regulars IOMMURLBEntries (entry of IOMMU > > >>> hardware) and notifications.> > > >>> In the notifications, we set explicitly if it is a MAPs or an UNMAP, > > >>> instead of trusting in entry permissions to differenciate them. > > >> differentiate > > PS: Thanks for the correction :). > > > >> > > >> Is it the real justification or is the rationale behind adding this type > > >> to be able to add new types of events? > > > > > > Hi Eric, > > > > > > Actually, the flag is to be able to tell the difference between > > > pre-existing events that were mixed. No new type of event involved, > > > just making it explicit. > > > > Well IOMMU_NOTIFIER_DEVIOTLB, introduced in subsequent patch is a new > > type of event. > > > > Yes, in that sense :). What I meant is that is only making it explicit > so part of the code is able to process it better or, at least, > differently. Is not like we enable new functionality or hardware with > the addition of the flag. The commit already has a reason without the > need of IOMMU_NOTIFIER_DEVIOTLB type, although a function could have > done the job better in that case IMO. > > But I can add it to the commit message if you think it is needed, of course. > > > > By the way in, > > [RFC v8 3/5] memory: Add IOMMU_DEVIOTLB_UNMAP IOMMUTLBNotificationType > > you should fix the commit msg (IOMMU_DEVIOTLB_UNMAP) or effectively use > > IOMMU_DEVIOTLB_UNMAP as suggested in a previous email. Hopefully you did > > not get the advise to rename before ;-) Sorry I joined the review too late. > > > > Ouch, thank you for the catch :). > > > > > > > > >>> > > >>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > >>> --- > > >>> hw/arm/smmu-common.c | 13 ++++--- > > >>> hw/arm/smmuv3.c | 13 ++++--- > > >>> hw/i386/intel_iommu.c | 88 ++++++++++++++++++++++++------------------- > > >>> hw/misc/tz-mpc.c | 32 +++++++++------- > > >>> hw/ppc/spapr_iommu.c | 15 ++++---- > > >>> include/exec/memory.h | 31 ++++++++------- > > >>> softmmu/memory.c | 16 +++----- > > >>> 7 files changed, 112 insertions(+), 96 deletions(-) > > >> > > >> you may add "orderFile = scripts/git.orderfile" in your .git/config diff > > >> section to get headers first > > >> > > >> [diff] > > >> orderFile = scripts/git.orderfile > > >> > > > > > > Didn't realize that, thanks! > > > > > >>> > > >>> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c > > >>> index 88d2c454f0..405d5c5325 100644 > > >>> --- a/hw/arm/smmu-common.c > > >>> +++ b/hw/arm/smmu-common.c > > >>> @@ -465,14 +465,15 @@ IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid) > > >>> /* Unmap the whole notifier's range */ > > >>> static void smmu_unmap_notifier_range(IOMMUNotifier *n) > > >>> { > > >>> - IOMMUTLBEntry entry; > > >>> + IOMMUTLBEvent event; > > >>> > > >>> - entry.target_as = &address_space_memory; > > >>> - entry.iova = n->start; > > >>> - entry.perm = IOMMU_NONE; > > >>> - entry.addr_mask = n->end - n->start; > > >>> + event.type = IOMMU_NOTIFIER_UNMAP; > > >>> + event.entry.target_as = &address_space_memory; > > >>> + event.entry.iova = n->start; > > >>> + event.entry.perm = IOMMU_NONE; > > >>> + event.entry.addr_mask = n->end - n->start; > > >>> > > >>> - memory_region_notify_iommu_one(n, &entry); > > >>> + memory_region_notify_iommu_one(n, &event); > > >>> } > > >>> > > >>> /* Unmap all notifiers attached to @mr */ > > >>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c > > >>> index 0a893ae918..62b0e289ca 100644 > > >>> --- a/hw/arm/smmuv3.c > > >>> +++ b/hw/arm/smmuv3.c > > >>> @@ -799,7 +799,7 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr, > > >>> uint8_t tg, uint64_t num_pages) > > >>> { > > >>> SMMUDevice *sdev = container_of(mr, SMMUDevice, iommu); > > >>> - IOMMUTLBEntry entry; > > >>> + IOMMUTLBEvent event; > > >>> uint8_t granule = tg; > > >>> > > >>> if (!tg) { > > >>> @@ -822,12 +822,13 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr, > > >>> granule = tt->granule_sz; > > >>> } > > >>> > > >>> - entry.target_as = &address_space_memory; > > >>> - entry.iova = iova; > > >>> - entry.addr_mask = num_pages * (1 << granule) - 1; > > >>> - entry.perm = IOMMU_NONE; > > >>> + event.type = IOMMU_NOTIFIER_UNMAP; > > >>> + event.entry.target_as = &address_space_memory; > > >>> + event.entry.iova = iova; > > >>> + event.entry.addr_mask = num_pages * (1 << granule) - 1; > > >>> + event.entry.perm = IOMMU_NONE; > > >>> > > >>> - memory_region_notify_iommu_one(n, &entry); > > >>> + memory_region_notify_iommu_one(n, &event); > > >>> } > > >>> > > >>> /* invalidate an asid/iova range tuple in all mr's */ > > >>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > > >>> index 2ad6b9d796..0c4aef5cb5 100644 > > >>> --- a/hw/i386/intel_iommu.c > > >>> +++ b/hw/i386/intel_iommu.c > > >>> @@ -1073,7 +1073,7 @@ static int vtd_iova_to_slpte(IntelIOMMUState *s, VTDContextEntry *ce, > > >>> } > > >>> } > > >>> > > >>> -typedef int (*vtd_page_walk_hook)(IOMMUTLBEntry *entry, void *private); > > >>> +typedef int (*vtd_page_walk_hook)(IOMMUTLBEvent *event, void *private); > > >>> > > >>> /** > > >>> * Constant information used during page walking > > >>> @@ -1094,11 +1094,12 @@ typedef struct { > > >>> uint16_t domain_id; > > >>> } vtd_page_walk_info; > > >>> > > >>> -static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) > > >>> +static int vtd_page_walk_one(IOMMUTLBEvent *event, vtd_page_walk_info *info) > > >>> { > > >>> VTDAddressSpace *as = info->as; > > >>> vtd_page_walk_hook hook_fn = info->hook_fn; > > >>> void *private = info->private; > > >>> + IOMMUTLBEntry *entry = &event->entry; > > >>> DMAMap target = { > > >>> .iova = entry->iova, > > >>> .size = entry->addr_mask, > > >>> @@ -1107,7 +1108,7 @@ static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) > > >>> }; > > >>> DMAMap *mapped = iova_tree_find(as->iova_tree, &target); > > >>> > > >>> - if (entry->perm == IOMMU_NONE && !info->notify_unmap) { > > >>> + if (event->type == IOMMU_NOTIFIER_UNMAP && !info->notify_unmap) { > > >>> trace_vtd_page_walk_one_skip_unmap(entry->iova, entry->addr_mask); > > >>> return 0; > > >>> } > > >>> @@ -1115,7 +1116,7 @@ static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) > > >>> assert(hook_fn); > > >>> > > >>> /* Update local IOVA mapped ranges */ > > >>> - if (entry->perm) { > > >>> + if (event->type == IOMMU_NOTIFIER_MAP) { > > >>> if (mapped) { > > >>> /* If it's exactly the same translation, skip */ > > >>> if (!memcmp(mapped, &target, sizeof(target))) { > > >>> @@ -1141,19 +1142,21 @@ static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) > > >>> int ret; > > >>> > > >>> /* Emulate an UNMAP */ > > >>> + event->type = IOMMU_NOTIFIER_UNMAP; > > >>> entry->perm = IOMMU_NONE; > > >>> trace_vtd_page_walk_one(info->domain_id, > > >>> entry->iova, > > >>> entry->translated_addr, > > >>> entry->addr_mask, > > >>> entry->perm); > > >>> - ret = hook_fn(entry, private); > > >>> + ret = hook_fn(event, private); > > >>> if (ret) { > > >>> return ret; > > >>> } > > >>> /* Drop any existing mapping */ > > >>> iova_tree_remove(as->iova_tree, &target); > > >>> - /* Recover the correct permission */ > > >>> + /* Recover the correct type */ > > >>> + event->type = IOMMU_NOTIFIER_MAP; > > >>> entry->perm = cache_perm; > > >>> } > > >>> } > > >>> @@ -1170,7 +1173,7 @@ static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) > > >>> trace_vtd_page_walk_one(info->domain_id, entry->iova, > > >>> entry->translated_addr, entry->addr_mask, > > >>> entry->perm); > > >>> - return hook_fn(entry, private); > > >>> + return hook_fn(event, private); > > >>> } > > >>> > > >>> /** > > >>> @@ -1191,7 +1194,7 @@ static int vtd_page_walk_level(dma_addr_t addr, uint64_t start, > > >>> uint32_t offset; > > >>> uint64_t slpte; > > >>> uint64_t subpage_size, subpage_mask; > > >>> - IOMMUTLBEntry entry; > > >>> + IOMMUTLBEvent event; > > >>> uint64_t iova = start; > > >>> uint64_t iova_next; > > >>> int ret = 0; > > >>> @@ -1245,13 +1248,15 @@ static int vtd_page_walk_level(dma_addr_t addr, uint64_t start, > > >>> * > > >>> * In either case, we send an IOTLB notification down. > > >>> */ > > >>> - entry.target_as = &address_space_memory; > > >>> - entry.iova = iova & subpage_mask; > > >>> - entry.perm = IOMMU_ACCESS_FLAG(read_cur, write_cur); > > >>> - entry.addr_mask = ~subpage_mask; > > >>> + event.entry.target_as = &address_space_memory; > > >>> + event.entry.iova = iova & subpage_mask; > > >>> + event.entry.perm = IOMMU_ACCESS_FLAG(read_cur, write_cur); > > >>> + event.entry.addr_mask = ~subpage_mask; > > >>> /* NOTE: this is only meaningful if entry_valid == true */ > > >>> - entry.translated_addr = vtd_get_slpte_addr(slpte, info->aw); > > >>> - ret = vtd_page_walk_one(&entry, info); > > >>> + event.entry.translated_addr = vtd_get_slpte_addr(slpte, info->aw); > > >>> + event.type = event.entry.perm ? IOMMU_NOTIFIER_MAP : > > >>> + IOMMU_NOTIFIER_UNMAP; > > >>> + ret = vtd_page_walk_one(&event, info); > > >>> } > > >>> > > >>> if (ret < 0) { > > >>> @@ -1430,10 +1435,10 @@ static int vtd_dev_to_context_entry(IntelIOMMUState *s, uint8_t bus_num, > > >>> return 0; > > >>> } > > >>> > > >>> -static int vtd_sync_shadow_page_hook(IOMMUTLBEntry *entry, > > >>> +static int vtd_sync_shadow_page_hook(IOMMUTLBEvent *event, > > >>> void *private) > > >>> { > > >>> - memory_region_notify_iommu((IOMMUMemoryRegion *)private, 0, *entry); > > >>> + memory_region_notify_iommu((IOMMUMemoryRegion *)private, 0, *event); > > >>> return 0; > > >>> } > > >>> > > >>> @@ -1993,14 +1998,17 @@ static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s, > > >>> * page tables. We just deliver the PSI down to > > >>> * invalidate caches. > > >>> */ > > >>> - IOMMUTLBEntry entry = { > > >>> - .target_as = &address_space_memory, > > >>> - .iova = addr, > > >>> - .translated_addr = 0, > > >>> - .addr_mask = size - 1, > > >>> - .perm = IOMMU_NONE, > > >>> + IOMMUTLBEvent event = { > > >>> + .type = IOMMU_NOTIFIER_UNMAP, > > >>> + .entry = { > > >>> + .target_as = &address_space_memory, > > >>> + .iova = addr, > > >>> + .translated_addr = 0, > > >>> + .addr_mask = size - 1, > > >>> + .perm = IOMMU_NONE, > > >>> + }, > > >>> }; > > >>> - memory_region_notify_iommu(&vtd_as->iommu, 0, entry); > > >>> + memory_region_notify_iommu(&vtd_as->iommu, 0, event); > > >>> } > > >>> } > > >>> } > > >>> @@ -2412,7 +2420,7 @@ static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s, > > >>> VTDInvDesc *inv_desc) > > >>> { > > >>> VTDAddressSpace *vtd_dev_as; > > >>> - IOMMUTLBEntry entry; > > >>> + IOMMUTLBEvent event; > > >>> struct VTDBus *vtd_bus; > > >>> hwaddr addr; > > >>> uint64_t sz; > > >>> @@ -2460,12 +2468,13 @@ static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s, > > >>> sz = VTD_PAGE_SIZE; > > >>> } > > >>> > > >>> - entry.target_as = &vtd_dev_as->as; > > >>> - entry.addr_mask = sz - 1; > > >>> - entry.iova = addr; > > >>> - entry.perm = IOMMU_NONE; > > >>> - entry.translated_addr = 0; > > >>> - memory_region_notify_iommu(&vtd_dev_as->iommu, 0, entry); > > >>> + event.type = IOMMU_NOTIFIER_UNMAP; > > >>> + event.entry.target_as = &vtd_dev_as->as; > > >>> + event.entry.addr_mask = sz - 1; > > >>> + event.entry.iova = addr; > > >>> + event.entry.perm = IOMMU_NONE; > > >>> + event.entry.translated_addr = 0; > > >>> + memory_region_notify_iommu(&vtd_dev_as->iommu, 0, event); > > >>> > > >>> done: > > >>> return true; > > >>> @@ -3486,19 +3495,20 @@ static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n) > > >>> size = remain = end - start + 1; > > >>> > > >>> while (remain >= VTD_PAGE_SIZE) { > > >>> - IOMMUTLBEntry entry; > > >>> + IOMMUTLBEvent event; > > >>> uint64_t mask = get_naturally_aligned_size(start, remain, s->aw_bits); > > >>> > > >>> assert(mask); > > >>> > > >>> - entry.iova = start; > > >>> - entry.addr_mask = mask - 1; > > >>> - entry.target_as = &address_space_memory; > > >>> - entry.perm = IOMMU_NONE; > > >>> + event.type = IOMMU_NOTIFIER_UNMAP; > > >>> + event.entry.iova = start; > > >>> + event.entry.addr_mask = mask - 1; > > >>> + event.entry.target_as = &address_space_memory; > > >>> + event.entry.perm = IOMMU_NONE; > > >>> /* This field is meaningless for unmap */ > > >>> - entry.translated_addr = 0; > > >>> + event.entry.translated_addr = 0; > > >>> > > >>> - memory_region_notify_iommu_one(n, &entry); > > >>> + memory_region_notify_iommu_one(n, &event); > > >>> > > >>> start += mask; > > >>> remain -= mask; > > >>> @@ -3534,9 +3544,9 @@ static void vtd_address_space_refresh_all(IntelIOMMUState *s) > > >>> vtd_switch_address_space_all(s); > > >>> } > > >>> > > >>> -static int vtd_replay_hook(IOMMUTLBEntry *entry, void *private) > > >>> +static int vtd_replay_hook(IOMMUTLBEvent *event, void *private) > > >>> { > > >>> - memory_region_notify_iommu_one((IOMMUNotifier *)private, entry); > > >>> + memory_region_notify_iommu_one((IOMMUNotifier *)private, event); > > >>> return 0; > > >>> } > > >>> > > >>> diff --git a/hw/misc/tz-mpc.c b/hw/misc/tz-mpc.c > > >>> index 98f151237f..30481e1c90 100644 > > >>> --- a/hw/misc/tz-mpc.c > > >>> +++ b/hw/misc/tz-mpc.c > > >>> @@ -82,8 +82,10 @@ static void tz_mpc_iommu_notify(TZMPC *s, uint32_t lutidx, > > >>> /* Called when the LUT word at lutidx has changed from oldlut to newlut; > > >>> * must call the IOMMU notifiers for the changed blocks. > > >>> */ > > >>> - IOMMUTLBEntry entry = { > > >>> - .addr_mask = s->blocksize - 1, > > >>> + IOMMUTLBEvent event = { > > >>> + .entry = { > > >>> + .addr_mask = s->blocksize - 1, > > >>> + } > > >>> }; > > >>> hwaddr addr = lutidx * s->blocksize * 32; > > >>> int i; > > >>> @@ -100,26 +102,28 @@ static void tz_mpc_iommu_notify(TZMPC *s, uint32_t lutidx, > > >>> block_is_ns = newlut & (1 << i); > > >>> > > >>> trace_tz_mpc_iommu_notify(addr); > > >>> - entry.iova = addr; > > >>> - entry.translated_addr = addr; > > >>> + event.entry.iova = addr; > > >>> + event.entry.translated_addr = addr; > > >>> > > >>> - entry.perm = IOMMU_NONE; > > >>> - memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, entry); > > >>> - memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, entry); > > >>> + event.type = IOMMU_NOTIFIER_UNMAP; > > >>> + event.entry.perm = IOMMU_NONE; > > >>> + memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, event); > > >>> + memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, event); > > >>> > > >>> - entry.perm = IOMMU_RW; > > >>> + event.type = IOMMU_NOTIFIER_MAP; > > >>> + event.entry.perm = IOMMU_RW; > > >>> if (block_is_ns) { > > >>> - entry.target_as = &s->blocked_io_as; > > >>> + event.entry.target_as = &s->blocked_io_as; > > >>> } else { > > >>> - entry.target_as = &s->downstream_as; > > >>> + event.entry.target_as = &s->downstream_as; > > >>> } > > >>> - memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, entry); > > >>> + memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, event); > > >>> if (block_is_ns) { > > >>> - entry.target_as = &s->downstream_as; > > >>> + event.entry.target_as = &s->downstream_as; > > >>> } else { > > >>> - entry.target_as = &s->blocked_io_as; > > >>> + event.entry.target_as = &s->blocked_io_as; > > >>> } > > >>> - memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, entry); > > >>> + memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, event); > > >>> } > > >>> } > > >>> > > >>> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c > > >>> index 0fecabc135..8bc3cff05d 100644 > > >>> --- a/hw/ppc/spapr_iommu.c > > >>> +++ b/hw/ppc/spapr_iommu.c > > >>> @@ -445,7 +445,7 @@ static void spapr_tce_reset(DeviceState *dev) > > >>> static target_ulong put_tce_emu(SpaprTceTable *tcet, target_ulong ioba, > > >>> target_ulong tce) > > >>> { > > >>> - IOMMUTLBEntry entry; > > >>> + IOMMUTLBEvent event; > > >>> hwaddr page_mask = IOMMU_PAGE_MASK(tcet->page_shift); > > >>> unsigned long index = (ioba - tcet->bus_offset) >> tcet->page_shift; > > >>> > > >>> @@ -457,12 +457,13 @@ static target_ulong put_tce_emu(SpaprTceTable *tcet, target_ulong ioba, > > >>> > > >>> tcet->table[index] = tce; > > >>> > > >>> - entry.target_as = &address_space_memory, > > >>> - entry.iova = (ioba - tcet->bus_offset) & page_mask; > > >>> - entry.translated_addr = tce & page_mask; > > >>> - entry.addr_mask = ~page_mask; > > >>> - entry.perm = spapr_tce_iommu_access_flags(tce); > > >>> - memory_region_notify_iommu(&tcet->iommu, 0, entry); > > >>> + event.entry.target_as = &address_space_memory, > > >>> + event.entry.iova = (ioba - tcet->bus_offset) & page_mask; > > >>> + event.entry.translated_addr = tce & page_mask; > > >>> + event.entry.addr_mask = ~page_mask; > > >>> + event.entry.perm = spapr_tce_iommu_access_flags(tce); > > >>> + event.type = entry.perm ? IOMMU_NOTIFIER_MAP : IOMMU_NOTIFIER_UNMAP; > > >>> + memory_region_notify_iommu(&tcet->iommu, 0, event); > > >>> > > >>> return H_SUCCESS; > > >>> } > > >>> diff --git a/include/exec/memory.h b/include/exec/memory.h > > >>> index 22c5f564d1..8a56707169 100644 > > >>> --- a/include/exec/memory.h > > >>> +++ b/include/exec/memory.h > > >>> @@ -70,11 +70,11 @@ typedef enum { > > >>> #define IOMMU_ACCESS_FLAG(r, w) (((r) ? IOMMU_RO : 0) | ((w) ? IOMMU_WO : 0)) > > >>> > > >>> struct IOMMUTLBEntry { > > >>> - AddressSpace *target_as; > > >>> - hwaddr iova; > > >>> - hwaddr translated_addr; > > >>> - hwaddr addr_mask; /* 0xfff = 4k translation */ > > >>> - IOMMUAccessFlags perm; > > >>> + AddressSpace *target_as; > > >>> + hwaddr iova; > > >>> + hwaddr translated_addr; > > >>> + hwaddr addr_mask; /* 0xfff = 4k translation */ > > >>> + IOMMUAccessFlags perm; > > >>> }; > > >>> > > >>> /* > > >>> @@ -106,6 +106,11 @@ struct IOMMUNotifier { > > >>> }; > > >>> typedef struct IOMMUNotifier IOMMUNotifier; > > >>> > > >>> +typedef struct IOMMUTLBEvent { > > >>> + IOMMUTLBEntry entry; > > >>> + IOMMUNotifierFlag type; > > >> nit: I would put the type field before the entry > > >>> +} IOMMUTLBEvent; > > >>> + > > >>> /* RAM is pre-allocated and passed into qemu_ram_alloc_from_ptr */ > > >>> #define RAM_PREALLOC (1 << 0) > > >>> > > >>> @@ -1265,13 +1270,13 @@ uint64_t memory_region_iommu_get_min_page_size(IOMMUMemoryRegion *iommu_mr); > > >>> * > > >> > > >> Above there is: > > >> > > >> * The notification type will be decided by entry.perm bits: > > >> * > > >> * - For UNMAP (cache invalidation) notifies: set entry.perm to IOMMU_NONE. > > >> * - For MAP (newly added entry) notifies: set entry.perm to the > > >> * permission of the page (which is definitely !IOMMU_NONE). > > >> > > >> I think this should be updated as the main selector now is the type. > > >> > > > > > > Definitely it needs to be updated, thanks! > > > > > >> I am a little bit concerned by the fact nothing really checks perm = > > >> IOMMU_NONE is consistent with UNMAP type. New call sites may forget > > >> this? shouldn't we check this in memory_region_notify_(one)_iommu? > > >> > > > > > > So it should be not checked that way from now on, but by the use of > > > `IOMMUTLBEvent.type`. So the caller should be responsible for set it > > > properly. > > > > > > Another way of making it explicit is by making that type a parameter > > > of memory_region_notify_(one)_iommu, and to keep IOMMUTLBEvent private > > > tomemory.c, but I'm not sure if the notification will need more and > > > more parameters, and I felt it scale better this way. > > Yep just wanted to share my concern. UNMAP necessary means > > perm=IOMMU_NONE. New callers may forget this ... an assert may enforce > > it but well. > > And yes, I think it is an improvement to add the assertion here. Thanks! > > Thanks > > > > Eric > > > > > > Thanks! > > > > > >>> * @iommu_mr: the memory region that was changed > > >>> * @iommu_idx: the IOMMU index for the translation table which has changed > > >>> - * @entry: the new entry in the IOMMU translation table. The entry > > >>> - * replaces all old entries for the same virtual I/O address range. > > >>> - * Deleted entries have .@perm == 0. > > >>> + * @event: TLB event with the new entry in the IOMMU translation table. > > >>> + * The entry replaces all old entries for the same virtual I/O address > > >>> + * range. > > >>> */ > > >>> void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, > > >>> int iommu_idx, > > >>> - IOMMUTLBEntry entry); > > >>> + IOMMUTLBEvent event); > > >>> > > >>> /** > > >>> * memory_region_notify_iommu_one: notify a change in an IOMMU translation > > >>> @@ -1281,12 +1286,12 @@ void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, > > >>> * notifies a specific notifier, not all of them. > > >>> * > > >>> * @notifier: the notifier to be notified > > >>> - * @entry: the new entry in the IOMMU translation table. The entry > > >>> - * replaces all old entries for the same virtual I/O address range. > > >>> - * Deleted entries have .@perm == 0. > > >>> + * @event: TLB event with the new entry in the IOMMU translation table. > > >>> + * The entry replaces all old entries for the same virtual I/O address > > >>> + * range. > > >>> */ > > >>> void memory_region_notify_iommu_one(IOMMUNotifier *notifier, > > >>> - IOMMUTLBEntry *entry); > > >>> + IOMMUTLBEvent *event); > > >>> > > >>> /** > > >>> * memory_region_register_iommu_notifier: register a notifier for changes to > > >>> diff --git a/softmmu/memory.c b/softmmu/memory.c > > >>> index 961c25b42f..09b3443eac 100644 > > >>> --- a/softmmu/memory.c > > >>> +++ b/softmmu/memory.c > > >>> @@ -1891,9 +1891,9 @@ void memory_region_unregister_iommu_notifier(MemoryRegion *mr, > > >>> } > > >>> > > >>> void memory_region_notify_iommu_one(IOMMUNotifier *notifier, > > >>> - IOMMUTLBEntry *entry) > > >>> + IOMMUTLBEvent *event) > > >>> { > > >>> - IOMMUNotifierFlag request_flags; > > >>> + IOMMUTLBEntry *entry = &event->entry; > > >>> hwaddr entry_end = entry->iova + entry->addr_mask; > > >>> > > >>> /* > > >>> @@ -1906,20 +1906,14 @@ void memory_region_notify_iommu_one(IOMMUNotifier *notifier, > > >>> > > >>> assert(entry->iova >= notifier->start && entry_end <= notifier->end); > > >>> > > >>> - if (entry->perm & IOMMU_RW) { > > >>> - request_flags = IOMMU_NOTIFIER_MAP; > > >>> - } else { > > >>> - request_flags = IOMMU_NOTIFIER_UNMAP; > > >>> - } > > >>> - > > >>> - if (notifier->notifier_flags & request_flags) { > > >>> + if (event->type & notifier->notifier_flags) { > > >>> notifier->notify(notifier, entry); > > >>> } > > >>> } > > >>> > > >>> void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, > > >>> int iommu_idx, > > >>> - IOMMUTLBEntry entry) > > >>> + IOMMUTLBEvent event) > > >>> { > > >>> IOMMUNotifier *iommu_notifier; > > >>> > > >>> @@ -1927,7 +1921,7 @@ void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, > > >>> > > >>> IOMMU_NOTIFIER_FOREACH(iommu_notifier, iommu_mr) { > > >>> if (iommu_notifier->iommu_idx == iommu_idx) { > > >>> - memory_region_notify_iommu_one(iommu_notifier, &entry); > > >>> + memory_region_notify_iommu_one(iommu_notifier, &event); > > >>> } > > >>> } > > >>> } > > >>> > > >> Thanks > > >> > > >> Eric > > >> > > > > >
diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c index 88d2c454f0..405d5c5325 100644 --- a/hw/arm/smmu-common.c +++ b/hw/arm/smmu-common.c @@ -465,14 +465,15 @@ IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid) /* Unmap the whole notifier's range */ static void smmu_unmap_notifier_range(IOMMUNotifier *n) { - IOMMUTLBEntry entry; + IOMMUTLBEvent event; - entry.target_as = &address_space_memory; - entry.iova = n->start; - entry.perm = IOMMU_NONE; - entry.addr_mask = n->end - n->start; + event.type = IOMMU_NOTIFIER_UNMAP; + event.entry.target_as = &address_space_memory; + event.entry.iova = n->start; + event.entry.perm = IOMMU_NONE; + event.entry.addr_mask = n->end - n->start; - memory_region_notify_iommu_one(n, &entry); + memory_region_notify_iommu_one(n, &event); } /* Unmap all notifiers attached to @mr */ diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c index 0a893ae918..62b0e289ca 100644 --- a/hw/arm/smmuv3.c +++ b/hw/arm/smmuv3.c @@ -799,7 +799,7 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr, uint8_t tg, uint64_t num_pages) { SMMUDevice *sdev = container_of(mr, SMMUDevice, iommu); - IOMMUTLBEntry entry; + IOMMUTLBEvent event; uint8_t granule = tg; if (!tg) { @@ -822,12 +822,13 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr, granule = tt->granule_sz; } - entry.target_as = &address_space_memory; - entry.iova = iova; - entry.addr_mask = num_pages * (1 << granule) - 1; - entry.perm = IOMMU_NONE; + event.type = IOMMU_NOTIFIER_UNMAP; + event.entry.target_as = &address_space_memory; + event.entry.iova = iova; + event.entry.addr_mask = num_pages * (1 << granule) - 1; + event.entry.perm = IOMMU_NONE; - memory_region_notify_iommu_one(n, &entry); + memory_region_notify_iommu_one(n, &event); } /* invalidate an asid/iova range tuple in all mr's */ diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 2ad6b9d796..0c4aef5cb5 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -1073,7 +1073,7 @@ static int vtd_iova_to_slpte(IntelIOMMUState *s, VTDContextEntry *ce, } } -typedef int (*vtd_page_walk_hook)(IOMMUTLBEntry *entry, void *private); +typedef int (*vtd_page_walk_hook)(IOMMUTLBEvent *event, void *private); /** * Constant information used during page walking @@ -1094,11 +1094,12 @@ typedef struct { uint16_t domain_id; } vtd_page_walk_info; -static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) +static int vtd_page_walk_one(IOMMUTLBEvent *event, vtd_page_walk_info *info) { VTDAddressSpace *as = info->as; vtd_page_walk_hook hook_fn = info->hook_fn; void *private = info->private; + IOMMUTLBEntry *entry = &event->entry; DMAMap target = { .iova = entry->iova, .size = entry->addr_mask, @@ -1107,7 +1108,7 @@ static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) }; DMAMap *mapped = iova_tree_find(as->iova_tree, &target); - if (entry->perm == IOMMU_NONE && !info->notify_unmap) { + if (event->type == IOMMU_NOTIFIER_UNMAP && !info->notify_unmap) { trace_vtd_page_walk_one_skip_unmap(entry->iova, entry->addr_mask); return 0; } @@ -1115,7 +1116,7 @@ static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) assert(hook_fn); /* Update local IOVA mapped ranges */ - if (entry->perm) { + if (event->type == IOMMU_NOTIFIER_MAP) { if (mapped) { /* If it's exactly the same translation, skip */ if (!memcmp(mapped, &target, sizeof(target))) { @@ -1141,19 +1142,21 @@ static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) int ret; /* Emulate an UNMAP */ + event->type = IOMMU_NOTIFIER_UNMAP; entry->perm = IOMMU_NONE; trace_vtd_page_walk_one(info->domain_id, entry->iova, entry->translated_addr, entry->addr_mask, entry->perm); - ret = hook_fn(entry, private); + ret = hook_fn(event, private); if (ret) { return ret; } /* Drop any existing mapping */ iova_tree_remove(as->iova_tree, &target); - /* Recover the correct permission */ + /* Recover the correct type */ + event->type = IOMMU_NOTIFIER_MAP; entry->perm = cache_perm; } } @@ -1170,7 +1173,7 @@ static int vtd_page_walk_one(IOMMUTLBEntry *entry, vtd_page_walk_info *info) trace_vtd_page_walk_one(info->domain_id, entry->iova, entry->translated_addr, entry->addr_mask, entry->perm); - return hook_fn(entry, private); + return hook_fn(event, private); } /** @@ -1191,7 +1194,7 @@ static int vtd_page_walk_level(dma_addr_t addr, uint64_t start, uint32_t offset; uint64_t slpte; uint64_t subpage_size, subpage_mask; - IOMMUTLBEntry entry; + IOMMUTLBEvent event; uint64_t iova = start; uint64_t iova_next; int ret = 0; @@ -1245,13 +1248,15 @@ static int vtd_page_walk_level(dma_addr_t addr, uint64_t start, * * In either case, we send an IOTLB notification down. */ - entry.target_as = &address_space_memory; - entry.iova = iova & subpage_mask; - entry.perm = IOMMU_ACCESS_FLAG(read_cur, write_cur); - entry.addr_mask = ~subpage_mask; + event.entry.target_as = &address_space_memory; + event.entry.iova = iova & subpage_mask; + event.entry.perm = IOMMU_ACCESS_FLAG(read_cur, write_cur); + event.entry.addr_mask = ~subpage_mask; /* NOTE: this is only meaningful if entry_valid == true */ - entry.translated_addr = vtd_get_slpte_addr(slpte, info->aw); - ret = vtd_page_walk_one(&entry, info); + event.entry.translated_addr = vtd_get_slpte_addr(slpte, info->aw); + event.type = event.entry.perm ? IOMMU_NOTIFIER_MAP : + IOMMU_NOTIFIER_UNMAP; + ret = vtd_page_walk_one(&event, info); } if (ret < 0) { @@ -1430,10 +1435,10 @@ static int vtd_dev_to_context_entry(IntelIOMMUState *s, uint8_t bus_num, return 0; } -static int vtd_sync_shadow_page_hook(IOMMUTLBEntry *entry, +static int vtd_sync_shadow_page_hook(IOMMUTLBEvent *event, void *private) { - memory_region_notify_iommu((IOMMUMemoryRegion *)private, 0, *entry); + memory_region_notify_iommu((IOMMUMemoryRegion *)private, 0, *event); return 0; } @@ -1993,14 +1998,17 @@ static void vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s, * page tables. We just deliver the PSI down to * invalidate caches. */ - IOMMUTLBEntry entry = { - .target_as = &address_space_memory, - .iova = addr, - .translated_addr = 0, - .addr_mask = size - 1, - .perm = IOMMU_NONE, + IOMMUTLBEvent event = { + .type = IOMMU_NOTIFIER_UNMAP, + .entry = { + .target_as = &address_space_memory, + .iova = addr, + .translated_addr = 0, + .addr_mask = size - 1, + .perm = IOMMU_NONE, + }, }; - memory_region_notify_iommu(&vtd_as->iommu, 0, entry); + memory_region_notify_iommu(&vtd_as->iommu, 0, event); } } } @@ -2412,7 +2420,7 @@ static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc) { VTDAddressSpace *vtd_dev_as; - IOMMUTLBEntry entry; + IOMMUTLBEvent event; struct VTDBus *vtd_bus; hwaddr addr; uint64_t sz; @@ -2460,12 +2468,13 @@ static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s, sz = VTD_PAGE_SIZE; } - entry.target_as = &vtd_dev_as->as; - entry.addr_mask = sz - 1; - entry.iova = addr; - entry.perm = IOMMU_NONE; - entry.translated_addr = 0; - memory_region_notify_iommu(&vtd_dev_as->iommu, 0, entry); + event.type = IOMMU_NOTIFIER_UNMAP; + event.entry.target_as = &vtd_dev_as->as; + event.entry.addr_mask = sz - 1; + event.entry.iova = addr; + event.entry.perm = IOMMU_NONE; + event.entry.translated_addr = 0; + memory_region_notify_iommu(&vtd_dev_as->iommu, 0, event); done: return true; @@ -3486,19 +3495,20 @@ static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n) size = remain = end - start + 1; while (remain >= VTD_PAGE_SIZE) { - IOMMUTLBEntry entry; + IOMMUTLBEvent event; uint64_t mask = get_naturally_aligned_size(start, remain, s->aw_bits); assert(mask); - entry.iova = start; - entry.addr_mask = mask - 1; - entry.target_as = &address_space_memory; - entry.perm = IOMMU_NONE; + event.type = IOMMU_NOTIFIER_UNMAP; + event.entry.iova = start; + event.entry.addr_mask = mask - 1; + event.entry.target_as = &address_space_memory; + event.entry.perm = IOMMU_NONE; /* This field is meaningless for unmap */ - entry.translated_addr = 0; + event.entry.translated_addr = 0; - memory_region_notify_iommu_one(n, &entry); + memory_region_notify_iommu_one(n, &event); start += mask; remain -= mask; @@ -3534,9 +3544,9 @@ static void vtd_address_space_refresh_all(IntelIOMMUState *s) vtd_switch_address_space_all(s); } -static int vtd_replay_hook(IOMMUTLBEntry *entry, void *private) +static int vtd_replay_hook(IOMMUTLBEvent *event, void *private) { - memory_region_notify_iommu_one((IOMMUNotifier *)private, entry); + memory_region_notify_iommu_one((IOMMUNotifier *)private, event); return 0; } diff --git a/hw/misc/tz-mpc.c b/hw/misc/tz-mpc.c index 98f151237f..30481e1c90 100644 --- a/hw/misc/tz-mpc.c +++ b/hw/misc/tz-mpc.c @@ -82,8 +82,10 @@ static void tz_mpc_iommu_notify(TZMPC *s, uint32_t lutidx, /* Called when the LUT word at lutidx has changed from oldlut to newlut; * must call the IOMMU notifiers for the changed blocks. */ - IOMMUTLBEntry entry = { - .addr_mask = s->blocksize - 1, + IOMMUTLBEvent event = { + .entry = { + .addr_mask = s->blocksize - 1, + } }; hwaddr addr = lutidx * s->blocksize * 32; int i; @@ -100,26 +102,28 @@ static void tz_mpc_iommu_notify(TZMPC *s, uint32_t lutidx, block_is_ns = newlut & (1 << i); trace_tz_mpc_iommu_notify(addr); - entry.iova = addr; - entry.translated_addr = addr; + event.entry.iova = addr; + event.entry.translated_addr = addr; - entry.perm = IOMMU_NONE; - memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, entry); - memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, entry); + event.type = IOMMU_NOTIFIER_UNMAP; + event.entry.perm = IOMMU_NONE; + memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, event); + memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, event); - entry.perm = IOMMU_RW; + event.type = IOMMU_NOTIFIER_MAP; + event.entry.perm = IOMMU_RW; if (block_is_ns) { - entry.target_as = &s->blocked_io_as; + event.entry.target_as = &s->blocked_io_as; } else { - entry.target_as = &s->downstream_as; + event.entry.target_as = &s->downstream_as; } - memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, entry); + memory_region_notify_iommu(&s->upstream, IOMMU_IDX_S, event); if (block_is_ns) { - entry.target_as = &s->downstream_as; + event.entry.target_as = &s->downstream_as; } else { - entry.target_as = &s->blocked_io_as; + event.entry.target_as = &s->blocked_io_as; } - memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, entry); + memory_region_notify_iommu(&s->upstream, IOMMU_IDX_NS, event); } } diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c index 0fecabc135..8bc3cff05d 100644 --- a/hw/ppc/spapr_iommu.c +++ b/hw/ppc/spapr_iommu.c @@ -445,7 +445,7 @@ static void spapr_tce_reset(DeviceState *dev) static target_ulong put_tce_emu(SpaprTceTable *tcet, target_ulong ioba, target_ulong tce) { - IOMMUTLBEntry entry; + IOMMUTLBEvent event; hwaddr page_mask = IOMMU_PAGE_MASK(tcet->page_shift); unsigned long index = (ioba - tcet->bus_offset) >> tcet->page_shift; @@ -457,12 +457,13 @@ static target_ulong put_tce_emu(SpaprTceTable *tcet, target_ulong ioba, tcet->table[index] = tce; - entry.target_as = &address_space_memory, - entry.iova = (ioba - tcet->bus_offset) & page_mask; - entry.translated_addr = tce & page_mask; - entry.addr_mask = ~page_mask; - entry.perm = spapr_tce_iommu_access_flags(tce); - memory_region_notify_iommu(&tcet->iommu, 0, entry); + event.entry.target_as = &address_space_memory, + event.entry.iova = (ioba - tcet->bus_offset) & page_mask; + event.entry.translated_addr = tce & page_mask; + event.entry.addr_mask = ~page_mask; + event.entry.perm = spapr_tce_iommu_access_flags(tce); + event.type = entry.perm ? IOMMU_NOTIFIER_MAP : IOMMU_NOTIFIER_UNMAP; + memory_region_notify_iommu(&tcet->iommu, 0, event); return H_SUCCESS; } diff --git a/include/exec/memory.h b/include/exec/memory.h index 22c5f564d1..8a56707169 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -70,11 +70,11 @@ typedef enum { #define IOMMU_ACCESS_FLAG(r, w) (((r) ? IOMMU_RO : 0) | ((w) ? IOMMU_WO : 0)) struct IOMMUTLBEntry { - AddressSpace *target_as; - hwaddr iova; - hwaddr translated_addr; - hwaddr addr_mask; /* 0xfff = 4k translation */ - IOMMUAccessFlags perm; + AddressSpace *target_as; + hwaddr iova; + hwaddr translated_addr; + hwaddr addr_mask; /* 0xfff = 4k translation */ + IOMMUAccessFlags perm; }; /* @@ -106,6 +106,11 @@ struct IOMMUNotifier { }; typedef struct IOMMUNotifier IOMMUNotifier; +typedef struct IOMMUTLBEvent { + IOMMUTLBEntry entry; + IOMMUNotifierFlag type; +} IOMMUTLBEvent; + /* RAM is pre-allocated and passed into qemu_ram_alloc_from_ptr */ #define RAM_PREALLOC (1 << 0) @@ -1265,13 +1270,13 @@ uint64_t memory_region_iommu_get_min_page_size(IOMMUMemoryRegion *iommu_mr); * * @iommu_mr: the memory region that was changed * @iommu_idx: the IOMMU index for the translation table which has changed - * @entry: the new entry in the IOMMU translation table. The entry - * replaces all old entries for the same virtual I/O address range. - * Deleted entries have .@perm == 0. + * @event: TLB event with the new entry in the IOMMU translation table. + * The entry replaces all old entries for the same virtual I/O address + * range. */ void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, int iommu_idx, - IOMMUTLBEntry entry); + IOMMUTLBEvent event); /** * memory_region_notify_iommu_one: notify a change in an IOMMU translation @@ -1281,12 +1286,12 @@ void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, * notifies a specific notifier, not all of them. * * @notifier: the notifier to be notified - * @entry: the new entry in the IOMMU translation table. The entry - * replaces all old entries for the same virtual I/O address range. - * Deleted entries have .@perm == 0. + * @event: TLB event with the new entry in the IOMMU translation table. + * The entry replaces all old entries for the same virtual I/O address + * range. */ void memory_region_notify_iommu_one(IOMMUNotifier *notifier, - IOMMUTLBEntry *entry); + IOMMUTLBEvent *event); /** * memory_region_register_iommu_notifier: register a notifier for changes to diff --git a/softmmu/memory.c b/softmmu/memory.c index 961c25b42f..09b3443eac 100644 --- a/softmmu/memory.c +++ b/softmmu/memory.c @@ -1891,9 +1891,9 @@ void memory_region_unregister_iommu_notifier(MemoryRegion *mr, } void memory_region_notify_iommu_one(IOMMUNotifier *notifier, - IOMMUTLBEntry *entry) + IOMMUTLBEvent *event) { - IOMMUNotifierFlag request_flags; + IOMMUTLBEntry *entry = &event->entry; hwaddr entry_end = entry->iova + entry->addr_mask; /* @@ -1906,20 +1906,14 @@ void memory_region_notify_iommu_one(IOMMUNotifier *notifier, assert(entry->iova >= notifier->start && entry_end <= notifier->end); - if (entry->perm & IOMMU_RW) { - request_flags = IOMMU_NOTIFIER_MAP; - } else { - request_flags = IOMMU_NOTIFIER_UNMAP; - } - - if (notifier->notifier_flags & request_flags) { + if (event->type & notifier->notifier_flags) { notifier->notify(notifier, entry); } } void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, int iommu_idx, - IOMMUTLBEntry entry) + IOMMUTLBEvent event) { IOMMUNotifier *iommu_notifier; @@ -1927,7 +1921,7 @@ void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, IOMMU_NOTIFIER_FOREACH(iommu_notifier, iommu_mr) { if (iommu_notifier->iommu_idx == iommu_idx) { - memory_region_notify_iommu_one(iommu_notifier, &entry); + memory_region_notify_iommu_one(iommu_notifier, &event); } } }
This way we can tell between regulars IOMMURLBEntries (entry of IOMMU hardware) and notifications. In the notifications, we set explicitly if it is a MAPs or an UNMAP, instead of trusting in entry permissions to differenciate them. Signed-off-by: Eugenio Pérez <eperezma@redhat.com> --- hw/arm/smmu-common.c | 13 ++++--- hw/arm/smmuv3.c | 13 ++++--- hw/i386/intel_iommu.c | 88 ++++++++++++++++++++++++------------------- hw/misc/tz-mpc.c | 32 +++++++++------- hw/ppc/spapr_iommu.c | 15 ++++---- include/exec/memory.h | 31 ++++++++------- softmmu/memory.c | 16 +++----- 7 files changed, 112 insertions(+), 96 deletions(-)