Re: [PATCH v7 14/24] iommu/arm-smmu-v3-kvm: Shadow the command queue

[Mostafa Saleh <smostafa@google.com>]

Hi Seb,

On Thu, Jul 23, 2026 at 02:55:08PM +0000, Sebastian Ene wrote:
> On Wed, Jul 15, 2026 at 11:58:55AM +0000, Mostafa Saleh wrote:
> > +	 * size as the host.
> > +	 * Only populate base_dma and llq.max_n_shift, the hypervisor will init
> > +	 * the rest.
> > +	 */
> > +	cmdq_base = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, SMMU_KVM_CMDQ_ORDER);
> > +	if (!cmdq_base)
> > +		return -ENOMEM;
> 
> Hi Mostafa,
> 
> Isn't this over-allocating when PAGE_SIZE > 4kB ? 

Yes, that means the queue have different size depending on PAGE_SIZE,
In the kernel driver CMDQ_MAX_SZ_SHIFT is defined in terms of page
size also, but I think in the hypervisor it doesn't really matter,
I can change this to a fixed size and use alloc_pages_exact().

> 
> > +
> > +	smmu->cmdq.base_dma = virt_to_phys(cmdq_base);
> > +	smmu->cmdq.llq.max_n_shift = SMMU_KVM_CMDQ_ORDER + PAGE_SHIFT - CMDQ_ENT_SZ_SHIFT;
> > +
> >  	if (of_dma_is_coherent(dev->of_node))
> >  		smmu->features |= ARM_SMMU_FEAT_COHERENCY;
> >  
> > 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 af06c832fc6f..9f76f4e82341 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
> > @@ -11,7 +11,6 @@
> >  #include <nvhe/trap_handler.h>
> >  
> >  #include "arm_smmu_v3.h"
> > -#include "../arm-smmu-v3.h"
> >  
> >  size_t __ro_after_init kvm_hyp_arm_smmu_v3_count;
> >  struct hyp_arm_smmu_v3_device *kvm_hyp_arm_smmu_v3_smmus;
> > @@ -21,10 +20,68 @@ struct hyp_arm_smmu_v3_device *kvm_hyp_arm_smmu_v3_smmus;
> >  	     (smmu) != &kvm_hyp_arm_smmu_v3_smmus[kvm_hyp_arm_smmu_v3_count]; \
> >  	     (smmu)++)
> >  
> > +#define cmdq_size(cmdq)	((1 << ((cmdq)->llq.max_n_shift)) * CMDQ_ENT_DWORDS * 8)
> > +
> > +static bool is_cmdq_enabled(struct hyp_arm_smmu_v3_device *smmu)
> > +{
> > +	return FIELD_GET(CR0_CMDQEN, smmu->cr0);
> > +}
> > +
> > +/*
> > + * CMDQ, STE host copies are accessed by the hypervisor, we share them to
> > + * - Prevent the host from passing protected VM memory.
> > + * - Having them mapped in the hyp page table.
> > + */
> > +static int smmu_share_pages(phys_addr_t addr, size_t size)
> > +{
> > +	size_t nr_pages = PAGE_ALIGN(size + (addr & ~PAGE_MASK)) >> PAGE_SHIFT;
> > +	phys_addr_t base = addr & PAGE_MASK;
> > +	int i, ret;
> > +
> > +	for (i = 0 ; i < nr_pages ; ++i) {
> > +		if (__pkvm_host_share_hyp((base + i * PAGE_SIZE) >> PAGE_SHIFT)) {
> > +			while (i--)
> > +				__pkvm_host_unshare_hyp((base + i * PAGE_SIZE) >> PAGE_SHIFT);
> > +			return -EPERM;
> > +		}
> > +	}
> > +
> > +	ret = hyp_pin_shared_mem(hyp_phys_to_virt(base),
> > +				 hyp_phys_to_virt(base + nr_pages * PAGE_SIZE));
> > +	if (ret) {
> > +		for (i = 0 ; i < nr_pages ; ++i)
> > +			__pkvm_host_unshare_hyp((base + i * PAGE_SIZE) >> PAGE_SHIFT);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int smmu_unshare_pages(phys_addr_t addr, size_t size)
> > +{
> > +	size_t nr_pages = PAGE_ALIGN(size + (addr & ~PAGE_MASK)) >> PAGE_SHIFT;
> > +	phys_addr_t base = addr & PAGE_MASK;
> > +	int i, ret;
> > +
> > +	hyp_unpin_shared_mem(hyp_phys_to_virt(base),
> > +			     hyp_phys_to_virt(base + nr_pages * PAGE_SIZE));
> > +
> > +	for (i = 0 ; i < nr_pages ; ++i) {
> > +		ret = __pkvm_host_unshare_hyp((base + i * PAGE_SIZE) >> PAGE_SHIFT);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  /* Put the device in a state that can be probed by the host driver. */
> >  static void smmu_deinit_device(struct hyp_arm_smmu_v3_device *smmu)
> >  {
> >  	WARN_ON(__pkvm_hyp_donate_host_mmio(smmu->mmio_addr, smmu->mmio_size));
> > +
> > +	if (smmu->cmdq.base)
> > +		WARN_ON(__pkvm_hyp_donate_host(smmu->cmdq.base_dma >> PAGE_SHIFT,
> > +					       cmdq_size(&smmu->cmdq) >> PAGE_SHIFT));
> >  	smmu->base = NULL;
> >  }
> >  
> > @@ -75,6 +132,31 @@ static int smmu_probe(struct hyp_arm_smmu_v3_device *smmu)
> >  	return 0;
> >  }
> >  
> > +/*
> > + * The kernel part of the driver will allocate the shadow cmdq,
> > + * and zero it. This function only donates it.
> > + */
> > +static int smmu_init_cmdq(struct hyp_arm_smmu_v3_device *smmu)
> > +{
> > +	size_t cmdq_nr_pages = cmdq_size(&smmu->cmdq) >> PAGE_SHIFT;
> > +	int ret;
> > +
> > +	ret = __pkvm_host_donate_hyp(smmu->cmdq.base_dma >> PAGE_SHIFT, cmdq_nr_pages);
> > +	if (ret)
> > +		return ret;
> > +
> > +	smmu->cmdq.base = hyp_phys_to_virt(smmu->cmdq.base_dma);
> > +	smmu->cmdq.prod_reg = smmu->base + ARM_SMMU_CMDQ_PROD;
> > +	smmu->cmdq.cons_reg = smmu->base + ARM_SMMU_CMDQ_CONS;
> > +	smmu->cmdq.q_base = smmu->cmdq.base_dma |
> > +			    FIELD_PREP(Q_BASE_LOG2SIZE, smmu->cmdq.llq.max_n_shift);
> > +	smmu->cmdq.ent_dwords = CMDQ_ENT_DWORDS;
> > +	writel_relaxed(0, smmu->cmdq.prod_reg);
> > +	writel_relaxed(0, smmu->cmdq.cons_reg);
> > +	writeq_relaxed(smmu->cmdq.q_base, smmu->base + ARM_SMMU_CMDQ_BASE);
> 
> do we need a dsb here ?

I do not think so, why would it be needed? No data written at this
point. When commands are written, writel() is used to advance the
queue pointer which includes a barrier to enusre that the commands
are observed.

Thanks,
Mostafa


> 
> 
> Thanks,
> Sebastian