On 3/26/25 8:16 PM, Nicolin Chen wrote:
> 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.
I was rather talking about the array of cmd itself but I guess it is
left unchanged. don't know if we are supposed to retry sending failed
cmds or not.
>
>> 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;
cons is updated but error is not logged in this patch.
> + }
> + }
>
>>> + 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?
Yes worth to dig in and add a comment
Thanks
Eric
>
>>> +/**
>>> + * 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
>
