Re: [RFC PATCH v2 13/20] hw/arm/smmuv3-accel: Introduce helpers to batch and issue cache invalidations

[Nicolin Chen <nicolinc@nvidia.com>]

On Wed, Mar 26, 2025 at 02:38:04PM +0100, Eric Auger wrote:
> > +/* Update batch->ncmds to the number of execute cmds */
> > +int smmuv3_accel_issue_cmd_batch(SMMUState *bs, SMMUCommandBatch *batch)
> > +{
> > +    SMMUv3AccelState *s_accel = ARM_SMMUV3_ACCEL(bs);
> > +    uint32_t total = batch->ncmds;
> > +    IOMMUFDViommu *viommu_core;
> > +    int ret;
> > +
> > +    if (!bs->accel) {
> > +        return 0;
> > +    }
> > +
> > +    if (!s_accel->viommu) {
> > +        return 0;
> > +    }
> > +    viommu_core = &s_accel->viommu->core;
> > +    ret = iommufd_backend_invalidate_cache(viommu_core->iommufd,
> > +                                           viommu_core->viommu_id,
> > +                                           IOMMU_VIOMMU_INVALIDATE_DATA_ARM_SMMUV3,
> > +                                           sizeof(Cmd), &batch->ncmds,
> > +                                           batch->cmds);
> > +    if (total != batch->ncmds) {
> > +        error_report("%s failed: ret=%d, total=%d, done=%d",
> > +                      __func__, ret, total, batch->ncmds);
> some commands may have been executed (batch->ncmds !=0). Is the
> batch_cmds array updated accordingly? In the kernel doc I don't see any
> mention of that.

The uAPI kdoc of ioctl(IOMMU_HWPT_INVALIDATE) mentions:
 * @entry_num: Input the number of cache invalidation requests in the array.
 *             Output the number of requests successfully handled by kernel.

> Do you need to report a cmd_error as we do for some
> other cmds?

Yes, we do. And we did (in this patch)? cons would be updated:
+    if (batch->ncmds && (dev_cache != batch->dev_cache)) {
+        ret = smmuv3_accel_issue_cmd_batch(bs, batch);
+        if (ret) {
+            *cons = batch->cons[batch->ncmds];
+            return ret;
+        }
+    }

> > +        return ret;
> > +    }
> > +
> > +    batch->ncmds = 0;
> > +    batch->dev_cache = false;
> > +    return ret;
> > +}
> > +
> > +int smmuv3_accel_batch_cmds(SMMUState *bs, SMMUDevice *sdev,
> I was confused by the name. The helper adds a single Cmd to the batch,
> right? so batch_cmd would better suited.

Yea, it could be "smmuv3_accel_batch_cmd".

> > +                            SMMUCommandBatch *batch, Cmd *cmd,
> > +                            uint32_t *cons, bool dev_cache)
> > +{
> > +    int ret;
> > +
> > +    if (!bs->accel) {
> > +        return 0;
> > +    }
> > +
> > +    if (sdev) {
> > +        SMMUv3AccelDevice *accel_dev;
> > +        accel_dev = container_of(sdev, SMMUv3AccelDevice, sdev);
> > +        if (!accel_dev->s1_hwpt) {

> can it happen? in the positive can you add some comment to describe in
> which condition?

I recall this is for device cache specifically (i.e. CGFI_CD[_ALL]
and ATC_INV) that I had in smmuv3_cmdq_consume(). Perhaps it gets
here because Shameer separated the accel code from the non-accel
smmuv3 file.

This condition is to check if the device is attached to an accel
HWPT, particularly to exclude commands being issued for emulated
devices. Surely, if a device isn't attached to an accel stage-1
HWPT any more, we probably shouldn't forward the commands to the
kernel? Though I start to suspect that we might need a lock for
accel_dev->s1_hwpt?

> > +/**
> > + * SMMUCommandBatch - batch of invalidation commands for smmuv3-accel
> > + * @cmds: Pointer to list of commands
> > + * @cons: Pointer to list of CONS corresponding to the commands
> > + * @ncmds: Total ncmds in the batch

> number of commands

OK.

> > + * @dev_cache: Issue to a device cache

> indicate whether the invalidation command batch targets device cache?

Maybe "invalidation command batch targeting device cache or TLB".

Thanks
Nicolin