On Tue, Jan 19, 2021 at 05:28:21PM +0000, Robin Murphy wrote:
> On 2021-01-08 14:52, Jean-Philippe Brucker wrote:
> > +#define EVTQ_1_PRIV (1UL << 33)
> > +#define EVTQ_1_EXEC (1UL << 34)
> > +#define EVTQ_1_READ (1UL << 35)
>
> Nit: personally I'd find it a little clearer if these were named PnU, InD,
> and RnW to match the architecture, but quite possibly that's just me and
> those are gibberish to everyone else...
No problem, I think it's still decipherable without a spec
> > +bool arm_smmu_master_iopf_enabled(struct arm_smmu_master *master)
> > +{
> > + bool enabled;
> > +
> > + mutex_lock(&sva_lock);
> > + enabled = master->iopf_enabled;
> > + mutex_unlock(&sva_lock);
>
> Forgive me for being dim, but what's the locking synchronising against here?
> If we're expecting that master->iopf_enabled can change at any time, isn't
> whatever we've read potentially already invalid as soon as we've dropped the
> lock?
Right, no reason to lock this. I doubt the lock in sva_enabled() is
necessary either, I could remove it in a separate patch.
> > +static int arm_smmu_page_response(struct device *dev,
> > + struct iommu_fault_event *unused,
> > + struct iommu_page_response *resp)
> > +{
> > + struct arm_smmu_cmdq_ent cmd = {0};
> > + struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> > + int sid = master->streams[0].id;
>
> If that's going to be the case, should we explicitly prevent multi-stream
> devices from opting in to faults at all?
Sure I'll add a check in iopf_supported(). Dealing with multi-stream
devices should be easy enough (record the incoming SID into
iommu_fault_event and fetch it back here), it just didn't seem necessary
for the moment.
> > + if (evt[1] & EVTQ_1_STALL) {
> > + flt->type = IOMMU_FAULT_PAGE_REQ;
> > + flt->prm = (struct iommu_fault_page_request) {
> > + .flags = IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE,
> > + .grpid = FIELD_GET(EVTQ_1_STAG, evt[1]),
> > + .perm = perm,
> > + .addr = FIELD_GET(EVTQ_2_ADDR, evt[2]),
> > + };
> > +
> > + if (ssid_valid) {
> > + flt->prm.flags |= IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;
> > + flt->prm.pasid = FIELD_GET(EVTQ_0_SSID, evt[0]);
> > + }
>
> So if we get a bad ATS request with R=1, or a TLB/CFG conflict or any other
> imp-def event which happens to have bit 95 set, we might try to report it as
> something pageable? I would have thought we should look at the event code
> before *anything* else.
Yes I definitely need to fix this
> > @@ -2250,6 +2383,12 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> > smmu_domain->s1_cfg.s1cdmax, master->ssid_bits);
> > ret = -EINVAL;
> > goto out_unlock;
> > + } else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 &&
> > + smmu_domain->stall_enabled != master->stall_enabled) {
>
> I appreciate that it's probably a fair bit more complex, but it would be
> nice to at least plan for resolving this decision later (i.e. at a point
> where a caller shows an interest in actually using stalls) in future.
> Obviously the first devices advertising stall capabilities will be the ones
> that do want to use it for their primary functionality, that are driving the
> work here. However once this all matures, firmwares may start annotating any
> stallable devices as such for completeness, rather than assuming any
> specific usage. At that point it would be a pain if, say, assigning two
> devices to the same VFIO domain for old-fashioned pinned DMA, was suddenly
> prevented for irrelevant reasons just because of a DT/IORT update.
It is more complex but possible. Device drivers signal their intent to use
stall by enabling IOMMU_DEV_FEAT_IOPF, so we can postpone setting CD.S
until then. We'll still need to make sure all devices attached to a domain
support it, and prevent attaching a device that can't handle stall to a
stall-enabled domain since it would inherit all CDs. Then there will be
drivers wanting to receive stall events for context #0 and handle them by
issuing iommu_map() calls (unpinned VFIO, mentioned by Baolu on patch
3). That requires setting and clearing CD.S live. So it is doable but I'd
rather leave it for later.
> > @@ -2785,6 +2943,7 @@ static int arm_smmu_cmdq_init(struct arm_smmu_device *smmu)
> > static int arm_smmu_init_queues(struct arm_smmu_device *smmu)
> > {
> > int ret;
> > + bool sva = arm_smmu_sva_supported(smmu);
> > /* cmdq */
> > ret = arm_smmu_init_one_queue(smmu, &smmu->cmdq.q, ARM_SMMU_CMDQ_PROD,
> > @@ -2804,6 +2963,12 @@ static int arm_smmu_init_queues(struct arm_smmu_device *smmu)
> > if (ret)
> > return ret;
> > + if (sva && smmu->features & ARM_SMMU_FEAT_STALLS) {
>
> Surely you could just test for ARM_SMMU_FEAT_SVA by now rather than go
> through the whole of arm_smmu_sva_supported() again?
Oh right, that was dumb
Thanks for the review
Jean
