linux-kernel - Re: [PATCH v7 12/14] iommu/arm-smmu-v3: Introduce struct arm_smmu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z7zlH74/orq9HF7Q@Asurada-Nvidia>
Date: Mon, 24 Feb 2025 13:31:11 -0800
From: Nicolin Chen <nicolinc@...dia.com>
To: Pranjal Shrivastava <praan@...gle.com>
CC: <jgg@...dia.com>, <kevin.tian@...el.com>, <corbet@....net>,
	<will@...nel.org>, <joro@...tes.org>, <suravee.suthikulpanit@....com>,
	<robin.murphy@....com>, <dwmw2@...radead.org>, <baolu.lu@...ux.intel.com>,
	<shuah@...nel.org>, <linux-kernel@...r.kernel.org>, <iommu@...ts.linux.dev>,
	<linux-arm-kernel@...ts.infradead.org>, <linux-kselftest@...r.kernel.org>,
	<linux-doc@...r.kernel.org>, <eric.auger@...hat.com>,
	<jean-philippe@...aro.org>, <mdf@...nel.org>, <mshavit@...gle.com>,
	<shameerali.kolothum.thodi@...wei.com>, <smostafa@...gle.com>,
	<ddutile@...hat.com>, <yi.l.liu@...el.com>, <patches@...ts.linux.dev>
Subject: Re: [PATCH v7 12/14] iommu/arm-smmu-v3: Introduce struct
 arm_smmu_vmaster

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