Hi Peter, On 05/24/2018 07:03 PM, Peter Maydell wrote: > On 24 May 2018 at 16:29, Auger Eric <eric.au...@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 >