[PATCH v2 0/3] kvmtool: Limit IPA space to what is actually required

[PATCH v2 0/3] kvmtool: Limit IPA space to what is actually required

[Marc Zyngier]

KVM hacking on the Apple M1 SoC has shown that kvmtool (and other
VMMs) make pretty poor use of the IPA space parameter (read: do not
use it and just pass 0). This results in a guest that cannot boot
(recent kernels will just send the VMM packing), and in general means
we don't benefit from smaller page tables at stage-2.

This series does three things:
- It switches kvmtool away from the default 40bit, allowing large VMs
  to be created (I have booted a 4TB VM)
- It reduces the requested IPA space to be as small as possible
- It tells the user why the VM cannot boot when the IPA space required
  exceeds that of the HW

With these changes, kvmtool is able to spawn a VM on IPA-challenged
systems such the Apple M1.

* From v1:
  - Use KVM_VM_TYPE_ARM_IPA_SIZE()
  - Rebased on a recent HEAD

Marc Zyngier (3):
  kvmtool: Abstract KVM_VM_TYPE into a weak function
  kvmtool: arm64: Use the maximum supported IPA size when creating the
    VM
  kvmtool: arm64: Configure VM with the minimal required IPA space

 arm/aarch64/include/kvm/kvm-arch.h | 19 ++++++++++++++---
 arm/aarch64/kvm.c                  | 33 ++++++++++++++++++++++++++++++
 include/kvm/kvm.h                  |  1 +
 kvm.c                              |  7 ++++++-
 4 files changed, 56 insertions(+), 4 deletions(-)

-- 
2.30.2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v2 1/3] kvmtool: Abstract KVM_VM_TYPE into a weak function

[Marc Zyngier]

Most architectures pass a fixed value for their VM type. However,
arm64 uses it as a parameter describing the size of the guest's
physical address space.

In order to support this, introduce a kvm__get_vm_type() helper
that only returns KVM_VM_TYPE for now.

Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 include/kvm/kvm.h | 1 +
 kvm.c             | 7 ++++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
index 56e9c8e3..ad732e56 100644
--- a/include/kvm/kvm.h
+++ b/include/kvm/kvm.h
@@ -114,6 +114,7 @@ int kvm__init(struct kvm *kvm);
 struct kvm *kvm__new(void);
 int kvm__recommended_cpus(struct kvm *kvm);
 int kvm__max_cpus(struct kvm *kvm);
+int kvm__get_vm_type(struct kvm *kvm);
 void kvm__init_ram(struct kvm *kvm);
 int kvm__exit(struct kvm *kvm);
 bool kvm__load_firmware(struct kvm *kvm, const char *firmware_filename);
diff --git a/kvm.c b/kvm.c
index e327541d..5bc66c8b 100644
--- a/kvm.c
+++ b/kvm.c
@@ -428,6 +428,11 @@ int kvm__max_cpus(struct kvm *kvm)
 	return ret;
 }
 
+int __attribute__((weak)) kvm__get_vm_type(struct kvm *kvm)
+{
+	return KVM_VM_TYPE;
+}
+
 int kvm__init(struct kvm *kvm)
 {
 	int ret;
@@ -461,7 +466,7 @@ int kvm__init(struct kvm *kvm)
 		goto err_sys_fd;
 	}
 
-	kvm->vm_fd = ioctl(kvm->sys_fd, KVM_CREATE_VM, KVM_VM_TYPE);
+	kvm->vm_fd = ioctl(kvm->sys_fd, KVM_CREATE_VM, kvm__get_vm_type(kvm));
 	if (kvm->vm_fd < 0) {
 		pr_err("KVM_CREATE_VM ioctl");
 		ret = kvm->vm_fd;
-- 
2.30.2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v2 2/3] kvmtool: arm64: Use the maximum supported IPA size when creating the VM

[Marc Zyngier]

Instead of just asking the the default VM size, request the maximum
IPA size to the kernel, and use this at VM creation time.

The IPA space is parametrized accordingly.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arm/aarch64/include/kvm/kvm-arch.h | 19 ++++++++++++++++---
 arm/aarch64/kvm.c                  | 15 +++++++++++++++
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/arm/aarch64/include/kvm/kvm-arch.h b/arm/aarch64/include/kvm/kvm-arch.h
index 55ef8ed1..159567b9 100644
--- a/arm/aarch64/include/kvm/kvm-arch.h
+++ b/arm/aarch64/include/kvm/kvm-arch.h
@@ -3,10 +3,23 @@
 
 struct kvm;
 unsigned long long kvm__arch_get_kern_offset(struct kvm *kvm, int fd);
+int kvm__arch_get_ipa_limit(struct kvm *kvm);
 
-#define ARM_MAX_MEMORY(kvm)	((kvm)->cfg.arch.aarch32_guest	?	\
-				ARM_LOMAP_MAX_MEMORY		:	\
-				ARM_HIMAP_MAX_MEMORY)
+#define ARM_MAX_MEMORY(kvm)	({					\
+	u64 max_ram;							\
+									\
+	if ((kvm)->cfg.arch.aarch32_guest) {				\
+		max_ram = ARM_LOMAP_MAX_MEMORY;				\
+	} else {							\
+		int ipabits = kvm__arch_get_ipa_limit(kvm);		\
+		if (ipabits <= 0)					\
+			max_ram = ARM_HIMAP_MAX_MEMORY;			\
+		else							\
+			max_ram = (1ULL << ipabits) - ARM_MEMORY_AREA;	\
+	}								\
+									\
+	max_ram;							\
+})
 
 #include "arm-common/kvm-arch.h"
 
diff --git a/arm/aarch64/kvm.c b/arm/aarch64/kvm.c
index 49e1dd31..d03a27f2 100644
--- a/arm/aarch64/kvm.c
+++ b/arm/aarch64/kvm.c
@@ -46,3 +46,18 @@ fail:
 	return 0x80000;
 }
 
+int kvm__arch_get_ipa_limit(struct kvm *kvm)
+{
+	int ret;
+
+	ret = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION, KVM_CAP_ARM_VM_IPA_SIZE);
+	if (ret <= 0)
+		ret = 0;
+
+	return ret;
+}
+
+int kvm__get_vm_type(struct kvm *kvm)
+{
+	return KVM_VM_TYPE_ARM_IPA_SIZE(kvm__arch_get_ipa_limit(kvm));
+}
-- 
2.30.2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Marc Zyngier]

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.

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;
+
+	/* Otherwise, compute the minimal required IPA size */
+	max_ipa = ARM_MEMORY_AREA + kvm->cfg.ram_size - 1;
+	ipa_bits = max(32, fls_long(max_ipa));
+	pr_debug("max_ipa %lx ipa_bits %d max_ipa_bits %d",
+		 max_ipa, ipa_bits, max_ipa_bits);
+
+	if (ipa_bits > max_ipa_bits)
+		die("Memory too large for this system (needs %d bits, %d available)", ipa_bits, max_ipa_bits);
+
+	return KVM_VM_TYPE_ARM_IPA_SIZE(ipa_bits);
 }
-- 
2.30.2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 1/3] kvmtool: Abstract KVM_VM_TYPE into a weak function

[Oliver Upton]

On Sun, Aug 22, 2021 at 8:25 AM Marc Zyngier <maz@kernel.org> wrote:
>
> Most architectures pass a fixed value for their VM type. However,
> arm64 uses it as a parameter describing the size of the guest's
> physical address space.
>
> In order to support this, introduce a kvm__get_vm_type() helper
> that only returns KVM_VM_TYPE for now.
>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  include/kvm/kvm.h | 1 +
>  kvm.c             | 7 ++++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/include/kvm/kvm.h b/include/kvm/kvm.h
> index 56e9c8e3..ad732e56 100644
> --- a/include/kvm/kvm.h
> +++ b/include/kvm/kvm.h
> @@ -114,6 +114,7 @@ int kvm__init(struct kvm *kvm);
>  struct kvm *kvm__new(void);
>  int kvm__recommended_cpus(struct kvm *kvm);
>  int kvm__max_cpus(struct kvm *kvm);
> +int kvm__get_vm_type(struct kvm *kvm);
>  void kvm__init_ram(struct kvm *kvm);
>  int kvm__exit(struct kvm *kvm);
>  bool kvm__load_firmware(struct kvm *kvm, const char *firmware_filename);
> diff --git a/kvm.c b/kvm.c
> index e327541d..5bc66c8b 100644
> --- a/kvm.c
> +++ b/kvm.c
> @@ -428,6 +428,11 @@ int kvm__max_cpus(struct kvm *kvm)
>         return ret;
>  }
>
> +int __attribute__((weak)) kvm__get_vm_type(struct kvm *kvm)
> +{
> +       return KVM_VM_TYPE;
> +}
> +
>  int kvm__init(struct kvm *kvm)
>  {
>         int ret;
> @@ -461,7 +466,7 @@ int kvm__init(struct kvm *kvm)
>                 goto err_sys_fd;
>         }
>
> -       kvm->vm_fd = ioctl(kvm->sys_fd, KVM_CREATE_VM, KVM_VM_TYPE);
> +       kvm->vm_fd = ioctl(kvm->sys_fd, KVM_CREATE_VM, kvm__get_vm_type(kvm));
>         if (kvm->vm_fd < 0) {
>                 pr_err("KVM_CREATE_VM ioctl");
>                 ret = kvm->vm_fd;
> --
> 2.30.2
>
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Reviewed-by: Oliver Upton <oupton@google.com>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 2/3] kvmtool: arm64: Use the maximum supported IPA size when creating the VM

[Oliver Upton]

On Sun, Aug 22, 2021 at 8:25 AM Marc Zyngier <maz@kernel.org> wrote:
>
> Instead of just asking the the default VM size, request the maximum
> IPA size to the kernel, and use this at VM creation time.
>
> The IPA space is parametrized accordingly.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arm/aarch64/include/kvm/kvm-arch.h | 19 ++++++++++++++++---
>  arm/aarch64/kvm.c                  | 15 +++++++++++++++
>  2 files changed, 31 insertions(+), 3 deletions(-)
>
> diff --git a/arm/aarch64/include/kvm/kvm-arch.h b/arm/aarch64/include/kvm/kvm-arch.h
> index 55ef8ed1..159567b9 100644
> --- a/arm/aarch64/include/kvm/kvm-arch.h
> +++ b/arm/aarch64/include/kvm/kvm-arch.h
> @@ -3,10 +3,23 @@
>
>  struct kvm;
>  unsigned long long kvm__arch_get_kern_offset(struct kvm *kvm, int fd);
> +int kvm__arch_get_ipa_limit(struct kvm *kvm);
>
> -#define ARM_MAX_MEMORY(kvm)    ((kvm)->cfg.arch.aarch32_guest  ?       \
> -                               ARM_LOMAP_MAX_MEMORY            :       \
> -                               ARM_HIMAP_MAX_MEMORY)
> +#define ARM_MAX_MEMORY(kvm)    ({                                      \
> +       u64 max_ram;                                                    \
> +                                                                       \
> +       if ((kvm)->cfg.arch.aarch32_guest) {                            \
> +               max_ram = ARM_LOMAP_MAX_MEMORY;                         \
> +       } else {                                                        \
> +               int ipabits = kvm__arch_get_ipa_limit(kvm);             \
> +               if (ipabits <= 0)                                       \
> +                       max_ram = ARM_HIMAP_MAX_MEMORY;                 \
> +               else                                                    \
> +                       max_ram = (1ULL << ipabits) - ARM_MEMORY_AREA;  \
> +       }                                                               \
> +                                                                       \
> +       max_ram;                                                        \
> +})
>
>  #include "arm-common/kvm-arch.h"
>
> diff --git a/arm/aarch64/kvm.c b/arm/aarch64/kvm.c
> index 49e1dd31..d03a27f2 100644
> --- a/arm/aarch64/kvm.c
> +++ b/arm/aarch64/kvm.c
> @@ -46,3 +46,18 @@ fail:
>         return 0x80000;
>  }
>
> +int kvm__arch_get_ipa_limit(struct kvm *kvm)
> +{
> +       int ret;
> +
> +       ret = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION, KVM_CAP_ARM_VM_IPA_SIZE);
> +       if (ret <= 0)
> +               ret = 0;
> +
> +       return ret;
> +}
> +
> +int kvm__get_vm_type(struct kvm *kvm)
> +{
> +       return KVM_VM_TYPE_ARM_IPA_SIZE(kvm__arch_get_ipa_limit(kvm));
> +}
> --
> 2.30.2
>
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Reviewed-by: Oliver Upton <oupton@google.com>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Oliver Upton]

Marc,

On Sun, Aug 22, 2021 at 8:25 AM Marc Zyngier <maz@kernel.org> wrote:
>
> 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.
>
> 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;
> +
> +       /* Otherwise, compute the minimal required IPA size */
> +       max_ipa = ARM_MEMORY_AREA + kvm->cfg.ram_size - 1;
> +       ipa_bits = max(32, fls_long(max_ipa));
> +       pr_debug("max_ipa %lx ipa_bits %d max_ipa_bits %d",
> +                max_ipa, ipa_bits, max_ipa_bits);
> +
> +       if (ipa_bits > max_ipa_bits)
> +               die("Memory too large for this system (needs %d bits, %d available)", ipa_bits, max_ipa_bits);

I imagine it may not be immediately obvious to a kvmtool user what
this log line means, like what 'bits' are being referred to here.
Would it be more helpful to report the maximum allowed memory size for
the system, as derived from the max IPA?

--
Thanks,
Oliver

> +
> +       return KVM_VM_TYPE_ARM_IPA_SIZE(ipa_bits);
>  }
> --
> 2.30.2
>
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Marc Zyngier]

On Sun, 22 Aug 2021 21:05:16 +0100,
Oliver Upton <oupton@google.com> wrote:
> 
> Marc,
> 
> On Sun, Aug 22, 2021 at 8:25 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > 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.
> >
> > 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;
> > +
> > +       /* Otherwise, compute the minimal required IPA size */
> > +       max_ipa = ARM_MEMORY_AREA + kvm->cfg.ram_size - 1;
> > +       ipa_bits = max(32, fls_long(max_ipa));
> > +       pr_debug("max_ipa %lx ipa_bits %d max_ipa_bits %d",
> > +                max_ipa, ipa_bits, max_ipa_bits);
> > +
> > +       if (ipa_bits > max_ipa_bits)
> > +               die("Memory too large for this system (needs %d bits, %d available)", ipa_bits, max_ipa_bits);
> 
> I imagine it may not be immediately obvious to a kvmtool user what
> this log line means, like what 'bits' are being referred to here.
> Would it be more helpful to report the maximum allowed memory size for
> the system, as derived from the max IPA?

That's consistent with what "the other VMM" does as well. But sure,
happy to amend the message if people feel strongly about it.

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

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

[Oliver Upton]

On Mon, Aug 23, 2021 at 2:36 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Sun, 22 Aug 2021 21:05:16 +0100,
> Oliver Upton <oupton@google.com> wrote:
> >
> > Marc,
> >
> > On Sun, Aug 22, 2021 at 8:25 AM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > 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.
> > >
> > > 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;
> > > +
> > > +       /* Otherwise, compute the minimal required IPA size */
> > > +       max_ipa = ARM_MEMORY_AREA + kvm->cfg.ram_size - 1;
> > > +       ipa_bits = max(32, fls_long(max_ipa));
> > > +       pr_debug("max_ipa %lx ipa_bits %d max_ipa_bits %d",
> > > +                max_ipa, ipa_bits, max_ipa_bits);
> > > +
> > > +       if (ipa_bits > max_ipa_bits)
> > > +               die("Memory too large for this system (needs %d bits, %d available)", ipa_bits, max_ipa_bits);
> >
> > I imagine it may not be immediately obvious to a kvmtool user what
> > this log line means, like what 'bits' are being referred to here.
> > Would it be more helpful to report the maximum allowed memory size for
> > the system, as derived from the max IPA?
>
> That's consistent with what "the other VMM" does as well.

Lol
> But sure,
> happy to amend the message if people feel strongly about it.

Eh, maybe not worth the extra math in the end. My nit would be to say
"needs %d IPA bits" in the message. But regardless:

Reviewed-by: Oliver Upton <oupton@google.com>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 2/3] kvmtool: arm64: Use the maximum supported IPA size when creating the VM

[Andre Przywara]

On Sun, 22 Aug 2021 16:25:25 +0100
Marc Zyngier <maz@kernel.org> wrote:

Hi Marc,

> Instead of just asking the the default VM size, request the maximum
> IPA size to the kernel, and use this at VM creation time.
> 
> The IPA space is parametrized accordingly.

Thanks for the change!

> Signed-off-by: Marc Zyngier <maz@kernel.org>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>  arm/aarch64/include/kvm/kvm-arch.h | 19 ++++++++++++++++---
>  arm/aarch64/kvm.c                  | 15 +++++++++++++++
>  2 files changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/arm/aarch64/include/kvm/kvm-arch.h b/arm/aarch64/include/kvm/kvm-arch.h
> index 55ef8ed1..159567b9 100644
> --- a/arm/aarch64/include/kvm/kvm-arch.h
> +++ b/arm/aarch64/include/kvm/kvm-arch.h
> @@ -3,10 +3,23 @@
>  
>  struct kvm;
>  unsigned long long kvm__arch_get_kern_offset(struct kvm *kvm, int fd);
> +int kvm__arch_get_ipa_limit(struct kvm *kvm);
>  
> -#define ARM_MAX_MEMORY(kvm)	((kvm)->cfg.arch.aarch32_guest	?	\
> -				ARM_LOMAP_MAX_MEMORY		:	\
> -				ARM_HIMAP_MAX_MEMORY)
> +#define ARM_MAX_MEMORY(kvm)	({					\
> +	u64 max_ram;							\
> +									\
> +	if ((kvm)->cfg.arch.aarch32_guest) {				\
> +		max_ram = ARM_LOMAP_MAX_MEMORY;				\
> +	} else {							\
> +		int ipabits = kvm__arch_get_ipa_limit(kvm);		\
> +		if (ipabits <= 0)					\
> +			max_ram = ARM_HIMAP_MAX_MEMORY;			\
> +		else							\
> +			max_ram = (1ULL << ipabits) - ARM_MEMORY_AREA;	\
> +	}								\
> +									\
> +	max_ram;							\
> +})
>  
>  #include "arm-common/kvm-arch.h"
>  
> diff --git a/arm/aarch64/kvm.c b/arm/aarch64/kvm.c
> index 49e1dd31..d03a27f2 100644
> --- a/arm/aarch64/kvm.c
> +++ b/arm/aarch64/kvm.c
> @@ -46,3 +46,18 @@ fail:
>  	return 0x80000;
>  }
>  
> +int kvm__arch_get_ipa_limit(struct kvm *kvm)
> +{
> +	int ret;
> +
> +	ret = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION, KVM_CAP_ARM_VM_IPA_SIZE);
> +	if (ret <= 0)
> +		ret = 0;
> +
> +	return ret;
> +}
> +
> +int kvm__get_vm_type(struct kvm *kvm)
> +{
> +	return KVM_VM_TYPE_ARM_IPA_SIZE(kvm__arch_get_ipa_limit(kvm));
> +}

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Andre Przywara]

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?

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

Just a nit anyway, the patch looks correct otherwise:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> +
> +	/* Otherwise, compute the minimal required IPA size */
> +	max_ipa = ARM_MEMORY_AREA + kvm->cfg.ram_size - 1;
> +	ipa_bits = max(32, fls_long(max_ipa));
> +	pr_debug("max_ipa %lx ipa_bits %d max_ipa_bits %d",
> +		 max_ipa, ipa_bits, max_ipa_bits);
> +
> +	if (ipa_bits > max_ipa_bits)
> +		die("Memory too large for this system (needs %d bits, %d available)", ipa_bits, max_ipa_bits);
> +
> +	return KVM_VM_TYPE_ARM_IPA_SIZE(ipa_bits);
>  }

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Marc Zyngier]

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

Re: [PATCH v2 0/3] kvmtool: Limit IPA space to what is actually required

[Will Deacon]

On Sun, 22 Aug 2021 16:25:23 +0100, Marc Zyngier wrote:
> KVM hacking on the Apple M1 SoC has shown that kvmtool (and other
> VMMs) make pretty poor use of the IPA space parameter (read: do not
> use it and just pass 0). This results in a guest that cannot boot
> (recent kernels will just send the VMM packing), and in general means
> we don't benefit from smaller page tables at stage-2.
> 
> This series does three things:
> - It switches kvmtool away from the default 40bit, allowing large VMs
>   to be created (I have booted a 4TB VM)
> - It reduces the requested IPA space to be as small as possible
> - It tells the user why the VM cannot boot when the IPA space required
>   exceeds that of the HW
> 
> [...]

Applied to kvmtool (master), thanks!

[1/3] kvmtool: Abstract KVM_VM_TYPE into a weak function
      https://git.kernel.org/will/kvmtool/c/4250819de93b
[2/3] kvmtool: arm64: Use the maximum supported IPA size when creating the VM
      https://git.kernel.org/will/kvmtool/c/066b5c06c4e3
[3/3] kvmtool: arm64: Configure VM with the minimal required IPA space
      https://git.kernel.org/will/kvmtool/c/bdb86d0c0c95

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm