[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Zrti-uek_5WCQ2J8@google.com>
Date: Tue, 13 Aug 2024 13:43:22 +0000
From: Mostafa Saleh <smostafa@...gle.com>
To: Robin Murphy <robin.murphy@....com>
Cc: linux-kernel@...r.kernel.org, iommu@...ts.linux.dev,
linux-arm-kernel@...ts.infradead.org, will@...nel.org,
joro@...tes.org, jgg@...pe.ca, nicolinc@...dia.com,
mshavit@...gle.com
Subject: Re: [PATCH 2/2] iommu/arm-smmu-v3: Report stalled S2 events
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@...gle.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. */
Powered by blists - more mailing lists