Re: [RFC PATCH v3 13/15] hw/arm/smmuv3: Forward invalidation commands to hw

[Nicolin Chen <nicolinc@nvidia.com>]

On Mon, Sep 08, 2025 at 05:22:35AM -0700, Shameer Kolothum wrote:
> > -----Original Message-----
> > From: Eric Auger <eric.auger@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