On Tue, Jul 15, 2025 at 11:46:09AM +0100, Jonathan Cameron wrote:
> > 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.
smmuv3_accel_batch_cmd() internally sets that, every time it's
invoked to add a new command in the batch.
Shameer, let's add some comments explaining the batch function.
> > +
>
> > + 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.
That's a good point. Shameer, I think we need some fine-grinding
here, validating the return value from the ioctl, for which the
kernel will only return -EIO or -ETIMEOUT on failure, indicating
either an SMMU_CERROR_ILL or an SMMU_CERROR_ATC_INV_SYNC.
> > + qemu_log_mask(LOG_GUEST_ERROR, "Illegal command type: %d\n",
> > + CMD_TYPE(&batch.cmds[batch.ncmds]));
> > + cmd_error = SMMU_CERROR_ILL;
Thanks
Nicolin
