Re: [PATCH v2 3/3] kvmtool: arm64: Configure VM with the minimal required IPA space

[Marc Zyngier <maz@kernel.org>]

On Tue, 24 Aug 2021 01:47:34 +0100,
Andre Przywara <andre.przywara@arm.com> wrote:
> 
> On Sun, 22 Aug 2021 16:25:26 +0100
> Marc Zyngier <maz@kernel.org> wrote:
> 
> Hi Marc,
> 
> > There is some value in keeping the IPA space small, as it reduces
> > the size of the stage-2 page tables.
> > 
> > Let's compute the required space at VM creation time, and inform
> > the kernel of our requirements.
> 
> You mentioned some kernel bug in the first version of this patch, I
> guess the fix for this is 262b003d059c?
> It seems to me that this is somewhat of a regression on older host
> kernels, when trying to run a guest with "-m 2048", for instance?
> Should we teach kvmtool about this bug, and do a check for the bug
> condition, when kvm__register_ram() returns with -EFAULT? And give users
> a hint to try with one MB more or less guest RAM? Or maybe try
> this automatically?

I guess an additional patch could do a retry with one more bit of IPA
space.

However, the fix for this bug was backported to all stable kernels.
Given that the audience for kvmtool is pretty limited, I'd rather only
add this if someone actively complains.

> 
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arm/aarch64/kvm.c | 20 +++++++++++++++++++-
> >  1 file changed, 19 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arm/aarch64/kvm.c b/arm/aarch64/kvm.c
> > index d03a27f2..4e66a22e 100644
> > --- a/arm/aarch64/kvm.c
> > +++ b/arm/aarch64/kvm.c
> > @@ -3,6 +3,7 @@
> >  #include <asm/image.h>
> >  
> >  #include <linux/byteorder.h>
> > +#include <kvm/util.h>
> >  
> >  /*
> >   * Return the TEXT_OFFSET value that the guest kernel expects. Note
> > @@ -59,5 +60,22 @@ int kvm__arch_get_ipa_limit(struct kvm *kvm)
> >  
> >  int kvm__get_vm_type(struct kvm *kvm)
> >  {
> > -	return KVM_VM_TYPE_ARM_IPA_SIZE(kvm__arch_get_ipa_limit(kvm));
> > +	unsigned int ipa_bits, max_ipa_bits;
> > +	unsigned long max_ipa;
> > +
> > +	/* If we're running on an old kernel, use 0 as the VM type */
> > +	max_ipa_bits = kvm__arch_get_ipa_limit(kvm);
> > +	if (!max_ipa_bits)
> > +		return 0;
> 
> Should this return KVM_VM_TYPE, as it does at the moment? Or is this
> more confusing than helpful?

I think that'd be really confusing. If you wanted a name, it really
should be something like KVM_LEGACY_40BIT_IPA_DONT_EVER_USE.

> 
> Just a nit anyway, the patch looks correct otherwise:
> 
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Thanks,

	M.

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