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

[Jean-Philippe Brucker <jean-philippe@linaro.org>]

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@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;