Re: [PATCH v6 16/25] iommu/arm-smmu-v3-kvm: Add CMDQ functions

[Vincent Donnefort <vdonnefort@google.com>]

On Fri, May 01, 2026 at 11:19:18AM +0000, Mostafa Saleh wrote:
> Add functions to access the command queue, there are 2 main usage:
> - Hypervisor's own commands, as TLB invalidation, would use functions
>   as smmu_send_cmd(), which creates and sends a command.
> - Add host commands to the shadow command queue, after being filtered,
>   these will be added with smmu_add_cmd_raw.
> 
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  14 ++-
>  .../iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3.c  | 107 ++++++++++++++++++
>  2 files changed, 115 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> index f904f4d19609..3fc499608d76 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -1156,19 +1156,21 @@ u32 smmu_idr5_to_oas(u32 reg);
>  unsigned long smmu_idr5_to_pgsize(u32 reg);
>  
>  /* Queue functions shared between kernel and hyp. */
> -static inline bool queue_has_space(struct arm_smmu_ll_queue *q, u32 n)
> +static inline u32 queue_space(struct arm_smmu_ll_queue *q)
>  {
> -	u32 space, prod, cons;
> +	u32 prod, cons;
>  
>  	prod = Q_IDX(q, q->prod);
>  	cons = Q_IDX(q, q->cons);
>  
>  	if (Q_WRP(q, q->prod) == Q_WRP(q, q->cons))
> -		space = (1 << q->max_n_shift) - (prod - cons);
> -	else
> -		space = cons - prod;
> +		return (1 << q->max_n_shift) - (prod - cons);
> +	return cons - prod;
> +}
>  
> -	return space >= n;
> +static inline bool queue_has_space(struct arm_smmu_ll_queue *q, u32 n)
> +{
> +	return queue_space(q) >= n;
>  }
>  
>  static inline bool queue_full(struct arm_smmu_ll_queue *q)
> diff --git a/drivers/iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3.c
> index 3b77796dafc7..aac455599728 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/pkvm/arm-smmu-v3.c
> @@ -6,6 +6,7 @@
>   */
>  #include <asm/kvm_hyp.h>
>  
> +#include <nvhe/clock.h>
>  #include <nvhe/iommu.h>
>  #include <nvhe/mem_protect.h>
>  #include <nvhe/trap_handler.h>
> @@ -22,6 +23,31 @@ struct hyp_arm_smmu_v3_device *kvm_hyp_arm_smmu_v3_smmus;
>  
>  #define cmdq_size(cmdq)	((1 << ((cmdq)->llq.max_n_shift)) * CMDQ_ENT_DWORDS * 8)

I only looked at that smmu_wait() as this is the hyp_clock_ns() caller.

>  
> +/*
> + * Wait until @cond is true.
> + * Return 0 on success, or -ETIMEDOUT

nit: Would be nice to explain use_wfe. 

> + */
> +#define smmu_wait(use_wfe, _cond)					\

nit:, (__use_wfe, __cond), or (use_fw, cond)

> +({									\
> +	int __ret = 0;							\
> +	u64 delay = hyp_clock_ns() + ARM_SMMU_POLL_TIMEOUT_US * 1000;	\

Does it help to lower the risk for the 128-bits fallback if we keep the
resolution to _us?

Also, ARM_SMMU_POLL_TIMEOUT_US seems to be 1s. Blocking that long at EL2 is
really terrible. Any chance to lower that? 

> +									\
> +	while (!(_cond)) {						\
> +		if (use_wfe) {						\
> +			wfe();						\
> +			if ((_cond))					\

nit: I believe there's no need for the double () here is there?

> +				break;					\
> +		} else {						\
> +			cpu_relax();					\
> +		}							\
> +		if (hyp_clock_ns() >= delay) {				\
> +			__ret = -ETIMEDOUT;				\
> +			break;						\
> +		}							\
> +	}								\
> +	__ret;								\
> +})
> +

[...]