Re: [PATCH v7 01/10] iommu: Add pasids field in struct iommu_device

[Baolu Lu]

Hi Kevin,

Thank you for reviewing my patches.

On 2022/5/24 17:24, Tian, Kevin wrote:
> > From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> > Sent: Thursday, May 19, 2022 3:21 PM
> >
> > Use this field to keep the number of supported PASIDs that an IOMMU
> > hardware is able to support. This is a generic attribute of an IOMMU
> > and lifting it into the per-IOMMU device structure makes it possible
> > to allocate a PASID for device without calls into the IOMMU drivers.
> > Any iommu driver which suports PASID related features should set this
> > field before enabling them on the devices.
> >
> > Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> > Reviewed-by: Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx>
> > ---
> >   include/linux/iommu.h                       | 2 ++
> >   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 1 +
> >   drivers/iommu/intel/dmar.c                  | 4 ++++
> >   3 files changed, 7 insertions(+)
> >
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index 5e1afe169549..da423e87f248 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -318,12 +318,14 @@ struct iommu_domain_ops {
> >    * @list: Used by the iommu-core to keep a list of registered iommus
> >    * @ops: iommu-ops for talking to this iommu
> >    * @dev: struct device for sysfs handling
> > + * @pasids: number of supported PASIDs
> >    */
> >   struct iommu_device {
> >   	struct list_head list;
> >   	const struct iommu_ops *ops;
> >   	struct fwnode_handle *fwnode;
> >   	struct device *dev;
> > +	u32 pasids;
>
> max_pasid or nr_pasids?

max_pasid looks better.

> >   };
> >
> >   /**
> > 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 88817a3376ef..6e2cd082c670 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -3546,6 +3546,7 @@ static int arm_smmu_device_hw_probe(struct
> > arm_smmu_device *smmu)
> >   	/* SID/SSID sizes */
> >   	smmu->ssid_bits = FIELD_GET(IDR1_SSIDSIZE, reg);
> >   	smmu->sid_bits = FIELD_GET(IDR1_SIDSIZE, reg);
> > +	smmu->iommu.pasids = smmu->ssid_bits;
> >
> >   	/*
> >   	 * If the SMMU supports fewer bits than would fill a single L2 stream
> > diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
> > index 4de960834a1b..1c3cf267934d 100644
> > --- a/drivers/iommu/intel/dmar.c
> > +++ b/drivers/iommu/intel/dmar.c
> > @@ -1126,6 +1126,10 @@ static int alloc_iommu(struct dmar_drhd_unit
> > *drhd)
> >
> >   	raw_spin_lock_init(&iommu->register_lock);
> >
> > +	/* Supports full 20-bit PASID in scalable mode. */
> > +	if (ecap_pasid(iommu->ecap))
> > +		iommu->iommu.pasids = 1UL << 20;
> > +
>
> supported pasid bits is reported by ecap_pss(). I don't think we should
> assume 20bits here.

Yes. I overlooked this. Thanks for reminding.

Another thing I need to improve is that scalable mode could be disabled.
This field should be 0 in that case.

> >   	/*
> >   	 * This is only for hotplug; at boot time intel_iommu_enabled won't
> >   	 * be set yet. When intel_iommu_init() runs, it registers the units
> > --
> > 2.25.1

Best regards,
baolu