From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46576) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fLtec-0004Tr-2x for qemu-devel@nongnu.org; Thu, 24 May 2018 13:03:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fLteb-00013C-7P for qemu-devel@nongnu.org; Thu, 24 May 2018 13:03:54 -0400 Received: from mail-ot0-x244.google.com ([2607:f8b0:4003:c0f::244]:39541) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1fLtea-00012p-VC for qemu-devel@nongnu.org; Thu, 24 May 2018 13:03:53 -0400 Received: by mail-ot0-x244.google.com with SMTP id l12-v6so2793999oth.6 for ; Thu, 24 May 2018 10:03:52 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <0774094d-d3e1-5983-ccb8-3d3412bf44fc@redhat.com> References: <20180521140402.23318-1-peter.maydell@linaro.org> <20180521140402.23318-16-peter.maydell@linaro.org> <0774094d-d3e1-5983-ccb8-3d3412bf44fc@redhat.com> From: Peter Maydell Date: Thu, 24 May 2018 18:03:31 +0100 Message-ID: Content-Type: text/plain; charset="UTF-8" Subject: Re: [Qemu-devel] [PATCH 15/27] iommu: Add IOMMU index argument to notifier APIs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Auger Eric Cc: qemu-arm , QEMU Developers , Paolo Bonzini , Richard Henderson , =?UTF-8?B?QWxleCBCZW5uw6ll?= , "patches@linaro.org" On 24 May 2018 at 16:29, Auger Eric 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