On 10/27/25 1:20 PM, Shameer Kolothum wrote:
> Hi Eric,
>
>> -----Original Message-----
>> From: Eric Auger <[email protected]>
>> Sent: 27 October 2025 10:14
>> To: Shameer Kolothum <[email protected]>; qemu-
>> [email protected]; [email protected]
>> Cc: [email protected]; Jason Gunthorpe <[email protected]>; Nicolin
>> Chen <[email protected]>; [email protected]; [email protected];
>> Nathan Chen <[email protected]>; Matt Ochs <[email protected]>;
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]
>> Subject: Re: [PATCH v4 13/27] hw/arm/smmuv3-accel: Add support to issue
>> invalidation cmd to host
>>
>> External email: Use caution opening links or attachments
>>
>>
>> Hi Shameer,
>>
>> On 9/29/25 3:36 PM, Shameer Kolothum wrote:
>>> Provide a helper and use that to issue the invalidation cmd to host SMMUv3.
>>> We only issue one cmd at a time for now.
>>>
>>> Support for batching of commands will be added later after analysing
>>> the impact.
>>>
>>> Signed-off-by: Shameer Kolothum <[email protected]>
>>> ---
>>> hw/arm/smmuv3-accel.c | 38
>> ++++++++++++++++++++++++++++++++++++++
>>> hw/arm/smmuv3-accel.h | 8 ++++++++
>>> hw/arm/smmuv3.c | 30 ++++++++++++++++++++++++++++++
>>> 3 files changed, 76 insertions(+)
>>>
>>> diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c index
>>> f4e01fba6d..9ad8595ce2 100644
>>> --- a/hw/arm/smmuv3-accel.c
>>> +++ b/hw/arm/smmuv3-accel.c
>>> @@ -218,6 +218,44 @@ bool
>> smmuv3_accel_install_nested_ste_range(SMMUv3State *s, SMMUSIDRange
>> *range,
>>> return true;
>>> }
>>>
>>> +/*
>>> + * This issues the invalidation cmd to the host SMMUv3.
>>> + * Note: sdev can be NULL for certain invalidation commands
>>> + * e.g., SMMU_CMD_TLBI_NH_ASID, SMMU_CMD_TLBI_NH_VA etc.
>>> + */
>>> +bool smmuv3_accel_issue_inv_cmd(SMMUv3State *bs, void *cmd,
>> SMMUDevice *sdev,
>>> + Error **errp) {
>>> + SMMUv3State *s = ARM_SMMUV3(bs);
>>> + SMMUv3AccelState *s_accel = s->s_accel;
>>> + IOMMUFDViommu *viommu_core;
>>> + uint32_t entry_num = 1;
>>> +
>>> + if (!s->accel || !s_accel->viommu) {
>> Can you explain/document why !s_accel->viommu is handled as no error?
> !s_accel->viommu basically means there are no vfio-pci devices with iommufd
> backend. I will add a comment.
>
>>> + return true;
>>> + }
>>> +
>>> + /*
>>> + * We may end up here for any emulated PCI bridge or root port type
>> devices.
>>> + * However, passing invalidation commands with sid (eg: CFGI_CD) to host
>>> + * SMMUv3 only matters for vfio-pci endpoint devices. Hence check that
>>> if
>>> + * sdev is valid.
>> Only propagate errors to host if the SDEV matches a VFIO devices?
> The vdev is only allocated for a valid idev device. The below !accel_dev->vdev
> checks that.
yes. I was suggesting a rephrasing
>
>>> + */
>>> + if (sdev) {
>>> + SMMUv3AccelDevice *accel_dev = container_of(sdev,
>> SMMUv3AccelDevice,
>>> + sdev);
>>> + if (!accel_dev->vdev) {
>>> + return true;
>>> + }
>>> + }
>>> +
>>> + viommu_core = &s_accel->viommu->core;
>>> + return iommufd_backend_invalidate_cache(
>>> + viommu_core->iommufd, viommu_core->viommu_id,
>>> + IOMMU_VIOMMU_INVALIDATE_DATA_ARM_SMMUV3,
>>> + sizeof(Cmd), &entry_num, cmd, errp);
>> what shall we do if iommufd_backend_invalidate_cache() succeeds with
>> output entry_num is different from onput one? In current case we have
>> entry_num = 1 so maybe this is not possible but we might leave a comment at
>> least?
> Ok. I will add that to comment.
>
>>> +}
>>> +
>>> static SMMUv3AccelDevice *smmuv3_accel_get_dev(SMMUState *bs,
>> SMMUPciBus *sbus,
>>> PCIBus *bus, int
>>> devfn) { diff --git a/hw/arm/smmuv3-accel.h b/hw/arm/smmuv3-accel.h
>>> index 6242614c00..3bdba47616 100644
>>> --- a/hw/arm/smmuv3-accel.h
>>> +++ b/hw/arm/smmuv3-accel.h
>>> @@ -46,6 +46,8 @@ bool smmuv3_accel_install_nested_ste(SMMUv3State
>> *s, SMMUDevice *sdev, int sid,
>>> Error **errp); bool
>>> smmuv3_accel_install_nested_ste_range(SMMUv3State *s,
>> SMMUSIDRange *range,
>>> Error **errp);
>>> +bool smmuv3_accel_issue_inv_cmd(SMMUv3State *s, void *cmd,
>> SMMUDevice *sdev,
>>> + Error **errp);
>>> #else
>>> static inline void smmuv3_accel_init(SMMUv3State *s) { @@ -62,6
>>> +64,12 @@ smmuv3_accel_install_nested_ste_range(SMMUv3State *s,
>>> SMMUSIDRange *range, {
>>> return true;
>>> }
>>> +static inline bool
>>> +smmuv3_accel_issue_inv_cmd(SMMUv3State *s, void *cmd, SMMUDevice
>> *sdev,
>>> + Error **errp) {
>>> + return true;
>>> +}
>>> #endif
>>>
>>> #endif /* HW_ARM_SMMUV3_ACCEL_H */
>>> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c index
>>> 1fd8aaa0c7..3963bdc87f 100644
>>> --- a/hw/arm/smmuv3.c
>>> +++ b/hw/arm/smmuv3.c
>>> @@ -1381,6 +1381,7 @@ static int
>> smmuv3_cmdq_consume(SMMUv3State *s)
>>> {
>>> uint32_t sid = CMD_SID(&cmd);
>>> SMMUDevice *sdev = smmu_find_sdev(bs, sid);
>>> + Error *local_err = NULL;
>>>
>>> if (CMD_SSEC(&cmd)) {
>>> cmd_error = SMMU_CERROR_ILL; @@ -1393,11 +1394,17 @@
>>> static int smmuv3_cmdq_consume(SMMUv3State *s)
>>>
>>> trace_smmuv3_cmdq_cfgi_cd(sid);
>>> smmuv3_flush_config(sdev);
>>> + if (!smmuv3_accel_issue_inv_cmd(s, &cmd, sdev, &local_err)) {
>>> + error_report_err(local_err);
>>> + cmd_error = SMMU_CERROR_ILL;
>>> + break;
>>> + }
>>> break;
>>> }
>>> case SMMU_CMD_TLBI_NH_ASID:
>>> {
>>> int asid = CMD_ASID(&cmd);
>>> + Error *local_err = NULL;
>>> int vmid = -1;
>>>
>>> if (!STAGE1_SUPPORTED(s)) { @@ -1416,6 +1423,11 @@ static
>>> int smmuv3_cmdq_consume(SMMUv3State *s)
>>> trace_smmuv3_cmdq_tlbi_nh_asid(asid);
>>> smmu_inv_notifiers_all(&s->smmu_state);
>>> smmu_iotlb_inv_asid_vmid(bs, asid, vmid);
>>> + if (!smmuv3_accel_issue_inv_cmd(s, &cmd, NULL, &local_err)) {
>>> + error_report_err(local_err);
>>> + cmd_error = SMMU_CERROR_ILL;
>>> + break;
>>> + }
>>> break;
>>> }
>>> case SMMU_CMD_TLBI_NH_ALL:
>> do we check somewhere that accel mode only applies to stage=1?
> See patch #7. We mandate stage-1 for accel=on case.
Yes. I noticed that later on. thanks
Eric
>
> Thanks,
> Shameer
>
