| Message ID | 20180521140402.23318-16-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iommu: support txattrs, support TCG execution, implement TZ MPC | expand |
On 05/21/2018 07:03 AM, Peter Maydell wrote: > Add support for multiple IOMMU indexes to the IOMMU notifier APIs. > When initializing a notifier with iommu_notifier_init(), the caller > must pass the IOMMU index that it is interested in. When a change > happens, the IOMMU implementation must pass > memory_region_notify_iommu() the IOMMU index that has changed and > that notifiers must be called for. > > IOMMUs which support only a single index don't need to change. > Callers which only really support working with IOMMUs with a single > index can use the result of passing MEMTXATTRS_UNSPECIFIED to > memory_region_iommu_attrs_to_index(). > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
Peter Maydell <peter.maydell@linaro.org> writes: > Add support for multiple IOMMU indexes to the IOMMU notifier APIs. > When initializing a notifier with iommu_notifier_init(), the caller > must pass the IOMMU index that it is interested in. When a change > happens, the IOMMU implementation must pass > memory_region_notify_iommu() the IOMMU index that has changed and > that notifiers must be called for. > > IOMMUs which support only a single index don't need to change. > Callers which only really support working with IOMMUs with a single > index can use the result of passing MEMTXATTRS_UNSPECIFIED to > memory_region_iommu_attrs_to_index(). > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > include/exec/memory.h | 11 ++++++++++- > hw/i386/intel_iommu.c | 4 ++-- > hw/ppc/spapr_iommu.c | 2 +- > hw/s390x/s390-pci-inst.c | 4 ++-- > hw/vfio/common.c | 6 +++++- > hw/virtio/vhost.c | 7 ++++++- > memory.c | 8 +++++++- > 7 files changed, 33 insertions(+), 9 deletions(-) > > diff --git a/include/exec/memory.h b/include/exec/memory.h > index f6226fb263..4e6b125add 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -71,6 +71,7 @@ struct IOMMUTLBEntry { > hwaddr iova; > hwaddr translated_addr; > hwaddr addr_mask; /* 0xfff = 4k translation */ > + int iommu_idx; > IOMMUAccessFlags perm; > }; > > @@ -98,18 +99,21 @@ struct IOMMUNotifier { > /* Notify for address space range start <= addr <= end */ > hwaddr start; > hwaddr end; > + int iommu_idx; Its a minor thing but are we ever expecting iommu_idx to ever be negative? > --- a/memory.c > +++ b/memory.c > @@ -1802,6 +1802,9 @@ void memory_region_register_iommu_notifier(MemoryRegion *mr, > iommu_mr = IOMMU_MEMORY_REGION(mr); > assert(n->notifier_flags != IOMMU_NOTIFIER_NONE); > assert(n->start <= n->end); > + assert(n->iommu_idx >= 0 && > + n->iommu_idx < memory_region_iommu_num_indexes(iommu_mr)); > + And then this would only have to assert we haven't exceeded the upper bound. -- Alex Bennée
Hi Peter, On 05/21/2018 04:03 PM, Peter Maydell wrote: > Add support for multiple IOMMU indexes to the IOMMU notifier APIs. > When initializing a notifier with iommu_notifier_init(), the caller > must pass the IOMMU index that it is interested in. When a change > happens, the IOMMU implementation must pass > memory_region_notify_iommu() the IOMMU index that has changed and > that notifiers must be called for. > > IOMMUs which support only a single index don't need to change. > Callers which only really support working with IOMMUs with a single > index can use the result of passing MEMTXATTRS_UNSPECIFIED to > memory_region_iommu_attrs_to_index(). > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > include/exec/memory.h | 11 ++++++++++- > hw/i386/intel_iommu.c | 4 ++-- > hw/ppc/spapr_iommu.c | 2 +- > hw/s390x/s390-pci-inst.c | 4 ++-- > hw/vfio/common.c | 6 +++++- > hw/virtio/vhost.c | 7 ++++++- > memory.c | 8 +++++++- > 7 files changed, 33 insertions(+), 9 deletions(-) > > diff --git a/include/exec/memory.h b/include/exec/memory.h > index f6226fb263..4e6b125add 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -71,6 +71,7 @@ struct IOMMUTLBEntry { > hwaddr iova; > hwaddr translated_addr; > hwaddr addr_mask; /* 0xfff = 4k translation */ > + int iommu_idx; I don't get why ne need iommu_idx field here. On translate the caller has it. On notify the notifier has it? > IOMMUAccessFlags perm; > }; > > @@ -98,18 +99,21 @@ struct IOMMUNotifier { > /* Notify for address space range start <= addr <= end */ > hwaddr start; > hwaddr end; > + int iommu_idx; > QLIST_ENTRY(IOMMUNotifier) node; > }; > typedef struct IOMMUNotifier IOMMUNotifier; > > static inline void iommu_notifier_init(IOMMUNotifier *n, IOMMUNotify fn, > IOMMUNotifierFlag flags, > - hwaddr start, hwaddr end) > + hwaddr start, hwaddr end, > + int iommu_idx) > { > n->notify = fn; > n->notifier_flags = flags; > n->start = start; > n->end = end; > + n->iommu_idx = iommu_idx; > } > > /* > @@ -323,7 +327,10 @@ typedef struct IOMMUMemoryRegionClass { > * Optional method: if this method is not provided, then > * memory_region_iommu_num_indexes() will return 1, indicating that > * only a single IOMMU index is supported. > + * > + * @iommu: the IOMMUMemoryRegion > */ > + int (*num_indexes)(IOMMUMemoryRegion *iommu); > } IOMMUMemoryRegionClass; > > typedef struct CoalescedMemoryRange CoalescedMemoryRange; > @@ -1028,11 +1035,13 @@ uint64_t memory_region_iommu_get_min_page_size(IOMMUMemoryRegion *iommu_mr); > * should be notified with an UNMAP followed by a MAP. > * > * @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. > */ > void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, > + int iommu_idx, > IOMMUTLBEntry entry); > > /** > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index fb31de9416..b8c9354b0b 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -1376,7 +1376,7 @@ static void vtd_iotlb_domain_invalidate(IntelIOMMUState *s, uint16_t domain_id) > static int vtd_page_invalidate_notify_hook(IOMMUTLBEntry *entry, > void *private) > { > - memory_region_notify_iommu((IOMMUMemoryRegion *)private, *entry); > + memory_region_notify_iommu((IOMMUMemoryRegion *)private, 0, *entry); > return 0; > } > > @@ -1826,7 +1826,7 @@ static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s, > entry.iova = addr; > entry.perm = IOMMU_NONE; > entry.translated_addr = 0; > - memory_region_notify_iommu(&vtd_dev_as->iommu, entry); > + memory_region_notify_iommu(&vtd_dev_as->iommu, 0, entry); > > done: > return true; > diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c > index aaa6010d5c..301708e45e 100644 > --- a/hw/ppc/spapr_iommu.c > +++ b/hw/ppc/spapr_iommu.c > @@ -428,7 +428,7 @@ static target_ulong put_tce_emu(sPAPRTCETable *tcet, target_ulong ioba, > 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, entry); > + memory_region_notify_iommu(&tcet->iommu, 0, entry); > > return H_SUCCESS; > } > diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c > index d1a5f79678..7b61367ee3 100644 > --- a/hw/s390x/s390-pci-inst.c > +++ b/hw/s390x/s390-pci-inst.c > @@ -589,7 +589,7 @@ static void s390_pci_update_iotlb(S390PCIIOMMU *iommu, S390IOTLBEntry *entry) > } > > notify.perm = IOMMU_NONE; > - memory_region_notify_iommu(&iommu->iommu_mr, notify); > + memory_region_notify_iommu(&iommu->iommu_mr, 0, notify); > notify.perm = entry->perm; > } > > @@ -601,7 +601,7 @@ static void s390_pci_update_iotlb(S390PCIIOMMU *iommu, S390IOTLBEntry *entry) > g_hash_table_replace(iommu->iotlb, &cache->iova, cache); > } > > - memory_region_notify_iommu(&iommu->iommu_mr, notify); > + memory_region_notify_iommu(&iommu->iommu_mr, 0, notify); > } > > int rpcit_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2, uintptr_t ra) > diff --git a/hw/vfio/common.c b/hw/vfio/common.c > index 8e57265edf..fb396cf00a 100644 > --- a/hw/vfio/common.c > +++ b/hw/vfio/common.c > @@ -507,6 +507,7 @@ static void vfio_listener_region_add(MemoryListener *listener, > if (memory_region_is_iommu(section->mr)) { > VFIOGuestIOMMU *giommu; > IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr); > + int iommu_idx; > > trace_vfio_listener_region_add_iommu(iova, end); > /* > @@ -523,10 +524,13 @@ static void vfio_listener_region_add(MemoryListener *listener, > llend = int128_add(int128_make64(section->offset_within_region), > section->size); > llend = int128_sub(llend, int128_one()); > + iommu_idx = memory_region_iommu_attrs_to_index(iommu_mr, > + MEMTXATTRS_UNSPECIFIED); In that case VFIO ideally wants to be notified for any guest update (whatever the page set) to reprogram the physical IOMMU corresponding entries and doesn't want to register a notifier per iommu_idx. Also it does not know which ones are supported. Is there a corresponding iommu_idx value? MEMTXATTRS_ANY? Thanks Eric > iommu_notifier_init(&giommu->n, vfio_iommu_map_notify, > IOMMU_NOTIFIER_ALL, > section->offset_within_region, > - int128_get64(llend)); > + int128_get64(llend), > + iommu_idx); > QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next); > > memory_region_register_iommu_notifier(section->mr, &giommu->n); > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index 48f4fd7cc9..6218df7683 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -657,6 +657,8 @@ static void vhost_iommu_region_add(MemoryListener *listener, > iommu_listener); > struct vhost_iommu *iommu; > Int128 end; > + int iommu_idx; > + IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr); > > if (!memory_region_is_iommu(section->mr)) { > return; > @@ -666,10 +668,13 @@ static void vhost_iommu_region_add(MemoryListener *listener, > end = int128_add(int128_make64(section->offset_within_region), > section->size); > end = int128_sub(end, int128_one()); > + iommu_idx = memory_region_iommu_attrs_to_index(iommu_mr, > + MEMTXATTRS_UNSPECIFIED); > iommu_notifier_init(&iommu->n, vhost_iommu_unmap_notify, > IOMMU_NOTIFIER_UNMAP, > section->offset_within_region, > - int128_get64(end)); > + int128_get64(end), > + iommu_idx); > iommu->mr = section->mr; > iommu->iommu_offset = section->offset_within_address_space - > section->offset_within_region; > diff --git a/memory.c b/memory.c > index 07d5fa7862..accb28d694 100644 > --- a/memory.c > +++ b/memory.c > @@ -1802,6 +1802,9 @@ void memory_region_register_iommu_notifier(MemoryRegion *mr, > iommu_mr = IOMMU_MEMORY_REGION(mr); > assert(n->notifier_flags != IOMMU_NOTIFIER_NONE); > assert(n->start <= n->end); > + assert(n->iommu_idx >= 0 && > + n->iommu_idx < memory_region_iommu_num_indexes(iommu_mr)); > + > QLIST_INSERT_HEAD(&iommu_mr->iommu_notify, n, node); > memory_region_update_iommu_notify_flags(iommu_mr); > } > @@ -1894,6 +1897,7 @@ void memory_region_notify_one(IOMMUNotifier *notifier, > } > > void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, > + int iommu_idx, > IOMMUTLBEntry entry) > { > IOMMUNotifier *iommu_notifier; > @@ -1901,7 +1905,9 @@ void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, > assert(memory_region_is_iommu(MEMORY_REGION(iommu_mr))); > > IOMMU_NOTIFIER_FOREACH(iommu_notifier, iommu_mr) { > - memory_region_notify_one(iommu_notifier, &entry); > + if (iommu_notifier->iommu_idx == iommu_idx) { > + memory_region_notify_one(iommu_notifier, &entry); > + } > } > } > >
On 24 May 2018 at 16:29, Auger Eric <eric.auger@redhat.com> wrote: > On 05/21/2018 04:03 PM, Peter Maydell wrote: >> diff --git a/include/exec/memory.h b/include/exec/memory.h >> index f6226fb263..4e6b125add 100644 >> --- a/include/exec/memory.h >> +++ b/include/exec/memory.h >> @@ -71,6 +71,7 @@ struct IOMMUTLBEntry { >> hwaddr iova; >> hwaddr translated_addr; >> hwaddr addr_mask; /* 0xfff = 4k translation */ >> + int iommu_idx; > I don't get why ne need iommu_idx field here. On translate the caller > has it. On notify the notifier has it? I think this is an accidental leftover from some earlier draft; nothing in the patchset actually reads or writes this field, and it should be deleted. >> IOMMUAccessFlags perm; >> }; >> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >> index 8e57265edf..fb396cf00a 100644 >> --- a/hw/vfio/common.c >> +++ b/hw/vfio/common.c >> @@ -507,6 +507,7 @@ static void vfio_listener_region_add(MemoryListener *listener, >> if (memory_region_is_iommu(section->mr)) { >> VFIOGuestIOMMU *giommu; >> IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr); >> + int iommu_idx; >> >> trace_vfio_listener_region_add_iommu(iova, end); >> /* >> @@ -523,10 +524,13 @@ static void vfio_listener_region_add(MemoryListener *listener, >> llend = int128_add(int128_make64(section->offset_within_region), >> section->size); >> llend = int128_sub(llend, int128_one()); >> + iommu_idx = memory_region_iommu_attrs_to_index(iommu_mr, >> + MEMTXATTRS_UNSPECIFIED); > In that case VFIO ideally wants to be notified for any guest update > (whatever the page set) to reprogram the physical IOMMU corresponding > entries and doesn't want to register a notifier per iommu_idx. Also it > does not know which ones are supported. Is there a corresponding > iommu_idx value? MEMTXATTRS_ANY? If VFIO is actually dealing with an IOMMU that needs to handle multiple possible transactions for different tx attributes, then it needs to know about all of them, because how it programs the physical IOMMU needs to be different for "map X to Y for Secure transactions" versus "map X to Y for NonSecure" (say). (This would require new kernel APIs, I assume.) If, as is currently the case, the VFIO infrastructure assumes that the IOMMU will always translate any transaction from a device identically regardless of its transaction attributes, then it should just use MEMTXATTRS_UNSPECIFIED, and trust that the emulated IOMMU will translate those correctly. (There might be scope for VFIO checking that the IOMMU really does, eg that it is only using one iommu index?) Basically, VFIO is shadowing the behaviour of the emulated IOMMU to reflect it into the kernel; if the IOMMU it's shadowing is complicated then VFIO is going to need to be similarly complicated, and "merge updates for different page tables down into one" is not going to be the right behaviour. thanks -- PMM
Hi Peter, On 05/24/2018 07:03 PM, Peter Maydell wrote: > On 24 May 2018 at 16:29, Auger Eric <eric.auger@redhat.com> wrote: >> On 05/21/2018 04:03 PM, Peter Maydell wrote: >>> diff --git a/include/exec/memory.h b/include/exec/memory.h >>> index f6226fb263..4e6b125add 100644 >>> --- a/include/exec/memory.h >>> +++ b/include/exec/memory.h >>> @@ -71,6 +71,7 @@ struct IOMMUTLBEntry { >>> hwaddr iova; >>> hwaddr translated_addr; >>> hwaddr addr_mask; /* 0xfff = 4k translation */ >>> + int iommu_idx; >> I don't get why ne need iommu_idx field here. On translate the caller >> has it. On notify the notifier has it? > > I think this is an accidental leftover from some earlier draft; > nothing in the patchset actually reads or writes this field, and > it should be deleted. > >>> IOMMUAccessFlags perm; >>> }; >>> > >>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c >>> index 8e57265edf..fb396cf00a 100644 >>> --- a/hw/vfio/common.c >>> +++ b/hw/vfio/common.c >>> @@ -507,6 +507,7 @@ static void vfio_listener_region_add(MemoryListener *listener, >>> if (memory_region_is_iommu(section->mr)) { >>> VFIOGuestIOMMU *giommu; >>> IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr); >>> + int iommu_idx; >>> >>> trace_vfio_listener_region_add_iommu(iova, end); >>> /* >>> @@ -523,10 +524,13 @@ static void vfio_listener_region_add(MemoryListener *listener, >>> llend = int128_add(int128_make64(section->offset_within_region), >>> section->size); >>> llend = int128_sub(llend, int128_one()); >>> + iommu_idx = memory_region_iommu_attrs_to_index(iommu_mr, >>> + MEMTXATTRS_UNSPECIFIED); >> In that case VFIO ideally wants to be notified for any guest update >> (whatever the page set) to reprogram the physical IOMMU corresponding >> entries and doesn't want to register a notifier per iommu_idx. Also it >> does not know which ones are supported. Is there a corresponding >> iommu_idx value? MEMTXATTRS_ANY? > > If VFIO is actually dealing with an IOMMU that needs to handle > multiple possible transactions for different tx attributes, then > it needs to know about all of them, because how it programs > the physical IOMMU needs to be different for "map X to Y for > Secure transactions" versus "map X to Y for NonSecure" (say). > (This would require new kernel APIs, I assume.) Hum agreed. In any case the iommu_idx must be passed to VFIO along with the notification, either as part of the notifier itself or in the IOMMUTLBEntry. So VFIO may need to enumerate the supported iommu_idx and register a notifier for relevant ones. Thanks Eric > > If, as is currently the case, the VFIO infrastructure assumes that > the IOMMU will always translate any transaction from a device > identically regardless of its transaction attributes, then it > should just use MEMTXATTRS_UNSPECIFIED, and trust that the > emulated IOMMU will translate those correctly. (There might be > scope for VFIO checking that the IOMMU really does, eg that > it is only using one iommu index?) > > Basically, VFIO is shadowing the behaviour of the emulated > IOMMU to reflect it into the kernel; if the IOMMU it's shadowing > is complicated then VFIO is going to need to be similarly > complicated, and "merge updates for different page tables > down into one" is not going to be the right behaviour. > > thanks > -- PMM >
On 23 May 2018 at 10:08, Alex Bennée <alex.bennee@linaro.org> wrote: > > Peter Maydell <peter.maydell@linaro.org> writes: > >> Add support for multiple IOMMU indexes to the IOMMU notifier APIs. >> When initializing a notifier with iommu_notifier_init(), the caller >> must pass the IOMMU index that it is interested in. When a change >> happens, the IOMMU implementation must pass >> memory_region_notify_iommu() the IOMMU index that has changed and >> that notifiers must be called for. >> >> IOMMUs which support only a single index don't need to change. >> Callers which only really support working with IOMMUs with a single >> index can use the result of passing MEMTXATTRS_UNSPECIFIED to >> memory_region_iommu_attrs_to_index(). >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> --- >> include/exec/memory.h | 11 ++++++++++- >> hw/i386/intel_iommu.c | 4 ++-- >> hw/ppc/spapr_iommu.c | 2 +- >> hw/s390x/s390-pci-inst.c | 4 ++-- >> hw/vfio/common.c | 6 +++++- >> hw/virtio/vhost.c | 7 ++++++- >> memory.c | 8 +++++++- >> 7 files changed, 33 insertions(+), 9 deletions(-) >> >> diff --git a/include/exec/memory.h b/include/exec/memory.h >> index f6226fb263..4e6b125add 100644 >> --- a/include/exec/memory.h >> +++ b/include/exec/memory.h >> @@ -71,6 +71,7 @@ struct IOMMUTLBEntry { >> hwaddr iova; >> hwaddr translated_addr; >> hwaddr addr_mask; /* 0xfff = 4k translation */ >> + int iommu_idx; >> IOMMUAccessFlags perm; >> }; >> >> @@ -98,18 +99,21 @@ struct IOMMUNotifier { >> /* Notify for address space range start <= addr <= end */ >> hwaddr start; >> hwaddr end; >> + int iommu_idx; > > Its a minor thing but are we ever expecting iommu_idx to ever be > negative? Coming back to this one -- no, we don't expect negative iommu_idxs. But on the other hand we don't ever expect negative TCG mmu_indexes either, and we use 'int' for those... thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 23 May 2018 at 10:08, Alex Bennée <alex.bennee@linaro.org> wrote: >> >> Peter Maydell <peter.maydell@linaro.org> writes: >> >>> Add support for multiple IOMMU indexes to the IOMMU notifier APIs. >>> When initializing a notifier with iommu_notifier_init(), the caller >>> must pass the IOMMU index that it is interested in. When a change >>> happens, the IOMMU implementation must pass >>> memory_region_notify_iommu() the IOMMU index that has changed and >>> that notifiers must be called for. >>> >>> IOMMUs which support only a single index don't need to change. >>> Callers which only really support working with IOMMUs with a single >>> index can use the result of passing MEMTXATTRS_UNSPECIFIED to >>> memory_region_iommu_attrs_to_index(). >>> >>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >>> --- >>> include/exec/memory.h | 11 ++++++++++- >>> hw/i386/intel_iommu.c | 4 ++-- >>> hw/ppc/spapr_iommu.c | 2 +- >>> hw/s390x/s390-pci-inst.c | 4 ++-- >>> hw/vfio/common.c | 6 +++++- >>> hw/virtio/vhost.c | 7 ++++++- >>> memory.c | 8 +++++++- >>> 7 files changed, 33 insertions(+), 9 deletions(-) >>> >>> diff --git a/include/exec/memory.h b/include/exec/memory.h >>> index f6226fb263..4e6b125add 100644 >>> --- a/include/exec/memory.h >>> +++ b/include/exec/memory.h >>> @@ -71,6 +71,7 @@ struct IOMMUTLBEntry { >>> hwaddr iova; >>> hwaddr translated_addr; >>> hwaddr addr_mask; /* 0xfff = 4k translation */ >>> + int iommu_idx; >>> IOMMUAccessFlags perm; >>> }; >>> >>> @@ -98,18 +99,21 @@ struct IOMMUNotifier { >>> /* Notify for address space range start <= addr <= end */ >>> hwaddr start; >>> hwaddr end; >>> + int iommu_idx; >> >> Its a minor thing but are we ever expecting iommu_idx to ever be >> negative? > > Coming back to this one -- no, we don't expect negative iommu_idxs. > But on the other hand we don't ever expect negative TCG mmu_indexes > either, and we use 'int' for those... That's a clean-up right there ;-) As I said it's a minor thing but I generally favour clearest type for the range you are going to see. That said the enum stuff on the older clangs did confuse me so there is that... -- Alex Bennée
On 4 June 2018 at 16:09, Alex Bennée <alex.bennee@linaro.org> wrote: > > Peter Maydell <peter.maydell@linaro.org> writes: > >> On 23 May 2018 at 10:08, Alex Bennée <alex.bennee@linaro.org> wrote: >>> Its a minor thing but are we ever expecting iommu_idx to ever be >>> negative? >> >> Coming back to this one -- no, we don't expect negative iommu_idxs. >> But on the other hand we don't ever expect negative TCG mmu_indexes >> either, and we use 'int' for those... > > That's a clean-up right there ;-) > > As I said it's a minor thing but I generally favour clearest type for > the range you are going to see. That said the enum stuff on the older > clangs did confuse me so there is that... I prefer the "don't use unsigned unless you need the unsigned-integer semantics" approach. (For instance most loop indexes won't ever be negative, but "int i" is the usual thing there.) thanks -- PMM
diff --git a/include/exec/memory.h b/include/exec/memory.h index f6226fb263..4e6b125add 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -71,6 +71,7 @@ struct IOMMUTLBEntry { hwaddr iova; hwaddr translated_addr; hwaddr addr_mask; /* 0xfff = 4k translation */ + int iommu_idx; IOMMUAccessFlags perm; }; @@ -98,18 +99,21 @@ struct IOMMUNotifier { /* Notify for address space range start <= addr <= end */ hwaddr start; hwaddr end; + int iommu_idx; QLIST_ENTRY(IOMMUNotifier) node; }; typedef struct IOMMUNotifier IOMMUNotifier; static inline void iommu_notifier_init(IOMMUNotifier *n, IOMMUNotify fn, IOMMUNotifierFlag flags, - hwaddr start, hwaddr end) + hwaddr start, hwaddr end, + int iommu_idx) { n->notify = fn; n->notifier_flags = flags; n->start = start; n->end = end; + n->iommu_idx = iommu_idx; } /* @@ -323,7 +327,10 @@ typedef struct IOMMUMemoryRegionClass { * Optional method: if this method is not provided, then * memory_region_iommu_num_indexes() will return 1, indicating that * only a single IOMMU index is supported. + * + * @iommu: the IOMMUMemoryRegion */ + int (*num_indexes)(IOMMUMemoryRegion *iommu); } IOMMUMemoryRegionClass; typedef struct CoalescedMemoryRange CoalescedMemoryRange; @@ -1028,11 +1035,13 @@ uint64_t memory_region_iommu_get_min_page_size(IOMMUMemoryRegion *iommu_mr); * should be notified with an UNMAP followed by a MAP. * * @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. */ void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, + int iommu_idx, IOMMUTLBEntry entry); /** diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index fb31de9416..b8c9354b0b 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -1376,7 +1376,7 @@ static void vtd_iotlb_domain_invalidate(IntelIOMMUState *s, uint16_t domain_id) static int vtd_page_invalidate_notify_hook(IOMMUTLBEntry *entry, void *private) { - memory_region_notify_iommu((IOMMUMemoryRegion *)private, *entry); + memory_region_notify_iommu((IOMMUMemoryRegion *)private, 0, *entry); return 0; } @@ -1826,7 +1826,7 @@ static bool vtd_process_device_iotlb_desc(IntelIOMMUState *s, entry.iova = addr; entry.perm = IOMMU_NONE; entry.translated_addr = 0; - memory_region_notify_iommu(&vtd_dev_as->iommu, entry); + memory_region_notify_iommu(&vtd_dev_as->iommu, 0, entry); done: return true; diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c index aaa6010d5c..301708e45e 100644 --- a/hw/ppc/spapr_iommu.c +++ b/hw/ppc/spapr_iommu.c @@ -428,7 +428,7 @@ static target_ulong put_tce_emu(sPAPRTCETable *tcet, target_ulong ioba, 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, entry); + memory_region_notify_iommu(&tcet->iommu, 0, entry); return H_SUCCESS; } diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c index d1a5f79678..7b61367ee3 100644 --- a/hw/s390x/s390-pci-inst.c +++ b/hw/s390x/s390-pci-inst.c @@ -589,7 +589,7 @@ static void s390_pci_update_iotlb(S390PCIIOMMU *iommu, S390IOTLBEntry *entry) } notify.perm = IOMMU_NONE; - memory_region_notify_iommu(&iommu->iommu_mr, notify); + memory_region_notify_iommu(&iommu->iommu_mr, 0, notify); notify.perm = entry->perm; } @@ -601,7 +601,7 @@ static void s390_pci_update_iotlb(S390PCIIOMMU *iommu, S390IOTLBEntry *entry) g_hash_table_replace(iommu->iotlb, &cache->iova, cache); } - memory_region_notify_iommu(&iommu->iommu_mr, notify); + memory_region_notify_iommu(&iommu->iommu_mr, 0, notify); } int rpcit_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2, uintptr_t ra) diff --git a/hw/vfio/common.c b/hw/vfio/common.c index 8e57265edf..fb396cf00a 100644 --- a/hw/vfio/common.c +++ b/hw/vfio/common.c @@ -507,6 +507,7 @@ static void vfio_listener_region_add(MemoryListener *listener, if (memory_region_is_iommu(section->mr)) { VFIOGuestIOMMU *giommu; IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr); + int iommu_idx; trace_vfio_listener_region_add_iommu(iova, end); /* @@ -523,10 +524,13 @@ static void vfio_listener_region_add(MemoryListener *listener, llend = int128_add(int128_make64(section->offset_within_region), section->size); llend = int128_sub(llend, int128_one()); + iommu_idx = memory_region_iommu_attrs_to_index(iommu_mr, + MEMTXATTRS_UNSPECIFIED); iommu_notifier_init(&giommu->n, vfio_iommu_map_notify, IOMMU_NOTIFIER_ALL, section->offset_within_region, - int128_get64(llend)); + int128_get64(llend), + iommu_idx); QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next); memory_region_register_iommu_notifier(section->mr, &giommu->n); diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 48f4fd7cc9..6218df7683 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -657,6 +657,8 @@ static void vhost_iommu_region_add(MemoryListener *listener, iommu_listener); struct vhost_iommu *iommu; Int128 end; + int iommu_idx; + IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr); if (!memory_region_is_iommu(section->mr)) { return; @@ -666,10 +668,13 @@ static void vhost_iommu_region_add(MemoryListener *listener, end = int128_add(int128_make64(section->offset_within_region), section->size); end = int128_sub(end, int128_one()); + iommu_idx = memory_region_iommu_attrs_to_index(iommu_mr, + MEMTXATTRS_UNSPECIFIED); iommu_notifier_init(&iommu->n, vhost_iommu_unmap_notify, IOMMU_NOTIFIER_UNMAP, section->offset_within_region, - int128_get64(end)); + int128_get64(end), + iommu_idx); iommu->mr = section->mr; iommu->iommu_offset = section->offset_within_address_space - section->offset_within_region; diff --git a/memory.c b/memory.c index 07d5fa7862..accb28d694 100644 --- a/memory.c +++ b/memory.c @@ -1802,6 +1802,9 @@ void memory_region_register_iommu_notifier(MemoryRegion *mr, iommu_mr = IOMMU_MEMORY_REGION(mr); assert(n->notifier_flags != IOMMU_NOTIFIER_NONE); assert(n->start <= n->end); + assert(n->iommu_idx >= 0 && + n->iommu_idx < memory_region_iommu_num_indexes(iommu_mr)); + QLIST_INSERT_HEAD(&iommu_mr->iommu_notify, n, node); memory_region_update_iommu_notify_flags(iommu_mr); } @@ -1894,6 +1897,7 @@ void memory_region_notify_one(IOMMUNotifier *notifier, } void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, + int iommu_idx, IOMMUTLBEntry entry) { IOMMUNotifier *iommu_notifier; @@ -1901,7 +1905,9 @@ void memory_region_notify_iommu(IOMMUMemoryRegion *iommu_mr, assert(memory_region_is_iommu(MEMORY_REGION(iommu_mr))); IOMMU_NOTIFIER_FOREACH(iommu_notifier, iommu_mr) { - memory_region_notify_one(iommu_notifier, &entry); + if (iommu_notifier->iommu_idx == iommu_idx) { + memory_region_notify_one(iommu_notifier, &entry); + } } }
Add support for multiple IOMMU indexes to the IOMMU notifier APIs. When initializing a notifier with iommu_notifier_init(), the caller must pass the IOMMU index that it is interested in. When a change happens, the IOMMU implementation must pass memory_region_notify_iommu() the IOMMU index that has changed and that notifiers must be called for. IOMMUs which support only a single index don't need to change. Callers which only really support working with IOMMUs with a single index can use the result of passing MEMTXATTRS_UNSPECIFIED to memory_region_iommu_attrs_to_index(). Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- include/exec/memory.h | 11 ++++++++++- hw/i386/intel_iommu.c | 4 ++-- hw/ppc/spapr_iommu.c | 2 +- hw/s390x/s390-pci-inst.c | 4 ++-- hw/vfio/common.c | 6 +++++- hw/virtio/vhost.c | 7 ++++++- memory.c | 8 +++++++- 7 files changed, 33 insertions(+), 9 deletions(-) -- 2.17.0