| Message ID | 062cf0a1e2b8ec6f068262cc68498b8d72b04bcc.1715147377.git.nicolinc@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add Tegra241 (Grace) CMDQV Support (part 1/2) | expand |
On Tue, May 07, 2024 at 10:56:54PM -0700, Nicolin Chen wrote: > When 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. > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > --- > 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(-) > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > index d1098991d64e..baf20e9976d3 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -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. ? > -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; > + } 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 Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Jason
On Sun, May 12, 2024 at 01:06:13PM -0300, Jason Gunthorpe wrote: > On Tue, May 07, 2024 at 10:56:54PM -0700, Nicolin Chen wrote: > > When 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. > > > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > > --- > > 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(-) > > > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > index d1098991d64e..baf20e9976d3 100644 > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > @@ -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. 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. > 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? > > -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; > > + } > > 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? In a nesting case, the host-level VCMDQ runs those comands, i.e. HYP_OWN=1 meaning no command limitation. Thanks Nicolin
On Sun, May 12, 2024 at 03:09:25PM -0700, Nicolin Chen wrote: > > > -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. Yeah, it isn't a real problem, it just looks a little messy and should have a small comment someplace at least.. > 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? 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
On Tue, May 14, 2024 at 12:15:13PM -0300, Jason Gunthorpe wrote: > On Sun, May 12, 2024 at 03:09:25PM -0700, Nicolin Chen wrote: > > > > -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. > > Yeah, it isn't a real problem, it just looks a little messy and > should have a small comment someplace at least.. > > > 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? > > 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
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index d1098991d64e..baf20e9976d3 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -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) { diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h index 604e26a292e7..2c1fe7e129cd 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h @@ -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; } diff --git a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c index ec4767e3859e..e7a281131e5d 100644 --- a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +++ b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c @@ -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]) {
When 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. Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> --- 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(-)