Re: [PATCH v1 14/14] iommu/arm-smmu-v3: Add arm_smmu_cache_invalidate_user
From: Robin Murphy
Date: Fri Mar 10 2023 - 12:54:14 EST
On 2023-03-10 03:51, Nicolin Chen wrote:
On Thu, Mar 09, 2023 at 02:49:14PM +0000, Robin Murphy wrote:
External email: Use caution opening links or attachments
On 2023-03-09 10:53, Nicolin Chen wrote:
Add arm_smmu_cache_invalidate_user() function for user space to invalidate
TLB entries and Context Descriptors, since either an IO page table entrie
or a Context Descriptor in the user space is still cached by the hardware.
The input user_data is defined in "struct iommu_hwpt_invalidate_arm_smmuv3"
that contains the essential data for corresponding invalidation commands.
Co-developed-by: Eric Auger <eric.auger@xxxxxxxxxx>
Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
Signed-off-by: Nicolin Chen <nicolinc@xxxxxxxxxx>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 56 +++++++++++++++++++++
1 file changed, 56 insertions(+)
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 ac63185ae268..7d73eab5e7f4 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2880,9 +2880,65 @@ static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
arm_smmu_sva_remove_dev_pasid(domain, dev, pasid);
}
+static void arm_smmu_cache_invalidate_user(struct iommu_domain *domain,
+ void *user_data)
+{
+ struct iommu_hwpt_invalidate_arm_smmuv3 *inv_info = user_data;
+ struct arm_smmu_cmdq_ent cmd = { .opcode = inv_info->opcode };
+ struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+ struct arm_smmu_device *smmu = smmu_domain->smmu;
+ size_t granule_size = inv_info->granule_size;
+ unsigned long iova = 0;
+ size_t size = 0;
+ int ssid = 0;
+
+ if (!smmu || !smmu_domain->s2 || domain->type != IOMMU_DOMAIN_NESTED)
+ return;
+
+ switch (inv_info->opcode) {
+ case CMDQ_OP_CFGI_CD:
+ case CMDQ_OP_CFGI_CD_ALL:
+ return arm_smmu_sync_cd(smmu_domain, inv_info->ssid, true);
Since we let the guest choose its own S1Fmt (and S1CDMax, yet not
S1DSS?), how can we assume leaf = true here?
The s1dss is forwarded in the user_data structure too. So, the
driver should have set that too down to a nested STE. Will add
this missing pathway.
And you are right that the guest OS can use a 2-level table, so
we should set leaf = false to cover all cases, I think.
+ case CMDQ_OP_TLBI_NH_VA:
+ cmd.tlbi.asid = inv_info->asid;
+ fallthrough;
+ case CMDQ_OP_TLBI_NH_VAA:
+ if (!granule_size || !(granule_size & smmu->pgsize_bitmap) ||
Non-range invalidations with TG=0 are perfectly legal, and should not be
ignored.
I assume that you are talking about the pgsize_bitmap check.
QEMU embeds a !tg case into the granule_size [1]. So it might
not be straightforward to cover that case. Let me see how to
untangle different cases and handle them accordingly.
Oh, double-checking patch #2, that might be me misunderstanding the
interface. I hadn't realised that the UAPI was apparently modelled on
arm_smmu_tlb_inv_range_asid() rather than actual SMMU commands :)
I really think UAPI should reflect the hardware and encode TG and TTL
directly. Especially since there's technically a flaw in the current
driver where we assume TTL in cases where it isn't actually known, thus
may potentially fail to invalidate level 2 block entries when removing a
level 1 table, since io-pgtable passes the level 3 granule in that case.
When range invalidation came along, the distinction between "all leaves
are definitely at the last level" and "use last-level granularity to
make sure everything at at any level is hit" started to matter, but the
interface never caught up. It hasn't seemed desperately urgent to fix
(who does 1GB+ unmaps outside of VFIO teardown anyway?), but we must
definitely not bake the same mistake into user ABI.
Of course, there might then be cases where we need to transform
non-range commands into range commands for the sake of workarounds, but
that's our own problem to deal with.
[1] https://patchew.org/QEMU/20200824094811.15439-1-peter.maydell@xxxxxxxxxx/20200824094811.15439-9-peter.maydell@xxxxxxxxxx/
+ granule_size & ~(1ULL << __ffs(granule_size)))
If that's intended to mean is_power_of_2(), please just use is_power_of_2().
+ return;
+
+ iova = inv_info->range.start;
+ size = inv_info->range.last - inv_info->range.start + 1;
If the design here is that user_data is so deeply driver-specific and
special to the point that it can't possibly be passed as a type-checked
union of the known and publicly-visible UAPI types that it is, wouldn't
it make sense to just encode the whole thing in the expected format and
not have to make these kinds of niggling little conversions at both ends?
Hmm, that makes sense to me.
I just tracked back the history of Eric's previous work. There
was a mismatch between guest and host that RIL isn't supported
by the hardware. Now, guest can have whatever information it'd
need from the host to send supported instructions.
+ if (!size)
+ return;
+
+ cmd.tlbi.vmid = smmu_domain->s2->s2_cfg.vmid;
+ cmd.tlbi.leaf = inv_info->flags & IOMMU_SMMUV3_CMDQ_TLBI_VA_LEAF;
+ __arm_smmu_tlb_inv_range(&cmd, iova, size, granule_size, smmu_domain);
+ break;
+ case CMDQ_OP_TLBI_NH_ASID:
+ cmd.tlbi.asid = inv_info->asid;
+ fallthrough;
+ case CMDQ_OP_TLBI_NH_ALL:
+ cmd.tlbi.vmid = smmu_domain->s2->s2_cfg.vmid;
+ arm_smmu_cmdq_issue_cmd_with_sync(smmu, &cmd);
+ break;
+ case CMDQ_OP_ATC_INV:
+ ssid = inv_info->ssid;
+ iova = inv_info->range.start;
+ size = inv_info->range.last - inv_info->range.start + 1;
+ break;
Can we do any better than multiplying every single ATC_INV command, even
for random bogus StreamIDs, into multiple commands across every physical
device? In fact, I'm not entirely confident this isn't problematic, if
the guest wishes to send invalidations for one device specifically while
it's put some other device into a state where sending it a command would
do something bad. At the very least, it's liable to be confusing if the
guest sends a command for one StreamID but gets an error back for a
different one.
We'd need here an sid translation from the guest value to the
host value to specify a device, so as not to multiply the cmd
with the device list, if I understand it correctly?
I guess it depends on whether IOMMUFD is aware of the vSID->device
relationships that the VMM is using. If so, then it should be OK for the
VMM to pass through the vSID directly, and we can translate and
sanity-check it internally. Otherwise, the interface might have to
require the VMM to translate vSID->RID and pass the corresponding host
RID, which we can then map back to a SID (userspace cannot do the full
vSID->SID by itself, and even if it could that would probably be more
awkward to validate).
And if we expect ATS, what about PRI? Per patch #4 you're currently
offering that to the guest as well.
Oh, I should have probably blocked PRI. The PRI and the fault
injection will be followed after the basic 2-stage translation
patches. And I don't have a supporting hardware to test PRI.
+ default:
+ return;
What about NSNH_ALL? That still needs to invalidate all the S1 context
that the guest *thinks* it's invalidating.
NSNH_ALL is translated to NH_ALL at the guest level. But maybe
it should have been done here instead.
Yes. It seems the worst of both worlds to have an interface which takes
raw opcodes rather than an enum of supported commands, but still
requires userspace to know which opcodes are supported and which ones
don't work as expected even though they are entirely reasonable to use
in the context of the stage-1-only SMMU being emulated.
Thanks,
Robin.