On Mon, Sep 08, 2025 at 05:22:35AM -0700, Shameer Kolothum wrote:
> > -----Original Message-----
> > From: Eric Auger <eric.au...@redhat.com>
> > 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);
> > so you are provisionning space for n commands found in the queue,
> > independently on knowing whether they will be batched, ie. only
> > invalidation commands are. Then commands are added in the batch one by
> > one and you increment batch->ncmds in smmuv3_accel_batch_cmd. I agree
> > with Jonathan. This looks weird. AT least I would introduce a kelper
> > that inits a Back of ncmds and I would make all the batch fields
> > private. You you end up with the init +
> > smmuv3_accel_add_cmd_to_batch(batch, cmd). Then independently on the
> > ncmds you can issue a smmuv3_accel_issue_cmd_batch that would return if
> > there is nothing in the batch. You also need a batch deallocation
> > helper.
>
> Agree, at present we pre-allocate irrespective of whether there will any
> Invalidation cmds or not. I will take another look and incorporate your above
> suggestion to improve this.
Perhaps we can try a different data structure for those two batch
elements, e.g. using a GArray.
This allows an unknown "ncmd" at the beginning, so an invalidation
command will be just appended to the array in the _batch().
Then, in the _submit(), we can convert it to regular array:
Cmd *cmds = (Cmd *)g_array_free(batch->cmds, FALSE);
// passing cmds to ioctl
g_free(cmds);
> I remember I expressed in the past my concern about having
> > commands executed out of order. I don't remember out conclusion on that
> > but this shall be clearly studied and conclusion shall be put in the
> > commit message.
>
> Yes, you did, and I missed it. Sorry about that.
>
> I think it is safe to honour the execution order of Guest here. From a quick
> glance, I
> couldn’t find anything related to a safe out of order execution guidance from
> SMMUv3 specification. Also, we can't be sure how Guest will be
> modified/optimised
> in the future to completely rule out problems if we do out-of-order
> executions.
>
> Hence, my plan for next is to start batching if we see Invalidation cmds and
> submit
> the batch If any non-invalidation commands are encountered in between.
That sounds good to me.
> @Nicolin, do you foresee any issues with above approach? From the current
> Host SMMUV3 driver, batching of commands is mainly used for invalidations
> (except for certain arm_smmu_cmdq_issue_cmd_with_sync() cases). So
> I guess we are good from a performance optimisation point of view if we can
> cover invalidations as above.
Yes. It shouldn't impact perf so long as the guest OS is sane.
Nicolin
