| Message ID | ee70b82c8268e9a6746486ddcb3e8e7f32f0629f.1712977210.git.nicolinc@nvidia.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Add Tegra241 (Grace) CMDQV Support (part 1/2) | expand |
> -----Original Message----- > From: Nicolin Chen <nicolinc@nvidia.com> > Sent: Saturday, April 13, 2024 4:44 AM > To: will@kernel.org; robin.murphy@arm.com > Cc: joro@8bytes.org; jgg@nvidia.com; thierry.reding@gmail.com; > vdumpa@nvidia.com; jonathanh@nvidia.com; linux-kernel@vger.kernel.org; > iommu@lists.linux.dev; linux-arm-kernel@lists.infradead.org; linux- > tegra@vger.kernel.org > Subject: [PATCH v5 6/6] iommu/tegra241-cmdqv: Limit CMDs for guest owned > VINTF > > 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 cmds to make sure every single command > 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. Just curious, suppose there is a malicious guest with a compromised kernel which bypasses the HYP_OWN bit check and issues other commands, does the hardware has the capability to detect it and not make the host unstable in any way? Thanks, Shameer > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 7 +-- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 5 ++- > .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 43 ++++++++++++++++++- > 3 files changed, 49 insertions(+), 6 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 ba7a933c1efb..9af6659ea488 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -352,10 +352,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, > + u64 *cmds, int n) > { > if (smmu->tegra241_cmdqv) > - return tegra241_cmdqv_get_cmdq(smmu); > + return tegra241_cmdqv_get_cmdq(smmu, cmds, n); > > return &smmu->cmdq; > } > @@ -765,7 +766,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 5b8e463c28eb..fdc3d570cf43 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h > @@ -836,7 +836,8 @@ static inline void > arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain, > struct tegra241_cmdqv * > tegra241_cmdqv_acpi_probe(struct arm_smmu_device *smmu, int id); > 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, > + u64 *cmds, int n); > #else /* CONFIG_TEGRA241_CMDQV */ > static inline struct tegra241_cmdqv * > tegra241_cmdqv_acpi_probe(struct arm_smmu_device *smmu, int id) > @@ -850,7 +851,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, u64 *cmds, int > n) > { > 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 15683123a4ce..7aeaf810980c 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c > +++ b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c > @@ -262,6 +262,7 @@ struct tegra241_vcmdq { > * struct tegra241_vintf - Virtual Interface > * @idx: Global index in the CMDQV HW > * @enabled: Enabled or not > + * @hyp_own: Owned by hypervisor (in-kernel) > * @error: Status error or not > * @cmdqv: CMDQV HW pointer > * @vcmdqs: List of VCMDQ pointers > @@ -271,6 +272,7 @@ struct tegra241_vintf { > u16 idx; > > bool enabled; > + bool hyp_own; > atomic_t error; /* Race between interrupts and get_cmdq() */ > > struct tegra241_cmdqv *cmdqv; > @@ -369,7 +371,32 @@ static irqreturn_t tegra241_cmdqv_isr(int irq, void > *devid) > return IRQ_HANDLED; > } > > -struct arm_smmu_cmdq *tegra241_cmdqv_get_cmdq(struct > arm_smmu_device *smmu) > +static bool tegra241_vintf_support_cmds(struct tegra241_vintf *vintf, > + u64 *cmds, int n) > +{ > + int i; > + > + /* VINTF owned by hypervisor can execute any command */ > + if (vintf->hyp_own) > + return true; > + > + /* Guest-owned VINTF must Check against the list of supported CMDs > */ > + for (i = 0; i < n; i++) { > + switch (FIELD_GET(CMDQ_0_OP, cmds[i * > CMDQ_ENT_DWORDS])) { > + case CMDQ_OP_TLBI_NH_ASID: > + case CMDQ_OP_TLBI_NH_VA: > + case CMDQ_OP_ATC_INV: > + continue; > + default: > + return false; > + } > + } > + > + return true; > +} > + > +struct arm_smmu_cmdq *tegra241_cmdqv_get_cmdq(struct > arm_smmu_device *smmu, > + u64 *cmds, int n) > { > struct tegra241_cmdqv *cmdqv = smmu->tegra241_cmdqv; > struct tegra241_vintf *vintf = cmdqv->vintfs[0]; > @@ -386,6 +413,10 @@ struct arm_smmu_cmdq > *tegra241_cmdqv_get_cmdq(struct arm_smmu_device *smmu) > if (atomic_read(&vintf->error)) > return &smmu->cmdq; > > + /* Unsupported CMDs go for smmu->cmdq pathway */ > + if (!tegra241_vintf_support_cmds(vintf, cmds, n)) > + 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 > @@ -575,6 +606,11 @@ int tegra241_cmdqv_device_reset(struct > arm_smmu_device *smmu) > if (ret) > return ret; > > + /* > + * 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); > vintf_writel(regval, CONFIG); > > @@ -582,6 +618,11 @@ int tegra241_cmdqv_device_reset(struct > arm_smmu_device *smmu) > 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(CONFIG)); > vintf->enabled = !!(VINTF_ENABLED & vintf_readl(STATUS)); > atomic_set(&vintf->error, > !!FIELD_GET(VINTF_STATUS, vintf_readl(STATUS))); > -- > 2.43.0 >
On Wed, Apr 17, 2024 at 03:12:57PM +0000, Shameerali Kolothum Thodi wrote: > > -----Original Message----- > > From: Nicolin Chen <nicolinc@nvidia.com> > > Sent: Saturday, April 13, 2024 4:44 AM > > To: will@kernel.org; robin.murphy@arm.com > > Cc: joro@8bytes.org; jgg@nvidia.com; thierry.reding@gmail.com; > > vdumpa@nvidia.com; jonathanh@nvidia.com; linux-kernel@vger.kernel.org; > > iommu@lists.linux.dev; linux-arm-kernel@lists.infradead.org; linux- > > tegra@vger.kernel.org > > Subject: [PATCH v5 6/6] iommu/tegra241-cmdqv: Limit CMDs for guest owned > > VINTF > > > > 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 cmds to make sure every single command > > 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. > > Just curious, suppose there is a malicious guest with a compromised kernel > which bypasses the HYP_OWN bit check and issues other commands, does > the hardware has the capability to detect it and not make the host unstable > in any way? Only that specific VINTF/VCMDQ would stop and error an IRQ. Even a device command (ATC_INV) would be strictly checked against the Stream ID table that's set at the host level. 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 ba7a933c1efb..9af6659ea488 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -352,10 +352,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, + u64 *cmds, int n) { if (smmu->tegra241_cmdqv) - return tegra241_cmdqv_get_cmdq(smmu); + return tegra241_cmdqv_get_cmdq(smmu, cmds, n); return &smmu->cmdq; } @@ -765,7 +766,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 5b8e463c28eb..fdc3d570cf43 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h @@ -836,7 +836,8 @@ static inline void arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain, struct tegra241_cmdqv * tegra241_cmdqv_acpi_probe(struct arm_smmu_device *smmu, int id); 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, + u64 *cmds, int n); #else /* CONFIG_TEGRA241_CMDQV */ static inline struct tegra241_cmdqv * tegra241_cmdqv_acpi_probe(struct arm_smmu_device *smmu, int id) @@ -850,7 +851,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, u64 *cmds, int n) { 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 15683123a4ce..7aeaf810980c 100644 --- a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c +++ b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c @@ -262,6 +262,7 @@ struct tegra241_vcmdq { * struct tegra241_vintf - Virtual Interface * @idx: Global index in the CMDQV HW * @enabled: Enabled or not + * @hyp_own: Owned by hypervisor (in-kernel) * @error: Status error or not * @cmdqv: CMDQV HW pointer * @vcmdqs: List of VCMDQ pointers @@ -271,6 +272,7 @@ struct tegra241_vintf { u16 idx; bool enabled; + bool hyp_own; atomic_t error; /* Race between interrupts and get_cmdq() */ struct tegra241_cmdqv *cmdqv; @@ -369,7 +371,32 @@ static irqreturn_t tegra241_cmdqv_isr(int irq, void *devid) return IRQ_HANDLED; } -struct arm_smmu_cmdq *tegra241_cmdqv_get_cmdq(struct arm_smmu_device *smmu) +static bool tegra241_vintf_support_cmds(struct tegra241_vintf *vintf, + u64 *cmds, int n) +{ + int i; + + /* VINTF owned by hypervisor can execute any command */ + if (vintf->hyp_own) + return true; + + /* Guest-owned VINTF must Check against the list of supported CMDs */ + for (i = 0; i < n; i++) { + switch (FIELD_GET(CMDQ_0_OP, cmds[i * CMDQ_ENT_DWORDS])) { + case CMDQ_OP_TLBI_NH_ASID: + case CMDQ_OP_TLBI_NH_VA: + case CMDQ_OP_ATC_INV: + continue; + default: + return false; + } + } + + return true; +} + +struct arm_smmu_cmdq *tegra241_cmdqv_get_cmdq(struct arm_smmu_device *smmu, + u64 *cmds, int n) { struct tegra241_cmdqv *cmdqv = smmu->tegra241_cmdqv; struct tegra241_vintf *vintf = cmdqv->vintfs[0]; @@ -386,6 +413,10 @@ struct arm_smmu_cmdq *tegra241_cmdqv_get_cmdq(struct arm_smmu_device *smmu) if (atomic_read(&vintf->error)) return &smmu->cmdq; + /* Unsupported CMDs go for smmu->cmdq pathway */ + if (!tegra241_vintf_support_cmds(vintf, cmds, n)) + 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 @@ -575,6 +606,11 @@ int tegra241_cmdqv_device_reset(struct arm_smmu_device *smmu) if (ret) return ret; + /* + * 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); vintf_writel(regval, CONFIG); @@ -582,6 +618,11 @@ int tegra241_cmdqv_device_reset(struct arm_smmu_device *smmu) 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(CONFIG)); vintf->enabled = !!(VINTF_ENABLED & vintf_readl(STATUS)); atomic_set(&vintf->error, !!FIELD_GET(VINTF_STATUS, vintf_readl(STATUS)));
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 cmds to make sure every single command 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 | 7 +-- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 5 ++- .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 43 ++++++++++++++++++- 3 files changed, 49 insertions(+), 6 deletions(-)