Use it to store all vSMMU-related data. The vsid (Virtual Stream ID) will
be the first use case. Since the vsid reader will be the eventq handler
that already holds a streams_mutex, reuse that to fenche the vmaster too.
Also add a pair of arm_smmu_attach_prepare/commit_vmaster helpers to set
or unset the master->vmaster point. Put these helpers inside the existing
arm_smmu_attach_prepare/commit().
For identity/blocked ops that don't call arm_smmu_attach_prepare/commit(),
add a simpler arm_smmu_master_clear_vmaster helper to unset the vmaster.
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 27 ++++++++++
.../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 53 +++++++++++++++++++
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 15 +++++-
3 files changed, 94 insertions(+), 1 deletion(-)
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 bd9d7c85576a..85352504343b 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -799,6 +799,11 @@ struct arm_smmu_stream {
struct rb_node node;
};
+struct arm_smmu_vmaster {
+ struct arm_vsmmu *vsmmu;
+ unsigned long vsid;
+};
+
struct arm_smmu_event {
u8 stall : 1,
ssv : 1,
@@ -824,6 +829,8 @@ struct arm_smmu_master {
struct arm_smmu_device *smmu;
struct device *dev;
struct arm_smmu_stream *streams;
+ /* Locked by smmu->streams_mutex */
+ struct arm_smmu_vmaster *vmaster;
/* Locked by the iommu core using the group mutex */
struct arm_smmu_ctx_desc_cfg cd_table;
unsigned int num_streams;
@@ -972,6 +979,7 @@ struct arm_smmu_attach_state {
bool disable_ats;
ioasid_t ssid;
/* Resulting state */
+ struct arm_smmu_vmaster *vmaster;
bool ats_enabled;
};
@@ -1055,9 +1063,28 @@ struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev,
struct iommu_domain *parent,
struct iommufd_ctx *ictx,
unsigned int viommu_type);
+int arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state,
+ struct iommu_domain *domain);
+void arm_smmu_attach_commit_vmaster(struct arm_smmu_attach_state *state);
+void arm_smmu_master_clear_vmaster(struct arm_smmu_master *master);
#else
#define arm_smmu_hw_info NULL
#define arm_vsmmu_alloc NULL
+
+static inline int
+arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state,
+ struct iommu_domain *domain)
+{
+ return 0; /* NOP */
+}
+
+static inline void
+arm_smmu_attach_commit_vmaster(struct arm_smmu_attach_state *state)
+{
+}
+static inline void arm_smmu_master_clear_vmaster(struct arm_smmu_master *master)
+{
+}
#endif /* CONFIG_ARM_SMMU_V3_IOMMUFD */
#endif /* _ARM_SMMU_V3_H */
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
index 5aa2e7af58b4..364d8469a480 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
@@ -85,6 +85,59 @@ static void arm_smmu_make_nested_domain_ste(
}
}
+int arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state,
+ struct iommu_domain *domain)
+{
+ struct arm_smmu_nested_domain *nested_domain;
+ struct arm_smmu_vmaster *vmaster;
+ unsigned long vsid;
+ int ret;
+
+ iommu_group_mutex_assert(state->master->dev);
+
+ if (domain->type != IOMMU_DOMAIN_NESTED)
+ return 0;
+ nested_domain = to_smmu_nested_domain(domain);
+
+ /* Skip invalid vSTE */
+ if (!(nested_domain->ste[0] & cpu_to_le64(STRTAB_STE_0_V)))
+ return 0;
+
+ ret = iommufd_viommu_get_vdev_id(&nested_domain->vsmmu->core,
+ state->master->dev, &vsid);
+ if (ret)
+ return ret;
+
+ vmaster = kzalloc(sizeof(*vmaster), GFP_KERNEL);
+ if (!vmaster)
+ return -ENOMEM;
+ vmaster->vsmmu = nested_domain->vsmmu;
+ vmaster->vsid = vsid;
+ state->vmaster = vmaster;
+
+ return 0;
+}
+
+void arm_smmu_attach_commit_vmaster(struct arm_smmu_attach_state *state)
+{
+ struct arm_smmu_master *master = state->master;
+
+ mutex_lock(&master->smmu->streams_mutex);
+ if (state->vmaster != master->vmaster) {
+ kfree(master->vmaster);
+ master->vmaster = state->vmaster;
+ }
+ mutex_unlock(&master->smmu->streams_mutex);
+}
+
+void arm_smmu_master_clear_vmaster(struct arm_smmu_master *master)
+{
+ mutex_lock(&master->smmu->streams_mutex);
+ kfree(master->vmaster);
+ master->vmaster = NULL;
+ mutex_unlock(&master->smmu->streams_mutex);
+}
+
static int arm_smmu_attach_dev_nested(struct iommu_domain *domain,
struct device *dev)
{
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 358072b4e293..9e50bcee69d1 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2803,6 +2803,7 @@ int arm_smmu_attach_prepare(struct arm_smmu_attach_state *state,
struct arm_smmu_domain *smmu_domain =
to_smmu_domain_devices(new_domain);
unsigned long flags;
+ int ret;
/*
* arm_smmu_share_asid() must not see two domains pointing to the same
@@ -2832,9 +2833,15 @@ int arm_smmu_attach_prepare(struct arm_smmu_attach_state *state,
}
if (smmu_domain) {
+ ret = arm_smmu_attach_prepare_vmaster(state, new_domain);
+ if (ret)
+ return ret;
+
master_domain = kzalloc(sizeof(*master_domain), GFP_KERNEL);
- if (!master_domain)
+ if (!master_domain) {
+ kfree(state->vmaster);
return -ENOMEM;
+ }
master_domain->master = master;
master_domain->ssid = state->ssid;
if (new_domain->type == IOMMU_DOMAIN_NESTED)
@@ -2861,6 +2868,7 @@ int arm_smmu_attach_prepare(struct arm_smmu_attach_state *state,
spin_unlock_irqrestore(&smmu_domain->devices_lock,
flags);
kfree(master_domain);
+ kfree(state->vmaster);
return -EINVAL;
}
@@ -2893,6 +2901,8 @@ void arm_smmu_attach_commit(struct arm_smmu_attach_state *state)
lockdep_assert_held(&arm_smmu_asid_lock);
+ arm_smmu_attach_commit_vmaster(state);
+
if (state->ats_enabled && !master->ats_enabled) {
arm_smmu_enable_ats(master);
} else if (state->ats_enabled && master->ats_enabled) {
@@ -3162,6 +3172,7 @@ static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
struct arm_smmu_ste ste;
struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+ arm_smmu_master_clear_vmaster(master);
arm_smmu_make_bypass_ste(master->smmu, &ste);
arm_smmu_attach_dev_ste(domain, dev, &ste, STRTAB_STE_1_S1DSS_BYPASS);
return 0;
@@ -3180,7 +3191,9 @@ static int arm_smmu_attach_dev_blocked(struct iommu_domain *domain,
struct device *dev)
{
struct arm_smmu_ste ste;
+ struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+ arm_smmu_master_clear_vmaster(master);
arm_smmu_make_abort_ste(&ste);
arm_smmu_attach_dev_ste(domain, dev, &ste,
STRTAB_STE_1_S1DSS_TERMINATE);
--
2.43.0oN sAt, Feb 22, 2025 at 07:54:09AM -0800, Nicolin Chen wrote:
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> index 5aa2e7af58b4..364d8469a480 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> @@ -85,6 +85,59 @@ static void arm_smmu_make_nested_domain_ste(
> }
> }
>
> +int arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state,
> + struct iommu_domain *domain)
> +{
> + struct arm_smmu_nested_domain *nested_domain;
> + struct arm_smmu_vmaster *vmaster;
> + unsigned long vsid;
> + int ret;
> +
> + iommu_group_mutex_assert(state->master->dev);
> +
> + if (domain->type != IOMMU_DOMAIN_NESTED)
> + return 0;
> + nested_domain = to_smmu_nested_domain(domain);
> +
> + /* Skip invalid vSTE */
> + if (!(nested_domain->ste[0] & cpu_to_le64(STRTAB_STE_0_V)))
> + return 0;
> +
> + ret = iommufd_viommu_get_vdev_id(&nested_domain->vsmmu->core,
> + state->master->dev, &vsid);
> + if (ret)
> + return ret;
> +
> + vmaster = kzalloc(sizeof(*vmaster), GFP_KERNEL);
> + if (!vmaster)
> + return -ENOMEM;
> + vmaster->vsmmu = nested_domain->vsmmu;
> + vmaster->vsid = vsid;
> + state->vmaster = vmaster;
> +
> + return 0;
> +}
> +
> +void arm_smmu_attach_commit_vmaster(struct arm_smmu_attach_state *state)
> +{
> + struct arm_smmu_master *master = state->master;
> +
> + mutex_lock(&master->smmu->streams_mutex);
> + if (state->vmaster != master->vmaster) {
> + kfree(master->vmaster);
> + master->vmaster = state->vmaster;
> + }
Does this condition suggest that we might end up calling
`arm_smmu_attach_prepare_vmaster()` multiple times before __actually__
commiting to a vmaster?
> + mutex_unlock(&master->smmu->streams_mutex);
> +}
> +
> +void arm_smmu_master_clear_vmaster(struct arm_smmu_master *master)
> +{
> + mutex_lock(&master->smmu->streams_mutex);
> + kfree(master->vmaster);
> + master->vmaster = NULL;
> + mutex_unlock(&master->smmu->streams_mutex);
> +}
> +
> static int arm_smmu_attach_dev_nested(struct iommu_domain *domain,
> struct device *dev)
> {
> 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 358072b4e293..9e50bcee69d1 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2803,6 +2803,7 @@ int arm_smmu_attach_prepare(struct arm_smmu_attach_state *state,
> struct arm_smmu_domain *smmu_domain =
> to_smmu_domain_devices(new_domain);
> unsigned long flags;
> + int ret;
>
> /*
> * arm_smmu_share_asid() must not see two domains pointing to the same
> @@ -2832,9 +2833,15 @@ int arm_smmu_attach_prepare(struct arm_smmu_attach_state *state,
> }
>
> if (smmu_domain) {
> + ret = arm_smmu_attach_prepare_vmaster(state, new_domain);
IMO, this adds a little confusion for folks not using iommufd.
I guess it'd be cleaner if we invoke this below within the:
`if (new_domain->type == IOMMU_DOMAIN_NESTED)` condition instead of
simply returning from the function if the new_domain->type isn't NESTED.
> + if (ret)
> + return ret;
> +
> master_domain = kzalloc(sizeof(*master_domain), GFP_KERNEL);
> - if (!master_domain)
> + if (!master_domain) {
> + kfree(state->vmaster);
> return -ENOMEM;
> + }
> master_domain->master = master;
> master_domain->ssid = state->ssid;
> if (new_domain->type == IOMMU_DOMAIN_NESTED)
> @@ -2861,6 +2868,7 @@ int arm_smmu_attach_prepare(struct arm_smmu_attach_state *state,
> spin_unlock_irqrestore(&smmu_domain->devices_lock,
> flags);
> kfree(master_domain);
> + kfree(state->vmaster);
> return -EINVAL;
> }
>
> @@ -2893,6 +2901,8 @@ void arm_smmu_attach_commit(struct arm_smmu_attach_state *state)
>
> lockdep_assert_held(&arm_smmu_asid_lock);
>
> + arm_smmu_attach_commit_vmaster(state);
> +
> if (state->ats_enabled && !master->ats_enabled) {
> arm_smmu_enable_ats(master);
> } else if (state->ats_enabled && master->ats_enabled) {
> @@ -3162,6 +3172,7 @@ static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
> struct arm_smmu_ste ste;
> struct arm_smmu_master *master = dev_iommu_priv_get(dev);
>
> + arm_smmu_master_clear_vmaster(master);
> arm_smmu_make_bypass_ste(master->smmu, &ste);
> arm_smmu_attach_dev_ste(domain, dev, &ste, STRTAB_STE_1_S1DSS_BYPASS);
> return 0;
> @@ -3180,7 +3191,9 @@ static int arm_smmu_attach_dev_blocked(struct iommu_domain *domain,
> struct device *dev)
> {
> struct arm_smmu_ste ste;
> + struct arm_smmu_master *master = dev_iommu_priv_get(dev);
>
> + arm_smmu_master_clear_vmaster(master);
> arm_smmu_make_abort_ste(&ste);
> arm_smmu_attach_dev_ste(domain, dev, &ste,
> STRTAB_STE_1_S1DSS_TERMINATE);
>
Thanks,
Praan
On Mon, Feb 24, 2025 at 08:35:56PM +0000, Pranjal Shrivastava wrote:
> oN sAt, Feb 22, 2025 at 07:54:09AM -0800, Nicolin Chen wrote:
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> > index 5aa2e7af58b4..364d8469a480 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> > @@ -85,6 +85,59 @@ static void arm_smmu_make_nested_domain_ste(
> > }
> > }
> >
> > +int arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state,
> > + struct iommu_domain *domain)
> > +{
> > + struct arm_smmu_nested_domain *nested_domain;
> > + struct arm_smmu_vmaster *vmaster;
> > + unsigned long vsid;
> > + int ret;
> > +
> > + iommu_group_mutex_assert(state->master->dev);
> > +
> > + if (domain->type != IOMMU_DOMAIN_NESTED)
> > + return 0;
> > + nested_domain = to_smmu_nested_domain(domain);
> > +
> > + /* Skip invalid vSTE */
> > + if (!(nested_domain->ste[0] & cpu_to_le64(STRTAB_STE_0_V)))
> > + return 0;
> > +
> > + ret = iommufd_viommu_get_vdev_id(&nested_domain->vsmmu->core,
> > + state->master->dev, &vsid);
> > + if (ret)
> > + return ret;
> > +
> > + vmaster = kzalloc(sizeof(*vmaster), GFP_KERNEL);
> > + if (!vmaster)
> > + return -ENOMEM;
> > + vmaster->vsmmu = nested_domain->vsmmu;
> > + vmaster->vsid = vsid;
> > + state->vmaster = vmaster;
> > +
> > + return 0;
> > +}
> > +
> > +void arm_smmu_attach_commit_vmaster(struct arm_smmu_attach_state *state)
> > +{
> > + struct arm_smmu_master *master = state->master;
> > +
> > + mutex_lock(&master->smmu->streams_mutex);
> > + if (state->vmaster != master->vmaster) {
> > + kfree(master->vmaster);
> > + master->vmaster = state->vmaster;
> > + }
>
> Does this condition suggest that we might end up calling
> `arm_smmu_attach_prepare_vmaster()` multiple times before __actually__
> commiting to a vmaster?
No. prepare() and commit() are 1:1. How is it interpreted to have
"multiple times"?
> > + mutex_unlock(&master->smmu->streams_mutex);
> > +}
> > +
> > +void arm_smmu_master_clear_vmaster(struct arm_smmu_master *master)
> > +{
> > + mutex_lock(&master->smmu->streams_mutex);
> > + kfree(master->vmaster);
> > + master->vmaster = NULL;
> > + mutex_unlock(&master->smmu->streams_mutex);
> > +}
> > +
> > static int arm_smmu_attach_dev_nested(struct iommu_domain *domain,
> > struct device *dev)
> > {
> > 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 358072b4e293..9e50bcee69d1 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -2803,6 +2803,7 @@ int arm_smmu_attach_prepare(struct arm_smmu_attach_state *state,
> > struct arm_smmu_domain *smmu_domain =
> > to_smmu_domain_devices(new_domain);
> > unsigned long flags;
> > + int ret;
> >
> > /*
> > * arm_smmu_share_asid() must not see two domains pointing to the same
> > @@ -2832,9 +2833,15 @@ int arm_smmu_attach_prepare(struct arm_smmu_attach_state *state,
> > }
> >
> > if (smmu_domain) {
> > + ret = arm_smmu_attach_prepare_vmaster(state, new_domain);
>
> IMO, this adds a little confusion for folks not using iommufd.
>
> I guess it'd be cleaner if we invoke this below within the:
> `if (new_domain->type == IOMMU_DOMAIN_NESTED)` condition instead of
> simply returning from the function if the new_domain->type isn't NESTED.
But the arm_smmu_attach_commit_vmaster() still has to be
unconditional as !NESTED domain should clean the vamster away..
Nicolin
On Mon, Feb 24, 2025 at 01:31:11PM -0800, Nicolin Chen wrote:
> On Mon, Feb 24, 2025 at 08:35:56PM +0000, Pranjal Shrivastava wrote:
> > oN sAt, Feb 22, 2025 at 07:54:09AM -0800, Nicolin Chen wrote:
> > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> > > index 5aa2e7af58b4..364d8469a480 100644
> > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> > > @@ -85,6 +85,59 @@ static void arm_smmu_make_nested_domain_ste(
> > > }
> > > }
> > >
> > > +int arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state,
> > > + struct iommu_domain *domain)
> > > +{
> > > + struct arm_smmu_nested_domain *nested_domain;
> > > + struct arm_smmu_vmaster *vmaster;
> > > + unsigned long vsid;
> > > + int ret;
> > > +
> > > + iommu_group_mutex_assert(state->master->dev);
> > > +
> > > + if (domain->type != IOMMU_DOMAIN_NESTED)
> > > + return 0;
> > > + nested_domain = to_smmu_nested_domain(domain);
> > > +
> > > + /* Skip invalid vSTE */
> > > + if (!(nested_domain->ste[0] & cpu_to_le64(STRTAB_STE_0_V)))
> > > + return 0;
> > > +
> > > + ret = iommufd_viommu_get_vdev_id(&nested_domain->vsmmu->core,
> > > + state->master->dev, &vsid);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + vmaster = kzalloc(sizeof(*vmaster), GFP_KERNEL);
> > > + if (!vmaster)
> > > + return -ENOMEM;
> > > + vmaster->vsmmu = nested_domain->vsmmu;
> > > + vmaster->vsid = vsid;
> > > + state->vmaster = vmaster;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +void arm_smmu_attach_commit_vmaster(struct arm_smmu_attach_state *state)
> > > +{
> > > + struct arm_smmu_master *master = state->master;
> > > +
> > > + mutex_lock(&master->smmu->streams_mutex);
> > > + if (state->vmaster != master->vmaster) {
> > > + kfree(master->vmaster);
> > > + master->vmaster = state->vmaster;
> > > + }
> >
> > Does this condition suggest that we might end up calling
> > `arm_smmu_attach_prepare_vmaster()` multiple times before __actually__
> > commiting to a vmaster?
>
> No. prepare() and commit() are 1:1. How is it interpreted to have
> "multiple times"?
Ohh alright. I was just confused about why do we need to check:
`if (state->vmaster != master->vmaster)` ?
>
> > > + mutex_unlock(&master->smmu->streams_mutex);
> > > +}
> > > +
> > > +void arm_smmu_master_clear_vmaster(struct arm_smmu_master *master)
> > > +{
> > > + mutex_lock(&master->smmu->streams_mutex);
> > > + kfree(master->vmaster);
> > > + master->vmaster = NULL;
> > > + mutex_unlock(&master->smmu->streams_mutex);
> > > +}
> > > +
> > > static int arm_smmu_attach_dev_nested(struct iommu_domain *domain,
> > > struct device *dev)
> > > {
> > > 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 358072b4e293..9e50bcee69d1 100644
> > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > @@ -2803,6 +2803,7 @@ int arm_smmu_attach_prepare(struct arm_smmu_attach_state *state,
> > > struct arm_smmu_domain *smmu_domain =
> > > to_smmu_domain_devices(new_domain);
> > > unsigned long flags;
> > > + int ret;
> > >
> > > /*
> > > * arm_smmu_share_asid() must not see two domains pointing to the same
> > > @@ -2832,9 +2833,15 @@ int arm_smmu_attach_prepare(struct arm_smmu_attach_state *state,
> > > }
> > >
> > > if (smmu_domain) {
> > > + ret = arm_smmu_attach_prepare_vmaster(state, new_domain);
> >
> > IMO, this adds a little confusion for folks not using iommufd.
> >
> > I guess it'd be cleaner if we invoke this below within the:
> > `if (new_domain->type == IOMMU_DOMAIN_NESTED)` condition instead of
> > simply returning from the function if the new_domain->type isn't NESTED.
>
> But the arm_smmu_attach_commit_vmaster() still has to be
> unconditional as !NESTED domain should clean the vamster away..
>
Ack. Right.
Thanks,
Praan
On Mon, Feb 24, 2025 at 09:53:58PM +0000, Pranjal Shrivastava wrote:
> On Mon, Feb 24, 2025 at 01:31:11PM -0800, Nicolin Chen wrote:
> > On Mon, Feb 24, 2025 at 08:35:56PM +0000, Pranjal Shrivastava wrote:
> > > oN sAt, Feb 22, 2025 at 07:54:09AM -0800, Nicolin Chen wrote:
> > > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> > > > index 5aa2e7af58b4..364d8469a480 100644
> > > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> > > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> > > > @@ -85,6 +85,59 @@ static void arm_smmu_make_nested_domain_ste(
> > > > }
> > > > }
> > > >
> > > > +int arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state,
> > > > + struct iommu_domain *domain)
> > > > +{
> > > > + struct arm_smmu_nested_domain *nested_domain;
> > > > + struct arm_smmu_vmaster *vmaster;
> > > > + unsigned long vsid;
> > > > + int ret;
> > > > +
> > > > + iommu_group_mutex_assert(state->master->dev);
> > > > +
> > > > + if (domain->type != IOMMU_DOMAIN_NESTED)
> > > > + return 0;
> > > > + nested_domain = to_smmu_nested_domain(domain);
> > > > +
> > > > + /* Skip invalid vSTE */
> > > > + if (!(nested_domain->ste[0] & cpu_to_le64(STRTAB_STE_0_V)))
> > > > + return 0;
> > > > +
> > > > + ret = iommufd_viommu_get_vdev_id(&nested_domain->vsmmu->core,
> > > > + state->master->dev, &vsid);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + vmaster = kzalloc(sizeof(*vmaster), GFP_KERNEL);
> > > > + if (!vmaster)
> > > > + return -ENOMEM;
> > > > + vmaster->vsmmu = nested_domain->vsmmu;
> > > > + vmaster->vsid = vsid;
> > > > + state->vmaster = vmaster;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +void arm_smmu_attach_commit_vmaster(struct arm_smmu_attach_state *state)
> > > > +{
> > > > + struct arm_smmu_master *master = state->master;
> > > > +
> > > > + mutex_lock(&master->smmu->streams_mutex);
> > > > + if (state->vmaster != master->vmaster) {
> > > > + kfree(master->vmaster);
> > > > + master->vmaster = state->vmaster;
> > > > + }
> > >
> > > Does this condition suggest that we might end up calling
> > > `arm_smmu_attach_prepare_vmaster()` multiple times before __actually__
> > > commiting to a vmaster?
> >
> > No. prepare() and commit() are 1:1. How is it interpreted to have
> > "multiple times"?
>
> Ohh alright. I was just confused about why do we need to check:
> `if (state->vmaster != master->vmaster)` ?
Hmm, it's probably not necessary, since we always allocate a new
vmaster pointer to the "state" or set a NULL.
I will clean that up a bit.
Thanks
Nicolin
On Mon, Feb 24, 2025 at 02:24:55PM -0800, Nicolin Chen wrote:
> On Mon, Feb 24, 2025 at 09:53:58PM +0000, Pranjal Shrivastava wrote:
> > On Mon, Feb 24, 2025 at 01:31:11PM -0800, Nicolin Chen wrote:
> > > On Mon, Feb 24, 2025 at 08:35:56PM +0000, Pranjal Shrivastava wrote:
> > > > oN sAt, Feb 22, 2025 at 07:54:09AM -0800, Nicolin Chen wrote:
> > > > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> > > > > index 5aa2e7af58b4..364d8469a480 100644
> > > > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> > > > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> > > > > @@ -85,6 +85,59 @@ static void arm_smmu_make_nested_domain_ste(
> > > > > }
> > > > > }
> > > > >
> > > > > +int arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state,
> > > > > + struct iommu_domain *domain)
> > > > > +{
> > > > > + struct arm_smmu_nested_domain *nested_domain;
> > > > > + struct arm_smmu_vmaster *vmaster;
> > > > > + unsigned long vsid;
> > > > > + int ret;
> > > > > +
> > > > > + iommu_group_mutex_assert(state->master->dev);
> > > > > +
> > > > > + if (domain->type != IOMMU_DOMAIN_NESTED)
> > > > > + return 0;
> > > > > + nested_domain = to_smmu_nested_domain(domain);
> > > > > +
> > > > > + /* Skip invalid vSTE */
> > > > > + if (!(nested_domain->ste[0] & cpu_to_le64(STRTAB_STE_0_V)))
> > > > > + return 0;
> > > > > +
> > > > > + ret = iommufd_viommu_get_vdev_id(&nested_domain->vsmmu->core,
> > > > > + state->master->dev, &vsid);
> > > > > + if (ret)
> > > > > + return ret;
> > > > > +
> > > > > + vmaster = kzalloc(sizeof(*vmaster), GFP_KERNEL);
> > > > > + if (!vmaster)
> > > > > + return -ENOMEM;
> > > > > + vmaster->vsmmu = nested_domain->vsmmu;
> > > > > + vmaster->vsid = vsid;
> > > > > + state->vmaster = vmaster;
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +void arm_smmu_attach_commit_vmaster(struct arm_smmu_attach_state *state)
> > > > > +{
> > > > > + struct arm_smmu_master *master = state->master;
> > > > > +
> > > > > + mutex_lock(&master->smmu->streams_mutex);
> > > > > + if (state->vmaster != master->vmaster) {
> > > > > + kfree(master->vmaster);
> > > > > + master->vmaster = state->vmaster;
> > > > > + }
> > > >
> > > > Does this condition suggest that we might end up calling
> > > > `arm_smmu_attach_prepare_vmaster()` multiple times before __actually__
> > > > commiting to a vmaster?
> > >
> > > No. prepare() and commit() are 1:1. How is it interpreted to have
> > > "multiple times"?
> >
> > Ohh alright. I was just confused about why do we need to check:
> > `if (state->vmaster != master->vmaster)` ?
>
> Hmm, it's probably not necessary, since we always allocate a new
> vmaster pointer to the "state" or set a NULL.
>
> I will clean that up a bit.
I made the following change on top of this patch (will squash):
-------------------------------------------------------------
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
index 364d8469a480..2c1a51c360fe 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
@@ -95,8 +95,6 @@ int arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state,
iommu_group_mutex_assert(state->master->dev);
- if (domain->type != IOMMU_DOMAIN_NESTED)
- return 0;
nested_domain = to_smmu_nested_domain(domain);
/* Skip invalid vSTE */
@@ -122,19 +120,9 @@ void arm_smmu_attach_commit_vmaster(struct arm_smmu_attach_state *state)
{
struct arm_smmu_master *master = state->master;
- mutex_lock(&master->smmu->streams_mutex);
- if (state->vmaster != master->vmaster) {
- kfree(master->vmaster);
- master->vmaster = state->vmaster;
- }
- mutex_unlock(&master->smmu->streams_mutex);
-}
-
-void arm_smmu_master_clear_vmaster(struct arm_smmu_master *master)
-{
mutex_lock(&master->smmu->streams_mutex);
kfree(master->vmaster);
- master->vmaster = NULL;
+ master->vmaster = state->vmaster;
mutex_unlock(&master->smmu->streams_mutex);
}
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 9e50bcee69d1..b9d0cf571da0 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2833,9 +2833,11 @@ int arm_smmu_attach_prepare(struct arm_smmu_attach_state *state,
}
if (smmu_domain) {
- ret = arm_smmu_attach_prepare_vmaster(state, new_domain);
- if (ret)
- return ret;
+ if (new_domain->type == IOMMU_DOMAIN_NESTED) {
+ ret = arm_smmu_attach_prepare_vmaster(state, new_domain);
+ if (ret)
+ return ret;
+ }
master_domain = kzalloc(sizeof(*master_domain), GFP_KERNEL);
if (!master_domain) {
@@ -3171,8 +3173,9 @@ static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
{
struct arm_smmu_ste ste;
struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+ struct arm_smmu_attach_state state = { .master = master };
- arm_smmu_master_clear_vmaster(master);
+ arm_smmu_attach_commit_vmaster(&state);
arm_smmu_make_bypass_ste(master->smmu, &ste);
arm_smmu_attach_dev_ste(domain, dev, &ste, STRTAB_STE_1_S1DSS_BYPASS);
return 0;
@@ -3192,8 +3195,9 @@ static int arm_smmu_attach_dev_blocked(struct iommu_domain *domain,
{
struct arm_smmu_ste ste;
struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+ struct arm_smmu_attach_state state = { .master = master };
- arm_smmu_master_clear_vmaster(master);
+ arm_smmu_attach_commit_vmaster(&state);
arm_smmu_make_abort_ste(&ste);
arm_smmu_attach_dev_ste(domain, dev, &ste,
STRTAB_STE_1_S1DSS_TERMINATE);
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 85352504343b..eeec302f1b4b 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -1066,7 +1066,6 @@ struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev,
int arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state,
struct iommu_domain *domain);
void arm_smmu_attach_commit_vmaster(struct arm_smmu_attach_state *state);
-void arm_smmu_master_clear_vmaster(struct arm_smmu_master *master);
#else
#define arm_smmu_hw_info NULL
#define arm_vsmmu_alloc NULL
@@ -1082,9 +1081,6 @@ static inline void
arm_smmu_attach_commit_vmaster(struct arm_smmu_attach_state *state)
{
}
-static inline void arm_smmu_master_clear_vmaster(struct arm_smmu_master *master)
-{
-}
#endif /* CONFIG_ARM_SMMU_V3_IOMMUFD */
#endif /* _ARM_SMMU_V3_H */
-------------------------------------------------------------
Thanks
Nicolin
On Mon, Feb 24, 2025 at 03:45:33PM -0800, Nicolin Chen wrote:
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> @@ -95,8 +95,6 @@ int arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state,
>
> iommu_group_mutex_assert(state->master->dev);
>
> - if (domain->type != IOMMU_DOMAIN_NESTED)
> - return 0;
> nested_domain = to_smmu_nested_domain(domain);
>
> /* Skip invalid vSTE */
> @@ -122,19 +120,9 @@ void arm_smmu_attach_commit_vmaster(struct arm_smmu_attach_state *state)
> {
> struct arm_smmu_master *master = state->master;
>
> - mutex_lock(&master->smmu->streams_mutex);
> - if (state->vmaster != master->vmaster) {
> - kfree(master->vmaster);
> - master->vmaster = state->vmaster;
> - }
> - mutex_unlock(&master->smmu->streams_mutex);
> -}
> -
> -void arm_smmu_master_clear_vmaster(struct arm_smmu_master *master)
> -{
> mutex_lock(&master->smmu->streams_mutex);
> kfree(master->vmaster);
> - master->vmaster = NULL;
> + master->vmaster = state->vmaster;
> mutex_unlock(&master->smmu->streams_mutex);
> }
I'd leave the clear_vmaster just for clarity. Commit should not be
unpaired with prepare in the other functions.
It looks fine with this on top too
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Jason
On Tue, Feb 25, 2025 at 12:02:25PM -0400, Jason Gunthorpe wrote:
> On Mon, Feb 24, 2025 at 03:45:33PM -0800, Nicolin Chen wrote:
>
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> > @@ -95,8 +95,6 @@ int arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state,
> >
> > iommu_group_mutex_assert(state->master->dev);
> >
> > - if (domain->type != IOMMU_DOMAIN_NESTED)
> > - return 0;
> > nested_domain = to_smmu_nested_domain(domain);
> >
> > /* Skip invalid vSTE */
> > @@ -122,19 +120,9 @@ void arm_smmu_attach_commit_vmaster(struct arm_smmu_attach_state *state)
> > {
> > struct arm_smmu_master *master = state->master;
> >
> > - mutex_lock(&master->smmu->streams_mutex);
> > - if (state->vmaster != master->vmaster) {
> > - kfree(master->vmaster);
> > - master->vmaster = state->vmaster;
> > - }
> > - mutex_unlock(&master->smmu->streams_mutex);
> > -}
> > -
> > -void arm_smmu_master_clear_vmaster(struct arm_smmu_master *master)
> > -{
> > mutex_lock(&master->smmu->streams_mutex);
> > kfree(master->vmaster);
> > - master->vmaster = NULL;
> > + master->vmaster = state->vmaster;
> > mutex_unlock(&master->smmu->streams_mutex);
> > }
>
> I'd leave the clear_vmaster just for clarity. Commit should not be
> unpaired with prepare in the other functions.
>
Ack. I'd like to pair prepare and commit as well. I'm just confused
about the check if (state->vmaster != master->vmaster). Maybe a helpful
comment about what are we checking for would make things cleaner.
With this nit:
Reviewed-by: Pranjal Shrivastavat <praan@google.com>
Thanks,
Praan
On Tue, Feb 25, 2025 at 12:02:25PM -0400, Jason Gunthorpe wrote:
> On Mon, Feb 24, 2025 at 03:45:33PM -0800, Nicolin Chen wrote:
>
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> > @@ -95,8 +95,6 @@ int arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state,
> >
> > iommu_group_mutex_assert(state->master->dev);
> >
> > - if (domain->type != IOMMU_DOMAIN_NESTED)
> > - return 0;
> > nested_domain = to_smmu_nested_domain(domain);
> >
> > /* Skip invalid vSTE */
> > @@ -122,19 +120,9 @@ void arm_smmu_attach_commit_vmaster(struct arm_smmu_attach_state *state)
> > {
> > struct arm_smmu_master *master = state->master;
> >
> > - mutex_lock(&master->smmu->streams_mutex);
> > - if (state->vmaster != master->vmaster) {
> > - kfree(master->vmaster);
> > - master->vmaster = state->vmaster;
> > - }
> > - mutex_unlock(&master->smmu->streams_mutex);
> > -}
> > -
> > -void arm_smmu_master_clear_vmaster(struct arm_smmu_master *master)
> > -{
> > mutex_lock(&master->smmu->streams_mutex);
> > kfree(master->vmaster);
> > - master->vmaster = NULL;
> > + master->vmaster = state->vmaster;
> > mutex_unlock(&master->smmu->streams_mutex);
> > }
>
> I'd leave the clear_vmaster just for clarity. Commit should not be
> unpaired with prepare in the other functions.
>
> It looks fine with this on top too
>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Ack. I added it back and a #ifdef to the vmaster:
+void arm_smmu_master_clear_vmaster(struct arm_smmu_master *master)
+{
+ struct arm_smmu_attach_state state = { .master = master };
+
+ arm_smmu_attach_commit_vmaster(&state);
+}
[...]
@@ -824,6 +829,9 @@ struct arm_smmu_master {
struct arm_smmu_device *smmu;
struct device *dev;
struct arm_smmu_stream *streams;
+#ifdef CONFIG_ARM_SMMU_V3_IOMMUFD
+ struct arm_smmu_vmaster *vmaster; /* use smmu->streams_mutex */
+#endif
/* Locked by the iommu core using the group mutex */
struct arm_smmu_ctx_desc_cfg cd_table;
unsigned int num_streams;
@@ -972,6 +980,9 @@ struct arm_smmu_attach_state {
bool disable_ats;
ioasid_t ssid;
/* Resulting state */
+#ifdef CONFIG_ARM_SMMU_V3_IOMMUFD
+ struct arm_smmu_vmaster *vmaster;
+#endif
bool ats_enabled;
};
Thanks
Nicolin
On Tue, Feb 25, 2025 at 08:41:27AM -0800, Nicolin Chen wrote:
> On Tue, Feb 25, 2025 at 12:02:25PM -0400, Jason Gunthorpe wrote:
> > On Mon, Feb 24, 2025 at 03:45:33PM -0800, Nicolin Chen wrote:
> >
> > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> > > @@ -95,8 +95,6 @@ int arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state,
> > >
> > > iommu_group_mutex_assert(state->master->dev);
> > >
> > > - if (domain->type != IOMMU_DOMAIN_NESTED)
> > > - return 0;
> > > nested_domain = to_smmu_nested_domain(domain);
> > >
> > > /* Skip invalid vSTE */
> > > @@ -122,19 +120,9 @@ void arm_smmu_attach_commit_vmaster(struct arm_smmu_attach_state *state)
> > > {
> > > struct arm_smmu_master *master = state->master;
> > >
> > > - mutex_lock(&master->smmu->streams_mutex);
> > > - if (state->vmaster != master->vmaster) {
> > > - kfree(master->vmaster);
> > > - master->vmaster = state->vmaster;
> > > - }
> > > - mutex_unlock(&master->smmu->streams_mutex);
> > > -}
> > > -
> > > -void arm_smmu_master_clear_vmaster(struct arm_smmu_master *master)
> > > -{
> > > mutex_lock(&master->smmu->streams_mutex);
> > > kfree(master->vmaster);
> > > - master->vmaster = NULL;
> > > + master->vmaster = state->vmaster;
> > > mutex_unlock(&master->smmu->streams_mutex);
> > > }
> >
> > I'd leave the clear_vmaster just for clarity. Commit should not be
> > unpaired with prepare in the other functions.
> >
> > It looks fine with this on top too
> >
> > Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
>
> Ack. I added it back and a #ifdef to the vmaster:
>
> +void arm_smmu_master_clear_vmaster(struct arm_smmu_master *master)
> +{
> + struct arm_smmu_attach_state state = { .master = master };
> +
> + arm_smmu_attach_commit_vmaster(&state);
> +}
> [...]
> @@ -824,6 +829,9 @@ struct arm_smmu_master {
> struct arm_smmu_device *smmu;
> struct device *dev;
> struct arm_smmu_stream *streams;
> +#ifdef CONFIG_ARM_SMMU_V3_IOMMUFD
> + struct arm_smmu_vmaster *vmaster; /* use smmu->streams_mutex */
> +#endif
> /* Locked by the iommu core using the group mutex */
> struct arm_smmu_ctx_desc_cfg cd_table;
> unsigned int num_streams;
> @@ -972,6 +980,9 @@ struct arm_smmu_attach_state {
> bool disable_ats;
> ioasid_t ssid;
> /* Resulting state */
> +#ifdef CONFIG_ARM_SMMU_V3_IOMMUFD
> + struct arm_smmu_vmaster *vmaster;
> +#endif
> bool ats_enabled;
> };
>
Umm.. I'm not too sure how I feel about these #ifdefs _between_ a struct
definition. Given that currently, the arm_smmu_v3.h file doesn't have
such `#ifdef CONFIG`s between structs. I'd say, in case
CONFIG_ARM_SMMU_V3_IOMMUFD is turned off, we can simply leave the
vmaster ptr NULL?
-Praan
On Tue, Feb 25, 2025 at 05:08:16PM +0000, Pranjal Shrivastava wrote:
> > @@ -824,6 +829,9 @@ struct arm_smmu_master {
> > struct arm_smmu_device *smmu;
> > struct device *dev;
> > struct arm_smmu_stream *streams;
> > +#ifdef CONFIG_ARM_SMMU_V3_IOMMUFD
> > + struct arm_smmu_vmaster *vmaster; /* use smmu->streams_mutex */
> > +#endif
> > /* Locked by the iommu core using the group mutex */
> > struct arm_smmu_ctx_desc_cfg cd_table;
> > unsigned int num_streams;
> > @@ -972,6 +980,9 @@ struct arm_smmu_attach_state {
> > bool disable_ats;
> > ioasid_t ssid;
> > /* Resulting state */
> > +#ifdef CONFIG_ARM_SMMU_V3_IOMMUFD
> > + struct arm_smmu_vmaster *vmaster;
> > +#endif
> > bool ats_enabled;
> > };
> >
>
> Umm.. I'm not too sure how I feel about these #ifdefs _between_ a struct
> definition. Given that currently, the arm_smmu_v3.h file doesn't have
> such `#ifdef CONFIG`s between structs. I'd say, in case
> CONFIG_ARM_SMMU_V3_IOMMUFD is turned off, we can simply leave the
> vmaster ptr NULL?
OK. Will drop..
Nicolin
© 2016 - 2026 Red Hat, Inc.