Re: [PATCH 2/2] iommu/arm-smmu-v3: Report stalled S2 events

[Mostafa Saleh <smostafa@google.com>]

Hi Robin,

On Tue, Aug 13, 2024 at 12:57:02PM +0100, Robin Murphy wrote:
> On 12/08/2024 9:52 pm, Mostafa Saleh wrote:
> > Previously, S2 stall was disabled and in case there was an event it
> > wouldn't be reported on the assumption that it's always pinned  by VFIO.
> > 
> > However, now since we can enable stall, devices that use S2 outside
> > VFIO should be able to report the stalls similar to S1.
> > 
> > Also, to keep the old behaviour were S2 events from nested domains were
> > not reported as they are pinned (from VFIO) add a new flag to track this.
> > 
> > Signed-off-by: Mostafa Saleh <smostafa@google.com>
> > ---
> >   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 18 +++++++++++++-----
> >   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  2 ++
> >   2 files changed, 15 insertions(+), 5 deletions(-)
> > 
> > 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 8d573d9ca93c..ffa865529d73 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -1733,6 +1733,7 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
> >   	u32 sid = FIELD_GET(EVTQ_0_SID, evt[0]);
> >   	struct iopf_fault fault_evt = { };
> >   	struct iommu_fault *flt = &fault_evt.fault;
> > +	struct arm_smmu_domain *smmu_domain;
> >   	switch (FIELD_GET(EVTQ_0_ID, evt[0])) {
> >   	case EVT_ID_TRANSLATION_FAULT:
> > @@ -1744,10 +1745,6 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
> >   		return -EOPNOTSUPP;
> >   	}
> > -	/* Stage-2 is always pinned at the moment */
> > -	if (evt[1] & EVTQ_1_S2)
> > -		return -EFAULT;
> > -
> >   	if (!(evt[1] & EVTQ_1_STALL))
> >   		return -EOPNOTSUPP;
> > @@ -1782,6 +1779,15 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
> >   		goto out_unlock;
> >   	}
> > +	/* It is guaranteed that smmu_domain exists as EVTQ_1_STALL is checked. */
> > +	smmu_domain = to_smmu_domain(iommu_get_domain_for_dev(master->dev));
> > +
> > +	/* nesting domain is always pinned at the moment */
> > +	if (smmu_domain->enable_nesting) {
> 
> Ugh, has the whole enable_nesting method still not gone away already?

It should go away with Jason latest nesting patches:
https://lore.kernel.org/linux-iommu/0-v1-54e734311a7f+14f72-smmuv3_nesting_jgg@nvidia.com/T/#mc5558cb88b10fcdc4b91076cf813123cac75f11d

I can just ignore that here, and only remove the S2 check, and as
VFIO anyway doesn't register a handler it shouldn't really matter.
So, it's one line change, I can also squash with the previous patch,
please let me know what you think.

Thanks,
Mostafa

> 
> However, at least for now, isn't this functionally equivalent to just
> testing !(smmu->features & ARM_SMMU_FEAT_TRANS_S1) anyway? We still won't be
> able to differentiate a nominally-pinned non-nested VFIO domain from a
> nominally-stallable non-VFIO domain on S2-only hardware.
> 
> Thanks,
> Robin.
> 
> > +		ret = -EINVAL;
> > +		goto out_unlock;
> > +	}
> > +
> >   	iommu_report_device_fault(master->dev, &fault_evt);
> >   out_unlock:
> >   	mutex_unlock(&smmu->streams_mutex);
> > @@ -3373,8 +3379,10 @@ static int arm_smmu_enable_nesting(struct iommu_domain *domain)
> >   	mutex_lock(&smmu_domain->init_mutex);
> >   	if (smmu_domain->smmu)
> >   		ret = -EPERM;
> > -	else
> > +	else {
> >   		smmu_domain->stage = ARM_SMMU_DOMAIN_S2;
> > +		smmu_domain->enable_nesting = true;
> > +	}
> >   	mutex_unlock(&smmu_domain->init_mutex);
> >   	return ret;
> > 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 0dc7ad43c64c..f66efeec2cf8 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> > @@ -745,6 +745,8 @@ struct arm_smmu_domain {
> >   	spinlock_t			devices_lock;
> >   	struct mmu_notifier		mmu_notifier;
> > +
> > +	bool				enable_nesting;
> >   };
> >   /* The following are exposed for testing purposes. */