From: Nicolin Chen <nicolinc@nvidia.com>
Use the provided smmuv3-accel helper functions to issue the
command to physical SMMUv3.
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
hw/arm/smmuv3-internal.h | 11 ++++++++
hw/arm/smmuv3.c | 58 +++++++++++++++++++++++++++++++++++++++-
2 files changed, 68 insertions(+), 1 deletion(-)
diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
index 4602ae6728..546f8faac0 100644
--- a/hw/arm/smmuv3-internal.h
+++ b/hw/arm/smmuv3-internal.h
@@ -235,6 +235,17 @@ static inline bool smmuv3_gerror_irq_enabled(SMMUv3State *s)
#define Q_CONS_WRAP(q) (((q)->cons & WRAP_MASK(q)) >> (q)->log2size)
#define Q_PROD_WRAP(q) (((q)->prod & WRAP_MASK(q)) >> (q)->log2size)
+static inline int smmuv3_q_ncmds(SMMUQueue *q)
+{
+ uint32_t prod = Q_PROD(q);
+ uint32_t cons = Q_CONS(q);
+
+ if (Q_PROD_WRAP(q) == Q_CONS_WRAP(q))
+ return prod - cons;
+ else
+ return WRAP_MASK(q) - cons + prod;
+}
+
static inline bool smmuv3_q_full(SMMUQueue *q)
{
return ((q->cons ^ q->prod) & WRAP_INDEX_MASK(q)) == WRAP_MASK(q);
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 83159db1d4..e0f225d0df 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -1297,10 +1297,18 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
SMMUCmdError cmd_error = SMMU_CERROR_NONE;
SMMUQueue *q = &s->cmdq;
SMMUCommandType type = 0;
+ SMMUCommandBatch batch = {};
+ uint32_t ncmds = 0;
+
if (!smmuv3_cmdq_enabled(s)) {
return 0;
}
+
+ ncmds = smmuv3_q_ncmds(q);
+ batch.cmds = g_new0(Cmd, ncmds);
+ batch.cons = g_new0(uint32_t, ncmds);
+
/*
* some commands depend on register values, typically CR0. In case those
* register values change while handling the command, spec says it
@@ -1395,6 +1403,13 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
trace_smmuv3_cmdq_cfgi_cd(sid);
smmuv3_flush_config(sdev);
+
+ if (smmuv3_accel_batch_cmds(sdev->smmu, sdev, &batch, &cmd,
+ &q->cons, true)) {
+ cmd_error = SMMU_CERROR_ILL;
+ break;
+ }
+
break;
}
case SMMU_CMD_TLBI_NH_ASID:
@@ -1418,6 +1433,13 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
trace_smmuv3_cmdq_tlbi_nh_asid(asid);
smmu_inv_notifiers_all(&s->smmu_state);
smmu_iotlb_inv_asid_vmid(bs, asid, vmid);
+
+ if (smmuv3_accel_batch_cmds(bs, NULL, &batch, &cmd, &q->cons,
+ false)) {
+ cmd_error = SMMU_CERROR_ILL;
+ break;
+ }
+
break;
}
case SMMU_CMD_TLBI_NH_ALL:
@@ -1445,6 +1467,12 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
trace_smmuv3_cmdq_tlbi_nsnh();
smmu_inv_notifiers_all(&s->smmu_state);
smmu_iotlb_inv_all(bs);
+
+ if (smmuv3_accel_batch_cmds(bs, NULL, &batch, &cmd, &q->cons,
+ false)) {
+ cmd_error = SMMU_CERROR_ILL;
+ break;
+ }
break;
case SMMU_CMD_TLBI_NH_VAA:
case SMMU_CMD_TLBI_NH_VA:
@@ -1453,7 +1481,24 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
break;
}
smmuv3_range_inval(bs, &cmd, SMMU_STAGE_1);
+
+ if (smmuv3_accel_batch_cmds(bs, NULL, &batch, &cmd, &q->cons,
+ false)) {
+ cmd_error = SMMU_CERROR_ILL;
+ break;
+ }
+ break;
+ case SMMU_CMD_ATC_INV:
+ {
+ SMMUDevice *sdev = smmu_find_sdev(bs, CMD_SID(&cmd));
+
+ if (smmuv3_accel_batch_cmds(sdev->smmu, sdev, &batch, &cmd,
+ &q->cons, true)) {
+ cmd_error = SMMU_CERROR_ILL;
+ break;
+ }
break;
+ }
case SMMU_CMD_TLBI_S12_VMALL:
{
int vmid = CMD_VMID(&cmd);
@@ -1485,7 +1530,6 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
case SMMU_CMD_TLBI_EL2_ASID:
case SMMU_CMD_TLBI_EL2_VA:
case SMMU_CMD_TLBI_EL2_VAA:
- case SMMU_CMD_ATC_INV:
case SMMU_CMD_PRI_RESP:
case SMMU_CMD_RESUME:
case SMMU_CMD_STALL_TERM:
@@ -1511,12 +1555,24 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
queue_cons_incr(q);
}
+ qemu_mutex_lock(&s->mutex);
+ if (!cmd_error && batch.ncmds) {
+ if (smmuv3_accel_issue_cmd_batch(bs, &batch)) {
+ q->cons = batch.cons[batch.ncmds];
+ cmd_error = SMMU_CERROR_ILL;
+ }
+ }
+ qemu_mutex_unlock(&s->mutex);
+
if (cmd_error) {
trace_smmuv3_cmdq_consume_error(smmu_cmd_string(type), cmd_error);
smmu_write_cmdq_err(s, cmd_error);
smmuv3_trigger_irq(s, SMMU_IRQ_GERROR, R_GERROR_CMDQ_ERR_MASK);
}
+ g_free(batch.cmds);
+ g_free(batch.cons);
+
trace_smmuv3_cmdq_consume_out(Q_PROD(q), Q_CONS(q),
Q_PROD_WRAP(q), Q_CONS_WRAP(q));
--
2.34.1
On 3/11/25 3:10 PM, Shameer Kolothum wrote:
> From: Nicolin Chen <nicolinc@nvidia.com>
>
> Use the provided smmuv3-accel helper functions to issue the
> command to physical SMMUv3.
>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> hw/arm/smmuv3-internal.h | 11 ++++++++
> hw/arm/smmuv3.c | 58 +++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 68 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index 4602ae6728..546f8faac0 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -235,6 +235,17 @@ static inline bool smmuv3_gerror_irq_enabled(SMMUv3State *s)
> #define Q_CONS_WRAP(q) (((q)->cons & WRAP_MASK(q)) >> (q)->log2size)
> #define Q_PROD_WRAP(q) (((q)->prod & WRAP_MASK(q)) >> (q)->log2size)
>
> +static inline int smmuv3_q_ncmds(SMMUQueue *q)
> +{
> + uint32_t prod = Q_PROD(q);
> + uint32_t cons = Q_CONS(q);
> +
> + if (Q_PROD_WRAP(q) == Q_CONS_WRAP(q))
> + return prod - cons;
> + else
> + return WRAP_MASK(q) - cons + prod;
> +}
> +
> static inline bool smmuv3_q_full(SMMUQueue *q)
> {
> return ((q->cons ^ q->prod) & WRAP_INDEX_MASK(q)) == WRAP_MASK(q);
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 83159db1d4..e0f225d0df 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -1297,10 +1297,18 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
> SMMUCmdError cmd_error = SMMU_CERROR_NONE;
> SMMUQueue *q = &s->cmdq;
> SMMUCommandType type = 0;
> + SMMUCommandBatch batch = {};
> + uint32_t ncmds = 0;
> +
>
> if (!smmuv3_cmdq_enabled(s)) {
> return 0;
> }
> +
> + ncmds = smmuv3_q_ncmds(q);
> + batch.cmds = g_new0(Cmd, ncmds);
> + batch.cons = g_new0(uint32_t, ncmds);
> +
> /*
> * some commands depend on register values, typically CR0. In case those
> * register values change while handling the command, spec says it
> @@ -1395,6 +1403,13 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
>
> trace_smmuv3_cmdq_cfgi_cd(sid);
> smmuv3_flush_config(sdev);
> +
> + if (smmuv3_accel_batch_cmds(sdev->smmu, sdev, &batch, &cmd,
> + &q->cons, true)) {
> + cmd_error = SMMU_CERROR_ILL;
OK so now I see you record the error. You can ignore the previous comment.
> + break;
> + }
> +
> break;
> }
> case SMMU_CMD_TLBI_NH_ASID:
> @@ -1418,6 +1433,13 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
> trace_smmuv3_cmdq_tlbi_nh_asid(asid);
> smmu_inv_notifiers_all(&s->smmu_state);
> smmu_iotlb_inv_asid_vmid(bs, asid, vmid);
> +
> + if (smmuv3_accel_batch_cmds(bs, NULL, &batch, &cmd, &q->cons,
> + false)) {
> + cmd_error = SMMU_CERROR_ILL;
> + break;
> + }
> +
> break;
> }
> case SMMU_CMD_TLBI_NH_ALL:
> @@ -1445,6 +1467,12 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
> trace_smmuv3_cmdq_tlbi_nsnh();
> smmu_inv_notifiers_all(&s->smmu_state);
> smmu_iotlb_inv_all(bs);
> +
> + if (smmuv3_accel_batch_cmds(bs, NULL, &batch, &cmd, &q->cons,
> + false)) {
> + cmd_error = SMMU_CERROR_ILL;
> + break;
> + }
> break;
> case SMMU_CMD_TLBI_NH_VAA:
> case SMMU_CMD_TLBI_NH_VA:
> @@ -1453,7 +1481,24 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
> break;
> }
> smmuv3_range_inval(bs, &cmd, SMMU_STAGE_1);
> +
> + if (smmuv3_accel_batch_cmds(bs, NULL, &batch, &cmd, &q->cons,
> + false)) {
> + cmd_error = SMMU_CERROR_ILL;
> + break;
> + }
> + break;
> + case SMMU_CMD_ATC_INV:
To me the code below shall be put in a separate patch as it introduces
the suport for a new cmd. Also it shall be properly documented in the
commit msg
> + {
> + SMMUDevice *sdev = smmu_find_sdev(bs, CMD_SID(&cmd));
> +
> + if (smmuv3_accel_batch_cmds(sdev->smmu, sdev, &batch, &cmd,
> + &q->cons, true)) {
> + cmd_error = SMMU_CERROR_ILL;
> + break;
> + }
> break;
> + }
> case SMMU_CMD_TLBI_S12_VMALL:
> {
> int vmid = CMD_VMID(&cmd);
> @@ -1485,7 +1530,6 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
> case SMMU_CMD_TLBI_EL2_ASID:
> case SMMU_CMD_TLBI_EL2_VA:
> case SMMU_CMD_TLBI_EL2_VAA:
> - case SMMU_CMD_ATC_INV:
> case SMMU_CMD_PRI_RESP:
> case SMMU_CMD_RESUME:
> case SMMU_CMD_STALL_TERM:
> @@ -1511,12 +1555,24 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
> queue_cons_incr(q);
> }
>
> + qemu_mutex_lock(&s->mutex);
> + if (!cmd_error && batch.ncmds) {
> + if (smmuv3_accel_issue_cmd_batch(bs, &batch)) {
> + q->cons = batch.cons[batch.ncmds];
> + cmd_error = SMMU_CERROR_ILL;
> + }
> + }
> + qemu_mutex_unlock(&s->mutex);
> +
> if (cmd_error) {
> trace_smmuv3_cmdq_consume_error(smmu_cmd_string(type), cmd_error);
> smmu_write_cmdq_err(s, cmd_error);
> smmuv3_trigger_irq(s, SMMU_IRQ_GERROR, R_GERROR_CMDQ_ERR_MASK);
> }
>
> + g_free(batch.cmds);
> + g_free(batch.cons);
> +
> trace_smmuv3_cmdq_consume_out(Q_PROD(q), Q_CONS(q),
> Q_PROD_WRAP(q), Q_CONS_WRAP(q));
>
Eric
Hi,
On 3/11/25 3:10 PM, Shameer Kolothum wrote:
> From: Nicolin Chen <nicolinc@nvidia.com>
>
> Use the provided smmuv3-accel helper functions to issue the
> command to physical SMMUv3.
>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> hw/arm/smmuv3-internal.h | 11 ++++++++
> hw/arm/smmuv3.c | 58 +++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 68 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index 4602ae6728..546f8faac0 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -235,6 +235,17 @@ static inline bool smmuv3_gerror_irq_enabled(SMMUv3State *s)
> #define Q_CONS_WRAP(q) (((q)->cons & WRAP_MASK(q)) >> (q)->log2size)
> #define Q_PROD_WRAP(q) (((q)->prod & WRAP_MASK(q)) >> (q)->log2size)
>
> +static inline int smmuv3_q_ncmds(SMMUQueue *q)
> +{
> + uint32_t prod = Q_PROD(q);
> + uint32_t cons = Q_CONS(q);
> +
> + if (Q_PROD_WRAP(q) == Q_CONS_WRAP(q))
> + return prod - cons;
> + else
> + return WRAP_MASK(q) - cons + prod;
> +}
> +
> static inline bool smmuv3_q_full(SMMUQueue *q)
> {
> return ((q->cons ^ q->prod) & WRAP_INDEX_MASK(q)) == WRAP_MASK(q);
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 83159db1d4..e0f225d0df 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -1297,10 +1297,18 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
> SMMUCmdError cmd_error = SMMU_CERROR_NONE;
> SMMUQueue *q = &s->cmdq;
> SMMUCommandType type = 0;
> + SMMUCommandBatch batch = {};
> + uint32_t ncmds = 0;
init looks useless
> +
>
> if (!smmuv3_cmdq_enabled(s)) {
> return 0;
> }
> +
> + ncmds = smmuv3_q_ncmds(q);
> + batch.cmds = g_new0(Cmd, ncmds);
> + batch.cons = g_new0(uint32_t, ncmds);
> +
> /*
> * some commands depend on register values, typically CR0. In case those
> * register values change while handling the command, spec says it
> @@ -1395,6 +1403,13 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
>
> trace_smmuv3_cmdq_cfgi_cd(sid);
> smmuv3_flush_config(sdev);
> +
> + if (smmuv3_accel_batch_cmds(sdev->smmu, sdev, &batch, &cmd,
> + &q->cons, true)) {
> + cmd_error = SMMU_CERROR_ILL;
I understand you collect all batchable commands all together (those
sharing the same dev_cache prop) and the batch is executed either when
the cache target changes or at the very end of the queue consumption.
Since you don't batch all kinds of commands don't you have a risk to
send commands out of order?
Eric
> + break;
> + }
> +
> break;
> }
> case SMMU_CMD_TLBI_NH_ASID:
> @@ -1418,6 +1433,13 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
> trace_smmuv3_cmdq_tlbi_nh_asid(asid);
> smmu_inv_notifiers_all(&s->smmu_state);
> smmu_iotlb_inv_asid_vmid(bs, asid, vmid);
> +
> + if (smmuv3_accel_batch_cmds(bs, NULL, &batch, &cmd, &q->cons,
> + false)) {
> + cmd_error = SMMU_CERROR_ILL;
> + break;
> + }
> +
> break;
> }
> case SMMU_CMD_TLBI_NH_ALL:
> @@ -1445,6 +1467,12 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
> trace_smmuv3_cmdq_tlbi_nsnh();
> smmu_inv_notifiers_all(&s->smmu_state);
> smmu_iotlb_inv_all(bs);
> +
> + if (smmuv3_accel_batch_cmds(bs, NULL, &batch, &cmd, &q->cons,
> + false)) {
> + cmd_error = SMMU_CERROR_ILL;
> + break;
> + }
> break;
> case SMMU_CMD_TLBI_NH_VAA:
> case SMMU_CMD_TLBI_NH_VA:
> @@ -1453,7 +1481,24 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
> break;
> }
> smmuv3_range_inval(bs, &cmd, SMMU_STAGE_1);
> +
> + if (smmuv3_accel_batch_cmds(bs, NULL, &batch, &cmd, &q->cons,
> + false)) {
> + cmd_error = SMMU_CERROR_ILL;
> + break;
> + }
> + break;
> + case SMMU_CMD_ATC_INV:
> + {
> + SMMUDevice *sdev = smmu_find_sdev(bs, CMD_SID(&cmd));
> +
> + if (smmuv3_accel_batch_cmds(sdev->smmu, sdev, &batch, &cmd,
> + &q->cons, true)) {
> + cmd_error = SMMU_CERROR_ILL;
> + break;
> + }
> break;
> + }
> case SMMU_CMD_TLBI_S12_VMALL:
> {
> int vmid = CMD_VMID(&cmd);
> @@ -1485,7 +1530,6 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
> case SMMU_CMD_TLBI_EL2_ASID:
> case SMMU_CMD_TLBI_EL2_VA:
> case SMMU_CMD_TLBI_EL2_VAA:
> - case SMMU_CMD_ATC_INV:
> case SMMU_CMD_PRI_RESP:
> case SMMU_CMD_RESUME:
> case SMMU_CMD_STALL_TERM:
> @@ -1511,12 +1555,24 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
> queue_cons_incr(q);
> }
>
> + qemu_mutex_lock(&s->mutex);
> + if (!cmd_error && batch.ncmds) {
> + if (smmuv3_accel_issue_cmd_batch(bs, &batch)) {
> + q->cons = batch.cons[batch.ncmds];
> + cmd_error = SMMU_CERROR_ILL;
> + }
> + }
> + qemu_mutex_unlock(&s->mutex);
> +
> if (cmd_error) {
> trace_smmuv3_cmdq_consume_error(smmu_cmd_string(type), cmd_error);
> smmu_write_cmdq_err(s, cmd_error);
> smmuv3_trigger_irq(s, SMMU_IRQ_GERROR, R_GERROR_CMDQ_ERR_MASK);
> }
>
> + g_free(batch.cmds);
> + g_free(batch.cons);
> +
> trace_smmuv3_cmdq_consume_out(Q_PROD(q), Q_CONS(q),
> Q_PROD_WRAP(q), Q_CONS_WRAP(q));
>
Thanks
Eric
On Wed, Mar 26, 2025 at 03:16:18PM +0100, Eric Auger wrote:
> > @@ -1395,6 +1403,13 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
> >
> > trace_smmuv3_cmdq_cfgi_cd(sid);
> > smmuv3_flush_config(sdev);
> > +
> > + if (smmuv3_accel_batch_cmds(sdev->smmu, sdev, &batch, &cmd,
> > + &q->cons, true)) {
> > + cmd_error = SMMU_CERROR_ILL;
> I understand you collect all batchable commands all together (those
> sharing the same dev_cache prop) and the batch is executed either when
> the cache target changes or at the very end of the queue consumption.
> Since you don't batch all kinds of commands don't you have a risk to
> send commands out of order?
Yes, that could happen. But would it have some real risk?
This practice has an assumption that the guest OS would group
each batch with a proper CMD_SYNC like Linux does. So it could
reduce the amount of ioctls. If we can think of some real risk
when the guest OS doesn't, yes, I think we would have to flush
the batch if any non-accel command appear in-between.
Thanks
Nicolin
On 3/26/25 8:27 PM, Nicolin Chen wrote:
> On Wed, Mar 26, 2025 at 03:16:18PM +0100, Eric Auger wrote:
>>> @@ -1395,6 +1403,13 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
>>>
>>> trace_smmuv3_cmdq_cfgi_cd(sid);
>>> smmuv3_flush_config(sdev);
>>> +
>>> + if (smmuv3_accel_batch_cmds(sdev->smmu, sdev, &batch, &cmd,
>>> + &q->cons, true)) {
>>> + cmd_error = SMMU_CERROR_ILL;
>> I understand you collect all batchable commands all together (those
>> sharing the same dev_cache prop) and the batch is executed either when
>> the cache target changes or at the very end of the queue consumption.
>> Since you don't batch all kinds of commands don't you have a risk to
>> send commands out of order?
> Yes, that could happen. But would it have some real risk?
OK. I don't know but this needs to be studied.
Eric
>
> This practice has an assumption that the guest OS would group
> each batch with a proper CMD_SYNC like Linux does. So it could
> reduce the amount of ioctls. If we can think of some real risk
> when the guest OS doesn't, yes, I think we would have to flush
> the batch if any non-accel command appear in-between.
>
> Thanks
> Nicolin
>
© 2016 - 2025 Red Hat, Inc.