Eric,
> From: Auger Eric <eric.auger@xxxxxxxxxx>
> Sent: Monday, July 20, 2020 8:38 PM
>
> Yi,
>
> On 7/20/20 12:18 PM, Liu, Yi L wrote:
> > Hi Eric,
> >
> >> From: Auger Eric <eric.auger@xxxxxxxxxx>
> >> Sent: Monday, July 20, 2020 12:06 AM
> >>
> >> Hi Yi,
> >>
> >> On 7/12/20 1:21 PM, Liu Yi L wrote:
> >>> When an IOMMU domain with nesting attribute is used for guest SVA, a
> >>> system-wide PASID is allocated for binding with the device and the domain.
> >>> For security reason, we need to check the PASID passsed from user-space.
> >> passed
> >
> > got it.
> >
> >>> e.g. page table bind/unbind and PASID related cache invalidation.
> >>>
> >>> Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
> >>> CC: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
> >>> Cc: Alex Williamson <alex.williamson@xxxxxxxxxx>
> >>> Cc: Eric Auger <eric.auger@xxxxxxxxxx>
> >>> Cc: Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx>
> >>> Cc: Joerg Roedel <joro@xxxxxxxxxx>
> >>> Cc: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> >>> Signed-off-by: Liu Yi L <yi.l.liu@xxxxxxxxx>
> >>> Signed-off-by: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
> >>> ---
> >>> drivers/iommu/intel/iommu.c | 10 ++++++++++
> >>> drivers/iommu/intel/svm.c | 7 +++++--
> >>> 2 files changed, 15 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> >>> index 4d54198..a9504cb 100644
> >>> --- a/drivers/iommu/intel/iommu.c
> >>> +++ b/drivers/iommu/intel/iommu.c
> >>> @@ -5436,6 +5436,7 @@ intel_iommu_sva_invalidate(struct iommu_domain
> >> *domain, struct device *dev,
> >>> int granu = 0;
> >>> u64 pasid = 0;
> >>> u64 addr = 0;
> >>> + void *pdata;
> >>>
> >>> granu = to_vtd_granularity(cache_type, inv_info->granularity);
> >>> if (granu == -EINVAL) {
> >>> @@ -5456,6 +5457,15 @@ intel_iommu_sva_invalidate(struct iommu_domain
> >> *domain, struct device *dev,
> >>> (inv_info->granu.addr_info.flags &
> >> IOMMU_INV_ADDR_FLAGS_PASID))
> >>> pasid = inv_info->granu.addr_info.pasid;
> >>>
> >>> + pdata = ioasid_find(dmar_domain->ioasid_sid, pasid, NULL);
> >>> + if (!pdata) {
> >>> + ret = -EINVAL;
> >>> + goto out_unlock;
> >>> + } else if (IS_ERR(pdata)) {
> >>> + ret = PTR_ERR(pdata);
> >>> + goto out_unlock;
> >>> + }
> >>> +
> >>> switch (BIT(cache_type)) {
> >>> case IOMMU_CACHE_INV_TYPE_IOTLB:
> >>> /* HW will ignore LSB bits based on address mask */
> >>> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> >>> index d2c0e1a..212dee0 100644
> >>> --- a/drivers/iommu/intel/svm.c
> >>> +++ b/drivers/iommu/intel/svm.c
> >>> @@ -319,7 +319,7 @@ int intel_svm_bind_gpasid(struct iommu_domain
> *domain,
> >> struct device *dev,
> >>> dmar_domain = to_dmar_domain(domain);
> >>>
> >>> mutex_lock(&pasid_mutex);
> >>> - svm = ioasid_find(INVALID_IOASID_SET, data->hpasid, NULL);
> I meant while using INVALID_IOASID_SET instead of the actual
> dmar_domain->ioasid_sid. But I think I've now recovered, the asset is
> simply not used ;-)
oh, I think should be using dmar_domain->ioasid_sid from the beginning.
does it look good so far? :-)
Regards,
Yi Liu
> >> I do not get what the call was supposed to do before that patch?
> >
> > you mean patch 10/15 by "that patch", right? the ownership check should
> > be done as to prevent illegal bind request from userspace. before patch
> > 10/15, it should be added.
> >
> >>> + svm = ioasid_find(dmar_domain->ioasid_sid, data->hpasid, NULL);
> >>> if (IS_ERR(svm)) {
> >>> ret = PTR_ERR(svm);
> >>> goto out;
> >>> @@ -436,6 +436,7 @@ int intel_svm_unbind_gpasid(struct iommu_domain
> >> *domain,
> >>> struct device *dev, ioasid_t pasid)
> >>> {
> >>> struct intel_iommu *iommu = intel_svm_device_to_iommu(dev);
> >>> + struct dmar_domain *dmar_domain;
> >>> struct intel_svm_dev *sdev;
> >>> struct intel_svm *svm;
> >>> int ret = -EINVAL;
> >>> @@ -443,8 +444,10 @@ int intel_svm_unbind_gpasid(struct iommu_domain
> >> *domain,
> >>> if (WARN_ON(!iommu))
> >>> return -EINVAL;
> >>>
> >>> + dmar_domain = to_dmar_domain(domain);
> >>> +
> >>> mutex_lock(&pasid_mutex);
> >>> - svm = ioasid_find(INVALID_IOASID_SET, pasid, NULL);
> >>> + svm = ioasid_find(dmar_domain->ioasid_sid, pasid, NULL);
> >> just to make sure, about the locking, can't domain->ioasid_sid change
> >> under the hood?
> >
> > I guess not. intel_svm_unbind_gpasid() and iommu_domain_set_attr()
> > is called by vfio today, and within vfio, there is vfio_iommu->lock.
> OK
>
> Thanks
>
> Eric
> >
> > Regards,
> > Yi Liu
> >
> >>
> >> Thanks
> >>
> >> Eric
> >>> if (!svm) {
> >>> ret = -EINVAL;
> >>> goto out;
> >>>
> >
