From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752920AbcJJOrj (ORCPT ); Mon, 10 Oct 2016 10:47:39 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42518 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752492AbcJJOrh (ORCPT ); Mon, 10 Oct 2016 10:47:37 -0400 Subject: Re: [PATCH v13 03/15] iommu/dma: Allow MSI-only cookies To: Robin Murphy , Alex Williamson References: <1475743531-4780-1-git-send-email-eric.auger@redhat.com> <1475743531-4780-4-git-send-email-eric.auger@redhat.com> <20161006141717.13c9c111@t450s.home> <6b944a74-7429-caf2-507d-30d6ae336bec@arm.com> Cc: eric.auger.pro@gmail.com, christoffer.dall@linaro.org, marc.zyngier@arm.com, will.deacon@arm.com, joro@8bytes.org, tglx@linutronix.de, jason@lakedaemon.net, linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org, drjones@redhat.com, linux-kernel@vger.kernel.org, Bharat.Bhushan@freescale.com, pranav.sawargaonkar@gmail.com, p.fedin@samsung.com, iommu@lists.linux-foundation.org, Jean-Philippe.Brucker@arm.com, yehuday@marvell.com, Manish.Jaggi@caviumnetworks.com From: Auger Eric Message-ID: Date: Mon, 10 Oct 2016 16:47:24 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: <6b944a74-7429-caf2-507d-30d6ae336bec@arm.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Mon, 10 Oct 2016 14:47:31 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Robin, On 10/10/2016 16:26, Robin Murphy wrote: > Hi Alex, Eric, > > On 06/10/16 21:17, Alex Williamson wrote: >> On Thu, 6 Oct 2016 08:45:19 +0000 >> Eric Auger wrote: >> >>> From: Robin Murphy >>> >>> IOMMU domain users such as VFIO face a similar problem to DMA API ops >>> with regard to mapping MSI messages in systems where the MSI write is >>> subject to IOMMU translation. With the relevant infrastructure now in >>> place for managed DMA domains, it's actually really simple for other >>> users to piggyback off that and reap the benefits without giving up >>> their own IOVA management, and without having to reinvent their own >>> wheel in the MSI layer. >>> >>> Allow such users to opt into automatic MSI remapping by dedicating a >>> region of their IOVA space to a managed cookie. >>> >>> Signed-off-by: Robin Murphy >>> Signed-off-by: Eric Auger >>> >>> --- >>> >>> v1 -> v2: >>> - compared to Robin's version >>> - add NULL last param to iommu_dma_init_domain >>> - set the msi_geometry aperture >>> - I removed >>> if (base < U64_MAX - size) >>> reserve_iova(iovad, iova_pfn(iovad, base + size), ULONG_MAX); >>> don't get why we would reserve something out of the scope of the iova domain? >>> what do I miss? >>> --- >>> drivers/iommu/dma-iommu.c | 40 ++++++++++++++++++++++++++++++++++++++++ >>> include/linux/dma-iommu.h | 9 +++++++++ >>> 2 files changed, 49 insertions(+) >>> >>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c >>> index c5ab866..11da1a0 100644 >>> --- a/drivers/iommu/dma-iommu.c >>> +++ b/drivers/iommu/dma-iommu.c >>> @@ -716,3 +716,43 @@ void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg) >>> msg->address_lo += lower_32_bits(msi_page->iova); >>> } >>> } >>> + >>> +/** >>> + * iommu_get_dma_msi_region_cookie - Configure a domain for MSI remapping only >> >> Should this perhaps be iommu_setup_dma_msi_region_cookie, or something >> along those lines. I'm not sure what we're get'ing. Thanks, > > What we're getting is private third-party resources for the iommu_domain > given in the argument. It's a get/put rather than alloc/free model since > we operate opaquely on the domain as a container, rather than on the > actual resource in question (an IOVA allocator). > > Since this particular use case is slightly different from the normal > flow and has special initialisation requirements, it seemed a lot > cleaner to simply combine that initialisation operation with the > prerequisite "get" into a single call. Especially as it helps emphasise > that this is not 'normal' DMA cookie usage. I renamed iommu_get_dma_msi_region_cookie into iommu_setup_dma_msi_region. Is it a problem for you? > >> >> Alex >> >>> + * @domain: IOMMU domain to prepare >>> + * @base: Base address of IOVA region to use as the MSI remapping aperture >>> + * @size: Size of the desired MSI aperture >>> + * >>> + * Users who manage their own IOVA allocation and do not want DMA API support, >>> + * but would still like to take advantage of automatic MSI remapping, can use >>> + * this to initialise their own domain appropriately. >>> + */ >>> +int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain, >>> + dma_addr_t base, u64 size) >>> +{ >>> + struct iommu_dma_cookie *cookie; >>> + struct iova_domain *iovad; >>> + int ret; >>> + >>> + if (domain->type == IOMMU_DOMAIN_DMA) >>> + return -EINVAL; >>> + >>> + ret = iommu_get_dma_cookie(domain); >>> + if (ret) >>> + return ret; >>> + >>> + ret = iommu_dma_init_domain(domain, base, size, NULL); >>> + if (ret) { >>> + iommu_put_dma_cookie(domain); >>> + return ret; >>> + } > > It *is* necessary to explicitly reserve the upper part of the IOVA > domain here - the aforementioned "special initialisation" - because > dma_32bit_pfn is only an optimisation hint to prevent the allocator > walking down from the very top of the the tree every time when devices > with different DMA masks share a domain (I'm in two minds as to whether > to tweak the way the iommu-dma code uses it in this respect, now that I > fully understand things). The only actual upper limit to allocation is > the DMA mask passed into each alloc_iova() call, so if we want to ensure > IOVAs are really allocated within this specific region, we have to carve > out everything above it. thank you for the explanation. So I will restore the reserve then. Thanks Eric > > Robin. > >>> + >>> + domain->msi_geometry.aperture_start = base; >>> + domain->msi_geometry.aperture_end = base + size - 1; >>> + >>> + cookie = domain->iova_cookie; >>> + iovad = &cookie->iovad; >>> + >>> + return 0; >>> +} >>> +EXPORT_SYMBOL(iommu_get_dma_msi_region_cookie); >>> diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h >>> index 32c5890..1c55413 100644 >>> --- a/include/linux/dma-iommu.h >>> +++ b/include/linux/dma-iommu.h >>> @@ -67,6 +67,9 @@ int iommu_dma_mapping_error(struct device *dev, dma_addr_t dma_addr); >>> /* The DMA API isn't _quite_ the whole story, though... */ >>> void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg); >>> >>> +int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain, >>> + dma_addr_t base, u64 size); >>> + >>> #else >>> >>> struct iommu_domain; >>> @@ -90,6 +93,12 @@ static inline void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg) >>> { >>> } >>> >>> +static inline int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain, >>> + dma_addr_t base, u64 size) >>> +{ >>> + return -ENODEV; >>> +} >>> + >>> #endif /* CONFIG_IOMMU_DMA */ >>> #endif /* __KERNEL__ */ >>> #endif /* __DMA_IOMMU_H */ >> > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Auger Eric Subject: Re: [PATCH v13 03/15] iommu/dma: Allow MSI-only cookies Date: Mon, 10 Oct 2016 16:47:24 +0200 Message-ID: References: <1475743531-4780-1-git-send-email-eric.auger@redhat.com> <1475743531-4780-4-git-send-email-eric.auger@redhat.com> <20161006141717.13c9c111@t450s.home> <6b944a74-7429-caf2-507d-30d6ae336bec@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: yehuday-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org, drjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, jason-NLaQJdtUoK4Be96aLqz0jA@public.gmane.org, kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, marc.zyngier-5wv7dgnIgG8@public.gmane.org, p.fedin-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org, will.deacon-5wv7dgnIgG8@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, pranav.sawargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org, Manish.Jaggi-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org, christoffer.dall-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org, eric.auger.pro-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org To: Robin Murphy , Alex Williamson Return-path: In-Reply-To: <6b944a74-7429-caf2-507d-30d6ae336bec-5wv7dgnIgG8@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org List-Id: kvm.vger.kernel.org Hi Robin, On 10/10/2016 16:26, Robin Murphy wrote: > Hi Alex, Eric, > > On 06/10/16 21:17, Alex Williamson wrote: >> On Thu, 6 Oct 2016 08:45:19 +0000 >> Eric Auger wrote: >> >>> From: Robin Murphy >>> >>> IOMMU domain users such as VFIO face a similar problem to DMA API ops >>> with regard to mapping MSI messages in systems where the MSI write is >>> subject to IOMMU translation. With the relevant infrastructure now in >>> place for managed DMA domains, it's actually really simple for other >>> users to piggyback off that and reap the benefits without giving up >>> their own IOVA management, and without having to reinvent their own >>> wheel in the MSI layer. >>> >>> Allow such users to opt into automatic MSI remapping by dedicating a >>> region of their IOVA space to a managed cookie. >>> >>> Signed-off-by: Robin Murphy >>> Signed-off-by: Eric Auger >>> >>> --- >>> >>> v1 -> v2: >>> - compared to Robin's version >>> - add NULL last param to iommu_dma_init_domain >>> - set the msi_geometry aperture >>> - I removed >>> if (base < U64_MAX - size) >>> reserve_iova(iovad, iova_pfn(iovad, base + size), ULONG_MAX); >>> don't get why we would reserve something out of the scope of the iova domain? >>> what do I miss? >>> --- >>> drivers/iommu/dma-iommu.c | 40 ++++++++++++++++++++++++++++++++++++++++ >>> include/linux/dma-iommu.h | 9 +++++++++ >>> 2 files changed, 49 insertions(+) >>> >>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c >>> index c5ab866..11da1a0 100644 >>> --- a/drivers/iommu/dma-iommu.c >>> +++ b/drivers/iommu/dma-iommu.c >>> @@ -716,3 +716,43 @@ void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg) >>> msg->address_lo += lower_32_bits(msi_page->iova); >>> } >>> } >>> + >>> +/** >>> + * iommu_get_dma_msi_region_cookie - Configure a domain for MSI remapping only >> >> Should this perhaps be iommu_setup_dma_msi_region_cookie, or something >> along those lines. I'm not sure what we're get'ing. Thanks, > > What we're getting is private third-party resources for the iommu_domain > given in the argument. It's a get/put rather than alloc/free model since > we operate opaquely on the domain as a container, rather than on the > actual resource in question (an IOVA allocator). > > Since this particular use case is slightly different from the normal > flow and has special initialisation requirements, it seemed a lot > cleaner to simply combine that initialisation operation with the > prerequisite "get" into a single call. Especially as it helps emphasise > that this is not 'normal' DMA cookie usage. I renamed iommu_get_dma_msi_region_cookie into iommu_setup_dma_msi_region. Is it a problem for you? > >> >> Alex >> >>> + * @domain: IOMMU domain to prepare >>> + * @base: Base address of IOVA region to use as the MSI remapping aperture >>> + * @size: Size of the desired MSI aperture >>> + * >>> + * Users who manage their own IOVA allocation and do not want DMA API support, >>> + * but would still like to take advantage of automatic MSI remapping, can use >>> + * this to initialise their own domain appropriately. >>> + */ >>> +int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain, >>> + dma_addr_t base, u64 size) >>> +{ >>> + struct iommu_dma_cookie *cookie; >>> + struct iova_domain *iovad; >>> + int ret; >>> + >>> + if (domain->type == IOMMU_DOMAIN_DMA) >>> + return -EINVAL; >>> + >>> + ret = iommu_get_dma_cookie(domain); >>> + if (ret) >>> + return ret; >>> + >>> + ret = iommu_dma_init_domain(domain, base, size, NULL); >>> + if (ret) { >>> + iommu_put_dma_cookie(domain); >>> + return ret; >>> + } > > It *is* necessary to explicitly reserve the upper part of the IOVA > domain here - the aforementioned "special initialisation" - because > dma_32bit_pfn is only an optimisation hint to prevent the allocator > walking down from the very top of the the tree every time when devices > with different DMA masks share a domain (I'm in two minds as to whether > to tweak the way the iommu-dma code uses it in this respect, now that I > fully understand things). The only actual upper limit to allocation is > the DMA mask passed into each alloc_iova() call, so if we want to ensure > IOVAs are really allocated within this specific region, we have to carve > out everything above it. thank you for the explanation. So I will restore the reserve then. Thanks Eric > > Robin. > >>> + >>> + domain->msi_geometry.aperture_start = base; >>> + domain->msi_geometry.aperture_end = base + size - 1; >>> + >>> + cookie = domain->iova_cookie; >>> + iovad = &cookie->iovad; >>> + >>> + return 0; >>> +} >>> +EXPORT_SYMBOL(iommu_get_dma_msi_region_cookie); >>> diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h >>> index 32c5890..1c55413 100644 >>> --- a/include/linux/dma-iommu.h >>> +++ b/include/linux/dma-iommu.h >>> @@ -67,6 +67,9 @@ int iommu_dma_mapping_error(struct device *dev, dma_addr_t dma_addr); >>> /* The DMA API isn't _quite_ the whole story, though... */ >>> void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg); >>> >>> +int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain, >>> + dma_addr_t base, u64 size); >>> + >>> #else >>> >>> struct iommu_domain; >>> @@ -90,6 +93,12 @@ static inline void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg) >>> { >>> } >>> >>> +static inline int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain, >>> + dma_addr_t base, u64 size) >>> +{ >>> + return -ENODEV; >>> +} >>> + >>> #endif /* CONFIG_IOMMU_DMA */ >>> #endif /* __KERNEL__ */ >>> #endif /* __DMA_IOMMU_H */ >> > From mboxrd@z Thu Jan 1 00:00:00 1970 From: eric.auger@redhat.com (Auger Eric) Date: Mon, 10 Oct 2016 16:47:24 +0200 Subject: [PATCH v13 03/15] iommu/dma: Allow MSI-only cookies In-Reply-To: <6b944a74-7429-caf2-507d-30d6ae336bec@arm.com> References: <1475743531-4780-1-git-send-email-eric.auger@redhat.com> <1475743531-4780-4-git-send-email-eric.auger@redhat.com> <20161006141717.13c9c111@t450s.home> <6b944a74-7429-caf2-507d-30d6ae336bec@arm.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Robin, On 10/10/2016 16:26, Robin Murphy wrote: > Hi Alex, Eric, > > On 06/10/16 21:17, Alex Williamson wrote: >> On Thu, 6 Oct 2016 08:45:19 +0000 >> Eric Auger wrote: >> >>> From: Robin Murphy >>> >>> IOMMU domain users such as VFIO face a similar problem to DMA API ops >>> with regard to mapping MSI messages in systems where the MSI write is >>> subject to IOMMU translation. With the relevant infrastructure now in >>> place for managed DMA domains, it's actually really simple for other >>> users to piggyback off that and reap the benefits without giving up >>> their own IOVA management, and without having to reinvent their own >>> wheel in the MSI layer. >>> >>> Allow such users to opt into automatic MSI remapping by dedicating a >>> region of their IOVA space to a managed cookie. >>> >>> Signed-off-by: Robin Murphy >>> Signed-off-by: Eric Auger >>> >>> --- >>> >>> v1 -> v2: >>> - compared to Robin's version >>> - add NULL last param to iommu_dma_init_domain >>> - set the msi_geometry aperture >>> - I removed >>> if (base < U64_MAX - size) >>> reserve_iova(iovad, iova_pfn(iovad, base + size), ULONG_MAX); >>> don't get why we would reserve something out of the scope of the iova domain? >>> what do I miss? >>> --- >>> drivers/iommu/dma-iommu.c | 40 ++++++++++++++++++++++++++++++++++++++++ >>> include/linux/dma-iommu.h | 9 +++++++++ >>> 2 files changed, 49 insertions(+) >>> >>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c >>> index c5ab866..11da1a0 100644 >>> --- a/drivers/iommu/dma-iommu.c >>> +++ b/drivers/iommu/dma-iommu.c >>> @@ -716,3 +716,43 @@ void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg) >>> msg->address_lo += lower_32_bits(msi_page->iova); >>> } >>> } >>> + >>> +/** >>> + * iommu_get_dma_msi_region_cookie - Configure a domain for MSI remapping only >> >> Should this perhaps be iommu_setup_dma_msi_region_cookie, or something >> along those lines. I'm not sure what we're get'ing. Thanks, > > What we're getting is private third-party resources for the iommu_domain > given in the argument. It's a get/put rather than alloc/free model since > we operate opaquely on the domain as a container, rather than on the > actual resource in question (an IOVA allocator). > > Since this particular use case is slightly different from the normal > flow and has special initialisation requirements, it seemed a lot > cleaner to simply combine that initialisation operation with the > prerequisite "get" into a single call. Especially as it helps emphasise > that this is not 'normal' DMA cookie usage. I renamed iommu_get_dma_msi_region_cookie into iommu_setup_dma_msi_region. Is it a problem for you? > >> >> Alex >> >>> + * @domain: IOMMU domain to prepare >>> + * @base: Base address of IOVA region to use as the MSI remapping aperture >>> + * @size: Size of the desired MSI aperture >>> + * >>> + * Users who manage their own IOVA allocation and do not want DMA API support, >>> + * but would still like to take advantage of automatic MSI remapping, can use >>> + * this to initialise their own domain appropriately. >>> + */ >>> +int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain, >>> + dma_addr_t base, u64 size) >>> +{ >>> + struct iommu_dma_cookie *cookie; >>> + struct iova_domain *iovad; >>> + int ret; >>> + >>> + if (domain->type == IOMMU_DOMAIN_DMA) >>> + return -EINVAL; >>> + >>> + ret = iommu_get_dma_cookie(domain); >>> + if (ret) >>> + return ret; >>> + >>> + ret = iommu_dma_init_domain(domain, base, size, NULL); >>> + if (ret) { >>> + iommu_put_dma_cookie(domain); >>> + return ret; >>> + } > > It *is* necessary to explicitly reserve the upper part of the IOVA > domain here - the aforementioned "special initialisation" - because > dma_32bit_pfn is only an optimisation hint to prevent the allocator > walking down from the very top of the the tree every time when devices > with different DMA masks share a domain (I'm in two minds as to whether > to tweak the way the iommu-dma code uses it in this respect, now that I > fully understand things). The only actual upper limit to allocation is > the DMA mask passed into each alloc_iova() call, so if we want to ensure > IOVAs are really allocated within this specific region, we have to carve > out everything above it. thank you for the explanation. So I will restore the reserve then. Thanks Eric > > Robin. > >>> + >>> + domain->msi_geometry.aperture_start = base; >>> + domain->msi_geometry.aperture_end = base + size - 1; >>> + >>> + cookie = domain->iova_cookie; >>> + iovad = &cookie->iovad; >>> + >>> + return 0; >>> +} >>> +EXPORT_SYMBOL(iommu_get_dma_msi_region_cookie); >>> diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h >>> index 32c5890..1c55413 100644 >>> --- a/include/linux/dma-iommu.h >>> +++ b/include/linux/dma-iommu.h >>> @@ -67,6 +67,9 @@ int iommu_dma_mapping_error(struct device *dev, dma_addr_t dma_addr); >>> /* The DMA API isn't _quite_ the whole story, though... */ >>> void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg); >>> >>> +int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain, >>> + dma_addr_t base, u64 size); >>> + >>> #else >>> >>> struct iommu_domain; >>> @@ -90,6 +93,12 @@ static inline void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg) >>> { >>> } >>> >>> +static inline int iommu_get_dma_msi_region_cookie(struct iommu_domain *domain, >>> + dma_addr_t base, u64 size) >>> +{ >>> + return -ENODEV; >>> +} >>> + >>> #endif /* CONFIG_IOMMU_DMA */ >>> #endif /* __KERNEL__ */ >>> #endif /* __DMA_IOMMU_H */ >> >