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

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: Nicolin Chen <nicol...@nvidia.com>
> Sent: Tuesday, July 15, 2025 6:23 PM
> To: Jonathan Cameron <jonathan.came...@huawei.com>
> Cc: Shameerali Kolothum Thodi
> <shameerali.kolothum.th...@huawei.com>; Linuxarm
> <linux...@huawei.com>; qemu-...@nongnu.org; qemu-
> de...@nongnu.org; eric.au...@redhat.com; peter.mayd...@linaro.org;
> j...@nvidia.com; ddut...@redhat.com; berra...@redhat.com;
> nath...@nvidia.com; mo...@nvidia.com; smost...@google.com;
> Wangzhou (B) <wangzh...@hisilicon.com>; jiangkunkun
> <jiangkun...@huawei.com>; zhangfei....@linaro.org;
> zhenzhong.d...@intel.com; shameerkolot...@gmail.com
> Subject: Re: [RFC PATCH v3 13/15] hw/arm/smmuv3: Forward invalidation
> commands to hw
> 
> 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.

Yes. Will add.

> 
> > > +
> >
> > > +    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.

Yeah. I was not sure on this. Also on setting current cons pointer in case 
IOCTL 
return for some reason other than attempting the CMD. I will double check
this.
 
Thanks,
Shameer

> > > +            qemu_log_mask(LOG_GUEST_ERROR, "Illegal command type:
> %d\n",
> > > +                          CMD_TYPE(&batch.cmds[batch.ncmds]));
> > > +            cmd_error = SMMU_CERROR_ILL;
> 
> Thanks
> Nicolin