[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC v8 2/5] memory: Add IOMMUTLBEvent
|
From: |
Eugenio Perez Martin |
|
Subject: |
Re: [RFC v8 2/5] memory: Add IOMMUTLBEvent |
|
Date: |
Wed, 2 Sep 2020 15:58:00 +0200 |
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
> >>
> >
>
- Re: [RFC v8 1/5] memory: Rename memory_region_notify_one to memory_region_notify_iommu_one, (continued)
[RFC v8 3/5] memory: Add IOMMU_DEVIOTLB_UNMAP IOMMUTLBNotificationType, Eugenio Pérez, 2020/09/01
[RFC v8 5/5] memory: Skip bad range assertion if notifier is DEVIOTLB type, Eugenio Pérez, 2020/09/01