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

[Robin Murphy]

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@xxxxxxxxxx>
> ---
>   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?

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. */