Hi all,
This patch series refactors the arm-smmu-v3 driver and implements the
set_dev_pasid functionality for DMA and UNMANAGED iommu domains. As part
of this effort, we also refactor the arm-smmu-v3 driver such that each
iommu domain represent a single address space. In particular, stage 1
domains hold a single ContextDescriptor instead of the entire STE entry.
The refactor is arguably valuable independently from the set_dev_pasid
feature since an iommu_domain is conceptually closer to a single address
space than an entire STE. In addition this unlocks some nice clean-up of
the arm SVA implementation which today piggybacks SVA domains on the
"primary" domain.
This patch series makes some changes to SVA and PCIe, but I haven't
tested those features. The cd table allocations could also be further
optimized for masters that don't support multiple context. However, the
SMMUv3 Nested translation patch series has me worried about this
change. At a glance, it seems like this refactor conflicts with its
proposed uAPI. Is this refactor no longer an appropriate clean-up or
path forward for set_dev_pasid support? Or could a uAPI that only
exposes a single CD instead of the entire STE be an appropriate fit for
the nesting use cases?
Thanks,
Michael Shavit
Link: https://lore.kernel.org/all/CAKHBV24u9b-=cJCF=Kt=3B4hynOyNr6gmi0F6zpO6b1mHc0Z7g@mail.gmail.com
Link: https://lore.kernel.org/all/[email protected]/
Michael Shavit (5):
iommu/arm-smmu-v3: Move cdtable to arm_smmu_master
iommu/arm-smmu-v3: Add has_stage1 field
iommu/arm-smmu-v3: Simplify arm_smmu_enable_ats
iommu/arm-smmu-v3: Keep track of attached ssids
iommu/arm-smmu-v3: Implement set_dev_pasid
.../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 46 +-
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 432 ++++++++++++------
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 44 +-
3 files changed, 344 insertions(+), 178 deletions(-)
--
2.40.1.521.gf1e218fcd8-goog
Inferring the state of the STE based on attached domains becomes tricker
when multiple domains can be attached to a master on different PASIDs.
The new field allows the smmu driver to directly query the state of the
STE (S1 present, S2 present, neither) instead of inferring it from the
attached domain.
Signed-off-by: Michael Shavit <[email protected]>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 22 +++++++--------------
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 +
2 files changed, 8 insertions(+), 15 deletions(-)
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index cee3efff3c9fa..7b4399b5ba68b 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -980,9 +980,9 @@ static void arm_smmu_sync_cd(struct arm_smmu_master *master,
};
/*
- * There's nothing to sync if the STE isn't valid yet.
+ * There's nothing to sync if stage 1 hasn't been installed yet.
*/
- if (!master->domain)
+ if (!master->has_stage1)
return;
cmds.num = 0;
@@ -1278,20 +1278,11 @@ static void arm_smmu_write_strtab_ent(struct arm_smmu_master *master, u32 sid,
.sid = sid,
},
};
- struct iommu_domain *domain = NULL;
-
if (master) {
smmu = master->smmu;
- if (master->domain)
- domain = &master->domain->domain;
- }
- if (domain) {
- if (domain->type != IOMMU_DOMAIN_IDENTITY) {
- if (master->s2_cfg)
- s2_cfg = master->s2_cfg;
- else
- s1_cfg = &master->s1_cfg;
- }
+ if (master->has_stage1)
+ s1_cfg = &master->s1_cfg;
+ s2_cfg = master->s2_cfg;
}
if (val & STRTAB_STE_0_V) {
@@ -2367,9 +2358,9 @@ static void arm_smmu_detach_dev(struct arm_smmu_master *master)
list_del(&master->domain_head);
spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
- master->domain = NULL;
master->ats_enabled = false;
master->s2_cfg = NULL;
+ master->has_stage1 = false;
/*
* Note that this will end up calling arm_smmu_sync_cd() even though
* we're about to destroy the entire STE anyways. This is ok because
@@ -2426,6 +2417,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
* This isn't an issue because the STE hasn't been installed yet.
*/
if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
+ master->has_stage1 = true;
ret = arm_smmu_write_ctx_desc(master, 0, &smmu_domain->cd);
if (ret)
goto out_unlock;
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index 0b87c74bdf46e..d715794572b13 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -689,6 +689,7 @@ struct arm_smmu_master {
struct list_head domain_head;
struct arm_smmu_stream *streams;
struct arm_smmu_s1_cfg s1_cfg;
+ bool has_stage1;
struct arm_smmu_s2_cfg *s2_cfg;
unsigned int num_streams;
bool ats_enabled;
--
2.40.1.521.gf1e218fcd8-goog
On Thu, May 11, 2023 at 04:50:47AM +0800, Michael Shavit wrote:
> Hi all,
>
> This patch series refactors the arm-smmu-v3 driver and implements the
> set_dev_pasid functionality for DMA and UNMANAGED iommu domains. As part
> of this effort, we also refactor the arm-smmu-v3 driver such that each
> iommu domain represent a single address space. In particular, stage 1
> domains hold a single ContextDescriptor instead of the entire STE entry.
I'm not sure what you mean "holds" a ContextDescriptor?
Ideally the iommu_domain should only hold a pointer to some table
top. Depending on the domain type this would be a S1 IOPTE table, S2
IOPTE table or a CD table. Plus the non-table domains like IDENTITY
and blocked.
Logically when an iommu_domain is attached to a device or a PASID a
STE or CD is generated from the iommu_domain's configuration but the
iommu_domain doesn't "hold" it
When a kernel-owned CD table is used it should be held someplace else,
certianly not in the iommu_domain. Logically as a per-device
structure, but maybe with optimizations for sharing.
> The refactor is arguably valuable independently from the set_dev_pasid
> feature since an iommu_domain is conceptually closer to a single address
> space than an entire STE. In addition this unlocks some nice clean-up of
> the arm SVA implementation which today piggybacks SVA domains on the
> "primary" domain.
I always thought of this as sort of a pre-calculation of the STE, that
gets cached in the iommu_domain? Not sure the pre-calculation is that
valuable though..
> path forward for set_dev_pasid support? Or could a uAPI that only
> exposes a single CD instead of the entire STE be an appropriate fit for
> the nesting use cases?
The uAPI is to create an iommu_domain that holds a CD Table Top
located in user memory, it cannot deviate from this. These kinds of
iommu_domain's can only be pointed at by STEs.
Again it doesnt really "hold" the STE, but we can compute a STE that
points to the SD Table that it does hold.
Other than this, it is good to take this project on, getting PASID
support aligned with the new API is something that needs to be done
here!
Thanks,
Jason
This change enables the use of the iommu_attach_dev_pasid API for
UNMANAGED domains. The primary use-case is to allow in-kernel users of
the iommu API to manage domains with PASID. This change also allows for
future support of pasid in the DMA api.
Signed-off-by: Michael Shavit <[email protected]>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 165 +++++++++++++++++---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 +
2 files changed, 147 insertions(+), 19 deletions(-)
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 81f49a86c1266..468a7a30ffe7b 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2353,6 +2353,11 @@ static int arm_smmu_enable_pasid(struct arm_smmu_master *master)
return 0;
}
+static bool arm_smmu_master_has_pasid_domains(struct arm_smmu_master *master)
+{
+ return master->nr_attached_pasid_domains > 0;
+}
+
static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
{
struct pci_dev *pdev;
@@ -2396,6 +2401,33 @@ static void arm_smmu_detach_dev(struct arm_smmu_master *master)
arm_smmu_install_ste_for_dev(master);
}
+/*
+ * Once attached for the first time, a domain can no longer be attached to any
+ * master with a distinct upstream SMMU.
+ */
+static int arm_smmu_prepare_domain_for_smmu(struct device *dev,
+ struct arm_smmu_device *smmu,
+ struct arm_smmu_domain *smmu_domain)
+{
+ int ret = 0;
+
+ mutex_lock(&smmu_domain->init_mutex);
+ if (!smmu_domain->smmu) {
+ smmu_domain->smmu = smmu;
+ ret = arm_smmu_domain_finalise(&smmu_domain->domain);
+ if (ret) {
+ smmu_domain->smmu = NULL;
+ goto out_unlock;
+ }
+ } else if (smmu_domain->smmu != smmu) {
+ ret = -EINVAL;
+ goto out_unlock;
+ }
+out_unlock:
+ mutex_unlock(&smmu_domain->init_mutex);
+ return ret;
+}
+
static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
{
int ret = 0;
@@ -2411,6 +2443,10 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
master = dev_iommu_priv_get(dev);
smmu = master->smmu;
+ ret = arm_smmu_prepare_domain_for_smmu(dev, smmu, smmu_domain);
+ if (ret)
+ return ret;
+
/*
* Checking that SVA is disabled ensures that this device isn't bound to
* any mm, and can be safely detached from its old domain. Bonds cannot
@@ -2421,22 +2457,18 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
return -EBUSY;
}
- arm_smmu_detach_dev(master);
-
- mutex_lock(&smmu_domain->init_mutex);
-
- if (!smmu_domain->smmu) {
- smmu_domain->smmu = smmu;
- ret = arm_smmu_domain_finalise(domain);
- if (ret) {
- smmu_domain->smmu = NULL;
- goto out_unlock;
- }
- } else if (smmu_domain->smmu != smmu) {
- ret = -EINVAL;
- goto out_unlock;
+ /*
+ * Attaching a bypass or stage 2 domain would break any domains attached
+ * with pasid. Attaching an S1 domain should be feasible but requires
+ * more complicated logic to handle.
+ */
+ if (arm_smmu_master_has_pasid_domains(master)) {
+ dev_err(dev, "cannot attach - domain attached with pasid\n");
+ return -EBUSY;
}
+ arm_smmu_detach_dev(master);
+
/*
* Note that this will end up calling arm_smmu_sync_cd() before
* the master has been added to the devices list for this domain.
@@ -2446,7 +2478,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
master->has_stage1 = true;
ret = arm_smmu_write_ctx_desc(master, 0, &smmu_domain->cd);
if (ret)
- goto out_unlock;
+ return ret;
} else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S2 ||
smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED) {
master->s2_cfg = &smmu_domain->s2_cfg;
@@ -2473,11 +2505,74 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
arm_smmu_enable_ats(master, smmu_domain);
-out_unlock:
- mutex_unlock(&smmu_domain->init_mutex);
return ret;
}
+static int arm_smmu_set_dev_pasid(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid)
+{
+ int ret = 0;
+ unsigned long flags;
+ struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+ struct arm_smmu_device *smmu;
+ struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+ struct arm_smmu_attached_domain *attached_domain;
+ struct arm_smmu_master *master;
+
+ if (!fwspec)
+ return -ENOENT;
+
+ master = dev_iommu_priv_get(dev);
+ smmu = master->smmu;
+
+ ret = arm_smmu_prepare_domain_for_smmu(dev, smmu, smmu_domain);
+ if (ret)
+ return ret;
+
+ if (pasid == 0) {
+ dev_err(dev, "pasid 0 is reserved for the device's primary domain\n");
+ return -ENODEV;
+ }
+
+ if (smmu_domain->stage != ARM_SMMU_DOMAIN_S1) {
+ dev_err(dev, "set_dev_pasid only supports stage 1 domains\n");
+ return -EINVAL;
+ }
+
+ if (!master->has_stage1 || master->s2_cfg)
+ return -EBUSY;
+
+ attached_domain = kzalloc(sizeof(*attached_domain), GFP_KERNEL);
+ if (!attached_domain)
+ return -ENOMEM;
+
+ attached_domain->master = master;
+ attached_domain->domain = smmu_domain;
+ attached_domain->ssid = pasid;
+
+ master->nr_attached_pasid_domains += 1;
+ /*
+ * arm_smmu_share_asid may update the cd's asid value and write the
+ * ctx_desc for every attached_domains in the list. There's a potential
+ * race here regardless of whether we first the write the ctx_desc or
+ * first insert into the domain's list. Grabbing the asic_lock prevents
+ * SVA from changing the cd's ASID while the cd is being attached.
+ */
+ mutex_lock(&arm_smmu_asid_lock);
+ ret = arm_smmu_write_ctx_desc(master, pasid, &smmu_domain->cd);
+ if (ret) {
+ mutex_unlock(&arm_smmu_asid_lock);
+ kfree(attached_domain);
+ }
+
+ spin_lock_irqsave(&smmu_domain->attached_domains_lock, flags);
+ list_add(&attached_domain->domain_head, &smmu_domain->attached_domains);
+ spin_unlock_irqrestore(&smmu_domain->attached_domains_lock, flags);
+ mutex_unlock(&arm_smmu_asid_lock);
+
+ return 0;
+}
+
static int arm_smmu_map_pages(struct iommu_domain *domain, unsigned long iova,
phys_addr_t paddr, size_t pgsize, size_t pgcount,
int prot, gfp_t gfp, size_t *mapped)
@@ -2723,6 +2818,15 @@ static void arm_smmu_release_device(struct device *dev)
if (WARN_ON(arm_smmu_master_sva_enabled(master)))
iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
+ if (WARN_ON(master->nr_attached_pasid_domains != 0)) {
+ /*
+ * TODO: Do we need to handle this case?
+ * This requires a mechanism to obtain all the pasid domains
+ * that this master is attached to so that we can clean up the
+ * domain's attached_domain list.
+ */
+ }
+
arm_smmu_detach_dev(master);
arm_smmu_free_cd_tables(master);
arm_smmu_disable_pasid(master);
@@ -2858,12 +2962,34 @@ static int arm_smmu_def_domain_type(struct device *dev)
static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
{
struct iommu_domain *domain;
+ struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+ struct arm_smmu_domain *smmu_domain;
+ struct arm_smmu_attached_domain *attached_domain;
+ unsigned long flags;
- domain = iommu_get_domain_for_dev_pasid(dev, pasid, IOMMU_DOMAIN_SVA);
+ if (!master || pasid == 0)
+ return;
+
+ domain = iommu_get_domain_for_dev_pasid(dev, pasid, 0);
if (WARN_ON(IS_ERR(domain)) || !domain)
return;
+ if (domain->type == IOMMU_DOMAIN_SVA)
+ return arm_smmu_sva_remove_dev_pasid(domain, dev, pasid);
- arm_smmu_sva_remove_dev_pasid(domain, dev, pasid);
+ smmu_domain = to_smmu_domain(domain);
+ mutex_lock(&arm_smmu_asid_lock);
+ spin_lock_irqsave(&smmu_domain->attached_domains_lock, flags);
+ list_for_each_entry(attached_domain, &smmu_domain->attached_domains, domain_head) {
+ if (attached_domain->master != master ||
+ attached_domain->ssid != pasid)
+ continue;
+ list_del(&attached_domain->domain_head);
+ break;
+ }
+ spin_unlock_irqrestore(&smmu_domain->attached_domains_lock, flags);
+ arm_smmu_write_ctx_desc(master, pasid, NULL);
+ master->nr_attached_pasid_domains -= 1;
+ mutex_unlock(&arm_smmu_asid_lock);
}
static struct iommu_ops arm_smmu_ops = {
@@ -2883,6 +3009,7 @@ static struct iommu_ops arm_smmu_ops = {
.owner = THIS_MODULE,
.default_domain_ops = &(const struct iommu_domain_ops) {
.attach_dev = arm_smmu_attach_dev,
+ .set_dev_pasid = arm_smmu_set_dev_pasid,
.map_pages = arm_smmu_map_pages,
.unmap_pages = arm_smmu_unmap_pages,
.flush_iotlb_all = arm_smmu_flush_iotlb_all,
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index 35700534a0b4a..9ef63ea381f4b 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -705,6 +705,7 @@ struct arm_smmu_master {
bool iopf_enabled;
struct list_head bonds;
unsigned int ssid_bits;
+ unsigned int nr_attached_pasid_domains;
};
/* SMMU private data for an IOMMU domain */
--
2.40.1.521.gf1e218fcd8-goog
> Logically when an iommu_domain is attached to a device or a PASID a
> STE or CD is generated from the iommu_domain's configuration but the
> iommu_domain doesn't "hold" it
>
Ah yes, I was using iommu domain and arm_smmu_domain interchangeably
here since there's a 1:1 mapping between the two. In the current
smmuv3 implementation, arm_smmu_domain holds the s1cfg structure which
represents the s1 portion of an STE.
> I always thought of this as sort of a pre-calculation of the STE, that
> gets cached in the iommu_domain? Not sure the pre-calculation is that
> valuable though..
I think we can consider the case where an iommu domain is attached to
multiple masters as a form of pre-calculation and resource sharing.
When attaching an iommu domain that has been attached before, its
arm_smmu_domain contains an already finalized STE configuration that
can be immediately written to the smmu. I don't think there's anything
specific to SVA about this behavior however, SVA will do the same
amount of work whether the cd table is owned by some special iommu
domain or by the arm_smmu_master (since we require that special iommu
domain be attached to the master and disallow detaching it).
> Ideally the iommu_domain should only hold a pointer to some table
top. Depending on the domain type this would be a S1 IOPTE table, S2
IOPTE table or a CD table. Plus the non-table domains like IDENTITY
and blocked.
Gotcha. So this patch series should be less aggressive, but is
probably still workable with the nested domain patch series:
1. For (stage 1) unmanaged/dma and sva domains, arm_smmu_domains
should hold a single CD. For the nested domain series, arm_smmu_domain
can alternatively hold an entire s1cfg.
2. arm_smmu_master should own an s1cfg (which holds a cdtable) that is
used by unmanaged/dma and sva domains attached to this master.
3. arm_smmu_master also holds a pointer to the live s1cfg, which may
either points to its owned s1cfg, or the nested domain's s1cfg, or
null (bypass or stage2)
> > path forward for set_dev_pasid support? Or could a uAPI that only
> > exposes a single CD instead of the entire STE be an appropriate fit for
> > the nesting use cases?
>
> The uAPI is to create an iommu_domain that holds a CD Table Top
> located in user memory, it cannot deviate from this. These kinds of
> iommu_domain's can only be pointed at by STEs.
>
> Again it doesnt really "hold" the STE, but we can compute a STE that
> points to the SD Table that it does hold.
>
> Other than this, it is good to take this project on, getting PASID
> support aligned with the new API is something that needs to be done
> here!
>
> Thanks,
> Jason
On Thu, May 11, 2023 at 11:52:41AM +0800, Michael Shavit wrote:
> > Logically when an iommu_domain is attached to a device or a PASID a
> > STE or CD is generated from the iommu_domain's configuration but the
> > iommu_domain doesn't "hold" it
> >
>
> Ah yes, I was using iommu domain and arm_smmu_domain interchangeably
> here since there's a 1:1 mapping between the two. In the current
> smmuv3 implementation, arm_smmu_domain holds the s1cfg structure which
> represents the s1 portion of an STE.
I mean to include the arm_smmu_domain too..
Generally this sort of seemed OK in the code, I just wouldn't use the
word 'hold' - the iommu_domain may cache a computed STE or CD value
but that the actual steering or context tables are held else where
ie you insert an iommu_domain into a steering or context table, it
does not 'hold' a table entry.
> specific to SVA about this behavior however, SVA will do the same
> amount of work whether the cd table is owned by some special iommu
> domain or by the arm_smmu_master (since we require that special iommu
> domain be attached to the master and disallow detaching it).
The CD table for SVA definately should not be part of an iommu_domain,
moving it to the master seems reasonable.
> Gotcha. So this patch series should be less aggressive, but is
> probably still workable with the nested domain patch series:
> 1. For (stage 1) unmanaged/dma and sva domains, arm_smmu_domains
> should hold a single CD. For the nested domain series, arm_smmu_domain
> can alternatively hold an entire s1cfg.
These are just pre computed values the can help when inserting the
iommu_domain into a steering or CD table
> 2. arm_smmu_master should own an s1cfg (which holds a cdtable) that is
> used by unmanaged/dma and sva domains attached to this master.
The arm_smmu_master's cd table can be inserted into a steering table
> 3. arm_smmu_master also holds a pointer to the live s1cfg, which may
> either points to its owned s1cfg, or the nested domain's s1cfg, or
> null (bypass or stage2)
The steering table either points to the CD table owned by the
arm_smmu_master, a S1 domain held by an iommu_domain, or a S1 & CD
table owned by userspace represented by a special nested iommu_domain
and its internal parent.
If a kernel owned S2 it attached then the S1 points at the CD table
owned by the arm_smmu_master and the CD table points to the S2, same
as if there was PASID (IIRC, from memory I don't have the spec here
right now)
Think about it in terms of what object owns the table and what other
object(s) are inserted into the the table. Nothing "holds" a table
entry.
Jason
On 2023-05-11 05:33, Jason Gunthorpe wrote:
> On Thu, May 11, 2023 at 11:52:41AM +0800, Michael Shavit wrote:
>>> Logically when an iommu_domain is attached to a device or a PASID a
>>> STE or CD is generated from the iommu_domain's configuration but the
>>> iommu_domain doesn't "hold" it
>>>
>>
>> Ah yes, I was using iommu domain and arm_smmu_domain interchangeably
>> here since there's a 1:1 mapping between the two. In the current
>> smmuv3 implementation, arm_smmu_domain holds the s1cfg structure which
>> represents the s1 portion of an STE.
>
> I mean to include the arm_smmu_domain too..
>
> Generally this sort of seemed OK in the code, I just wouldn't use the
> word 'hold' - the iommu_domain may cache a computed STE or CD value
> but that the actual steering or context tables are held else where
>
> ie you insert an iommu_domain into a steering or context table, it
> does not 'hold' a table entry.
>
>> specific to SVA about this behavior however, SVA will do the same
>> amount of work whether the cd table is owned by some special iommu
>> domain or by the arm_smmu_master (since we require that special iommu
>> domain be attached to the master and disallow detaching it).
>
> The CD table for SVA definately should not be part of an iommu_domain,
> moving it to the master seems reasonable.
>
>> Gotcha. So this patch series should be less aggressive, but is
>> probably still workable with the nested domain patch series:
>> 1. For (stage 1) unmanaged/dma and sva domains, arm_smmu_domains
>> should hold a single CD. For the nested domain series, arm_smmu_domain
>> can alternatively hold an entire s1cfg.
>
> These are just pre computed values the can help when inserting the
> iommu_domain into a steering or CD table
Right, a stage 1 domain still logically owns the *contents* of a
corresponding CD, however in this design it can no longer own a physical
CD structure, because now every device attached to the same domain must
maintain its own distinct copy.
>> 2. arm_smmu_master should own an s1cfg (which holds a cdtable) that is
>> used by unmanaged/dma and sva domains attached to this master.
>
> The arm_smmu_master's cd table can be inserted into a steering table
Not sure what you mean there... STE.S1ContextPtr is essentially just a
pointer to an array of CD structures (which only contains 1 element when
PASIDs aren't enabled), so every master must own its own CD table
directly. There is no viable indirection if you want the abstraction to
bear any relation to reality.
>> 3. arm_smmu_master also holds a pointer to the live s1cfg, which may
>> either points to its owned s1cfg, or the nested domain's s1cfg, or
>> null (bypass or stage2)
>
> The steering table either points to the CD table owned by the
> arm_smmu_master, a S1 domain held by an iommu_domain, or a S1 & CD
> table owned by userspace represented by a special nested iommu_domain
> and its internal parent.
>
> If a kernel owned S2 it attached then the S1 points at the CD table
> owned by the arm_smmu_master and the CD table points to the S2, same
> as if there was PASID (IIRC, from memory I don't have the spec here
> right now)
Nope, CDs *only* represent stage 1 domains, stage 2 configuration is a
property of the STE, i.e. is directly connected to the arm_smmu_master.
Thanks,
Robin.
On Thu, May 11, 2023 at 01:33:58PM +0100, Robin Murphy wrote:
> > > 2. arm_smmu_master should own an s1cfg (which holds a cdtable) that is
> > > used by unmanaged/dma and sva domains attached to this master.
> >
> > The arm_smmu_master's cd table can be inserted into a steering table
>
> Not sure what you mean there... STE.S1ContextPtr is essentially just a
> pointer to an array of CD structures (which only contains 1 element when
> PASIDs aren't enabled), so every master must own its own CD table directly.
> There is no viable indirection if you want the abstraction to bear any
> relation to reality.
Yes, this is what I mean. Whenever we need a kernel owned CD table it
comes from the smmu master and is inserted into the steering table
owned by the arm_smmu_device.
"Insert" is just the usual verb we tend to use when talking about
these kinds of structures. Ie a PTE is inserted into a page table and
points at a page - a page table doesn't hold a PTE owned by the page.
So we have, basically, three kinds of tables, Steering/CD/IOPTE, they
are owned by their respective objects
arm_smmu_device/arm_smmu_master/arm_smmu_domain
And we insert pointers from Steering -> CD -> IOPTE as appropriate.
The only case a CD table is not in the arm_smmu_master is for nesting,
but we can still say that the nesting domain is inserted into the
steering table.
Jason