On Tue, Jul 01, 2025 at 04:02:35PM +0000, Pranjal Shrivastava wrote:
On Thu, Jun 26, 2025 at 12:34:58PM -0700, Nicolin Chen wrote:
/**
- enum iommu_hw_info_type - IOMMU Hardware Info Types
- @IOMMU_HW_INFO_TYPE_NONE: Output by the drivers that do not report hardware
@@ -598,12 +619,15 @@ struct iommu_hw_info_arm_smmuv3 {
- @IOMMU_HW_INFO_TYPE_DEFAULT: Input to request for a default type
- @IOMMU_HW_INFO_TYPE_INTEL_VTD: Intel VT-d iommu info type
- @IOMMU_HW_INFO_TYPE_ARM_SMMUV3: ARM SMMUv3 iommu info type
- @IOMMU_HW_INFO_TYPE_TEGRA241_CMDQV: NVIDIA Tegra241 CMDQV (extension for ARM
SMMUv3) info typeI know that the goal here is to mention that Tegra241 CMDQV is an extension for Arm SMMUv3, but this comment could be misunderstood as the "type" being an extension to IOMMU_HW_INFO_TYPE_ARM_SMMUV3. How about we
IOMMU_HW_INFO_TYPE_TEGRA241_CMDQV only reports CMDQV structure. VMM still needs to poll the IOMMU_HW_INFO_TYPE_ARM_SMMUV3. It's basically working as "type being an extension".
Sorry to be nit-picky here, I know that the code is clear, but I've seen people don't care to read more than the uapi descriptions. Maybe we could re-write this comment, here and everywhere else?
I can change this thought:
+ * @IOMMU_VIOMMU_TYPE_TEGRA241_CMDQV: NVIDIA Tegra241 CMDQV (extension for ARM + * SMMUv3) enabled ARM SMMUv3 type
+/**
- struct tegra241_vintf_sid - Virtual Interface Stream ID Replacement
- @core: Embedded iommufd_vdevice structure, holding virtual Stream ID
- @vintf: Parent VINTF pointer
- @sid: Physical Stream ID
- @idx: Replacement index in the VINTF
- */
+struct tegra241_vintf_sid {
- struct iommufd_vdevice core;
- struct tegra241_vintf *vintf;
- u32 sid;
- u8 idx;
};
AFAIU, This seems to be a handle for sid -> vintf mapping.. it yes, then I'm not sure if "Virtual Interface Stream ID Replacement" clarifies that?
No. It's for vSID to pSID mappings. I had it explained in commit log:
For ATC invalidation commands that hold an SID, it requires all devices to register their virtual SIDs to the SID_MATCH registers and their physical SIDs to the pairing SID_REPLACE registers, so that HW can use those as a lookup table to replace those virtual SIDs with the correct physical SIDs. Thus, implement the driver-allocated vDEVICE op with a tegra241_vintf_sid structure to allocate SID_REPLACE and to program the SIDs accordingly.
@@ -351,6 +394,29 @@ tegra241_cmdqv_get_cmdq(struct arm_smmu_device *smmu, /* HW Reset Functions */ +/*
- When a guest-owned VCMDQ is disabled, if the guest did not enqueue a CMD_SYNC
- following an ATC_INV command at the end of the guest queue while this ATC_INV
- is timed out, the TIMEOUT will not be reported until this VCMDQ gets assigned
- to the next VM, which will be a false alarm potentially causing some unwanted
- behavior in the new VM. Thus, a guest-owned VCMDQ must flush the TIMEOUT when
- it gets disabled. This can be done by just issuing a CMD_SYNC to SMMU CMDQ.
- */
+static void tegra241_vcmdq_hw_flush_timeout(struct tegra241_vcmdq *vcmdq) +{
- struct arm_smmu_device *smmu = &vcmdq->cmdqv->smmu;
- u64 cmd_sync[CMDQ_ENT_DWORDS] = {};
- cmd_sync[0] = FIELD_PREP(CMDQ_0_OP, CMDQ_OP_CMD_SYNC) |
FIELD_PREP(CMDQ_SYNC_0_CS, CMDQ_SYNC_0_CS_NONE);- /*
* It does not hurt to insert another CMD_SYNC, taking advantage of the* arm_smmu_cmdq_issue_cmdlist() that waits for the CMD_SYNC completion.*/- arm_smmu_cmdq_issue_cmdlist(smmu, &smmu->cmdq, cmd_sync, 1, true);
+}
If I'm getting this right, it issues a CMD_SYNC to the Host's CMDQ i.e. the non-CMDQV CMDQ, the main CMDQ of the SMMUv3? (i.e. the CMDQ present without the Tegra241 CMDQV extension?)
so.. basically on every VM switch, there would be an additional CMD_SYNC issued to the non-CMDQV CMDQ to flush the TIMEOUT and we'll poll for it's completion?
The main CMDQ exists regardless whether CMDQV extension is there or not. The CMD_SYNC can be issued to any (v)CMDQ. The smmu->cmdq is just the easiest one to use here.
@@ -380,6 +448,12 @@ static void tegra241_vcmdq_hw_deinit(struct tegra241_vcmdq *vcmdq) dev_dbg(vcmdq->cmdqv->dev, "%sdeinited\n", h); } +/* This function is for LVCMDQ, so @vcmdq must be mapped prior */ +static void _tegra241_vcmdq_hw_init(struct tegra241_vcmdq *vcmdq) +{
- writeq_relaxed(vcmdq->cmdq.q.q_base, REG_VCMDQ_PAGE1(vcmdq, BASE));
+}
Not sure why we broke this off to a function, will there be more stuff here or is this just to use it in tegra241_vcmdq_hw_init_user as well?
I can take it off.
@@ -429,6 +504,10 @@ static void tegra241_vintf_hw_deinit(struct tegra241_vintf *vintf) } } vintf_write_config(vintf, 0);
- for (sidx = 0; sidx < vintf->cmdqv->num_sids_per_vintf; sidx++) {
writel(0, REG_VINTF(vintf, SID_MATCH(sidx)));writel(0, REG_VINTF(vintf, SID_REPLACE(sidx)));- }
}
I'm assuming we call the de-init while switching VMs and hence we need to clear these to avoid spurious SID replacements in the new VM? Or do they not reset to 0 when the HW is reset?
The driver does not reset HW when tearing down a VM, but only sets VINTF's enable bit to 0. So, it should just set other configuration bits to 0 as well.
+static struct iommufd_viommu_ops tegra241_cmdqv_viommu_ops = {
- .destroy = tegra241_cmdqv_destroy_vintf_user,
- .alloc_domain_nested = arm_vsmmu_alloc_domain_nested,
- .cache_invalidate = arm_vsmmu_cache_invalidate,
I see that we currently use the main cmdq to issue these cache invalidations (there's a FIXME in arm_vsmmu_cache_invalidate). I was hoping for this series to change that but I'm assuming there's another series coming for that?
Meanwhile, I guess it'd be good to call that out for folks who have Grace and start trying out this feature.. I'm assuming they won't see as much perf improvement with this series alone since we're still using the main CMDQ in the upstream code?
VCMDQ only accelerates invalidation commands.
That is for non-invalidation commands that VCMDQ doesn't support, so they still have to go in the standard nesting pathway.
Let's add a line: /* for non-invalidation commands use */
Nicolin