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
