Add Tegra241 (Grace) CMDQV Support (part 1/2)
Subsystems (4)
v7 cover Add Tegra241 (Grace) CMDQV Support (part 1/2)
NVIDIA's Tegra241 (Grace) SoC has a CMDQ-Virtualization (CMDQV) hardware that extends standard ARM SMMUv3 to support multiple command queues with virtualization capabilities. Though this is similar to the ECMDQ in SMMU v3.3, CMDQV provides additional Virtual Interfaces (VINTFs) allowing VMs to have their own VINTFs and Virtual Command Queues (VCMDQs). The VCMDQs can only execute a limited set of commands, mainly invalidation commands when exclusively used by the VMs, compared to the standard SMMUv3 CMDQ.
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-v7
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
v7:
* 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 (5):
iommu/arm-smmu-v3: Make symbols public for CONFIG_TEGRA241_CMDQV 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: Add CS_NONE quirk for CONFIG_TEGRA241_CMDQV 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 | 164 ++-- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 67 +- .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 911 ++++++++++++++++++ 6 files changed, 1088 insertions(+), 67 deletions(-) create mode 100644 drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
Signature · 1 line
2.43.0
PATCH v7 1/6 iommu/arm-smmu-v3: Make symbols public for CONFIG_TEGRA241_CMDQV
The 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 | 20 +++++++++----------- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 9 +++++++++ 2 files changed, 18 insertions(+), 11 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +9 -11
@@ -366,8 +366,8 @@ static void arm_smmu_cmdq_build_sync_cmd(u64 *cmd, struct arm_smmu_device *smmu,arm_smmu_cmdq_build_cmd (cmd, &ent); } -static void __arm_smmu_cmdq_skip_err (struct arm_smmu_device *smmu, -struct arm_smmu_queue *q) +void __arm_smmu_cmdq_skip_err (struct arm_smmu_device *smmu, +struct arm_smmu_queue *q) {static const char *const cerror_str[] = { [CMDQ_ERR_CERROR_NONE_IDX ] ="No error" ,
@@ -3121,12 +3121,10 @@ static struct iommu_ops arm_smmu_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;
@@ -3164,9 +3162,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) +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 );
@@ -3191,7 +3189,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;
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +9 -0
@@ -794,6 +794,15 @@ bool arm_smmu_free_asid(struct arm_smmu_ctx_desc *cd);int arm_smmu_atc_inv_domain (struct arm_smmu_domain *smmu_domain,int ssid,unsigned long iova,size_t size); +void __arm_smmu_cmdq_skip_err (struct arm_smmu_device *smmu, +struct arm_smmu_queue *q); +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 v7 2/6 iommu/arm-smmu-v3: Issue a batch of commands to the same cmdq
In reply to Nicolin ChenThe 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 it 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
@@ -588,11 +588,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 ;/*
@@ -623,11 +623,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);
@@ -647,10 +647,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 ;
@@ -697,12 +697,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,
@@ -739,13 +740,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 ;
@@ -759,7 +760,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); }
@@ -835,7 +836,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",
@@ -870,7 +871,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,
@@ -885,6 +887,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)
@@ -893,12 +902,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 ; }
@@ -915,7 +926,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,
@@ -1156,7 +1168,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);
@@ -1995,7 +2007,7 @@ static int arm_smmu_atc_inv_master(struct arm_smmu_master *master)arm_smmu_atc_inv_to_cmd (IOMMU_NO_PASID ,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);
@@ -2035,7 +2047,7 @@ int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain, int ssid,arm_smmu_atc_inv_to_cmd (ssid, iova, size, &cmd); - 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, &smmu_domain->devices, domain_head) {
@@ -2112,7 +2124,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_domain->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
@@ -564,6 +564,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
Re: [PATCH v7 2/6] iommu/arm-smmu-v3: Issue a batch of commands to the same cmdq
Quoted 11 lines · Nicolin Chen
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_batchstructure, and decide the cmdq when initializing the batch. Pass it down to the bottom function. Update__arm_smmu_cmdq_issue_cmd()accordingly for single command issuers.
Quoted 6 lines
Suggested-by Jason Gunthorpe <[email protected]>Signed-off-by Nicolin Chen <[email protected]>--- 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(-)
Jason
PATCH v7 3/6 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 quirk for tegra241-cmdqv driver.
Note that this changes the type of CMD_SYNC in __arm_smmu_cmdq_skip_err, for ARM_SMMU_OPT_MSIPOLL=true cases, from previously a non-MSI one to an MSI one that is proven to still work by 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
@@ -325,16 +325,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 ; }
@@ -350,20 +340,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 , - }; +memset (cmd,0 ,1 <<CMDQ_ENT_SZ_SHIFT ); + 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 ; } void __arm_smmu_cmdq_skip_err(struct arm_smmu_device *smmu,
@@ -380,9 +373,6 @@ 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" );
@@ -416,7 +406,7 @@ 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
@@ -512,9 +512,6 @@ struct arm_smmu_cmdq_ent { } resume;#define CMDQ_OP_CMD_SYNC 0x46 -struct { -u64 msiaddr; - } sync; }; };
Signature · 1 line
2.43.0
Re: [PATCH v7 3/6] iommu/arm-smmu-v3: Enforce arm_smmu_cmdq_build_sync_cmd
Quoted 5 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_NONEquirk for tegra241-cmdqv driver.
Quoted 14 lines
Note that this changes the type of
CMD_SYNCin__arm_smmu_cmdq_skip_err, for ARM_SMMU_OPT_MSIPOLL=true cases, from previously a non-MSI one to an MSI one that is proven to still work by 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
Nice
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static struct arm_smmu_cmdq *arm_smmu_get_cmdq(struct arm_smmu_device *smmu) · line 343
Quoted 8 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +1
-3@@ -350,20 +340,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 , - }; +memset (cmd,0 ,1 <<CMDQ_ENT_SZ_SHIFT );
The command would also benifit from its own type someday :\
Maybe this should just be cmd[1] = 0 ?
Jason
Re: [PATCH v7 3/6] iommu/arm-smmu-v3: Enforce arm_smmu_cmdq_build_sync_cmd
Quoted 34 lines · Jason Gunthorpe
Quoted 5 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_NONEquirk for tegra241-cmdqv driver.Quoted 14 lines
Note that this changes the type of
CMD_SYNCin__arm_smmu_cmdq_skip_err, for ARM_SMMU_OPT_MSIPOLL=true cases, from previously a non-MSI one to an MSI one that is proven to still work by 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 p2Nice
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static struct arm_smmu_cmdq *arm_smmu_get_cmdq(struct arm_smmu_device *smmu) · line 343
Quoted 8 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +1
-3@@ -350,20 +340,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 , - }; +memset (cmd,0 ,1 <<CMDQ_ENT_SZ_SHIFT );The command would also benifit from its own type someday :\
Quoted 1 line
Maybe this should just be cmd[1] = 0 ?
Yes. I will add that.
Quoted 1 line
Reviewed-by Jason Gunthorpe <[email protected]>
Thanks!
Nicolin
PATCH v7 4/6 iommu/arm-smmu-v3: Add CS_NONE quirk for CONFIG_TEGRA241_CMDQV
In reply to Nicolin ChenThe CMDQV extension in NVIDIA Tegra241 SoC only supports CS_NONE in the CS field of CMD_SYNC. Add a quirk flag to accommodate that.
--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 8 +++++++- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 3 +++ 2 files changed, 10 insertions(+), 1 deletion(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +7 -1
@@ -345,6 +345,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 (q->quirks &CMDQ_QUIRK_SYNC_CS_NONE_ONLY ) { + 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 ;
@@ -690,7 +695,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 && + !(cmdq->q.quirks &CMDQ_QUIRK_SYNC_CS_NONE_ONLY ))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 +3 -0
@@ -543,6 +543,9 @@ struct arm_smmu_queue {u32 __iomem *prod_reg;u32 __iomem *cons_reg; + +#define CMDQ_QUIRK_SYNC_CS_NONE_ONLY BIT(0)/* CMD_SYNC CS field supports CS_NONE only */ +u32 quirks; };struct arm_smmu_queue_poll {
Signature · 1 line
2.43.0
PATCH v7 5/6 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).
However, it is very important for this in-kernel support to get merged and installed to VMs running on Grace-powered servers as soon as possible. So, later those servers would only need to upgrade their host kernels for the user-space support.
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.c | 52 +- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 50 + .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 877 ++++++++++++++++++ 6 files changed, 980 insertions(+), 12 deletions(-) create mode 100644 drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
MAINTAINERS +1 -0
@@ -21740,6 +21740,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
@@ -423,6 +423,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
@@ -3,4 +3,5 @@ obj-$(CONFIG_ARM_SMMU_V3) += arm_smmu_v3.o arm_smmu_v3-objs-y += arm-smmu-v3.o arm_smmu_v3-objs-$(CONFIG_ARM_SMMU_V3_SVA) += arm-smmu-v3-sva.o arm_smmu_v3-objs-$(CONFIG_ARM_SMMU_V3_KUNIT_TEST) += arm-smmu-v3-test.o +arm_smmu_v3-objs-$(CONFIG_TEGRA241_CMDQV) += tegra241-cmdqv.o arm_smmu_v3-objs := $(arm_smmu_v3-objs-y)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +40 -12
123 lines not on this page · raw message
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +50 -0
66 lines not on this page · raw message
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +877 -0
877 lines not on this page · raw message
Signature · 1 line
2.43.0
Re: [PATCH v7 5/6] iommu/arm-smmu-v3: Add in-kernel support for NVIDIA Tegra241 (Grace) CMDQV
Quoted 36 lines · Nicolin Chen
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +36
-0+/* MMIO helpers */ +#define cmdqv_readl (_cmdqv, _regname) \ + readl((_cmdqv)->base + TEGRA241_CMDQV_##_regname) +#define cmdqv_readl_relaxed (_cmdqv, _regname) \ + readl_relaxed((_cmdqv)->base + TEGRA241_CMDQV_##_regname) +#define cmdqv_readq_relaxed (_cmdqv, _regname) \ + readq_relaxed((_cmdqv)->base + TEGRA241_CMDQV_##_regname) +#define cmdqv_writel (_cmdqv, val, _regname) \ + writel((val), (_cmdqv)->base + TEGRA241_CMDQV_##_regname) +#define cmdqv_writel_relaxed (_cmdqv, val, _regname) \ + writel_relaxed((val), (_cmdqv)->base + TEGRA241_CMDQV_##_regname) + +#define vintf_readl (_vintf, _regname) \ + readl((_vintf)->base + TEGRA241_VINTF_##_regname) +#define vintf_readq_relaxed (_vintf, _regname) \ + readq_relaxed((_vintf)->base + TEGRA241_VINTF_##_regname) +#define vintf_readl_relaxed (_vintf, _regname) \ + readl_relaxed((_vintf)->base + TEGRA241_VINTF_##_regname) +#define vintf_writel (_vintf, val, _regname) \ + writel((val), (_vintf)->base + TEGRA241_VINTF_##_regname) +#define vintf_writel_relaxed (_vintf, val, _regname) \ + writel_relaxed((val), (_vintf)->base + TEGRA241_VINTF_##_regname) + +#define vcmdq_page0_readl (_vcmdq, _regname) \ + readl((_vcmdq)->page0 + TEGRA241_VCMDQ_##_regname) +#define vcmdq_page0_readl_relaxed (_vcmdq, _regname) \ + readl_relaxed((_vcmdq)->page0 + TEGRA241_VCMDQ_##_regname) +#define vcmdq_page0_writel (_vcmdq, val, _regname) \ + writel((val), (_vcmdq)->page0 + TEGRA241_VCMDQ_##_regname) +#define vcmdq_page0_writel_relaxed (_vcmdq, val, _regname) \ + writel_relaxed((val), (_vcmdq)->page0 + TEGRA241_VCMDQ_##_regname) + +#define vcmdq_page1_readl (_vcmdq, reg) \ + readl((_vcmdq)->page1 + TEGRA241_VCMDQ_##reg) +#define vcmdq_page1_readl_relaxed (_vcmdq, reg) \ + readl_relaxed((_vcmdq)->page1 + TEGRA241_VCMDQ_##reg)
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c line 142
Quoted 10 lines
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +10
-0+#define vcmdq_page1_readq_relaxed (_vcmdq, reg) \ + readq_relaxed((_vcmdq)->page1 + TEGRA241_VCMDQ_##reg) +#define vcmdq_page1_writel (_vcmdq, val, reg) \ + writel((val), (_vcmdq)->page1 + TEGRA241_VCMDQ_##reg) +#define vcmdq_page1_writel_relaxed (_vcmdq, val, reg) \ + writel_relaxed((val), (_vcmdq)->page1 + TEGRA241_VCMDQ_##reg) +#define vcmdq_page1_writeq (_vcmdq, val, reg) \ + writeq((val), (_vcmdq)->page1 + TEGRA241_VCMDQ_##reg) +#define vcmdq_page1_writeq_relaxed (_vcmdq, val, reg) \ + writeq_relaxed((val), (_vcmdq)->page1 + TEGRA241_VCMDQ_##reg)
These still need to be trimmed to only the ones being used. I still think it is a bad idea, maybe a middle ground is to wrapper the regsiter coding
writeq_relaxed(val, REG_CMDQ_PAGE1(vcmdq, XXX));
Is still short enough and safe enough without creating so much obfuscation..
I didn't notice anything else in this patch
Jason
Re: [PATCH v7 5/6] iommu/arm-smmu-v3: Add in-kernel support for NVIDIA Tegra241 (Grace) CMDQV
Quoted 20 lines · Jason Gunthorpe
Quoted 4 lines · Nicolin Chen
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +4
-0+#define vcmdq_page1_readl (_vcmdq, reg) \ + readl((_vcmdq)->page1 + TEGRA241_VCMDQ_##reg) +#define vcmdq_page1_readl_relaxed (_vcmdq, reg) \ + readl_relaxed((_vcmdq)->page1 + TEGRA241_VCMDQ_##reg)drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c line 142
Quoted 10 lines
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c new file +10
-0+#define vcmdq_page1_readq_relaxed (_vcmdq, reg) \ + readq_relaxed((_vcmdq)->page1 + TEGRA241_VCMDQ_##reg) +#define vcmdq_page1_writel (_vcmdq, val, reg) \ + writel((val), (_vcmdq)->page1 + TEGRA241_VCMDQ_##reg) +#define vcmdq_page1_writel_relaxed (_vcmdq, val, reg) \ + writel_relaxed((val), (_vcmdq)->page1 + TEGRA241_VCMDQ_##reg) +#define vcmdq_page1_writeq (_vcmdq, val, reg) \ + writeq((val), (_vcmdq)->page1 + TEGRA241_VCMDQ_##reg) +#define vcmdq_page1_writeq_relaxed (_vcmdq, val, reg) \ + writeq_relaxed((val), (_vcmdq)->page1 + TEGRA241_VCMDQ_##reg)These still need to be trimmed to only the ones being used. I still think it is a bad idea, maybe a middle ground is to wrapper the regsiter coding
Quoted 3 lines
writeq_relaxed(val, REG_CMDQ_PAGE1(vcmdq, XXX));Is still short enough and safe enough without creating so much obfuscation..
I think that is a much better practice! Let me try with it.
Thanks!
Nicolin
PATCH v7 6/6 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.
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 | 20 ++++++----- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 5 +-- .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 36 ++++++++++++++++++- 3 files changed, 49 insertions(+), 12 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +11 -9
@@ -332,10 +332,11 @@ 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) {if (arm_smmu_has_tegra241_cmdqv (smmu)) -return tegra241_cmdqv_get_cmdq (smmu); +return tegra241_cmdqv_get_cmdq (smmu, opcode);return &smmu->cmdq; }
@@ -871,7 +872,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,
@@ -887,10 +888,11 @@ 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) { 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,
@@ -1167,7 +1169,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, cmd.opcode);for (i =0 ; i < master->num_streams; i++) { cmd.cfgi.sid = master->streams[i].id;arm_smmu_cmdq_batch_add (smmu, &cmds, &cmd);
@@ -2006,7 +2008,7 @@ static int arm_smmu_atc_inv_master(struct arm_smmu_master *master)arm_smmu_atc_inv_to_cmd (IOMMU_NO_PASID ,0 ,0 , &cmd); -arm_smmu_cmdq_batch_init (master->smmu, &cmds); +arm_smmu_cmdq_batch_init (master->smmu, &cmds, cmd.opcode);for (i =0 ; i < master->num_streams; i++) { cmd.atc.sid = master->streams[i].id;arm_smmu_cmdq_batch_add (master->smmu, &cmds, &cmd);
@@ -2046,7 +2048,7 @@ int arm_smmu_atc_inv_domain(struct arm_smmu_domain *smmu_domain, int ssid,arm_smmu_atc_inv_to_cmd (ssid, iova, size, &cmd); -arm_smmu_cmdq_batch_init (smmu_domain->smmu, &cmds); +arm_smmu_cmdq_batch_init (smmu_domain->smmu, &cmds, cmd.opcode);spin_lock_irqsave (&smmu_domain->devices_lock, flags);list_for_each_entry (master, &smmu_domain->devices, domain_head) {
@@ -2123,7 +2125,7 @@ static void __arm_smmu_tlb_inv_range(struct arm_smmu_cmdq_ent *cmd, num_pages++; } -arm_smmu_cmdq_batch_init (smmu_domain->smmu, &cmds); +arm_smmu_cmdq_batch_init (smmu_domain->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 +3 -2
@@ -879,7 +879,8 @@ struct tegra241_cmdqv *tegra241_cmdqv_acpi_probe(struct arm_smmu_device *smmu, struct acpi_iort_node *node);void tegra241_cmdqv_device_remove (struct arm_smmu_device *smmu);int tegra241_cmdqv_device_reset (struct arm_smmu_device *smmu); -struct arm_smmu_cmdq *tegra241_cmdqv_get_cmdq (struct arm_smmu_device *smmu); +struct arm_smmu_cmdq *tegra241_cmdqv_get_cmdq (struct arm_smmu_device *smmu, +u8 opcode);#else /* CONFIG_TEGRA241_CMDQV */ static inline bool arm_smmu_has_tegra241_cmdqv (struct arm_smmu_device *smmu) {
@@ -903,7 +904,7 @@ static inline int tegra241_cmdqv_device_reset(struct arm_smmu_device *smmu) }static inline struct arm_smmu_cmdq * -tegra241_cmdqv_get_cmdq (struct arm_smmu_device *smmu) +tegra241_cmdqv_get_cmdq (struct arm_smmu_device *smmu,u8 opcode) {return NULL ; }
drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +35 -1
@@ -181,6 +181,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
@@ -189,6 +190,7 @@ struct tegra241_vintf {u16 idx;bool enabled; +bool hyp_own;struct tegra241_cmdqv *cmdqv;struct tegra241_vcmdq **lvcmdqs;
@@ -326,7 +328,25 @@ static irqreturn_t tegra241_cmdqv_isr(int irq, void *devid)/* Command Queue Selecting Function */ -struct arm_smmu_cmdq *tegra241_cmdqv_get_cmdq (struct arm_smmu_device *smmu) +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; + } +} + +struct arm_smmu_cmdq *tegra241_cmdqv_get_cmdq (struct arm_smmu_device *smmu, +u8 opcode) {struct tegra241_cmdqv *cmdqv = smmu->tegra241_cmdqv;struct tegra241_vintf *vintf = cmdqv->vintfs[0 ];
@@ -340,6 +360,10 @@ struct arm_smmu_cmdq *tegra241_cmdqv_get_cmdq(struct arm_smmu_device *smmu)if (!READ_ONCE (vintf->enabled))return &smmu->cmdq; +/* Unsupported CMD go for smmu->cmdq pathway */ +if (!tegra241_vintf_support_cmd (vintf, opcode)) +return &smmu->cmdq; +/* * Select a LVCMDQ to use. Here we use a temporal solution to * balance out traffic on cmdq issuing: each cmdq has its own
@@ -432,12 +456,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);vintf_writel (vintf, regval,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 &vintf_readl (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
Re: [PATCH v7 6/6] iommu/tegra241-cmdqv: Limit CMDs for guest owned VINTF
Quoted 22 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.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.Signed-off-by Nicolin Chen <[email protected]>--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 20 ++++++----- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 5 +-- .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 36 ++++++++++++++++++- 3 files changed, 49 insertions(+), 12 deletions(-)drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +0
-0
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent) · line 339
Quoted 10 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +3
-2@@ -332,10 +332,11 @@ 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) {if (arm_smmu_has_tegra241_cmdqv (smmu)) -return tegra241_cmdqv_get_cmdq (smmu); +return tegra241_cmdqv_get_cmdq (smmu, opcode);
It is worth a comment descrbing opcode here, I think.. At least the nesting invalidation will send mixed batches.
opcode is sort of a handle for a group of related commands. But what is the group? Minimally it is opcode + SYNC, right?
The caller must only send opcode + SYNC commands to this queue. The opcodes XX,YY,ZZ are interchangable and can be sent together.
?
Quoted 6 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +5
-1-struct arm_smmu_cmdq *tegra241_cmdqv_get_cmdq (struct arm_smmu_device *smmu) +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;
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent)
Quoted 10 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +10
-0+ +/* 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; + }
When I look at the nesting patch it also includes SYNC, NH_VAA, and NH_ALL. Are they supported here? VAA is not supported in the HW at all right? What about NH_ALL?
It looks fine otherwise
Jason
Re: [PATCH v7 6/6] iommu/tegra241-cmdqv: Limit CMDs for guest owned VINTF
Quoted 26 lines · Jason Gunthorpe
Quoted 25 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.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.Signed-off-by Nicolin Chen <[email protected]>--- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 20 ++++++----- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 5 +-- .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 36 ++++++++++++++++++- 3 files changed, 49 insertions(+), 12 deletions(-)drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +0
-0@@ -332,10 +332,11 @@ static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent)return 0 ; }
Quoted 10 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent) · line 339
Quoted 7 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +3
-2-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) {if (arm_smmu_has_tegra241_cmdqv (smmu)) -return tegra241_cmdqv_get_cmdq (smmu); +return tegra241_cmdqv_get_cmdq (smmu, opcode);It is worth a comment descrbing opcode here, I think.. At least the nesting invalidation will send mixed batches.
Right, this makes the "opcode" look bad, though we know that the "opcode" in the nesting invalidation doesn't matter because VCMDQ in that case supports all commands with HYP_OWN=1.
Quoted 7 lines
opcode is sort of a handle for a group of related commands. But what is the group? Minimally it is opcode + SYNC, right?
The caller must only send opcode + SYNC commands to this queue. The opcodes XX,YY,ZZ are interchangable and can be sent together.
?
The "opcode" is intended to mean the opcode of a repeated commands in an arm_smmu_cmdq_batch struct. And it is based on an assumption that the driver doesn't and won't mix different commands into an arm_smmu_cmdq_batch struct. Though it doesn't probably matter if a batch mixes NH_ASID and ATC_INV..
A CMD_SYNC, on the other hand, is outside the batch struct. And here is another assumption that CMD_SYNC is always supported by a VCMDQ..
I could clarify the "opcode" here with these assumptions. Or maybe we should think think of a better alternative?
Quoted 12 lines
Quoted 12 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +11
-1-struct arm_smmu_cmdq *tegra241_cmdqv_get_cmdq (struct arm_smmu_device *smmu) +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 :
Quoted 8 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
Quoted 4 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +4
-0+return true; +default : +return false; + }When I look at the nesting patch it also includes SYNC,
NH_VAA, andNH_ALL. Are they supported here? VAA is not supported in the HW at all right? What aboutNH_ALL?
In a nesting case, the host-level VCMDQ runs those comands, i.e. HYP_OWN=1 meaning no command limitation.
Thanks
Nicolin
Re: [PATCH v7 6/6] iommu/tegra241-cmdqv: Limit CMDs for guest owned VINTF
Quoted 11 lines · Nicolin Chen
Quoted 10 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent) · line 339
Quoted 7 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +3
-2-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) {if (arm_smmu_has_tegra241_cmdqv (smmu)) -return tegra241_cmdqv_get_cmdq (smmu); +return tegra241_cmdqv_get_cmdq (smmu, opcode);It is worth a comment descrbing opcode here, I think.. At least the nesting invalidation will send mixed batches.
Quoted 3 lines
Right, this makes the "opcode" look bad, though we know that the "opcode" in the nesting invalidation doesn't matter because VCMDQ in that case supports all commands with HYP_OWN=1.
Yeah, it isn't a real problem, it just looks a little messy and should have a small comment someplace at least..
Quoted 6 lines
A
CMD_SYNC, on the other hand, is outside the batch struct. And here is another assumption thatCMD_SYNCis always supported by a VCMDQ..I could clarify the "opcode" here with these assumptions. Or maybe we should think think of a better alternative?
I don't think it really needs to be more complex, but we should document that invalidation is going to be special and doesn't quite follow this rule.
Jason
Re: [PATCH v7 6/6] iommu/tegra241-cmdqv: Limit CMDs for guest owned VINTF
Quoted 23 lines · Jason Gunthorpe
Quoted 11 lines · Nicolin Chen
Quoted 10 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c static int arm_smmu_cmdq_build_cmd(u64 *cmd, struct arm_smmu_cmdq_ent *ent) · line 339
Quoted 7 lines
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +3
-2-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) {if (arm_smmu_has_tegra241_cmdqv (smmu)) -return tegra241_cmdqv_get_cmdq (smmu); +return tegra241_cmdqv_get_cmdq (smmu, opcode);It is worth a comment descrbing opcode here, I think.. At least the nesting invalidation will send mixed batches.
Quoted 3 lines
Right, this makes the "opcode" look bad, though we know that the "opcode" in the nesting invalidation doesn't matter because VCMDQ in that case supports all commands with HYP_OWN=1.
Yeah, it isn't a real problem, it just looks a little messy and should have a small comment someplace at least..
Quoted 4 lines
A
CMD_SYNC, on the other hand, is outside the batch struct. And here is another assumption thatCMD_SYNCis always supported by a VCMDQ..
Quoted 6 lines
Quoted 2 lines
I could clarify the "opcode" here with these assumptions. Or maybe we should think think of a better alternative?
I don't think it really needs to be more complex, but we should document that invalidation is going to be special and doesn't quite follow this rule.
Yea. I just added this:
@@ -333,10 +333,22 @@ 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)
{
+ /*
+ * TEGRA241 CMDQV has two modes to execute commands: host and guest.
+ * The host mode supports all the opcodes, while the guest mode only
+ * supports a few invalidation ones (check tegra241_vintf_support_cmd)
+ * and also a CMD_SYNC added by arm_smmu_cmdq_issue_cmdlist(..., true).
+ *
+ * Here pass in the representing opcode for either a single command or
+ * an arm_smmu_cmdq_batch, assuming that this SMMU driver will only add
+ * same type of commands into a batch as it does today or it will only
+ * mix supported invalidation commands in a batch.
+ */
if (arm_smmu_has_tegra241_cmdqv(smmu))
- return tegra241_cmdqv_get_cmdq(smmu);
+ return tegra241_cmdqv_get_cmdq(smmu, opcode);
return &smmu->cmdq;
}
Thanks
Nicolin