On Mon, 14 Jul 2025 16:59:39 +0100
Shameer Kolothum <shameerali.kolothum.th...@huawei.com> wrote:
> From: Nicolin Chen <nicol...@nvidia.com>
>
> Use the provided smmuv3-accel helper functions to issue the
> invalidation commands to host SMMUv3.
>
> Signed-off-by: Nicolin Chen <nicol...@nvidia.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.th...@huawei.com>
> ---
> hw/arm/smmuv3-internal.h | 11 +++++++++++
> hw/arm/smmuv3.c | 28 ++++++++++++++++++++++++++++
> 2 files changed, 39 insertions(+)
>
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index 8cb6a9238a..f3aeaf6375 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -233,6 +233,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
Else doesn't add anything, also, it's qemu so {}
> + 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 c94bfe6564..97ecca0764 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -1285,10 +1285,17 @@ 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;
>
> 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);
Where is batch.ncmds set? It is cleared but I'm missing it being set to
anything.
> +
> + qemu_mutex_lock(&s->mutex);
> + if (!cmd_error && batch.ncmds) {
> + if (!smmuv3_accel_issue_cmd_batch(bs, &batch)) {
> + if (batch.ncmds) {
> + q->cons = batch.cons[batch.ncmds - 1];
> + } else {
> + q->cons = batch.cons[0]; /* FIXME: Check */
> + }
Totally non obvious that a return of false from the issue call means
illegal command type. Maybe that will be obvious form comments
requested in previous patch review.
> + qemu_log_mask(LOG_GUEST_ERROR, "Illegal command type: %d\n",
> + CMD_TYPE(&batch.cmds[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));
>
