[RFC PATCH 24/45] KVM: arm64: smmu-v3: Setup stream table

[Mostafa Saleh]

On Mon, Feb 26, 2024 at 02:13:52PM +0000, Jean-Philippe Brucker wrote:
> On Fri, Feb 16, 2024 at 12:19:01PM +0000, Mostafa Saleh wrote:
> > On Tue, Jan 23, 2024 at 7:45 PM Jean-Philippe Brucker
> > <jean-philippe at linaro.org> wrote:
> > >
> > > Hi Mostafa,
> > >
> > > On Tue, Jan 16, 2024 at 08:59:41AM +0000, Mostafa Saleh wrote:
> > > > > +__maybe_unused
> > > > > +static int smmu_sync_ste(struct hyp_arm_smmu_v3_device *smmu, u32 sid)
> > > > > +{
> > > > > +       struct arm_smmu_cmdq_ent cmd = {
> > > > > +               .opcode = CMDQ_OP_CFGI_STE,
> > > > > +               .cfgi.sid = sid,
> > > > > +               .cfgi.leaf = true,
> > > > > +       };
> > > > > +
> > > > > +       return smmu_send_cmd(smmu, &cmd);
> > > > > +}
> > > > > +
> > > > I see the page tables are properly configured for ARM_SMMU_FEAT_COHERENCY but no
> > > > handling for the STE or CMDQ, I believe here we should have something as:
> > > > if (!(smmu->features & ARM_SMMU_FEAT_COHERENCY))
> > > >         kvm_flush_dcache_to_poc(step, STRTAB_STE_DWORDS << 3);
> > > >
> > > > Similarly in "smmu_add_cmd" for the command queue. Or use NC mapping
> > > > (which doesn't exist
> > > > upstream as far as I can see)
> > >
> > > Right, the host driver seems to do this. If I'm following correctly we end
> > > up with dma_direct_alloc() calling pgprot_dmacoherent() and get
> > > MT_NORMAL_NC, when the SMMU is declared non-coherent in DT/IORT.
> > >
> > > So we'd get mismatched attributes if hyp is then mapping these structures
> > > cacheable, but I don't remember how that works exactly. Might be fine
> > > since host donates the pages to hyp and we'd have a cache flush in
> > > between. I'll have to read up on that.
> > 
> > I guess that is not enough, as the hypervisor writes the STE/CMDQ at any time.
> > 
> > > Regardless, mapping NC seems cleaner, more readable. I'll see if I can add
> > > that attribute to kvm_pgtable_hyp_map().
> > 
> > There is a patch for that already in Android
> > https://android.googlesource.com/kernel/common/+/636c912401dec4d178f6cdf6073f546b15828cf7%5E%21/#F0
> 
> Nice, I've added this (rather than CMO, to avoid mismatched attributes)
> but don't have the hardware to test it:
> 
> diff --git a/arch/arm64/kvm/hyp/nvhe/iommu/arm-smmu-v3.c b/arch/arm64/kvm/hyp/nvhe/iommu/arm-smmu-v3.c
> index 4b0b70017f59..e43011b51ef4 100644
> --- a/arch/arm64/kvm/hyp/nvhe/iommu/arm-smmu-v3.c
> +++ b/arch/arm64/kvm/hyp/nvhe/iommu/arm-smmu-v3.c
> @@ -268,12 +268,17 @@ static int smmu_init_registers(struct hyp_arm_smmu_v3_device *smmu)
>  }
>  
>  /* Transfer ownership of structures from host to hyp */
> -static void *smmu_take_pages(u64 base, size_t size)
> +static void *smmu_take_pages(struct hyp_arm_smmu_v3_device *smmu, u64 base,
> +			     size_t size)
>  {
>  	void *hyp_ptr;
> +	enum kvm_pgtable_prot prot = PAGE_HYP;
> +
> +	if (!(smmu->features & ARM_SMMU_FEAT_COHERENCY))
> +		prot |= KVM_PGTABLE_PROT_NC;
>  
>  	hyp_ptr = hyp_phys_to_virt(base);
> -	if (pkvm_create_mappings(hyp_ptr, hyp_ptr + size, PAGE_HYP))
> +	if (pkvm_create_mappings(hyp_ptr, hyp_ptr + size, prot))
>  		return NULL;
>  
>  	return hyp_ptr;
> @@ -293,7 +298,7 @@ static int smmu_init_cmdq(struct hyp_arm_smmu_v3_device *smmu)
>  	cmdq_size = cmdq_nr_entries * CMDQ_ENT_DWORDS * 8;
>  
>  	cmdq_base &= Q_BASE_ADDR_MASK;
> -	smmu->cmdq_base = smmu_take_pages(cmdq_base, cmdq_size);
> +	smmu->cmdq_base = smmu_take_pages(smmu, cmdq_base, cmdq_size);
>  	if (!smmu->cmdq_base)
>  		return -EINVAL;
>  
> @@ -350,7 +355,7 @@ static int smmu_init_strtab(struct hyp_arm_smmu_v3_device *smmu)
>  	}
>  
>  	strtab_base &= STRTAB_BASE_ADDR_MASK;
> -	smmu->strtab_base = smmu_take_pages(strtab_base, strtab_size);
> +	smmu->strtab_base = smmu_take_pages(smmu, strtab_base, strtab_size);
>  	if (!smmu->strtab_base)
>  		return -EINVAL;

Thanks, that is missing the L2 for the STE, but I guess for that we can
just CMO for now, as the HW doen't update it, unlike the CMDQ which must
be mapped as NC and CMO won't be enough.

I am investigating to see if we can map the memory donated from the host
on demand with differet prot, in that case iommu_donate_pages can return
memory with the different attributes.

Thanks,
Mostafa