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

[Nicolin Chen <nicolinc@nvidia.com>]

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