Messages in this thread | | | Date | Wed, 22 Dec 2021 12:32:29 +0000 | Subject | Re: [PATCH v3 5/5] iommu/nvidia-grace-cmdqv: Limit CMDs for guest owned VINTF | From | Robin Murphy <> |
| |
On 2021-11-19 07:19, Nicolin Chen via iommu wrote: > When VCMDQs are assigned to a VINTF that is owned by a guest, not > hypervisor (HYP_OWN bit is unset), only TLB invalidation commands > are supported. This requires get_cmd() function to scan the input > cmd before selecting cmdq between smmu->cmdq and vintf->vcmdq, so > unsupported commands can still go through emulated smmu->cmdq. > > Also the guest shouldn't have HYP_OWN bit being set regardless of > guest kernel driver writing it or not, i.e. the user space driver > 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. > So instead of using the existing regval, this patch reads out the > register value explicitly to cache in vintf->cfg. > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 6 ++-- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 5 +-- > .../arm/arm-smmu-v3/nvidia-grace-cmdqv.c | 32 +++++++++++++++++-- > 3 files changed, 36 insertions(+), 7 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 b1182dd825fd..73941ccc1a3e 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -337,10 +337,10 @@ 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, u64 *cmds, int n) > { > if (smmu->nvidia_grace_cmdqv) > - return nvidia_grace_cmdqv_get_cmdq(smmu); > + return nvidia_grace_cmdqv_get_cmdq(smmu, cmds, n); > > return &smmu->cmdq; > } > @@ -747,7 +747,7 @@ static int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu, > u32 prod; > unsigned long flags; > bool owner; > - struct arm_smmu_cmdq *cmdq = arm_smmu_get_cmdq(smmu); > + struct arm_smmu_cmdq *cmdq = arm_smmu_get_cmdq(smmu, cmds, n); > struct arm_smmu_ll_queue llq, head; > int ret = 0; > > 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 24f93444aeeb..085c775c2eea 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > @@ -832,7 +832,8 @@ struct nvidia_grace_cmdqv * > nvidia_grace_cmdqv_acpi_probe(struct arm_smmu_device *smmu, > struct acpi_iort_node *node); > int nvidia_grace_cmdqv_device_reset(struct arm_smmu_device *smmu); > -struct arm_smmu_cmdq *nvidia_grace_cmdqv_get_cmdq(struct arm_smmu_device *smmu); > +struct arm_smmu_cmdq *nvidia_grace_cmdqv_get_cmdq(struct arm_smmu_device *smmu, > + u64 *cmds, int n); > #else /* CONFIG_NVIDIA_GRACE_CMDQV */ > static inline struct nvidia_grace_cmdqv * > nvidia_grace_cmdqv_acpi_probe(struct arm_smmu_device *smmu, > @@ -847,7 +848,7 @@ static inline int nvidia_grace_cmdqv_device_reset(struct arm_smmu_device *smmu) > } > > static inline struct arm_smmu_cmdq * > -nvidia_grace_cmdqv_get_cmdq(struct arm_smmu_device *smmu) > +nvidia_grace_cmdqv_get_cmdq(struct arm_smmu_device *smmu, u64 *cmds, int n) > { > return NULL; > } > diff --git a/drivers/iommu/arm/arm-smmu-v3/nvidia-grace-cmdqv.c b/drivers/iommu/arm/arm-smmu-v3/nvidia-grace-cmdqv.c > index c0d7351f13e2..71f6bc684e64 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/nvidia-grace-cmdqv.c > +++ b/drivers/iommu/arm/arm-smmu-v3/nvidia-grace-cmdqv.c > @@ -166,7 +166,8 @@ static int nvidia_grace_cmdqv_init_one_vcmdq(struct nvidia_grace_cmdqv *cmdqv, > return arm_smmu_cmdq_init(cmdqv->smmu, cmdq); > } > > -struct arm_smmu_cmdq *nvidia_grace_cmdqv_get_cmdq(struct arm_smmu_device *smmu) > +struct arm_smmu_cmdq * > +nvidia_grace_cmdqv_get_cmdq(struct arm_smmu_device *smmu, u64 *cmds, int n) > { > struct nvidia_grace_cmdqv *cmdqv = smmu->nvidia_grace_cmdqv; > struct nvidia_grace_cmdqv_vintf *vintf0 = &cmdqv->vintf0; > @@ -176,6 +177,24 @@ struct arm_smmu_cmdq *nvidia_grace_cmdqv_get_cmdq(struct arm_smmu_device *smmu) > if (!FIELD_GET(VINTF_STATUS, vintf0->status)) > return &smmu->cmdq; > > + /* Check for supported CMDs if VINTF is owned by guest (not hypervisor) */ > + if (!FIELD_GET(VINTF_HYP_OWN, vintf0->cfg)) { > + u64 opcode = (n) ? FIELD_GET(CMDQ_0_OP, cmds[0]) : CMDQ_OP_CMD_SYNC;
I'm not sure there was ever a conscious design decision that batches only ever contain one type of command - if something needs to start depending on that behaviour then that dependency probably wants to be clearly documented. Also, a sync on its own gets trapped to the main cmdq but a sync on the end of a batch of TLBIs or ATCIs goes to the VCMDQ, huh?
> + > + /* List all supported CMDs for vintf->cmdq pathway */ > + switch (opcode) { > + case CMDQ_OP_TLBI_NH_ASID: > + case CMDQ_OP_TLBI_NH_VA: > + case CMDQ_OP_TLBI_S12_VMALL: > + case CMDQ_OP_TLBI_S2_IPA:
Fun! Can the guest invalidate any VMID it feels like, or is there some additional magic on the host side that we're missing here?
> + case CMDQ_OP_ATC_INV: > + break; Ditto for StreamID here.
Robin.
> + default: > + /* Unsupported CMDs go for smmu->cmdq pathway */ > + return &smmu->cmdq; > + } > + } > + > /* > * Select a vcmdq to use. Here we use a temporal solution to > * balance out traffic on cmdq issuing: each cmdq has its own > @@ -199,13 +218,22 @@ int nvidia_grace_cmdqv_device_reset(struct arm_smmu_device *smmu) > vintf0->idx = 0; > vintf0->base = cmdqv->base + NVIDIA_CMDQV_VINTF(0); > > + /* > + * Note that HYP_OWN bit is wired to zero when running in guest kernel > + * regardless of enabling it here, as !HYP_OWN cmdqs have a restricted > + * set of supported commands, by following the HW design. > + */ > regval = FIELD_PREP(VINTF_HYP_OWN, 1); > writel(regval, vintf0->base + NVIDIA_VINTF_CONFIG); > > regval |= FIELD_PREP(VINTF_EN, 1); > writel(regval, vintf0->base + NVIDIA_VINTF_CONFIG); > > - vintf0->cfg = regval; > + /* > + * As being mentioned above, HYP_OWN bit is wired to zero for a guest > + * kernel, so read back regval from HW to ensure that reflects in cfg > + */ > + vintf0->cfg = readl(vintf0->base + NVIDIA_VINTF_CONFIG); > > ret = readl_relaxed_poll_timeout(vintf0->base + NVIDIA_VINTF_STATUS, > regval, regval == VINTF_ENABLED,
| |