Re: [PATCH] KVM: arm64: Cap default IPA size to the host's own size

[Marc Zyngier <maz@kernel.org>]

Hi Suzuki,

On Tue, 09 Mar 2021 11:09:48 +0000,
Suzuki Poulose <suzuki.poulose@arm.com> wrote:
> 
> 
> 
> > On 8 Mar 2021, at 17:46, Marc Zyngier <maz@kernel.org> wrote:
> > 
> > KVM/arm64 has forever used a 40bit default IPA space, partially
> > due to its 32bit heritage (where the only choice is 40bit).
> > 
> > However, there are implementations in the wild that have a *cough*
> > much smaller *cough* IPA space, which leads to a misprogramming of
> > VTCR_EL2, and a guest that is stuck on its first memory access
> > if userspace dares to ask for the default IPA setting (which most
> > VMMs do).
> > 
> > Instead, cap the default IPA size to what the host can actually
> > do, and spit out a one-off message on the console. The boot warning
> > is turned into a more meaningfull message, and the new behaviour
> > is also documented.
> > 
> > Although this is a userspace ABI change, it doesn't really change
> > much for userspace:
> > 
> > - the guest couldn't run before this change, while it now has
> >  a chance to if the memory range fits the reduced IPA space
> > 
> > - a memory slot that was accepted because it did fit the default
> >  IPA space but didn't fit the HW constraints is now properly
> >  rejected
> > 
> > The other thing that's left doing is to convince userspace to
> > actually use the IPA space setting instead of relying on the
> > antiquated default.
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> 
> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>

Thanks for that. Whilst I have your attention and given that you are
responsible for most of the variable IPA stuff... ;-)

I think we have another issue around the handling of our IPA
size. Let's say I create a VM with a 32bit IPA space. If I register a
2GB memslot at 0x8000000, I'm getting an error, which I think is
bogus.

I came to the conclusion that kvm_arch_prepare_memory_region() is a
bit overzealous when rejecting the memslot, and I used the following
patchlet to address it.

Does this seem sensible to you?

	M.

diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 77cb2d28f2a4..8711894db8c2 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -1312,8 +1312,7 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
 	 * Prevent userspace from creating a memory region outside of the IPA
 	 * space addressable by the KVM guest IPA space.
 	 */
-	if (memslot->base_gfn + memslot->npages >=
-	    (kvm_phys_size(kvm) >> PAGE_SHIFT))
+	if ((memslot->base_gfn + memslot->npages) > (kvm_phys_size(kvm) >> PAGE_SHIFT))
 		return -EFAULT;
 
 	mmap_read_lock(current->mm);

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm