From: Eric Auger <eric.auger@redhat.com>
To: Robin Murphy <robin.murphy@arm.com>,
Nicolin Chen <nicolinc@nvidia.com>,
jgg@nvidia.com, will@kernel.org
Cc: kevin.tian@intel.com, baolu.lu@linux.intel.com, joro@8bytes.org,
shameerali.kolothum.thodi@huawei.com, jean-philippe@linaro.org,
linux-arm-kernel@lists.infradead.org, iommu@lists.linux.dev,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 01/14] iommu: Add iommu_get_unmanaged_domain helper
Date: Fri, 10 Mar 2023 09:41:01 +0100 [thread overview]
Message-ID: <4938b20b-14d8-86f8-e80b-9d8ed9d8f28d@redhat.com> (raw)
In-Reply-To: <2118a147-ac95-d846-ad6f-85d7cebca46a@arm.com>
Hi,
On 3/9/23 13:51, Robin Murphy wrote:
> On 2023-03-09 10:53, Nicolin Chen wrote:
>> The nature of ITS virtualization on ARM is done via hypercalls, so
>> kernel
>> handles all IOVA mappings for the MSI doorbell in
>> iommu_dma_prepare_msi()
>> and iommu_dma_compose_msi_msg(). The current virtualization solution
>> with
>> a 2-stage nested translation setup is to do 1:1 IOVA mappings at stage-1
>> guest-level IO page table via a RMR region in guest-level IORT, aligning
>> with an IOVA region that's predefined and mapped in the host kernel:
>>
>> [stage-2 host level]
>> #define MSI_IOVA_BASE 0x8000000
>> #define MSI_IOVA_LENGTH 0x100000
>> ...
>> iommu_get_msi_cookie():
>> cookie->msi_iova = MSI_IOVA_BASE;
>> ...
>> iommu_dma_prepare_msi(its_pa):
>> domain = iommu_get_domain_for_dev(dev);
>> iommu_dma_get_msi_page(its_pa, domain):
>> cookie = domain->iova_cookie;
>> iova = iommu_dma_alloc_iova():
>> return cookie->msi_iova - size;
>> iommu_map(iova, its_pa, ...);
>>
>> [stage-1 guest level]
>> // Define in IORT a RMR [MSI_IOVA_BASE, MSI_IOVA_LENGTH]
>> ...
>> iommu_create_device_direct_mappings():
>> iommu_map(iova=MSI_IOVA_BASE, pa=MSI_IOVA_BASE,
>> len=MSI_IOVA_LENGTH);
>>
>> This solution calling iommu_get_domain_for_dev() needs the device to get
>> attached to a host-level iommu_domain that has the msi_cookie.
>>
>> On the other hand, IOMMUFD designs two iommu_domain objects to represent
>> the two stages: a stage-1 domain (IOMMU_DOMAIN_NESTED type) and a
>> stage-2
>> domain (IOMMU_DOMAIN_UNMANAGED type). In this design, the device will be
>> attached to the stage-1 domain representing a guest-level IO page table,
>> or a Context Descriptor Table in SMMU's term.
>>
>> This is obviously a mismatch, as the iommu_get_domain_for_dev() does not
>> return the correct domain pointer in iommu_dma_prepare_msi().
>>
>> Add an iommu_get_unmanaged_domain helper to allow drivers to return the
>> correct IOMMU_DOMAIN_UNMANAGED iommu_domain having the IOVA mappings for
>> the msi_cookie. Keep it in the iommu-priv header for internal use only.
>>
>> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
>> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
>> ---
>> drivers/iommu/dma-iommu.c | 5 +++--
>> drivers/iommu/iommu-priv.h | 15 +++++++++++++++
>> include/linux/iommu.h | 2 ++
>> 3 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>> index 99b2646cb5c7..6b0409d0ff85 100644
>> --- a/drivers/iommu/dma-iommu.c
>> +++ b/drivers/iommu/dma-iommu.c
>> @@ -31,6 +31,7 @@
>> #include <linux/vmalloc.h>
>> #include "dma-iommu.h"
>> +#include "iommu-priv.h"
>> struct iommu_dma_msi_page {
>> struct list_head list;
>> @@ -1652,7 +1653,7 @@ static struct iommu_dma_msi_page
>> *iommu_dma_get_msi_page(struct device *dev,
>> int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr)
>> {
>> struct device *dev = msi_desc_to_dev(desc);
>> - struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
>> + struct iommu_domain *domain = iommu_get_unmanaged_domain(dev);
>
> This still doesn't make sense - most of the time this will be expected
> to return the default DMA/identity domain if that's what the device is
> currently using. We can't know whether the current domain is managed
> or not until we look at it.
I tend to agree with Robin here. This was first introduced by
[PATCH v7 21/22] iommu/dma: Add support for mapping MSIs <https://lore.kernel.org/all/2273af20d844bd618c6a90b57e639700328ebf7f.1473695704.git.robin.murphy@arm.com/#r>
https://lore.kernel.org/all/2273af20d844bd618c6a90b57e639700328ebf7f.1473695704.git.robin.murphy@arm.com/
even before the support un VFIO use case which came later on. So using the "unmanaged" terminology sounds improper to me, at least.
Couldn't we use a parent/child terminology as used in the past in
[RFC v2] /dev/iommu uAPI proposal <https://lore.kernel.org/all/BN9PR11MB5433B1E4AE5B0480369F97178C189@BN9PR11MB5433.namprd11.prod.outlook.com/#r>
This would still hold for the former use case.
Thanks
Eric
>
> Just like every other caller of iommu_get_domain_for_dev(), what we
> want here is the current kernel-owned domain that we can inspect and
> maybe do standard IOMMU API things with. Why can't
> iommu_get_domain_for_dev() simply maintain that established usage
> model and return the kernel-owned s2_domain from a nested domain
> automatically? No IOMMU API user expects or needs it to return
> anything else (and IOMMUFD should certainly not be losing track of a
> nested domain within its own higher-level abstractions and needing to
> fall back on iommu_get_domain_for_dev()), so I really don't see a
> valid reason to overcomplicate things.
>
> Please note I stress "valid" since I'm not buying arbitrarily made-up
> conceptual purity arguments. A nested domain cannot be the "one true
> domain" that is an opaque combination of S1+S2; the IOMMU API view has
> to be more like the device is attached to both the nested domain and
> the parent stage 2 domain somewhat in parallel. Even when nesting is
> active, the S2 domain still exists as a domain in its own right, and
> still needs to be visible and operated on as such, for instance if
> memory is hotplugged in or out of the VM.
>
> TBH I'd also move the s2_domain pointer into the iommu_domain itself,
> since it's going to be a common feature for all nesting
> implementations, thus there seems little need to indirect lookups
> through the drivers at all.
>
> Thanks,
> Robin.
>
>> struct iommu_dma_msi_page *msi_page;
>> static DEFINE_MUTEX(msi_prepare_lock); /* see below */
>> @@ -1685,7 +1686,7 @@ int iommu_dma_prepare_msi(struct msi_desc
>> *desc, phys_addr_t msi_addr)
>> void iommu_dma_compose_msi_msg(struct msi_desc *desc, struct
>> msi_msg *msg)
>> {
>> struct device *dev = msi_desc_to_dev(desc);
>> - const struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
>> + const struct iommu_domain *domain =
>> iommu_get_unmanaged_domain(dev);
>> const struct iommu_dma_msi_page *msi_page;
>> msi_page = msi_desc_get_iommu_cookie(desc);
>> diff --git a/drivers/iommu/iommu-priv.h b/drivers/iommu/iommu-priv.h
>> index a6e694f59f64..da8044da9ad8 100644
>> --- a/drivers/iommu/iommu-priv.h
>> +++ b/drivers/iommu/iommu-priv.h
>> @@ -15,6 +15,21 @@ static inline const struct iommu_ops
>> *dev_iommu_ops(struct device *dev)
>> return dev->iommu->iommu_dev->ops;
>> }
>> +static inline struct iommu_domain
>> *iommu_get_unmanaged_domain(struct device *dev)
>> +{
>> + const struct iommu_ops *ops;
>> +
>> + if (!dev->iommu || !dev->iommu->iommu_dev)
>> + goto attached_domain;
>> +
>> + ops = dev_iommu_ops(dev);
>> + if (ops->get_unmanaged_domain)
>> + return ops->get_unmanaged_domain(dev);
>> +
>> +attached_domain:
>> + return iommu_get_domain_for_dev(dev);
>> +}
>> +
>> int iommu_group_replace_domain(struct iommu_group *group,
>> struct iommu_domain *new_domain);
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index 080278c8154d..76c65cc4fc15 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -275,6 +275,8 @@ struct iommu_ops {
>> struct iommu_domain *parent,
>> const void *user_data);
>> + struct iommu_domain *(*get_unmanaged_domain)(struct device *dev);
>> +
>> struct iommu_device *(*probe_device)(struct device *dev);
>> void (*release_device)(struct device *dev);
>> void (*probe_finalize)(struct device *dev);
>
next prev parent reply other threads:[~2023-03-10 8:44 UTC|newest]
Thread overview: 165+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-09 10:53 [PATCH v1 00/14] Add Nested Translation Support for SMMUv3 Nicolin Chen
2023-03-09 10:53 ` [PATCH v1 01/14] iommu: Add iommu_get_unmanaged_domain helper Nicolin Chen
2023-03-09 12:51 ` Robin Murphy
2023-03-09 14:19 ` Jason Gunthorpe
2023-03-09 19:04 ` Robin Murphy
2023-03-10 0:23 ` Jason Gunthorpe
2023-03-10 8:41 ` Eric Auger [this message]
2023-03-10 15:55 ` Jason Gunthorpe
2023-03-16 1:21 ` Nicolin Chen
2023-03-16 18:42 ` Robin Murphy
2023-03-16 20:01 ` Nicolin Chen
2023-03-20 12:51 ` Jason Gunthorpe
2023-03-10 10:14 ` Eric Auger
2023-03-10 15:33 ` Jason Gunthorpe
2023-03-10 15:44 ` Shameerali Kolothum Thodi
2023-03-10 15:56 ` Jason Gunthorpe
2023-03-10 16:07 ` Shameerali Kolothum Thodi
2023-03-10 16:21 ` Jason Gunthorpe
2023-03-10 16:30 ` Shameerali Kolothum Thodi
2023-03-10 17:03 ` Jason Gunthorpe
2023-03-22 16:07 ` Eric Auger
2023-03-22 17:02 ` Jason Gunthorpe
2023-03-22 17:41 ` Eric Auger
2023-03-22 18:07 ` Jason Gunthorpe
2023-03-16 19:51 ` Nicolin Chen
2023-03-16 19:56 ` Shameerali Kolothum Thodi
2023-03-22 15:44 ` Eric Auger
2023-03-09 10:53 ` [PATCH v1 02/14] iommufd: Add nesting related data structures for ARM SMMUv3 Nicolin Chen
2023-03-09 13:42 ` Jean-Philippe Brucker
2023-03-09 14:48 ` Jason Gunthorpe
2023-03-09 18:26 ` Jean-Philippe Brucker
2023-03-09 21:01 ` Jason Gunthorpe
2023-03-10 12:16 ` Jean-Philippe Brucker
2023-03-10 14:52 ` Robin Murphy
2023-03-10 15:25 ` Jason Gunthorpe
2023-03-10 15:57 ` Robin Murphy
2023-03-10 16:03 ` Jason Gunthorpe
2023-03-17 10:10 ` Tian, Kevin
2023-03-17 10:04 ` Tian, Kevin
2023-03-10 4:50 ` Nicolin Chen
2023-03-10 12:54 ` Jean-Philippe Brucker
2023-03-10 14:00 ` Jason Gunthorpe
2023-03-10 16:06 ` Jason Gunthorpe
2023-03-16 0:59 ` Nicolin Chen
2023-03-09 15:26 ` Shameerali Kolothum Thodi
2023-03-09 15:40 ` Jason Gunthorpe
2023-03-09 15:51 ` Shameerali Kolothum Thodi
2023-03-09 15:59 ` Jason Gunthorpe
2023-03-09 16:07 ` Shameerali Kolothum Thodi
2023-03-10 5:26 ` Nicolin Chen
2023-03-10 5:36 ` Nicolin Chen
2023-03-10 12:55 ` Jason Gunthorpe
2023-03-10 5:18 ` Nicolin Chen
2023-03-10 5:04 ` Nicolin Chen
2023-03-10 11:33 ` Eric Auger
2023-03-10 12:51 ` Jason Gunthorpe
2023-03-17 10:17 ` Tian, Kevin
2023-03-09 10:53 ` [PATCH v1 03/14] iommufd/device: Setup MSI on kernel-managed domains Nicolin Chen
2023-03-10 16:45 ` Eric Auger
2023-03-11 0:17 ` Nicolin Chen
2023-03-09 10:53 ` [PATCH v1 04/14] iommu/arm-smmu-v3: Add arm_smmu_hw_info Nicolin Chen
2023-03-09 13:03 ` Robin Murphy
2023-03-10 1:17 ` Nicolin Chen
2023-03-10 15:28 ` Robin Murphy
2023-03-16 0:13 ` Nicolin Chen
2023-03-16 15:19 ` Robin Murphy
2023-03-16 20:06 ` Nicolin Chen
2023-04-12 7:47 ` Nicolin Chen
2023-03-09 10:53 ` [PATCH v1 05/14] iommu/arm-smmu-v3: Remove ARM_SMMU_DOMAIN_NESTED Nicolin Chen
2023-03-10 16:39 ` Eric Auger
2023-03-10 17:05 ` Jason Gunthorpe
2023-03-11 0:24 ` Nicolin Chen
2023-03-11 0:23 ` Nicolin Chen
2023-03-09 10:53 ` [PATCH v1 06/14] iommu/arm-smmu-v3: Unset corresponding STE fields when s2_cfg is NULL Nicolin Chen
2023-03-09 13:13 ` Robin Murphy
2023-03-09 18:24 ` Shameerali Kolothum Thodi
2023-03-10 1:54 ` Nicolin Chen
2023-03-09 10:53 ` [PATCH v1 07/14] iommu/arm-smmu-v3: Add STRTAB_STE_0_CFG_NESTED for 2-stage translation Nicolin Chen
2023-03-09 10:53 ` [PATCH v1 08/14] iommu/arm-smmu-v3: Prepare for nested domain support Nicolin Chen
2023-03-10 20:39 ` Robin Murphy
2023-03-11 12:40 ` Nicolin Chen
2023-03-09 10:53 ` [PATCH v1 09/14] iommu/arm-smmu-v3: Implement arm_smmu_get_unmanaged_domain Nicolin Chen
2023-03-09 10:53 ` [PATCH v1 10/14] iommu/arm-smmu-v3: Pass in user_cfg to arm_smmu_domain_finalise Nicolin Chen
2023-03-09 10:53 ` [PATCH v1 11/14] iommu/arm-smmu-v3: Add arm_smmu_domain_alloc_user Nicolin Chen
2023-03-24 15:28 ` Eric Auger
2023-03-24 17:40 ` Nicolin Chen
2023-03-24 17:50 ` Jason Gunthorpe
2023-03-24 18:00 ` Nicolin Chen
2023-03-24 15:33 ` Eric Auger
2023-03-24 17:43 ` Nicolin Chen
2023-03-09 10:53 ` [PATCH v1 12/14] iommu/arm-smmu-v3: Support IOMMU_DOMAIN_NESTED type of allocations Nicolin Chen
2023-03-09 13:20 ` Robin Murphy
2023-03-09 14:28 ` Robin Murphy
2023-03-10 1:34 ` Nicolin Chen
2023-03-24 15:44 ` Eric Auger
2023-03-24 16:30 ` Jason Gunthorpe
2023-03-24 17:50 ` Nicolin Chen
2023-03-24 17:51 ` Jason Gunthorpe
2023-03-24 17:55 ` Nicolin Chen
2023-03-09 10:53 ` [PATCH v1 13/14] iommu/arm-smmu-v3: Add CMDQ_OP_TLBI_NH_VAA and CMDQ_OP_TLBI_NH_ALL Nicolin Chen
2023-03-09 13:44 ` Robin Murphy
2023-03-10 1:19 ` Nicolin Chen
2023-03-09 10:53 ` [PATCH v1 14/14] iommu/arm-smmu-v3: Add arm_smmu_cache_invalidate_user Nicolin Chen
2023-03-09 14:49 ` Robin Murphy
2023-03-09 15:31 ` Jason Gunthorpe
2023-03-10 4:20 ` Nicolin Chen
2023-03-10 16:19 ` Jason Gunthorpe
2023-03-11 11:56 ` Nicolin Chen
2023-03-11 12:53 ` Nicolin Chen
2023-03-20 13:03 ` Jason Gunthorpe
2023-03-20 15:56 ` Nicolin Chen
2023-03-20 16:04 ` Jason Gunthorpe
2023-03-20 16:59 ` Nicolin Chen
2023-03-20 18:45 ` Jason Gunthorpe
2023-03-20 21:22 ` Nicolin Chen
2023-03-20 22:19 ` Jason Gunthorpe
2023-03-22 20:57 ` Nicolin Chen
2023-03-23 12:17 ` Jason Gunthorpe
2023-03-17 9:41 ` Tian, Kevin
2023-03-17 14:24 ` Nicolin Chen
2023-03-20 12:59 ` Jason Gunthorpe
2023-03-20 16:12 ` Nicolin Chen
2023-03-20 18:00 ` Jason Gunthorpe
2023-03-21 8:34 ` Tian, Kevin
2023-03-21 11:48 ` Jason Gunthorpe
2023-03-22 6:42 ` Nicolin Chen
2023-03-22 12:43 ` Jason Gunthorpe
2023-03-22 17:11 ` Nicolin Chen
2023-03-22 17:28 ` Jason Gunthorpe
2023-03-22 19:21 ` Nicolin Chen
2023-03-22 19:41 ` Jason Gunthorpe
2023-03-22 20:43 ` Nicolin Chen
2023-03-23 12:16 ` Jason Gunthorpe
2023-03-23 18:13 ` Nicolin Chen
2023-03-23 18:27 ` Jason Gunthorpe
2023-03-24 9:02 ` Tian, Kevin
2023-03-24 14:57 ` Jason Gunthorpe
2023-03-24 17:35 ` Nicolin Chen
2023-03-28 3:03 ` Tian, Kevin
2023-03-24 8:47 ` Tian, Kevin
2023-03-24 14:44 ` Jason Gunthorpe
2023-03-28 2:48 ` Tian, Kevin
2023-03-28 12:26 ` Jason Gunthorpe
2023-03-31 8:09 ` Tian, Kevin
2023-03-17 9:24 ` Tian, Kevin
2023-03-10 3:51 ` Nicolin Chen
2023-03-10 17:53 ` Robin Murphy
2023-03-10 18:49 ` Jason Gunthorpe
2023-03-11 12:38 ` Nicolin Chen
2023-03-13 13:07 ` Robin Murphy
2023-03-16 0:01 ` Nicolin Chen
2023-03-16 14:58 ` Robin Murphy
2023-03-16 21:09 ` Nicolin Chen
2023-03-20 1:32 ` Nicolin Chen
2023-03-20 13:11 ` Jason Gunthorpe
2023-03-20 15:28 ` Nicolin Chen
2023-03-20 16:01 ` Jason Gunthorpe
2023-03-20 16:35 ` Nicolin Chen
2023-03-20 18:07 ` Jason Gunthorpe
2023-03-20 20:46 ` Nicolin Chen
2023-03-20 22:14 ` Jason Gunthorpe
2023-03-22 5:14 ` Nicolin Chen
2023-03-24 8:55 ` Tian, Kevin
2023-03-17 9:47 ` Tian, Kevin
2023-03-17 14:16 ` Nicolin Chen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4938b20b-14d8-86f8-e80b-9d8ed9d8f28d@redhat.com \
--to=eric.auger@redhat.com \
--cc=baolu.lu@linux.intel.com \
--cc=iommu@lists.linux.dev \
--cc=jean-philippe@linaro.org \
--cc=jgg@nvidia.com \
--cc=joro@8bytes.org \
--cc=kevin.tian@intel.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=nicolinc@nvidia.com \
--cc=robin.murphy@arm.com \
--cc=shameerali.kolothum.thodi@huawei.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).