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

[Shameerali Kolothum Thodi via <qemu-devel@nongnu.org>]

> -----Original Message-----
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Tuesday, July 15, 2025 6:23 PM
> To: Jonathan Cameron <jonathan.cameron@huawei.com>
> Cc: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; Linuxarm
> <linuxarm@huawei.com>; qemu-arm@nongnu.org; qemu-
> devel@nongnu.org; eric.auger@redhat.com; peter.maydell@linaro.org;
> jgg@nvidia.com; ddutile@redhat.com; berrange@redhat.com;
> nathanc@nvidia.com; mochs@nvidia.com; smostafa@google.com;
> Wangzhou (B) <wangzhou1@hisilicon.com>; jiangkunkun
> <jiangkunkun@huawei.com>; zhangfei.gao@linaro.org;
> zhenzhong.duan@intel.com; shameerkolothum@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