Hi Shameer, Nicolin,
On 3/11/25 3:10 PM, Shameer Kolothum wrote:
> From: Nicolin Chen <nicol...@nvidia.com>
>
> Inroduce an SMMUCommandBatch and some helpers to batch and issue the
> commands. Currently separate out TLBI commands and device cache commands
can you precise which "device cache commands" you are talking about?
> to avoid some errata on certain versions of SMMUs. Later it should check
worth to give more details about this famous errata here.
> IIDR register to detect if underlying SMMU hw has such an erratum.
>
> Signed-off-by: Nicolin Chen <nicol...@nvidia.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.th...@huawei.com>
> ---
> hw/arm/smmuv3-accel.c | 69 ++++++++++++++++++++++++++++++++++++++++
> hw/arm/smmuv3-internal.h | 29 +++++++++++++++++
> 2 files changed, 98 insertions(+)
>
> diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c
> index 76134d106a..09be838d22 100644
> --- a/hw/arm/smmuv3-accel.c
> +++ b/hw/arm/smmuv3-accel.c
> @@ -160,6 +160,75 @@ void smmuv3_accel_install_nested_ste(SMMUDevice *sdev,
> int sid)
> nested_data.ste[0]);
> }
>
> +/* 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. Do you need to report a cmd_error as we do for some
other cmds?
> + 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.
> + 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?
> + return 0;
> + }
> + }
> +
> + /*
> + * Currently separate out dev_cache and hwpt for safety, which might
> + * not be necessary if underlying HW SMMU does not have the errata.
> + *
> + * TODO check IIDR register values read from hw_info.
> + */
> + 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;
> + }
> + }
> + batch->dev_cache = dev_cache;
> + batch->cmds[batch->ncmds] = *cmd;
> + batch->cons[batch->ncmds++] = *cons;
> + return 0;
> +}
> +
> static bool
> smmuv3_accel_dev_attach_viommu(SMMUv3AccelDevice *accel_dev,
> HostIOMMUDeviceIOMMUFD *idev, Error **errp)
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index 46c8bcae14..4602ae6728 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -549,13 +549,42 @@ typedef struct CD {
> uint32_t word[16];
> } CD;
>
> +/**
> + * 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
> + * @dev_cache: Issue to a device cache
indicate whether the invalidation command batch targets device cache?
> + */
> +typedef struct SMMUCommandBatch {
> + Cmd *cmds;
> + uint32_t *cons;
> + uint32_t ncmds;
> + bool dev_cache;
> +} SMMUCommandBatch;
> +
> int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste,
> SMMUEventInfo *event);
> void smmuv3_flush_config(SMMUDevice *sdev);
>
> #if defined(CONFIG_ARM_SMMUV3_ACCEL) && defined(CONFIG_IOMMUFD)
> +int smmuv3_accel_issue_cmd_batch(SMMUState *bs, SMMUCommandBatch *batch);
> +int smmuv3_accel_batch_cmds(SMMUState *bs, SMMUDevice *sdev,
> + SMMUCommandBatch *batch, Cmd *cmd,
> + uint32_t *cons, bool dev_cache);
> void smmuv3_accel_install_nested_ste(SMMUDevice *sdev, int sid);
> #else
> +static inline int smmuv3_accel_issue_cmd_batch(SMMUState *bs,
> + SMMUCommandBatch *batch)
> +{
> + return 0;
> +}
> +static inline int smmuv3_accel_batch_cmds(SMMUState *bs, SMMUDevice *sdev,
> + SMMUCommandBatch *batch, Cmd *cmd,
> + uint32_t *cons, bool dev_cache)
> +{
> + return 0;
> +}
> static inline void smmuv3_accel_install_nested_ste(SMMUDevice *sdev, int sid)
> {
> }
Thanks
Eric
