Mailing List Archive

[RFC PATCH 09/21] xen/arm: vsmmuv3: Add support for cmdqueue handling
Add support for virtual cmdqueue handling for guests

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
xen/drivers/passthrough/arm/vsmmu-v3.c | 101 +++++++++++++++++++++++++
1 file changed, 101 insertions(+)

diff --git a/xen/drivers/passthrough/arm/vsmmu-v3.c b/xen/drivers/passthrough/arm/vsmmu-v3.c
index c3f99657e6..cc651a2dc8 100644
--- a/xen/drivers/passthrough/arm/vsmmu-v3.c
+++ b/xen/drivers/passthrough/arm/vsmmu-v3.c
@@ -1,5 +1,6 @@
/* SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause) */

+#include <xen/guest_access.h>
#include <xen/param.h>
#include <xen/sched.h>
#include <asm/mmio.h>
@@ -24,6 +25,26 @@
/* Struct to hold the vIOMMU ops and vIOMMU type */
extern const struct viommu_desc __read_mostly *cur_viommu;

+/* SMMUv3 command definitions */
+#define CMDQ_OP_PREFETCH_CFG 0x1
+#define CMDQ_OP_CFGI_STE 0x3
+#define CMDQ_OP_CFGI_ALL 0x4
+#define CMDQ_OP_CFGI_CD 0x5
+#define CMDQ_OP_CFGI_CD_ALL 0x6
+#define CMDQ_OP_TLBI_NH_ASID 0x11
+#define CMDQ_OP_TLBI_NH_VA 0x12
+#define CMDQ_OP_TLBI_NSNH_ALL 0x30
+#define CMDQ_OP_CMD_SYNC 0x46
+
+/* Queue Handling */
+#define Q_BASE(q) ((q)->q_base & Q_BASE_ADDR_MASK)
+#define Q_CONS_ENT(q) (Q_BASE(q) + Q_IDX(q, (q)->cons) * (q)->ent_size)
+#define Q_PROD_ENT(q) (Q_BASE(q) + Q_IDX(q, (q)->prod) * (q)->ent_size)
+
+/* Helper Macros */
+#define smmu_get_cmdq_enabled(x) FIELD_GET(CR0_CMDQEN, x)
+#define smmu_cmd_get_command(x) FIELD_GET(CMDQ_0_OP, x)
+
/* virtual smmu queue */
struct arm_vsmmu_queue {
uint64_t q_base; /* base register */
@@ -48,8 +69,80 @@ struct virt_smmu {
uint64_t gerror_irq_cfg0;
uint64_t evtq_irq_cfg0;
struct arm_vsmmu_queue evtq, cmdq;
+ spinlock_t cmd_queue_lock;
};

+/* Queue manipulation functions */
+static bool queue_empty(struct arm_vsmmu_queue *q)
+{
+ return Q_IDX(q, q->prod) == Q_IDX(q, q->cons) &&
+ Q_WRP(q, q->prod) == Q_WRP(q, q->cons);
+}
+
+static void queue_inc_cons(struct arm_vsmmu_queue *q)
+{
+ uint32_t cons = (Q_WRP(q, q->cons) | Q_IDX(q, q->cons)) + 1;
+ q->cons = Q_OVF(q->cons) | Q_WRP(q, cons) | Q_IDX(q, cons);
+}
+
+static void dump_smmu_command(uint64_t *command)
+{
+ gdprintk(XENLOG_ERR, "cmd 0x%02llx: %016lx %016lx\n",
+ smmu_cmd_get_command(command[0]), command[0], command[1]);
+}
+static int arm_vsmmu_handle_cmds(struct virt_smmu *smmu)
+{
+ struct arm_vsmmu_queue *q = &smmu->cmdq;
+ struct domain *d = smmu->d;
+ uint64_t command[CMDQ_ENT_DWORDS];
+ paddr_t addr;
+
+ if ( !smmu_get_cmdq_enabled(smmu->cr[0]) )
+ return 0;
+
+ while ( !queue_empty(q) )
+ {
+ int ret;
+
+ addr = Q_CONS_ENT(q);
+ ret = access_guest_memory_by_ipa(d, addr, command,
+ sizeof(command), false);
+ if ( ret )
+ return ret;
+
+ switch ( smmu_cmd_get_command(command[0]) )
+ {
+ case CMDQ_OP_CFGI_STE:
+ break;
+ case CMDQ_OP_PREFETCH_CFG:
+ case CMDQ_OP_CFGI_CD:
+ case CMDQ_OP_CFGI_CD_ALL:
+ case CMDQ_OP_CFGI_ALL:
+ case CMDQ_OP_CMD_SYNC:
+ break;
+ case CMDQ_OP_TLBI_NH_ASID:
+ case CMDQ_OP_TLBI_NSNH_ALL:
+ case CMDQ_OP_TLBI_NH_VA:
+ if ( !iommu_iotlb_flush_all(smmu->d, 1) )
+ break;
+ default:
+ gdprintk(XENLOG_ERR, "vSMMUv3: unhandled command\n");
+ dump_smmu_command(command);
+ break;
+ }
+
+ if ( ret )
+ {
+ gdprintk(XENLOG_ERR,
+ "vSMMUv3: command error %d while handling command\n",
+ ret);
+ dump_smmu_command(command);
+ }
+ queue_inc_cons(q);
+ }
+ return 0;
+}
+
static int vsmmuv3_mmio_write(struct vcpu *v, mmio_info_t *info,
register_t r, void *priv)
{
@@ -103,9 +196,15 @@ static int vsmmuv3_mmio_write(struct vcpu *v, mmio_info_t *info,
break;

case VREG32(ARM_SMMU_CMDQ_PROD):
+ spin_lock(&smmu->cmd_queue_lock);
reg32 = smmu->cmdq.prod;
vreg_reg32_update(&reg32, r, info);
smmu->cmdq.prod = reg32;
+
+ if ( arm_vsmmu_handle_cmds(smmu) )
+ gdprintk(XENLOG_ERR, "error handling vSMMUv3 commands\n");
+
+ spin_unlock(&smmu->cmd_queue_lock);
break;

case VREG32(ARM_SMMU_CMDQ_CONS):
@@ -321,6 +420,8 @@ static int vsmmuv3_init_single(struct domain *d, paddr_t addr, paddr_t size)
smmu->evtq.q_base = FIELD_PREP(Q_BASE_LOG2SIZE, SMMU_EVTQS);
smmu->evtq.ent_size = EVTQ_ENT_DWORDS * DWORDS_BYTES;

+ spin_lock_init(&smmu->cmd_queue_lock);
+
register_mmio_handler(d, &vsmmuv3_mmio_handler, addr, size, smmu);

/* Register the vIOMMU to be able to clean it up later. */
--
2.25.1
Re: [RFC PATCH 09/21] xen/arm: vsmmuv3: Add support for cmdqueue handling [ In reply to ]
Hi,

On 01/12/2022 16:02, Rahul Singh wrote:
> Add support for virtual cmdqueue handling for guests

This commit message is a bit light given the security implication of
this approach. See some of the questions below.

[...]

> +static int arm_vsmmu_handle_cmds(struct virt_smmu *smmu)
> +{
> + struct arm_vsmmu_queue *q = &smmu->cmdq;
> + struct domain *d = smmu->d;
> + uint64_t command[CMDQ_ENT_DWORDS];
> + paddr_t addr;
> +
> + if ( !smmu_get_cmdq_enabled(smmu->cr[0]) )
> + return 0;
> +
> + while ( !queue_empty(q) )
What is the size of the queue and what would happen if the guest fill up
the queue before triggering a call to arm_vsmmu_handle_cmds()?

> + {
> + int ret;
> +
> + addr = Q_CONS_ENT(q);
> + ret = access_guest_memory_by_ipa(d, addr, command,
> + sizeof(command), false);
> + if ( ret )
> + return ret;
> +
> + switch ( smmu_cmd_get_command(command[0]) )
> + {
> + case CMDQ_OP_CFGI_STE:
> + break;

It is not clear to me why there is a break here when...

> + case CMDQ_OP_PREFETCH_CFG:
> + case CMDQ_OP_CFGI_CD:
> + case CMDQ_OP_CFGI_CD_ALL:
> + case CMDQ_OP_CFGI_ALL:
> + case CMDQ_OP_CMD_SYNC:
> + break;

... the implementation for those is also empty.

> + case CMDQ_OP_TLBI_NH_ASID:
> + case CMDQ_OP_TLBI_NSNH_ALL:
> + case CMDQ_OP_TLBI_NH_VA:
> + if ( !iommu_iotlb_flush_all(smmu->d, 1) )
> + break;

It is not clear to me why you are implementing TLB flush right now but
not the other.

This call is also a massive hammer (the more if there are multiple
IOMMUs involved) and I can see this been a way for a guest to abuse the
vSMMUv3.

What is your end goal with the vSMMUv3, are you going to expose it to
untrusted environment?

> + default:
> + gdprintk(XENLOG_ERR, "vSMMUv3: unhandled command\n");
> + dump_smmu_command(command);

This would only be printed in a debug build. However, it is not clear to
me how a guest would notice that Xen didn't handle the command.

So I think this should be a gprintk() to enable the user to figure out
in production that something went wrong.

That said, isn't there a way to report error to the guest?

> + break;
> + }
> +
> + if ( ret )
> + {
> + gdprintk(XENLOG_ERR,
> + "vSMMUv3: command error %d while handling command\n",
> + ret);

Same here. However, ret is so far always 0 until here. It would be
preferable if this is introduced on the first user.

> + dump_smmu_command(command);
> + }
> + queue_inc_cons(q);
> + }
> + return 0;
> +}
> +
> static int vsmmuv3_mmio_write(struct vcpu *v, mmio_info_t *info,
> register_t r, void *priv)
> {
> @@ -103,9 +196,15 @@ static int vsmmuv3_mmio_write(struct vcpu *v, mmio_info_t *info,
> break;
>
> case VREG32(ARM_SMMU_CMDQ_PROD):
> + spin_lock(&smmu->cmd_queue_lock);
The amount of work done with the spin_lock() taken looks quite large.
This means a guest with N vCPUs could easily block N pCPUs for several
seconds.

How do you plan to address this?

Cheers,

--
Julien Grall