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
