| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6f020ea5-a45c-d21d-04b5-bdb2aef080f1@arm.com>
Date: Wed, 22 Dec 2021 12:32:29 +0000
From: Robin Murphy <robin.murphy@....com>
To: Nicolin Chen <nicolinc@...dia.com>, joro@...tes.org,
will@...nel.org
Cc: jean-philippe@...aro.org, linux-kernel@...r.kernel.org,
iommu@...ts.linux-foundation.org, linux-tegra@...r.kernel.org,
thierry.reding@...il.com, jgg@...dia.com,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3 5/5] iommu/nvidia-grace-cmdqv: Limit CMDs for guest
owned VINTF
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@...dia.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,
Powered by blists - more mailing lists