Add Tegra241 (Grace) CMDQV Support (part 1/2)
Subsystems (4)
v11 cover Add Tegra241 (Grace) CMDQV Support (part 1/2)
Thus, there are two parts of patch series to add its support: the basic in-kernel support as part 1, and the user-space support as part 2.
The in-kernel support is to detect/configure the CMDQV hardware and then allocate a VINTF with some VCMDQs for the kernel/hypervisor to use. Like ECMDQ, CMDQV also allows the kernel to use multiple VCMDQs, giving some limited performance improvement: up to 20% reduction of TLB invalidation time was measured by a multi-threaded DMA unmap benchmark, compared to a single queue.
The user-space support is to provide uAPIs (via IOMMUFD) for hypervisors in user space to passthrough VCMDQs to VMs, allowing these VMs to access the VCMDQs directly without trappings, i.e. no VM Exits. This gives huge performance improvements: 70% to 90% reductions of TLB invalidation time were measured by various DMA unmap tests running in a guest OS, compared to a nested SMMU CMDQ (with trappings).
This is the part-1 series:
- Preparatory changes to share the existing SMMU functions
- A new CMDQV driver and extending the SMMUv3 driver to interact with the new driver
- Limit the commands for a guest kernel.
It's available on Github:
https://github.com/nicolinc/iommufd/commits/vcmdq_in_kernel-v11
And the part-2 RFC series is also sent for discussion:
https://lore.kernel.org/all/[email protected]/
Note that this in-kernel support isn't confined to host kernels running on Grace-powered servers, but is also used by guest kernels running on VMs virtualized on those servers. So, those VMs must install the driver, ideally before the part 2 is merged. So, later those servers would only need to upgrade their host kernels without bothering the VMs.
Thank you!
Changelog
v11:
* Rebased on 6.11-rc2
* Fixed "vtinfs" typo in kdoc
* Maxed out the max_n_shift to hardware allowed value
v10:
https://lore.kernel.org/all/[email protected]/
* Rebased on 6.11-rc1
* Added impl design mimicing arm-smmu (v2) driver
* Replaced the CS_NONE quirk with a new smmu option
* Fixed misaligned 64-bit register reading in the isr()
* Explicitly assign opcode to arm_smmu_cmdq_batch_init() where cmd/ent might potentially not be initialized.
v9:
https://lore.kernel.org/all/[email protected]/
* Rebased on 6.10-rc3
* Replaced "base + offset" in write_config() with REG helpers
* Added "Reviewed-by" line from Jason, to the remaining PATCH-5
v8:
https://lore.kernel.org/all/[email protected]/
* Added "Reviewed-by" lines from Jason
* Replaced memset with a simple "cmd[1] = 0"
* Replaced MMIO read/write helpers with REG_* macros
* Dropped the racy static string in lvcmdq_error_header()
* Added a few lines of comments to arm_smmu_get_cmdq at the line calling tegra241_cmdqv_get_cmdq()
v7:
https://lore.kernel.org/all/[email protected]/
* Moved all public symbols into one single patch
* Enforced a command batch to use the same cmdq
* Enforced the use of arm_smmu_cmdq_build_sync_cmd()
* Reworked the tegra241-cmdqv driver patch
- Dropped logging macros, cmdqv->dev, and atomic
- Dropped devm_* and added tegra241_cmdqv_device_remove()
- Moved all structure allocations to cmdqv's probe() from device_reset() where only register configurations remains
- Switched the config macros to inline functions
- Optimized ISR routine with 64-bit reading MMIO
- Scan once per batch against command list
- Reorganized function locations
- Minor readability changes
v6:
https://lore.kernel.org/all/[email protected]/
* Reordered the patch sequence to fix git-bisect break
* Added a status cache to cmdqv/vintf/vcmdq structure
* Added gerror/gerrorn value match in hw_deinit()
* Minimized changes in __arm_smmu_cmdq_skip_err()
* Preallocated VCMDQs to VINTFs for stablility
v5:
https://lore.kernel.org/all/[email protected]/
* Improved print/mmio helpers
* Added proper register reset routines
* Reorganized init/deinit functions to share with VIOMMU callbacks in the upcoming part-2 user-space series (RFC)
v4:
https://lore.kernel.org/all/[email protected]/
* Rebased on v6.9-rc1
* Renamed to "tegra241-cmdqv", following other Grace kernel patches
* Added a set of print and MMIO helpers
* Reworked the guest limitation patch
v3:
https://lore.kernel.org/all/[email protected]/
* Dropped VMID and mdev patches to redesign later based on IOMMUFD
* Separated HYP_OWN part for guest support into a new patch
* Added new preparatory changes
v2:
https://lore.kernel.org/all/[email protected]/
* Added mdev interface support for hypervisor and VMs
* Added preparatory changes for mdev interface implementation
* PATCH-12 Changed ->issue_cmdlist() to ->get_cmdq() for a better integration with recently merged ECMDQ-related changes
v1:
https://lore.kernel.org/all/[email protected]/
Nate Watterson (1):
iommu/arm-smmu-v3: Add in-kernel support for NVIDIA Tegra241 (Grace)
CMDQV
Nicolin Chen (8):
iommu/arm-smmu-v3: Issue a batch of commands to the same cmdq iommu/arm-smmu-v3: Enforce arm_smmu_cmdq_build_sync_cmd iommu/arm-smmu-v3: Pass in cmdq pointer to
arm_smmu_cmdq_build_sync_cmd
iommu/arm-smmu-v3: Pass in cmdq pointer to arm_smmu_cmdq_init iommu/arm-smmu-v3: Make symbols public for CONFIG_TEGRA241_CMDQV iommu/arm-smmu-v3: Add ARM_SMMU_OPT_SECONDARY_CMDQ_CS_NONE_ONLY iommu/arm-smmu-v3: Add struct arm_smmu_impl iommu/tegra241-cmdqv: Limit CMDs for guest owned VINTF
MAINTAINERS | 1 + drivers/iommu/Kconfig | 11 + drivers/iommu/arm/arm-smmu-v3/Makefile | 1 + drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 199 ++-- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 51 +- .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 901 ++++++++++++++++++ 6 files changed, 1090 insertions(+), 74 deletions(-) create mode 100644 drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
Signature · 1 line
2.43.0
PATCH v11 1/9 iommu/arm-smmu-v3: Issue a batch of commands to the same cmdq
The driver calls in different places the arm_smmu_get_cmdq() helper, and it's fine to do so since the helper always returns the single SMMU CMDQ. However, with NVIDIA CMDQV extension or SMMU ECMDQ, there can be multiple cmdqs in the system to select one from. And either case requires a batch of commands to be issued to the same cmdq. Thus, a cmdq has to be decided in the higher-level callers.
Add a cmdq pointer in arm_smmu_cmdq_batch structure, and decide the cmdq when initializing the batch. Pass its pointer down to the bottom function. Update __arm_smmu_cmdq_issue_cmd() accordingly for single command issuers.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 44 +++++++++++++-------- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 + 2 files changed, 29 insertions(+), 16 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +28 -16
@@ -592,11 +592,11 @@ static void arm_smmu_cmdq_poll_valid_map(struct arm_smmu_cmdq *cmdq,/* Wait for the command queue to become non-full */ static int arm_smmu_cmdq_poll_until_not_full (struct arm_smmu_device *smmu, +struct arm_smmu_cmdq *cmdq,struct arm_smmu_ll_queue *llq) {unsigned long flags;struct arm_smmu_queue_poll qp; -struct arm_smmu_cmdq *cmdq =arm_smmu_get_cmdq (smmu);int ret =0 ;/*
@@ -627,11 +627,11 @@ static int arm_smmu_cmdq_poll_until_not_full(struct arm_smmu_device *smmu,* Must be called with the cmdq lock held in some capacity. */ static int __arm_smmu_cmdq_poll_until_msi (struct arm_smmu_device *smmu, +struct arm_smmu_cmdq *cmdq,struct arm_smmu_ll_queue *llq) {int ret =0 ;struct arm_smmu_queue_poll qp; -struct arm_smmu_cmdq *cmdq =arm_smmu_get_cmdq (smmu);u32 *cmd = (u32 *)(Q_ENT (&cmdq->q, llq->prod));queue_poll_init (smmu, &qp);
@@ -651,10 +651,10 @@ static int __arm_smmu_cmdq_poll_until_msi(struct arm_smmu_device *smmu,* Must be called with the cmdq lock held in some capacity. */ static int __arm_smmu_cmdq_poll_until_consumed (struct arm_smmu_device *smmu, +struct arm_smmu_cmdq *cmdq,struct arm_smmu_ll_queue *llq) {struct arm_smmu_queue_poll qp; -struct arm_smmu_cmdq *cmdq =arm_smmu_get_cmdq (smmu);u32 prod = llq->prod;int ret =0 ;
@@ -701,12 +701,13 @@ static int __arm_smmu_cmdq_poll_until_consumed(struct arm_smmu_device *smmu, }static int arm_smmu_cmdq_poll_until_sync (struct arm_smmu_device *smmu, +struct arm_smmu_cmdq *cmdq,struct arm_smmu_ll_queue *llq) {if (smmu->options &ARM_SMMU_OPT_MSIPOLL ) -return __arm_smmu_cmdq_poll_until_msi (smmu, llq); +return __arm_smmu_cmdq_poll_until_msi (smmu, cmdq, llq); -return __arm_smmu_cmdq_poll_until_consumed (smmu, llq); +return __arm_smmu_cmdq_poll_until_consumed (smmu, cmdq, llq); } static void arm_smmu_cmdq_write_entries(struct arm_smmu_cmdq *cmdq, u64 *cmds,
@@ -743,13 +744,13 @@ static void arm_smmu_cmdq_write_entries(struct arm_smmu_cmdq *cmdq, u64 *cmds,* CPU will appear before any of the commands from the other CPU. */ static int arm_smmu_cmdq_issue_cmdlist (struct arm_smmu_device *smmu, +struct arm_smmu_cmdq *cmdq,u64 *cmds,int n,bool sync) {u64 cmd_sync[CMDQ_ENT_DWORDS ];u32 prod;unsigned long flags;bool owner; -struct arm_smmu_cmdq *cmdq =arm_smmu_get_cmdq (smmu);struct arm_smmu_ll_queue llq, head;int ret =0 ;
@@ -763,7 +764,7 @@ static int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,while (!queue_has_space (&llq, n + sync)) {local_irq_restore (flags); -if (arm_smmu_cmdq_poll_until_not_full (smmu, &llq)) +if (arm_smmu_cmdq_poll_until_not_full (smmu, cmdq, &llq))dev_err_ratelimited (smmu->dev,"CMDQ timeout\n" );local_irq_save (flags); }
@@ -839,7 +840,7 @@ static int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,/* 5. If we are inserting a CMD_SYNC, we must wait for it to complete */ if (sync) { llq.prod =queue_inc_prod_n (&llq, n); - ret =arm_smmu_cmdq_poll_until_sync (smmu, &llq); + ret =arm_smmu_cmdq_poll_until_sync (smmu, cmdq, &llq);if (ret) { dev_err_ratelimited(smmu->dev, "CMD_SYNC timeout at 0x%08x [hwprod 0x%08x, hwcons 0x%08x]\n",
@@ -874,7 +875,8 @@ static int __arm_smmu_cmdq_issue_cmd(struct arm_smmu_device *smmu,return -EINVAL ; } -return arm_smmu_cmdq_issue_cmdlist (smmu, cmd,1 , sync); +return arm_smmu_cmdq_issue_cmdlist ( + smmu,arm_smmu_get_cmdq (smmu), cmd,1 , sync); } static int arm_smmu_cmdq_issue_cmd(struct arm_smmu_device *smmu,
@@ -889,6 +891,13 @@ static int arm_smmu_cmdq_issue_cmd_with_sync(struct arm_smmu_device *smmu,return __arm_smmu_cmdq_issue_cmd (smmu, ent, true); } +static void arm_smmu_cmdq_batch_init (struct arm_smmu_device *smmu, +struct arm_smmu_cmdq_batch *cmds) +{ + cmds->num =0 ; + cmds->cmdq =arm_smmu_get_cmdq (smmu); +} +static void arm_smmu_cmdq_batch_add (struct arm_smmu_device *smmu,struct arm_smmu_cmdq_batch *cmds,struct arm_smmu_cmdq_ent *cmd)
@@ -897,12 +906,14 @@ static void arm_smmu_cmdq_batch_add(struct arm_smmu_device *smmu,if (cmds->num ==CMDQ_BATCH_ENTRIES -1 && (smmu->options &ARM_SMMU_OPT_CMDQ_FORCE_SYNC )) { -arm_smmu_cmdq_issue_cmdlist (smmu, cmds->cmds, cmds->num, true); +arm_smmu_cmdq_issue_cmdlist (smmu, cmds->cmdq, cmds->cmds, + cmds->num, true); cmds->num =0 ; }if (cmds->num ==CMDQ_BATCH_ENTRIES ) { -arm_smmu_cmdq_issue_cmdlist (smmu, cmds->cmds, cmds->num, false); +arm_smmu_cmdq_issue_cmdlist (smmu, cmds->cmdq, cmds->cmds, + cmds->num, false); cmds->num =0 ; }
@@ -919,7 +930,8 @@ static void arm_smmu_cmdq_batch_add(struct arm_smmu_device *smmu,static int arm_smmu_cmdq_batch_submit (struct arm_smmu_device *smmu,struct arm_smmu_cmdq_batch *cmds) { -return arm_smmu_cmdq_issue_cmdlist (smmu, cmds->cmds, cmds->num, true); +return arm_smmu_cmdq_issue_cmdlist (smmu, cmds->cmdq, cmds->cmds, + cmds->num, true); } static void arm_smmu_page_response(struct device *dev, struct iopf_fault *unused,
@@ -1170,7 +1182,7 @@ static void arm_smmu_sync_cd(struct arm_smmu_master *master, }, }; - cmds.num =0 ; +arm_smmu_cmdq_batch_init (smmu, &cmds);for (i =0 ; i < master->num_streams; i++) { cmd.cfgi.sid = master->streams[i].id;arm_smmu_cmdq_batch_add (smmu, &cmds, &cmd);
@@ -2021,7 +2033,7 @@ static int arm_smmu_atc_inv_master(struct arm_smmu_master *master,arm_smmu_atc_inv_to_cmd (ssid,0 ,0 , &cmd); - cmds.num =0 ; +arm_smmu_cmdq_batch_init (master->smmu, &cmds);for (i =0 ; i < master->num_streams; i++) { cmd.atc.sid = master->streams[i].id;arm_smmu_cmdq_batch_add (master->smmu, &cmds, &cmd);
@@ -2059,7 +2071,7 @@ int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,if (!atomic_read (&smmu_domain->nr_ats_masters))return 0 ; - cmds.num =0 ; +arm_smmu_cmdq_batch_init (smmu_domain->smmu, &cmds);spin_lock_irqsave (&smmu_domain->devices_lock, flags); list_for_each_entry(master_domain, &smmu_domain->devices,
@@ -2141,7 +2153,7 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd, num_pages++; } - cmds.num =0 ; +arm_smmu_cmdq_batch_init (smmu, &cmds);while (iova < end) {if (smmu->features &ARM_SMMU_FEAT_RANGE_INV ) {
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +1 -0
@@ -570,6 +570,7 @@ struct arm_smmu_cmdq {struct arm_smmu_cmdq_batch {u64 cmds[CMDQ_BATCH_ENTRIES *CMDQ_ENT_DWORDS ]; +struct arm_smmu_cmdq *cmdq;int num; };
Signature · 1 line
2.43.0
PATCH v11 2/9 iommu/arm-smmu-v3: Enforce arm_smmu_cmdq_build_sync_cmd
In reply to Nicolin ChenThere is an existing arm_smmu_cmdq_build_sync_cmd() so the driver should call it at all places other than going through arm_smmu_cmdq_build_cmd() separately. This helps the following patch that adds a CS_NONE option.
Note that this changes the type of CMD_SYNC in __arm_smmu_cmdq_skip_err, in ARM_SMMU_OPT_MSIPOLL=true case, from previously a non-MSI one to now an MSI one that is proven to still work using a hacking test:
nvme: Adding to iommu group 10
nvme: --------hacking-----------
arm-smmu-v3: unexpected global error reported (0x00000001),
this could be serious
arm-smmu-v3: CMDQ error (cons 0x01000022): Illegal command
arm-smmu-v3: skipping command in error state:
arm-smmu-v3: 0x0000000000000000
arm-smmu-v3: 0x0000000000000000
nvme: -------recovered----------
nvme nvme0: 72/0/0 default/read/poll queues
nvme0n1: p1 p2
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 36 ++++++++------------- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 3 -- 2 files changed, 13 insertions(+), 26 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +13 -23
@@ -329,16 +329,6 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent) cmd[0 ] |=FIELD_PREP (CMDQ_RESUME_0_RESP , ent->resume.resp); cmd[1 ] |=FIELD_PREP (CMDQ_RESUME_1_STAG , ent->resume.stag);break ; -case CMDQ_OP_CMD_SYNC : -if (ent->sync.msiaddr) { - cmd[0 ] |=FIELD_PREP (CMDQ_SYNC_0_CS ,CMDQ_SYNC_0_CS_IRQ ); - cmd[1 ] |= ent->sync.msiaddr &CMDQ_SYNC_1_MSIADDR_MASK ; - }else { - cmd[0 ] |=FIELD_PREP (CMDQ_SYNC_0_CS ,CMDQ_SYNC_0_CS_SEV ); - } - cmd[0 ] |=FIELD_PREP (CMDQ_SYNC_0_MSH ,ARM_SMMU_SH_ISH ); - cmd[0 ] |=FIELD_PREP (CMDQ_SYNC_0_MSIATTR ,ARM_SMMU_MEMATTR_OIWB ); -break ;default :return -ENOENT ; }
@@ -354,20 +344,23 @@ static struct arm_smmu_cmdq *arm_smmu_get_cmdq(struct arm_smmu_device *smmu)static void arm_smmu_cmdq_build_sync_cmd (u64 *cmd,struct arm_smmu_device *smmu,struct arm_smmu_queue *q,u32 prod) { -struct arm_smmu_cmdq_ent ent = { - .opcode =CMDQ_OP_CMD_SYNC , - }; + cmd[1 ] =0 ; + cmd[0 ] =FIELD_PREP (CMDQ_0_OP ,CMDQ_OP_CMD_SYNC ) | +FIELD_PREP (CMDQ_SYNC_0_MSH ,ARM_SMMU_SH_ISH ) | +FIELD_PREP (CMDQ_SYNC_0_MSIATTR ,ARM_SMMU_MEMATTR_OIWB ); + +if (!(smmu->options &ARM_SMMU_OPT_MSIPOLL )) { + cmd[0 ] |=FIELD_PREP (CMDQ_SYNC_0_CS ,CMDQ_SYNC_0_CS_SEV ); +return ; + }/* * Beware that Hi16xx adds an extra 32 bits of goodness to its MSI * payload, so the write will zero the entire command on that platform. */ -if (smmu->options &ARM_SMMU_OPT_MSIPOLL ) { - ent.sync.msiaddr = q->base_dma +Q_IDX (&q->llq, prod) * - q->ent_dwords *8 ; - } - -arm_smmu_cmdq_build_cmd (cmd, &ent); + cmd[0 ] |=FIELD_PREP (CMDQ_SYNC_0_CS ,CMDQ_SYNC_0_CS_IRQ ); + cmd[1 ] |= (q->base_dma +Q_IDX (&q->llq, prod) * q->ent_dwords *8 ) & +CMDQ_SYNC_1_MSIADDR_MASK ; } static void __arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu,
@@ -384,9 +377,6 @@ static void __arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu,u64 cmd[CMDQ_ENT_DWORDS ];u32 cons =readl_relaxed (q->cons_reg);u32 idx =FIELD_GET (CMDQ_CONS_ERR , cons); -struct arm_smmu_cmdq_ent cmd_sync = { - .opcode =CMDQ_OP_CMD_SYNC , - };dev_err (smmu->dev,"CMDQ error (cons 0x%08x): %s\n" , cons, idx <ARRAY_SIZE (cerror_str) ? cerror_str[idx] :"Unknown" );
@@ -420,7 +410,7 @@ static void __arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu,dev_err (smmu->dev,"\t0x%016llx\n" , (unsigned long long )cmd[i]);/* Convert the erroneous command into a CMD_SYNC */ -arm_smmu_cmdq_build_cmd (cmd, &cmd_sync); +arm_smmu_cmdq_build_sync_cmd (cmd, smmu, q, cons);queue_write (Q_ENT (q, cons), cmd, q->ent_dwords); }
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +0 -3
@@ -518,9 +518,6 @@ struct arm_smmu_cmdq_ent { } resume;#define CMDQ_OP_CMD_SYNC 0x46 -struct { -u64 msiaddr; - } sync; }; };
Signature · 1 line
2.43.0
Will Deacon
maintainerRe: [PATCH v11 2/9] iommu/arm-smmu-v3: Enforce arm_smmu_cmdq_build_sync_cmd
Quoted 4 lines · Nicolin Chen
There is an existing
arm_smmu_cmdq_build_sync_cmd()so the driver should call it at all places other than going througharm_smmu_cmdq_build_cmd()separately. This helps the following patch that adds aCS_NONEoption.
Quoted 14 lines
Note that this changes the type of
CMD_SYNCin__arm_smmu_cmdq_skip_err, in ARM_SMMU_OPT_MSIPOLL=true case, from previously a non-MSI one to now an MSI one that is proven to still work using a hacking test:
nvme: Adding to iommu group 10
nvme: --------hacking-----------
arm-smmu-v3: unexpected global error reported (0x00000001),
this could be serious
arm-smmu-v3: CMDQ error (cons0x01000022): Illegal command
arm-smmu-v3: skipping command in error state:
arm-smmu-v3:0x0000000000000000
arm-smmu-v3:0x0000000000000000
nvme: -------recovered----------
nvme nvme0: 72/0/0 default/read/poll queues
nvme0n1: p1 p2
Hmm, I'm still a little wary of this. The only reason we emit a CMD_SYNC in __arm_smmu_cmdq_skip_err() is because the SMMU doesn't have a NOP command. So I'd really like the SYNC to have as little functionality as possible in this case.
I think we could either propapate the 'cs' field down to arm_smmu_cmdq_build_cmd() or just open-code the command-creation in __arm_smmu_cmdq_skip_err().
Will
Re: [PATCH v11 2/9] iommu/arm-smmu-v3: Enforce arm_smmu_cmdq_build_sync_cmd
Quoted 20 lines · Will Deacon
Quoted 4 lines · Nicolin Chen
There is an existing
arm_smmu_cmdq_build_sync_cmd()so the driver should call it at all places other than going througharm_smmu_cmdq_build_cmd()separately. This helps the following patch that adds aCS_NONEoption.Quoted 14 lines
Note that this changes the type of
CMD_SYNCin__arm_smmu_cmdq_skip_err, in ARM_SMMU_OPT_MSIPOLL=true case, from previously a non-MSI one to now an MSI one that is proven to still work using a hacking test:
nvme: Adding to iommu group 10
nvme: --------hacking-----------
arm-smmu-v3: unexpected global error reported (0x00000001),
this could be serious
arm-smmu-v3: CMDQ error (cons0x01000022): Illegal command
arm-smmu-v3: skipping command in error state:
arm-smmu-v3:0x0000000000000000
arm-smmu-v3:0x0000000000000000
nvme: -------recovered----------
nvme nvme0: 72/0/0 default/read/poll queues
nvme0n1: p1 p2
Quoted 8 lines
Hmm, I'm still a little wary of this. The only reason we emit a
CMD_SYNCin__arm_smmu_cmdq_skip_err()is because the SMMU doesn't have a NOP command. So I'd really like the SYNC to have as little functionality as possible in this case.I think we could either propapate the 'cs' field down to
arm_smmu_cmdq_build_cmd()or just open-code the command-creation in__arm_smmu_cmdq_skip_err().
OK. Let's go with your approach then:
+ if (arm_smmu_cmdq_needs_busy_polling(smmu, cmdq))
+ u64p_replace_bits(cmd, CMDQ_SYNC_0_CS_NONE, CMDQ_SYNC_0_CS);
Thanks
Nicolin
PATCH v11 3/9 iommu/arm-smmu-v3: Pass in cmdq pointer to arm_smmu_cmdq_build_sync_cmd
In reply to Nicolin ChenThe CMDQV extension on NVIDIA Tegra241 SoC only supports CS_NONE in the CS field of CMD_SYNC, v.s. standard SMMU CMDQ. Pass in the cmdq pointer directly, so the function can identify a different cmdq implementation.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +8 -5
@@ -342,8 +342,10 @@ static struct arm_smmu_cmdq *arm_smmu_get_cmdq(struct arm_smmu_device *smmu) }static void arm_smmu_cmdq_build_sync_cmd (u64 *cmd,struct arm_smmu_device *smmu, -struct arm_smmu_queue *q,u32 prod) +struct arm_smmu_cmdq *cmdq,u32 prod) { +struct arm_smmu_queue *q = &cmdq->q; + cmd[1 ] =0 ; cmd[0 ] =FIELD_PREP (CMDQ_0_OP ,CMDQ_OP_CMD_SYNC ) | FIELD_PREP(CMDQ_SYNC_0_MSH, ARM_SMMU_SH_ISH) |
@@ -364,7 +366,7 @@ static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu, }static void __arm_smmu_cmdq_skip_err (struct arm_smmu_device *smmu, -struct arm_smmu_queue *q) +struct arm_smmu_cmdq *cmdq) {static const char *const cerror_str[] = { [CMDQ_ERR_CERROR_NONE_IDX ] ="No error" ,
@@ -372,6 +374,7 @@ static void __arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu, [CMDQ_ERR_CERROR_ABT_IDX] = "Abort on command fetch", [CMDQ_ERR_CERROR_ATC_INV_IDX] = "ATC invalidate timeout", }; +struct arm_smmu_queue *q = &cmdq->q;int i;u64 cmd[CMDQ_ENT_DWORDS ];
@@ -410,14 +413,14 @@ static void __arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu,dev_err (smmu->dev,"\t0x%016llx\n" , (unsigned long long )cmd[i]);/* Convert the erroneous command into a CMD_SYNC */ -arm_smmu_cmdq_build_sync_cmd (cmd, smmu, q, cons); +arm_smmu_cmdq_build_sync_cmd (cmd, smmu, cmdq, cons);queue_write (Q_ENT (q, cons), cmd, q->ent_dwords); }static void arm_smmu_cmdq_skip_err (struct arm_smmu_device *smmu) { -__arm_smmu_cmdq_skip_err (smmu, &smmu->cmdq.q); +__arm_smmu_cmdq_skip_err (smmu, &smmu->cmdq); }/*
@@ -780,7 +783,7 @@ static int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,arm_smmu_cmdq_write_entries (cmdq, cmds, llq.prod, n);if (sync) { prod =queue_inc_prod_n (&llq, n); -arm_smmu_cmdq_build_sync_cmd (cmd_sync, smmu, &cmdq->q, prod); +arm_smmu_cmdq_build_sync_cmd (cmd_sync, smmu, cmdq, prod);queue_write (Q_ENT (&cmdq->q, prod), cmd_sync,CMDQ_ENT_DWORDS );/*
Signature · 1 line
2.43.0
Re: [PATCH v11 3/9] iommu/arm-smmu-v3: Pass in cmdq pointer to arm_smmu_cmdq_build_sync_cmd
Quoted 8 lines · Nicolin Chen
The CMDQV extension on NVIDIA Tegra241 SoC only supports
CS_NONEin the CS field ofCMD_SYNC, v.s. standard SMMU CMDQ. Pass in the cmdq pointer directly, so the function can identify a different cmdq implementation.Signed-off-by Nicolin Chen <[email protected]>--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
Jason
PATCH v11 4/9 iommu/arm-smmu-v3: Pass in cmdq pointer to arm_smmu_cmdq_init
In reply to Nicolin ChenSo that this function can be used by other cmdqs than &smmu->cmdq only.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +3 -3
@@ -3555,9 +3555,9 @@ static int arm_smmu_init_one_queue(struct arm_smmu_device *smmu,return 0 ; } -static int arm_smmu_cmdq_init (struct arm_smmu_device *smmu) +static int arm_smmu_cmdq_init (struct arm_smmu_device *smmu, +struct arm_smmu_cmdq *cmdq) { -struct arm_smmu_cmdq *cmdq = &smmu->cmdq;unsigned int nents =1 << cmdq->q.llq.max_n_shift;atomic_set (&cmdq->owner_prod,0 );
@@ -3582,7 +3582,7 @@ static int arm_smmu_init_queues(struct arm_smmu_device *smmu)if (ret)return ret; - ret =arm_smmu_cmdq_init (smmu); + ret =arm_smmu_cmdq_init (smmu, &smmu->cmdq);if (ret)return ret;
Signature · 1 line
2.43.0
Re: [PATCH v11 4/9] iommu/arm-smmu-v3: Pass in cmdq pointer to arm_smmu_cmdq_init
Quoted 6 lines · Nicolin Chen
So that this function can be used by other cmdqs than
&smmu->cmdqonly.Signed-off-by Nicolin Chen <[email protected]>--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Jason
PATCH v11 5/9 iommu/arm-smmu-v3: Make symbols public for CONFIG_TEGRA241_CMDQV
In reply to Nicolin ChenThe symbols __arm_smmu_cmdq_skip_err(), arm_smmu_init_one_queue(), and arm_smmu_cmdq_init() need to be used by the tegra241-cmdqv compilation unit in a following patch.
Remove the static and put prototypes in the header.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 18 ++++++++---------- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 9 +++++++++ 2 files changed, 17 insertions(+), 10 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +8 -10
@@ -365,8 +365,8 @@ static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu,CMDQ_SYNC_1_MSIADDR_MASK ; } -static void __arm_smmu_cmdq_skip_err (struct arm_smmu_device *smmu, -struct arm_smmu_cmdq *cmdq) +void __arm_smmu_cmdq_skip_err (struct arm_smmu_device *smmu, +struct arm_smmu_cmdq *cmdq) {static const char *const cerror_str[] = { [CMDQ_ERR_CERROR_NONE_IDX ] ="No error" ,
@@ -3512,12 +3512,10 @@ static struct iommu_dirty_ops arm_smmu_dirty_ops = { };/* Probing and initialisation functions */ -static int arm_smmu_init_one_queue (struct arm_smmu_device *smmu, -struct arm_smmu_queue *q, -void __iomem *page, -unsigned long prod_off, -unsigned long cons_off, -size_t dwords,const char *name) +int arm_smmu_init_one_queue (struct arm_smmu_device *smmu, +struct arm_smmu_queue *q,void __iomem *page, +unsigned long prod_off,unsigned long cons_off, +size_t dwords,const char *name) {size_t qsz;
@@ -3555,8 +3553,8 @@ static int arm_smmu_init_one_queue(struct arm_smmu_device *smmu,return 0 ; } -static int arm_smmu_cmdq_init (struct arm_smmu_device *smmu, -struct arm_smmu_cmdq *cmdq) +int arm_smmu_cmdq_init (struct arm_smmu_device *smmu, +struct arm_smmu_cmdq *cmdq) {unsigned int nents =1 << cmdq->q.llq.max_n_shift;
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +9 -0
@@ -813,6 +813,15 @@ void arm_smmu_tlb_inv_range_asid(unsigned long iova, size_t size, int asid,int arm_smmu_atc_inv_domain (struct arm_smmu_domain *smmu_domain,unsigned long iova,size_t size); +void __arm_smmu_cmdq_skip_err (struct arm_smmu_device *smmu, +struct arm_smmu_cmdq *cmdq); +int arm_smmu_init_one_queue (struct arm_smmu_device *smmu, +struct arm_smmu_queue *q,void __iomem *page, +unsigned long prod_off,unsigned long cons_off, +size_t dwords,const char *name); +int arm_smmu_cmdq_init (struct arm_smmu_device *smmu, +struct arm_smmu_cmdq *cmdq); +#ifdef CONFIG_ARM_SMMU_V3_SVA bool arm_smmu_sva_supported (struct arm_smmu_device *smmu);bool arm_smmu_master_sva_supported (struct arm_smmu_master *master);
Signature · 1 line
2.43.0
PATCH v11 6/9 iommu/arm-smmu-v3: Add ARM_SMMU_OPT_SECONDARY_CMDQ_CS_NONE_ONLY
In reply to Nicolin ChenThe CMDQV extension in NVIDIA Tegra241 SoC only supports CS_NONE in the CS field of CMD_SYNC. Add a new SMMU option to accommodate that.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 17 ++++++++++++++++- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 9 +++++---- 2 files changed, 21 insertions(+), 5 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +16 -1
@@ -341,6 +341,15 @@ static struct arm_smmu_cmdq *arm_smmu_get_cmdq(struct arm_smmu_device *smmu)return &smmu->cmdq; } +static bool arm_smmu_cmdq_needs_busy_polling (struct arm_smmu_device *smmu, +struct arm_smmu_cmdq *cmdq) +{ +if (cmdq == &smmu->cmdq) +return false; + +return smmu->options &ARM_SMMU_OPT_SECONDARY_CMDQ_CS_NONE_ONLY ; +} +static void arm_smmu_cmdq_build_sync_cmd (u64 *cmd,struct arm_smmu_device *smmu,struct arm_smmu_cmdq *cmdq,u32 prod) {
@@ -351,6 +360,11 @@ static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu,FIELD_PREP (CMDQ_SYNC_0_MSH ,ARM_SMMU_SH_ISH ) |FIELD_PREP (CMDQ_SYNC_0_MSIATTR ,ARM_SMMU_MEMATTR_OIWB ); +if (arm_smmu_cmdq_needs_busy_polling (smmu, cmdq)) { + cmd[0 ] |=FIELD_PREP (CMDQ_SYNC_0_CS ,CMDQ_SYNC_0_CS_NONE ); +return ; + } +if (!(smmu->options &ARM_SMMU_OPT_MSIPOLL )) { cmd[0 ] |=FIELD_PREP (CMDQ_SYNC_0_CS ,CMDQ_SYNC_0_CS_SEV );return ;
@@ -697,7 +711,8 @@ static int arm_smmu_cmdq_poll_until_sync(struct arm_smmu_device *smmu, struct arm_smmu_cmdq *cmdq, struct arm_smmu_ll_queue *llq) { -if (smmu->options &ARM_SMMU_OPT_MSIPOLL ) +if (smmu->options &ARM_SMMU_OPT_MSIPOLL && + !arm_smmu_cmdq_needs_busy_polling (smmu, cmdq))return __arm_smmu_cmdq_poll_until_msi (smmu, cmdq, llq);return __arm_smmu_cmdq_poll_until_consumed (smmu, cmdq, llq);
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +5 -4
@@ -658,10 +658,11 @@ struct arm_smmu_device {#define ARM_SMMU_FEAT_HD (1 << 22)u32 features; -#define ARM_SMMU_OPT_SKIP_PREFETCH (1 << 0) -#define ARM_SMMU_OPT_PAGE0_REGS_ONLY (1 << 1) -#define ARM_SMMU_OPT_MSIPOLL (1 << 2) -#define ARM_SMMU_OPT_CMDQ_FORCE_SYNC (1 << 3) +#define ARM_SMMU_OPT_SKIP_PREFETCH (1 << 0) +#define ARM_SMMU_OPT_PAGE0_REGS_ONLY (1 << 1) +#define ARM_SMMU_OPT_MSIPOLL (1 << 2) +#define ARM_SMMU_OPT_CMDQ_FORCE_SYNC (1 << 3) +#define ARM_SMMU_OPT_SECONDARY_CMDQ_CS_NONE_ONLY (1 << 4)u32 options;struct arm_smmu_cmdq cmdq;
Signature · 1 line
2.43.0
Re: [PATCH v11 6/9] iommu/arm-smmu-v3: Add ARM_SMMU_OPT_SECONDARY_CMDQ_CS_NONE_ONLY
Quoted 9 lines · Nicolin Chen
The CMDQV extension in NVIDIA Tegra241 SoC only supports
CS_NONEin the CS field ofCMD_SYNC. Add a new SMMU option to accommodate that.Suggested-by Will Deacon <[email protected]>Signed-off-by Nicolin Chen <[email protected]>--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 17 ++++++++++++++++- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 9 +++++---- 2 files changed, 21 insertions(+), 5 deletions(-)
Jason
PATCH v11 7/9 iommu/arm-smmu-v3: Add struct arm_smmu_impl
In reply to Nicolin ChenNVIDIA Tegra241 implemented SMMU in a slightly different way that supports a CMDQV extension feature as a secondary CMDQ for virtualization cases.
Mimicing the arm-smmu (v2) driver, introduce a new struct arm_smmu_impl to accommodate impl routines.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 67 +++++++++++++++++---- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 19 ++++++ 2 files changed, 74 insertions(+), 12 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +55 -12
@@ -338,7 +338,12 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent)static struct arm_smmu_cmdq *arm_smmu_get_cmdq (struct arm_smmu_device *smmu) { -return &smmu->cmdq; +struct arm_smmu_cmdq *cmdq =NULL ; + +if (smmu->impl && smmu->impl->get_secondary_cmdq) + cmdq = smmu->impl->get_secondary_cmdq (smmu); + +return cmdq ?: &smmu->cmdq; } static bool arm_smmu_cmdq_needs_busy_polling(struct arm_smmu_device *smmu,
@@ -4044,6 +4049,14 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu)return ret; } +if (smmu->impl && smmu->impl->device_reset) { + ret = smmu->impl->device_reset (smmu); +if (ret) { +dev_err (smmu->dev,"failed to reset impl\n" ); +return ret; + } + } +return 0 ; }
@@ -4347,8 +4360,23 @@ static void acpi_smmu_get_options(u32 model, struct arm_smmu_device *smmu)dev_notice (smmu->dev,"option mask 0x%x\n" , smmu->options); } -static int arm_smmu_device_acpi_probe (struct platform_device *pdev, -struct arm_smmu_device *smmu) +static struct arm_smmu_device * +arm_smmu_impl_acpi_probe (struct arm_smmu_device *smmu, +struct acpi_iort_node *node) +{ +/* +* DSDT might hold some SMMU extension, so we have no option but to go +* through the ACPI tables unconditionally. On success, this returns a +* copy of smmu struct holding an impl pointer. Otherwise, an impl may +* choose to return an ERR_PTR as an error out, or to return the pass- +* in smmu pointer as a fallback to the standard SMMU. +*/ +return arm_smmu_impl_acpi_dsdt_probe (smmu, node); +} + +static struct arm_smmu_device * +arm_smmu_device_acpi_probe (struct platform_device *pdev, +struct arm_smmu_device *smmu) {struct acpi_iort_smmu_v3 *iort_smmu;struct device *dev = smmu->dev;
@@ -4372,18 +4400,20 @@ static int arm_smmu_device_acpi_probe(struct platform_device *pdev, smmu->features |= ARM_SMMU_FEAT_HA; } -return 0 ; +return arm_smmu_impl_acpi_probe (smmu, node); }#else -static inline int arm_smmu_device_acpi_probe (struct platform_device *pdev, -struct arm_smmu_device *smmu) +static struct arm_smmu_device * +arm_smmu_device_acpi_probe (struct platform_device *pdev, +struct arm_smmu_device *smmu) { -return -ENODEV ; +return ERR_PTR (-ENODEV ); }#endif -static int arm_smmu_device_dt_probe (struct platform_device *pdev, -struct arm_smmu_device *smmu) +static struct arm_smmu_device * +arm_smmu_device_dt_probe (struct platform_device *pdev, +struct arm_smmu_device *smmu) {struct device *dev = &pdev->dev;u32 cells;
@@ -4401,7 +4431,7 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev,if (of_dma_is_coherent (dev->of_node)) smmu->features |=ARM_SMMU_FEAT_COHERENCY ; -return ret; +return ret ?ERR_PTR (ret) : smmu; }static unsigned long arm_smmu_resource_size (struct arm_smmu_device *smmu)
@@ -4453,6 +4483,14 @@ static void arm_smmu_rmr_install_bypass_ste(struct arm_smmu_device *smmu)iort_put_rmr_sids (dev_fwnode (smmu->dev), &rmr_list); } +static void arm_smmu_impl_remove (void *data) +{ +struct arm_smmu_device *smmu = data; + +if (smmu->impl && smmu->impl->device_remove) + smmu->impl->device_remove (smmu); +} +static int arm_smmu_device_probe (struct platform_device *pdev) {int irq, ret;
@@ -4467,10 +4505,14 @@ static int arm_smmu_device_probe(struct platform_device *pdev) smmu->dev = dev;if (dev->of_node) { - ret =arm_smmu_device_dt_probe (pdev, smmu); + smmu =arm_smmu_device_dt_probe (pdev, smmu); }else { - ret =arm_smmu_device_acpi_probe (pdev, smmu); + smmu =arm_smmu_device_acpi_probe (pdev, smmu); } +if (IS_ERR (smmu)) +return PTR_ERR (smmu); + + ret =devm_add_action_or_reset (dev, arm_smmu_impl_remove, smmu);if (ret)return ret;
@@ -4560,6 +4602,7 @@ static void arm_smmu_device_remove(struct platform_device *pdev) {struct arm_smmu_device *smmu =platform_get_drvdata (pdev); +arm_smmu_impl_remove (smmu);iommu_device_unregister (&smmu->iommu);iommu_device_sysfs_remove (&smmu->iommu);arm_smmu_device_disable (smmu);
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +19 -0
@@ -14,6 +14,9 @@#include <linux/mmzone.h> #include <linux/sizes.h> +struct arm_smmu_device ; +struct acpi_iort_node ; +/* MMIO registers */ #define ARM_SMMU_IDR0 0x0#define IDR0_ST_LVL GENMASK(28, 27)
@@ -627,9 +630,25 @@ struct arm_smmu_strtab_cfg {u32 strtab_base_cfg; }; +struct arm_smmu_impl { +int (*device_reset)(struct arm_smmu_device *smmu); +void (*device_remove)(struct arm_smmu_device *smmu); +struct arm_smmu_cmdq *(*get_secondary_cmdq)(struct arm_smmu_device *smmu); +}; + +static inline struct arm_smmu_device * +arm_smmu_impl_acpi_dsdt_probe (struct arm_smmu_device *smmu, +struct acpi_iort_node *node) +{ +return smmu; +} +/* An SMMUv3 instance */ struct arm_smmu_device {struct device *dev; +/* An SMMUv3 implementation */ +const struct arm_smmu_impl *impl; +void __iomem *base;void __iomem *page1;
Signature · 1 line
2.43.0
Re: [PATCH v11 7/9] iommu/arm-smmu-v3: Add struct arm_smmu_impl
Quoted 6 lines · Nicolin Chen
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +3
-2-static int arm_smmu_device_acpi_probe (struct platform_device *pdev, -struct arm_smmu_device *smmu) +static struct arm_smmu_device * +arm_smmu_impl_acpi_probe (struct arm_smmu_device *smmu, +struct acpi_iort_node *node)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void acpi_smmu_get_options(u32 model, struct arm_smmu_device *smmu) · line 4375
Quoted 10 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +10
-0+{ +/* +* DSDT might hold some SMMU extension, so we have no option but to go +* through the ACPI tables unconditionally. On success, this returns a +* copy of smmu struct holding an impl pointer. Otherwise, an impl may +* choose to return an ERR_PTR as an error out, or to return the pass- +* in smmu pointer as a fallback to the standard SMMU. +*/ +return arm_smmu_impl_acpi_dsdt_probe (smmu, node); +}
Lets generalize this a bit more and have the impl mechanism work for DT too.. Keep the main probe the same and add a new function after the dt/acpi steps:
smmu = arm_smmu_probe_impl(smmu); if (IS_ERR(smmu)) return PTR_ERR(smmu);
Which is more like:
/*
* Probe all the compiled in implementations. Each one checks to see if it
* matches this HW and if so returns a devm_krealloc'd arm_smmu_device which
* replaces the callers. Otherwise the original is returned or ERR_PTR. *
*/
static struct arm_smmu_device *arm_smmu_probe_impl(struct arm_smmu_device *orig_smmu)
{
struct arm_smmu_device *new_smmu;
int ret;
new_smmu = tegra241_cmdqv_acpi_dsdt_probe(orig_smmu);
if (new_smmu != ERR_PTR(-ENODEV))
goto out_new_impl;
return orig_smmu;out_new_impl:
if (IS_ERR(new_smmu)) return new_smmu; /* FIXME: check is this ordering OK during remove? */ ret = devm_add_action_or_reset(new_smmu->dev, arm_smmu_impl_remove, new_smmu); if (ret) return ERR_PTR(ret); return new_smmu; }
Easy to add new sub implementations. Provide an inline ENODEV sub in the header file for tegra241_cmdqv_acpi_dsdt_probe
Add something like this to the header to get the ACPI node:
static inline struct acpi_iort_node * arm_smmu_get_iort_node(struct arm_smmu_device *smmu)
{
return *(struct acpi_iort_node **)dev_get_platdata(smmu->dev);
}Since it isn't passed down
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void arm_smmu_device_remove(struct platform_device *pdev) · line 4605
Quoted 5 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +1
-0@@ -4560,6 +4602,7 @@ static void arm_smmu_device_remove(struct platform_device *pdev) {struct arm_smmu_device *smmu =platform_get_drvdata (pdev); +arm_smmu_impl_remove (smmu);
Can't call this if devm has been used to set it up, and this would be in the wrong order anyhow. Just remove it.. I guess the devm was put for this to avoid adding goto error unwind to probe?
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h struct arm_smmu_strtab_cfg { · line 637
Quoted 5 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +5
-0+struct arm_smmu_impl { +int (*device_reset)(struct arm_smmu_device *smmu); +void (*device_remove)(struct arm_smmu_device *smmu); +struct arm_smmu_cmdq *(*get_secondary_cmdq)(struct arm_smmu_device *smmu); +};
Can we put the word "ops" into this struct somehow? That would be a more typically kernely name.
arm_smmu_impl_ops perhaps?
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h struct arm_smmu_strtab_cfg { · line 649
Quoted 3 lines
struct
arm_smmu_device{drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +1
-0struct device *dev; +/* An SMMUv3 implementation */
The comment is self explanatory
Jason
Re: [PATCH v11 7/9] iommu/arm-smmu-v3: Add struct arm_smmu_impl
Hi Jason,
I've addressed all the comments here. Two additional replies below.
Quoted 16 lines · Jason Gunthorpe
/*
* Probe all the compiled in implementations. Each one checks to see if it
* matches this HW and if so returns a devm_krealloc'darm_smmu_devicewhich
* replaces the callers. Otherwise the original is returned orERR_PTR. *
*/
static structarm_smmu_device*arm_smmu_probe_impl(structarm_smmu_device*orig_smmu){ struct arm_smmu_device *new_smmu; int ret; new_smmu = tegra241_cmdqv_acpi_dsdt_probe(orig_smmu); if (new_smmu != ERR_PTR(-ENODEV)) goto out_new_impl; return orig_smmu;
Quoted 5 lines
out_new_impl:if (IS_ERR(new_smmu)) return new_smmu; /* FIXME: check is this ordering OK during remove? */
I am not able to test-verify this. At least CMDQV seems to be OK to remove after SMMU.
Quoted 9 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void arm_smmu_device_remove(struct platform_device *pdev) · line 4605
Quoted 5 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +1
-0@@ -4560,6 +4602,7 @@ static void arm_smmu_device_remove(struct platform_device *pdev) {struct arm_smmu_device *smmu =platform_get_drvdata (pdev); +arm_smmu_impl_remove (smmu);Can't call this if devm has been used to set it up, and this would be in the wrong order anyhow. Just remove it.. I guess the devm was put for this to avoid adding goto error unwind to probe?
I got that from Will's patch, and I think so, as it does simplify the unwind routine.
Thanks!
Nicolin
PATCH v11 8/9 iommu/arm-smmu-v3: Add in-kernel support for NVIDIA Tegra241 (Grace) CMDQV
In reply to Nicolin ChenFrom: Nate Watterson <[email protected]>
NVIDIA's Tegra241 Soc has a CMDQ-Virtualization (CMDQV) hardware, extending the standard ARM SMMU v3 IP to support multiple VCMDQs with virtualization capabilities. In terms of command queue, they are very like a standard SMMU CMDQ (or ECMDQs), but only support CS_NONE in the CS field of CMD_SYNC.
Add a new tegra241-cmdqv driver, and insert its structure pointer into the existing arm_smmu_device, and then add related function calls in the SMMUv3 driver to interact with the CMDQV driver.
In the CMDQV driver, add a minimal part for the in-kernel support: reserve VINTF0 for in-kernel use, and assign some of the VCMDQs to the VINTF0, and select one VCMDQ based on the current CPU ID to execute supported commands. This multi-queue design for in-kernel use gives some limited improvements:
up to 20% reduction of invalidation time was measured by a multi-threaded DMA unmap benchmark, compared to a single queue.
The other part of the CMDQV driver will be user-space support that gives a hypervisor running on the host OS to talk to the driver for virtualization use cases, allowing VMs to use VCMDQs without trappings, i.e. no VM Exits. This is designed based on IOMMUFD, and its RFC series is also under review. It will provide a guest OS a bigger improvement: 70% to 90% reductions of TLB invalidation time were measured by DMA unmap tests running in a guest, compared to nested SMMU CMDQ (with trappings).
As the initial version, the CMDQV driver only supports ACPI configurations.
--- MAINTAINERS | 1 + drivers/iommu/Kconfig | 11 + drivers/iommu/arm/arm-smmu-v3/Makefile | 1 + drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 9 + .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 868 ++++++++++++++++++ 5 files changed, 890 insertions(+) create mode 100644 drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
MAINTAINERS +1 -0
@@ -22472,6 +22472,7 @@ M: Thierry Reding <[email protected]> R: Krishna Reddy <[email protected]> L: [email protected] S: Supported +F: drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c F: drivers/iommu/arm/arm-smmu/arm-smmu-nvidia.c F: drivers/iommu/tegra*
drivers/iommu/Kconfig +11 -0
@@ -424,6 +424,17 @@ config ARM_SMMU_V3_KUNIT_TEST Enable this option to unit-test arm-smmu-v3 driver functions. If unsure, say N. + +config TEGRA241_CMDQV + bool "NVIDIA Tegra241 CMDQ-V extension support for ARM SMMUv3" + depends on ACPI + help + Support for NVIDIA CMDQ-Virtualization extension for ARM SMMUv3. The + CMDQ-V extension is similar to v3.3 ECMDQ for multi command queues + support, except with virtualization capabilities. + + Say Y here if your system is NVIDIA Tegra241 (Grace) or it has the same + CMDQ-V extension. endif config S390_IOMMU
drivers/iommu/arm/arm-smmu-v3/Makefile +1 -0
@@ -2,5 +2,6 @@ obj-$(CONFIG_ARM_SMMU_V3) += arm_smmu_v3.o arm_smmu_v3-y := arm-smmu-v3.o arm_smmu_v3-$(CONFIG_ARM_SMMU_V3_SVA) += arm-smmu-v3-sva.o +arm_smmu_v3-$(CONFIG_TEGRA241_CMDQV) += tegra241-cmdqv.o obj-$(CONFIG_ARM_SMMU_V3_KUNIT_TEST) += arm-smmu-v3-test.o
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +9 -0
@@ -636,10 +636,19 @@ struct arm_smmu_impl {struct arm_smmu_cmdq *(*get_secondary_cmdq)(struct arm_smmu_device *smmu); }; +#ifdef CONFIG_TEGRA241_CMDQV +struct arm_smmu_device * +tegra241_cmdqv_acpi_dsdt_probe (struct arm_smmu_device *smmu, +struct acpi_iort_node *node); +#endif +static inline struct arm_smmu_device *arm_smmu_impl_acpi_dsdt_probe (struct arm_smmu_device *smmu,struct acpi_iort_node *node) { +#ifdef CONFIG_TEGRA241_CMDQV + smmu =tegra241_cmdqv_acpi_dsdt_probe (smmu, node); +#endif return smmu; }
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +868 -0
868 lines not on this page · raw message
Signature · 1 line
2.43.0
Re: [PATCH v11 8/9] iommu/arm-smmu-v3: Add in-kernel support for NVIDIA Tegra241 (Grace) CMDQV
Quoted 18 lines · Nicolin Chen
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +18
-0+static struct arm_smmu_device * +tegra241_cmdqv_probe (struct arm_smmu_device *smmu, +struct resource *res,int irq) +{ +struct tegra241_cmdqv *cmdqv =NULL ; +struct tegra241_vintf *vintf; +void __iomem *base; +u32 regval; +int lidx; +int ret; + + base =ioremap (res->start,resource_size (res)); +if (IS_ERR (base)) { +dev_err (smmu->dev,"failed to ioremap: %ld\n" ,PTR_ERR (base)); + goto iounmap; + } + + regval =readl (base +TEGRA241_CMDQV_CONFIG );
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c line 774
Quoted 10 lines
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +10
-0+if (disable_cmdqv) { +dev_info (smmu->dev,"Detected disable_cmdqv=true\n" ); +writel (regval & ~CMDQV_EN , base +TEGRA241_CMDQV_CONFIG ); + goto iounmap; + } + + cmdqv =devm_krealloc (smmu->dev, smmu,sizeof (*cmdqv),GFP_KERNEL ); +if (!cmdqv) + goto iounmap; + smmu = &cmdqv->smmu;
Should stick a:
static_assert(offsetof(struct tegra241_cmdqv,smmu) == 0);
In here. The copy inside the krealloc won't work otherwise.
But this still seems Ok to me with the new ops
Jason
Re: [PATCH v11 8/9] iommu/arm-smmu-v3: Add in-kernel support for NVIDIA Tegra241 (Grace) CMDQV
Quoted 33 lines · Jason Gunthorpe
Quoted 18 lines · Nicolin Chen
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +18
-0+static struct arm_smmu_device * +tegra241_cmdqv_probe (struct arm_smmu_device *smmu, +struct resource *res,int irq) +{ +struct tegra241_cmdqv *cmdqv =NULL ; +struct tegra241_vintf *vintf; +void __iomem *base; +u32 regval; +int lidx; +int ret; + + base =ioremap (res->start,resource_size (res)); +if (IS_ERR (base)) { +dev_err (smmu->dev,"failed to ioremap: %ld\n" ,PTR_ERR (base)); + goto iounmap; + } + + regval =readl (base +TEGRA241_CMDQV_CONFIG );drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c line 774
Quoted 10 lines
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +10
-0+if (disable_cmdqv) { +dev_info (smmu->dev,"Detected disable_cmdqv=true\n" ); +writel (regval & ~CMDQV_EN , base +TEGRA241_CMDQV_CONFIG ); + goto iounmap; + } + + cmdqv =devm_krealloc (smmu->dev, smmu,sizeof (*cmdqv),GFP_KERNEL ); +if (!cmdqv) + goto iounmap; + smmu = &cmdqv->smmu;Should stick a:
Quoted 1 line
static_assert(offsetof(struct tegra241_cmdqv,smmu) == 0);
Done.
Thanks!
Nicolin
Quoted 5 lines
In here. The copy inside the krealloc won't work otherwise.
But this still seems Ok to me with the new ops
Jason
Will Deacon
maintainerRe: [PATCH v11 8/9] iommu/arm-smmu-v3: Add in-kernel support for NVIDIA Tegra241 (Grace) CMDQV
In reply to Nicolin ChenQuoted 40 lines · Nicolin Chen
From: Nate Watterson <[email protected]>
NVIDIA's Tegra241 Soc has a CMDQ-Virtualization (CMDQV) hardware, extending the standard ARM SMMU v3 IP to support multiple VCMDQs with virtualization capabilities. In terms of command queue, they are very like a standard SMMU CMDQ (or ECMDQs), but only support
CS_NONEin the CS field ofCMD_SYNC.Add a new tegra241-cmdqv driver, and insert its structure pointer into the existing
arm_smmu_device, and then add related function calls in the SMMUv3 driver to interact with the CMDQV driver.In the CMDQV driver, add a minimal part for the in-kernel support: reserve VINTF0 for in-kernel use, and assign some of the VCMDQs to the VINTF0, and select one VCMDQ based on the current CPU ID to execute supported commands. This multi-queue design for in-kernel use gives some limited improvements:
up to 20% reduction of invalidation time was measured by a multi-threaded DMA unmap benchmark, compared to a single queue.The other part of the CMDQV driver will be user-space support that gives a hypervisor running on the host OS to talk to the driver for virtualization use cases, allowing VMs to use VCMDQs without trappings, i.e. no VM Exits. This is designed based on IOMMUFD, and its RFC series is also under review. It will provide a guest OS a bigger improvement: 70% to 90% reductions of TLB invalidation time were measured by DMA unmap tests running in a guest, compared to nested SMMU CMDQ (with trappings).
As the initial version, the CMDQV driver only supports ACPI configurations.
Signed-off-by Nate Watterson <[email protected]>Reviewed-by Jason Gunthorpe <[email protected]>Co-developed-by Nicolin Chen <[email protected]>Signed-off-by Nicolin Chen <[email protected]>--- MAINTAINERS | 1 + drivers/iommu/Kconfig | 11 + drivers/iommu/arm/arm-smmu-v3/Makefile | 1 + drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 9 + .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 868 ++++++++++++++++++ 5 files changed, 890 insertions(+) create mode 100644 drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
[...]
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c line 863
Quoted 11 lines
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +11
-0+struct arm_smmu_device * +tegra241_cmdqv_acpi_dsdt_probe (struct arm_smmu_device *smmu, +struct acpi_iort_node *node) +{ +struct resource *res; +int irq; + +/* Keep the pointer smmu intact if !res */ + res =tegra241_cmdqv_find_acpi_resource (smmu, node, &irq); +if (!res) +return smmu;
Given that this probing code will end up running on non-tegra hardware when CONFIG_TEGRA241_CMDQV is enabled, please can you move the common part into the main driver?
Will
Re: [PATCH v11 8/9] iommu/arm-smmu-v3: Add in-kernel support for NVIDIA Tegra241 (Grace) CMDQV
Quoted 17 lines · Will Deacon
[...]
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c line 863
Quoted 11 lines
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +11
-0+struct arm_smmu_device * +tegra241_cmdqv_acpi_dsdt_probe (struct arm_smmu_device *smmu, +struct acpi_iort_node *node) +{ +struct resource *res; +int irq; + +/* Keep the pointer smmu intact if !res */ + res =tegra241_cmdqv_find_acpi_resource (smmu, node, &irq); +if (!res) +return smmu;Given that this probing code will end up running on non-tegra hardware when
CONFIG_TEGRA241_CMDQVis enabled, please can you move the common part into the main driver?
You mean moving the tegra241_cmdqv_acpi_dsdt_probe() into the main driver? I think this v11 puts a function call int he header already.
Also, would you please take a look at v12 where there is a little change cooperating Jason's remarks?
https://lore.kernel.org/linux-iommu/c342c29e51ce3a14c30bd1145a933f415a55b85f.1723754745.git.nicolinc@nvidia.com/
Thanks!
Nicolin
Will Deacon
maintainerRe: [PATCH v11 8/9] iommu/arm-smmu-v3: Add in-kernel support for NVIDIA Tegra241 (Grace) CMDQV
Quoted 20 lines · Nicolin Chen
Quoted 17 lines · Will Deacon
[...]
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c line 863
Quoted 11 lines
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +11
-0+struct arm_smmu_device * +tegra241_cmdqv_acpi_dsdt_probe (struct arm_smmu_device *smmu, +struct acpi_iort_node *node) +{ +struct resource *res; +int irq; + +/* Keep the pointer smmu intact if !res */ + res =tegra241_cmdqv_find_acpi_resource (smmu, node, &irq); +if (!res) +return smmu;Given that this probing code will end up running on non-tegra hardware when
CONFIG_TEGRA241_CMDQVis enabled, please can you move the common part into the main driver?
Quoted 2 lines
You mean moving the
tegra241_cmdqv_acpi_dsdt_probe()into the main driver? I think this v11 puts a function call int he header already.
Right. I basically want it so that !tegra folks don't have to open the tegra-specific file to look at the code running on their SoC.
We should also running that code at all on implementations which _do_ advertise a distinct model number in IORT.
Will
Re: [PATCH v11 8/9] iommu/arm-smmu-v3: Add in-kernel support for NVIDIA Tegra241 (Grace) CMDQV
Quoted 21 lines · Will Deacon
Quoted 20 lines · Nicolin Chen
Quoted 17 lines · Will Deacon
[...]
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c line 863
Quoted 11 lines
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +11
-0+struct arm_smmu_device * +tegra241_cmdqv_acpi_dsdt_probe (struct arm_smmu_device *smmu, +struct acpi_iort_node *node) +{ +struct resource *res; +int irq; + +/* Keep the pointer smmu intact if !res */ + res =tegra241_cmdqv_find_acpi_resource (smmu, node, &irq); +if (!res) +return smmu;Given that this probing code will end up running on non-tegra hardware when
CONFIG_TEGRA241_CMDQVis enabled, please can you move the common part into the main driver?
Quoted 8 lines
Quoted 2 lines
You mean moving the
tegra241_cmdqv_acpi_dsdt_probe()into the main driver? I think this v11 puts a function call int he header already.Right. I basically want it so that !tegra folks don't have to open the tegra-specific file to look at the code running on their SoC.
We should also running that code at all on implementations which _do_ advertise a distinct model number in IORT.
Done. I did my best to incorporate that and sent v13. Please check.
Thanks
Nicolin
PATCH v11 9/9 iommu/tegra241-cmdqv: Limit CMDs for guest owned VINTF
In reply to Nicolin ChenWhen VCMDQs are assigned to a VINTF owned by a guest (HYP_OWN bit unset), only TLB and ATC invalidation commands are supported by the VCMDQ HW. So, add a new helper to scan the input cmd to make sure it is supported when selecting a queue, though this assumes that SMMUv3 driver will only add the same type of commands into an arm_smmu_cmdq_batch as it does today.
Note that the guest VM shouldn't have HYP_OWN bit being set regardless of guest kernel driver writing it or not, i.e. the hypervisor running in the host OS should wire this bit to zero when trapping a write access to this VINTF_CONFIG register from a guest kernel.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 22 +++++++----- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 3 +- .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 35 ++++++++++++++++++- 3 files changed, 49 insertions(+), 11 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +13 -9
@@ -336,12 +336,13 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent)return 0 ; } -static struct arm_smmu_cmdq *arm_smmu_get_cmdq (struct arm_smmu_device *smmu) +static struct arm_smmu_cmdq *arm_smmu_get_cmdq (struct arm_smmu_device *smmu, +u8 opcode) {struct arm_smmu_cmdq *cmdq =NULL ;if (smmu->impl && smmu->impl->get_secondary_cmdq) - cmdq = smmu->impl->get_secondary_cmdq (smmu); + cmdq = smmu->impl->get_secondary_cmdq (smmu, opcode);return cmdq ?: &smmu->cmdq; }
@@ -889,7 +890,7 @@ static int __arm_smmu_cmdq_issue_cmd(struct arm_smmu_device *smmu, }return arm_smmu_cmdq_issue_cmdlist ( - smmu,arm_smmu_get_cmdq (smmu), cmd,1 , sync); + smmu,arm_smmu_get_cmdq (smmu, ent->opcode), cmd,1 , sync); } static int arm_smmu_cmdq_issue_cmd(struct arm_smmu_device *smmu,
@@ -905,10 +906,13 @@ static int arm_smmu_cmdq_issue_cmd_with_sync(struct arm_smmu_device *smmu, }static void arm_smmu_cmdq_batch_init (struct arm_smmu_device *smmu, -struct arm_smmu_cmdq_batch *cmds) +struct arm_smmu_cmdq_batch *cmds, +u8 opcode) { +WARN_ON_ONCE (!opcode); + cmds->num =0 ; - cmds->cmdq =arm_smmu_get_cmdq (smmu); + cmds->cmdq =arm_smmu_get_cmdq (smmu, opcode); } static void arm_smmu_cmdq_batch_add(struct arm_smmu_device *smmu,
@@ -1195,7 +1199,7 @@ static void arm_smmu_sync_cd(struct arm_smmu_master *master, }, }; -arm_smmu_cmdq_batch_init (smmu, &cmds); +arm_smmu_cmdq_batch_init (smmu, &cmds,CMDQ_OP_CFGI_CD );for (i =0 ; i < master->num_streams; i++) { cmd.cfgi.sid = master->streams[i].id;arm_smmu_cmdq_batch_add (smmu, &cmds, &cmd);
@@ -2046,7 +2050,7 @@ static int arm_smmu_atc_inv_master(struct arm_smmu_master *master,arm_smmu_atc_inv_to_cmd (ssid,0 ,0 , &cmd); -arm_smmu_cmdq_batch_init (master->smmu, &cmds); +arm_smmu_cmdq_batch_init (master->smmu, &cmds,CMDQ_OP_ATC_INV );for (i =0 ; i < master->num_streams; i++) { cmd.atc.sid = master->streams[i].id;arm_smmu_cmdq_batch_add (master->smmu, &cmds, &cmd);
@@ -2084,7 +2088,7 @@ int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain,if (!atomic_read (&smmu_domain->nr_ats_masters))return 0 ; -arm_smmu_cmdq_batch_init (smmu_domain->smmu, &cmds); +arm_smmu_cmdq_batch_init (smmu_domain->smmu, &cmds,CMDQ_OP_ATC_INV );spin_lock_irqsave (&smmu_domain->devices_lock, flags); list_for_each_entry(master_domain, &smmu_domain->devices,
@@ -2166,7 +2170,7 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd, num_pages++; } -arm_smmu_cmdq_batch_init (smmu, &cmds); +arm_smmu_cmdq_batch_init (smmu, &cmds, cmd->opcode);while (iova < end) {if (smmu->features &ARM_SMMU_FEAT_RANGE_INV ) {
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +2 -1
@@ -633,7 +633,8 @@ struct arm_smmu_strtab_cfg {struct arm_smmu_impl {int (*device_reset)(struct arm_smmu_device *smmu);void (*device_remove)(struct arm_smmu_device *smmu); -struct arm_smmu_cmdq *(*get_secondary_cmdq)(struct arm_smmu_device *smmu); +struct arm_smmu_cmdq *(*get_secondary_cmdq)(struct arm_smmu_device *smmu, +u8 opcode); };#ifdef CONFIG_TEGRA241_CMDQV
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +34 -1
@@ -144,6 +144,7 @@ struct tegra241_vcmdq {* struct tegra241_vintf - Virtual Interface * @idx: Global index in the CMDQV * @enabled: Enable status +* @hyp_own: Owned by hypervisor (in-kernel) * @cmdqv: Parent CMDQV pointer * @lvcmdqs: List of logical VCMDQ pointers * @base: MMIO base address
@@ -152,6 +153,7 @@ struct tegra241_vintf {u16 idx;bool enabled; +bool hyp_own;struct tegra241_cmdqv *cmdqv;struct tegra241_vcmdq **lvcmdqs;
@@ -301,8 +303,25 @@ static irqreturn_t tegra241_cmdqv_isr(int irq, void *devid)/* Command Queue Function */ +static bool tegra241_vintf_support_cmd (struct tegra241_vintf *vintf,u8 opcode) +{ +/* Hypervisor-owned VINTF can execute any command in its VCMDQs */ +if (READ_ONCE (vintf->hyp_own)) +return true; + +/* Guest-owned VINTF must check against the list of supported CMDs */ +switch (opcode) { +case CMDQ_OP_TLBI_NH_ASID : +case CMDQ_OP_TLBI_NH_VA : +case CMDQ_OP_ATC_INV : +return true; +default : +return false; + } +} +static struct arm_smmu_cmdq * -tegra241_cmdqv_get_cmdq (struct arm_smmu_device *smmu) +tegra241_cmdqv_get_cmdq (struct arm_smmu_device *smmu,u8 opcode) {struct tegra241_cmdqv *cmdqv = container_of(smmu, struct tegra241_cmdqv, smmu);
@@ -317,6 +336,10 @@ tegra241_cmdqv_get_cmdq(struct arm_smmu_device *smmu)if (!READ_ONCE (vintf->enabled))return NULL ; +/* Unsupported CMD go for smmu->cmdq pathway */ +if (!tegra241_vintf_support_cmd (vintf, opcode)) +return NULL ; +/* * Select a LVCMDQ to use. Here we use a temporal solution to * balance out traffic on cmdq issuing: each cmdq has its own
@@ -406,12 +429,22 @@ static int tegra241_vintf_hw_init(struct tegra241_vintf *vintf, bool hyp_own)tegra241_vintf_hw_deinit (vintf);/* Configure and enable VINTF */ +/* +* Note that HYP_OWN bit is wired to zero when running in guest kernel, +* whether enabling it here or not, as !HYP_OWN cmdq HWs only support a +* restricted set of supported commands. +*/ regval =FIELD_PREP (VINTF_HYP_OWN , hyp_own);writel (regval,REG_VINTF (vintf,CONFIG )); ret =vintf_write_config (vintf, regval |VINTF_EN );if (ret)return ret; +/* +* As being mentioned above, HYP_OWN bit is wired to zero for a guest +* kernel, so read it back from HW to ensure that reflects in hyp_own +*/ + vintf->hyp_own = !!(VINTF_HYP_OWN &readl (REG_VINTF (vintf,CONFIG )));for (lidx =0 ; lidx < vintf->cmdqv->num_lvcmdqs_per_vintf; lidx++) {if (vintf->lvcmdqs && vintf->lvcmdqs[lidx]) {
Signature · 1 line
2.43.0
Will Deacon
maintainerRe: [PATCH v11 9/9] iommu/tegra241-cmdqv: Limit CMDs for guest owned VINTF
Quoted 49 lines · Nicolin Chen
When VCMDQs are assigned to a VINTF owned by a guest (
HYP_OWNbit unset), only TLB and ATC invalidation commands are supported by the VCMDQ HW. So, add a new helper to scan the input cmd to make sure it is supported when selecting a queue, though this assumes that SMMUv3 driver will only add the same type of commands into anarm_smmu_cmdq_batchas it does today.Note that the guest VM shouldn't have
HYP_OWNbit being set regardless of guest kernel driver writing it or not, i.e. the hypervisor running in the host OS should wire this bit to zero when trapping a write access to thisVINTF_CONFIGregister from a guest kernel.Reviewed-by Jason Gunthorpe <[email protected]>Signed-off-by Nicolin Chen <[email protected]>--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 22 +++++++----- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 3 +- .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 35 ++++++++++++++++++- 3 files changed, 49 insertions(+), 11 deletions(-)drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +4
-3@@ -336,12 +336,13 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent)return 0 ; } -static struct arm_smmu_cmdq *arm_smmu_get_cmdq (struct arm_smmu_device *smmu) +static struct arm_smmu_cmdq *arm_smmu_get_cmdq (struct arm_smmu_device *smmu, +u8 opcode) {struct arm_smmu_cmdq *cmdq =NULL ;if (smmu->impl && smmu->impl->get_secondary_cmdq) - cmdq = smmu->impl->get_secondary_cmdq (smmu); + cmdq = smmu->impl->get_secondary_cmdq (smmu, opcode);return cmdq ?: &smmu->cmdq; }@@ -889,7 +890,7 @@ static int __arm_smmu_cmdq_issue_cmd(struct arm_smmu_device *smmu, }return arm_smmu_cmdq_issue_cmdlist ( - smmu,arm_smmu_get_cmdq (smmu), cmd,1 , sync); + smmu,arm_smmu_get_cmdq (smmu, ent->opcode), cmd,1 , sync); } static int arm_smmu_cmdq_issue_cmd(struct arm_smmu_device *smmu,
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static int arm_smmu_cmdq_issue_cmd_with_sync(struct arm_smmu_device *smmu, · line 912
Quoted 8 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +3
-1@@ -905,10 +906,13 @@ static int arm_smmu_cmdq_issue_cmd_with_sync(struct arm_smmu_device *smmu, }static void arm_smmu_cmdq_batch_init (struct arm_smmu_device *smmu, -struct arm_smmu_cmdq_batch *cmds) +struct arm_smmu_cmdq_batch *cmds, +u8 opcode) { +WARN_ON_ONCE (!opcode);
This seems like a fairly arbitrary warning. Remove it?
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static int arm_smmu_cmdq_issue_cmd_with_sync(struct arm_smmu_device *smmu, · line 915
Quoted 4 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +2
-1+ cmds->num =0 ; - cmds->cmdq =arm_smmu_get_cmdq (smmu); + cmds->cmdq =arm_smmu_get_cmdq (smmu, opcode);
If we stashed the opcode here, we could actually just enforce that all commands in the batch are the same type in arm_smmu_cmdq_batch_add().
Would that work better for you or not?
Will
Re: [PATCH v11 9/9] iommu/tegra241-cmdqv: Limit CMDs for guest owned VINTF
Quoted 8 lines · Will Deacon
Review
Quoted 6 lines
static void arm_smmu_cmdq_batch_init(struct
arm_smmu_device*smmu,(quoted diff) +3
-1- struct arm_smmu_cmdq_batch *cmds) + struct arm_smmu_cmdq_batch *cmds, + u8 opcode) { + WARN_ON_ONCE(!opcode);This seems like a fairly arbitrary warning. Remove it?
OK.
Quoted 9 lines
Review
Quoted 4 lines
(quoted diff) +2
-1+ cmds->num = 0; - cmds->cmdq = arm_smmu_get_cmdq(smmu); + cmds->cmdq = arm_smmu_get_cmdq(smmu, opcode);If we stashed the opcode here, we could actually just enforce that all commands in the batch are the same type in
arm_smmu_cmdq_batch_add().Would that work better for you or not?
A guested-owned queue is okay to mix different command types:
CMDQ_OP_TLBI_NH_ASID
CMDQ_OP_TLBI_NH_VA
CMDQ_OP_ATC_INV
So, limiting a batch to one single opcode isn't ideal. Instead, if we really have to apply an enforcement to every batch_add(), I think the cmdq structure would need a scan function pointer:
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +11 -2
@@ -918,2 +918,10 @@ static void arm_smmu_cmdq_batch_init(struct arm_smmu_device *smmu, +static bool arm_smmu_cmdq_supports_cmd (struct arm_smmu_cmdq *cmdq, +struct arm_smmu_cmdq_ent *ent) +{ +if (!cmdq->supports_cmd) +return true; +return cmdq->supports_cmd (ent); +} + static void arm_smmu_cmdq_batch_add(struct arm_smmu_device *smmu,
@@ -924,4 +932,5 @@ static void arm_smmu_cmdq_batch_add(struct arm_smmu_device *smmu, -if (cmds->num ==CMDQ_BATCH_ENTRIES -1 && - (smmu->options &ARM_SMMU_OPT_CMDQ_FORCE_SYNC )) { +if ((cmds->num ==CMDQ_BATCH_ENTRIES -1 && + (smmu->options &ARM_SMMU_OPT_CMDQ_FORCE_SYNC )) || + !arm_smmu_cmdq_supports_cmd (cmds->cmdq, cmd)) { arm_smmu_cmdq_issue_cmdlist(smmu, cmds->cmdq, cmds->cmds,
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +1 -0
@@ -616,2 +616,3 @@ struct arm_smmu_cmdq {atomic_t lock; +bool (*supports_cmd)(struct arm_smmu_cmdq_ent *ent); };
That being said, the whole thing doesn't seem to have a lot value at this moment, since the SMMU driver doesn't mix commands?
Thanks
Nicolin
Re: [PATCH v11 9/9] iommu/tegra241-cmdqv: Limit CMDs for guest owned VINTF
Quoted 48 lines · Nicolin Chen
Quoted 8 lines · Will Deacon
Review
Quoted 6 lines
static void arm_smmu_cmdq_batch_init(struct
arm_smmu_device*smmu,(quoted diff) +3
-1- struct arm_smmu_cmdq_batch *cmds) + struct arm_smmu_cmdq_batch *cmds, + u8 opcode) { + WARN_ON_ONCE(!opcode);This seems like a fairly arbitrary warning. Remove it?
OK.
Quoted 9 lines
Review
Quoted 4 lines
(quoted diff) +2
-1+ cmds->num = 0; - cmds->cmdq = arm_smmu_get_cmdq(smmu); + cmds->cmdq = arm_smmu_get_cmdq(smmu, opcode);If we stashed the opcode here, we could actually just enforce that all commands in the batch are the same type in
arm_smmu_cmdq_batch_add().Would that work better for you or not?
A guested-owned queue is okay to mix different command types:
CMDQ_OP_TLBI_NH_ASID
CMDQ_OP_TLBI_NH_VA
CMDQ_OP_ATC_INVSo, limiting a batch to one single opcode isn't ideal. Instead, if we really have to apply an enforcement to every
batch_add(), I think the cmdq structure would need a scan function pointer:drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +8
-0@@ -918,2 +918,10 @@ static void arm_smmu_cmdq_batch_init(struct arm_smmu_device *smmu, +static bool arm_smmu_cmdq_supports_cmd (struct arm_smmu_cmdq *cmdq, +struct arm_smmu_cmdq_ent *ent) +{ +if (!cmdq->supports_cmd) +return true; +return cmdq->supports_cmd (ent); +} + static void arm_smmu_cmdq_batch_add(struct arm_smmu_device *smmu,
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void arm_smmu_cmdq_batch_add(struct arm_smmu_device *smmu, · line 936
Quoted 7 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +3
-2@@ -924,4 +932,5 @@ static void arm_smmu_cmdq_batch_add(struct arm_smmu_device *smmu, -if (cmds->num ==CMDQ_BATCH_ENTRIES -1 && - (smmu->options &ARM_SMMU_OPT_CMDQ_FORCE_SYNC )) { +if ((cmds->num ==CMDQ_BATCH_ENTRIES -1 && + (smmu->options &ARM_SMMU_OPT_CMDQ_FORCE_SYNC )) || + !arm_smmu_cmdq_supports_cmd (cmds->cmdq, cmd)) { arm_smmu_cmdq_issue_cmdlist(smmu, cmds->cmdq, cmds->cmds,
We'd need re-init the batch after this too..
Nicolin
Quoted 14 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +1
-0@@ -616,2 +616,3 @@ struct arm_smmu_cmdq {atomic_t lock; +bool (*supports_cmd)(struct arm_smmu_cmdq_ent *ent); };That being said, the whole thing doesn't seem to have a lot value at this moment, since the SMMU driver doesn't mix commands?
Thanks
Nicolin
Re: [PATCH v11 9/9] iommu/tegra241-cmdqv: Limit CMDs for guest owned VINTF
Quoted 70 lines · Nicolin Chen
Quoted 48 lines · Nicolin Chen
Quoted 8 lines · Will Deacon
Review
Quoted 6 lines
static void arm_smmu_cmdq_batch_init(struct
arm_smmu_device*smmu,(quoted diff) +3
-1- struct arm_smmu_cmdq_batch *cmds) + struct arm_smmu_cmdq_batch *cmds, + u8 opcode) { + WARN_ON_ONCE(!opcode);This seems like a fairly arbitrary warning. Remove it?
OK.
Quoted 9 lines
Review
Quoted 4 lines
(quoted diff) +2
-1+ cmds->num = 0; - cmds->cmdq = arm_smmu_get_cmdq(smmu); + cmds->cmdq = arm_smmu_get_cmdq(smmu, opcode);If we stashed the opcode here, we could actually just enforce that all commands in the batch are the same type in
arm_smmu_cmdq_batch_add().Would that work better for you or not?
A guested-owned queue is okay to mix different command types:
CMDQ_OP_TLBI_NH_ASID
CMDQ_OP_TLBI_NH_VA
CMDQ_OP_ATC_INVSo, limiting a batch to one single opcode isn't ideal. Instead, if we really have to apply an enforcement to every
batch_add(), I think the cmdq structure would need a scan function pointer:drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +8
-0@@ -918,2 +918,10 @@ static void arm_smmu_cmdq_batch_init(struct arm_smmu_device *smmu, +static bool arm_smmu_cmdq_supports_cmd (struct arm_smmu_cmdq *cmdq, +struct arm_smmu_cmdq_ent *ent) +{ +if (!cmdq->supports_cmd) +return true; +return cmdq->supports_cmd (ent); +} + static void arm_smmu_cmdq_batch_add(struct arm_smmu_device *smmu,drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static void arm_smmu_cmdq_batch_add(struct arm_smmu_device *smmu, · line 936
Quoted 7 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +3
-2@@ -924,4 +932,5 @@ static void arm_smmu_cmdq_batch_add(struct arm_smmu_device *smmu, -if (cmds->num ==CMDQ_BATCH_ENTRIES -1 && - (smmu->options &ARM_SMMU_OPT_CMDQ_FORCE_SYNC )) { +if ((cmds->num ==CMDQ_BATCH_ENTRIES -1 && + (smmu->options &ARM_SMMU_OPT_CMDQ_FORCE_SYNC )) || + !arm_smmu_cmdq_supports_cmd (cmds->cmdq, cmd)) { arm_smmu_cmdq_issue_cmdlist(smmu, cmds->cmdq, cmds->cmds,We'd need re-init the batch after this too..
Nicolin
Quoted 9 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +1
-0@@ -616,2 +616,3 @@ struct arm_smmu_cmdq {atomic_t lock; +bool (*supports_cmd)(struct arm_smmu_cmdq_ent *ent); };
Quoted 2 lines
Quoted 2 lines
That being said, the whole thing doesn't seem to have a lot value at this moment, since the SMMU driver doesn't mix commands?
OK. I have added a patch for this. Let's just make things a bit perfect at once.
Here is a v13 branch that addressed most of your remarks here:
https://github.com/nicolinc/iommufd/commits/vcmdq_in_kernel-v13
Would you please let me know if you are okay with this?
Thank you
Nicolin
Re: [PATCH v11 9/9] iommu/tegra241-cmdqv: Limit CMDs for guest owned VINTF
Quoted 10 lines · Nicolin Chen
Quoted 10 lines
Quoted 10 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +1
-0@@ -616,2 +616,3 @@ struct arm_smmu_cmdq {atomic_t lock; +bool (*supports_cmd)(struct arm_smmu_cmdq_ent *ent); };
Quoted 8 lines
Quoted 2 lines
Quoted 2 lines
That being said, the whole thing doesn't seem to have a lot value at this moment, since the SMMU driver doesn't mix commands?
OK. I have added a patch for this. Let's just make things a bit perfect at once.
Here is a v13 branch that addressed most of your remarks here:
https://github.com/nicolinc/iommufd/commits/vcmdq_in_kernel-v13
It seems reasonable to me, it is sort of going back to my original suggestion to pass in some kind of flag to say which commands were going to be in the batch. Since all batches are simple op code right now this seems good enough for today!
Jason