Re: [PATCH v3 29/32] KVM: arm64: Wrap the host with a stage 2

From: Quentin Perret <qperret@google.com>
To: Will Deacon <will@kernel.org>
Cc: catalin.marinas@arm.com, maz@kernel.org, james.morse@arm.com,
	julien.thierry.kdev@gmail.com, suzuki.poulose@arm.com,
	android-kvm@google.com, linux-kernel@vger.kernel.org,
	kernel-team@android.com, kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel@lists.infradead.org, tabba@google.com,
	mark.rutland@arm.com, dbrazdil@google.com, mate.toth-pal@arm.com,
	seanjc@google.com, robh+dt@kernel.org
Subject: Re: [PATCH v3 29/32] KVM: arm64: Wrap the host with a stage 2
Date: Mon, 8 Mar 2021 13:38:07 +0000	[thread overview]
Message-ID: <YEYov+r6A3uil1aU@google.com> (raw)
In-Reply-To: <20210308124606.GA25879@willie-the-truck>

On Monday 08 Mar 2021 at 12:46:07 (+0000), Will Deacon wrote:
> __load_stage2() _only_ has the ISB if ARM64_WORKAROUND_SPECULATIVE_AT is
> present, whereas I think you need one unconditionall here so that the
> system register write has taken effect before the TLB invalidation.
> 
> It's similar to the comment at the end of __tlb_switch_to_guest().
> 
> Having said that, I do worry that ARM64_WORKAROUND_SPECULATIVE_AT probably
> needs a closer look in the world of pKVM, since it currently special-cases
> the host.

Yes, I see that now. I'll start looking into this.

> > > > +	__tlbi(vmalls12e1is);
> > > > +	dsb(ish);
> > > 
> > > Given that the TLB is invalidated on the boot path, please can you add
> > > a comment here about the stale entries which you need to invalidate?
> > 
> > Sure -- that is for HCR bits cached in TLBs for VMID 0. Thinking about
> > it I could probably reduce the tlbi scope as well.
> > 
> > > Also, does this need to be inner-shareable? I thought this function ran on
> > > each CPU.
> > 
> > Hmm, correct, nsh should do.
> 
> Cool, then you can do that for both the TLBI and the DSB instructions (and
> please add a comment that the invalidation is due to the HCR bits).

Sure.

> > > > +static void host_stage2_unmap_dev_all(void)
> > > > +{
> > > > +	struct kvm_pgtable *pgt = &host_kvm.pgt;
> > > > +	struct memblock_region *reg;
> > > > +	u64 addr = 0;
> > > > +	int i;
> > > > +
> > > > +	/* Unmap all non-memory regions to recycle the pages */
> > > > +	for (i = 0; i < hyp_memblock_nr; i++, addr = reg->base + reg->size) {
> > > > +		reg = &hyp_memory[i];
> > > > +		kvm_pgtable_stage2_unmap(pgt, addr, reg->base - addr);
> > > > +	}
> > > > +	kvm_pgtable_stage2_unmap(pgt, addr, ULONG_MAX);
> > > 
> > > Is this just going to return -ERANGE?
> > 
> > Hrmpf, yes, that wants BIT(pgt->ia_bits) I think. And that wants testing
> > as well, clearly.
> 
> Agreed, maybe it's worth checking the return value.

Ack, and hyp_panic if != 0, but that is probably preferable anyway.

> > > > +static int host_stage2_idmap(u64 addr)
> > > > +{
> > > > +	enum kvm_pgtable_prot prot = KVM_PGTABLE_PROT_R | KVM_PGTABLE_PROT_W;
> > > > +	struct kvm_mem_range range;
> > > > +	bool is_memory = find_mem_range(addr, &range);
> > > > +	struct hyp_pool *pool = is_memory ? &host_s2_mem : &host_s2_dev;
> > > > +	int ret;
> > > > +
> > > > +	if (is_memory)
> > > > +		prot |= KVM_PGTABLE_PROT_X;
> > > > +
> > > > +	hyp_spin_lock(&host_kvm.lock);
> > > > +	ret = kvm_pgtable_stage2_idmap_greedy(&host_kvm.pgt, addr, prot,
> > > > +					      &range, pool);
> > > > +	if (is_memory || ret != -ENOMEM)
> > > > +		goto unlock;
> > > > +	host_stage2_unmap_dev_all();
> > > > +	ret = kvm_pgtable_stage2_idmap_greedy(&host_kvm.pgt, addr, prot,
> > > > +					      &range, pool);
> > > 
> > > I find this _really_ hard to reason about, as range is passed by reference
> > > and we don't reset it after the first call returns -ENOMEM for an MMIO
> > > mapping. Maybe some commentary on why it's still valid here?
> > 
> > Sure, I'll add something. FWIW, that is intended -- -ENOMEM can only be
> > caused by the call to kvm_pgtable_stage2_map() which leaves the range
> > untouched. So, as long as we don't release the lock, the range returned
> > by the first call to kvm_pgtable_stage2_idmap_greedy() should still be
> > valid. I suppose I could call kvm_pgtable_stage2_map() directly the
> > second time to make it obvious but I thought this would expose the
> > internal of kvm_pgtable_stage2_idmap_greedy() a little bit too much.
> 
> I can see it both ways, but updating the kerneldoc for
> kvm_pgtable_stage2_idmap_greedy() to state in which cases the range is
> updated and how would be helpful. It just says "negative error code on
> failure" at the moment.

Alternatively I could expose the 'reduce' path (maybe with another name
e.g. stage2_find_compatible_range() or so) and remove the
kvm_pgtable_stage2_idmap_greedy() wrapper. So it'd be the caller's
responsibility to not release the lock in between
stage2_find_compatible_range() and kvm_pgtable_stage2_map() for
instance, but that sounds reasonable to me. And that would make it
explicit it's the _map() path that failed with -ENOMEM, and that the
range can be re-used the second time.

Thoughts?

Thanks,
Quentin