Skip to contentSkip to conversation

kvmtool: Limit IPA space to what is actually required

PATCH Applied Marc Zyngier 4 participants 13 messages Aug 22, 2021 – Aug 31, 2021 KVM arm64

lore
Share & tools
lore

Marc Zyngier

author maintainer
#0 lore raw

v2 cover kvmtool: Limit IPA space to what is actually required

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(-)
Signature · 6 lines
2.30.2

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
author maintainer
#1 lore raw

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

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 <[email protected]>
Signed-off-by Marc Zyngier <[email protected]>
---
 include/kvm/kvm.h | 1 +
 kvm.c             | 7 ++++++-
 2 files changed, 7 insertions(+), 1 deletion(-)
include/kvm/kvm.h +1 -0
index 56e9c8e3..ad732e56 100644
@@ -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);
kvm.c +6 -1
index e327541d..5bc66c8b 100644
@@ -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;
Signature · 6 lines
2.30.2

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Reviewed-by on v2 1/3
#2 lore raw

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

Quoted 55 lines · 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 <[email protected]>
Signed-off-by Marc Zyngier <[email protected]>
---
 include/kvm/kvm.h | 1 +
 kvm.c             | 7 ++++++-
 2 files changed, 7 insertions(+), 1 deletion(-)
include/kvm/kvm.h +1 -0
index 56e9c8e3..ad732e56 100644
@@ -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);
kvm.c +6 -2
index e327541d..5bc66c8b 100644
@@ -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

Quoted 4 lines

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Reviewed-by Oliver Upton <[email protected]>

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

author maintainer
#3 lore raw

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

In reply to 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 <[email protected]>
---
 arm/aarch64/include/kvm/kvm-arch.h | 19 ++++++++++++++++---
 arm/aarch64/kvm.c                  | 15 +++++++++++++++
 2 files changed, 31 insertions(+), 3 deletions(-)
arm/aarch64/include/kvm/kvm-arch.h +16 -3
index 55ef8ed1..159567b9 100644
@@ -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"
arm/aarch64/kvm.c +15 -0
index 49e1dd31..d03a27f2 100644
@@ -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));
+}
Signature · 6 lines
2.30.2

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Reviewed-by on v2 2/3
#4 lore raw

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

Quoted 69 lines · 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 <[email protected]>
---
 arm/aarch64/include/kvm/kvm-arch.h | 19 ++++++++++++++++---
 arm/aarch64/kvm.c                  | 15 +++++++++++++++
 2 files changed, 31 insertions(+), 3 deletions(-)
arm/aarch64/include/kvm/kvm-arch.h +16 -3
index 55ef8ed1..159567b9 100644
@@ -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"
arm/aarch64/kvm.c +15 -1
index 49e1dd31..d03a27f2 100644
@@ -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

Quoted 4 lines

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Reviewed-by Oliver Upton <[email protected]>

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Reviewed-by on v2 2/3
#5 lore raw

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

In reply to Marc Zyngier

On Sun, 22 Aug 2021 16:25:25 +0100
Marc Zyngier <[email protected]> wrote:

Hi Marc,

Quoted 4 lines

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!

Quoted 1 line
Signed-off-by Marc Zyngier <[email protected]>
Reviewed-by Andre Przywara <[email protected]>

Cheers,
Andre

Quoted 49 lines
---
 arm/aarch64/include/kvm/kvm-arch.h | 19 ++++++++++++++++---
 arm/aarch64/kvm.c                  | 15 +++++++++++++++
 2 files changed, 31 insertions(+), 3 deletions(-)
arm/aarch64/include/kvm/kvm-arch.h +16 -3
index 55ef8ed1..159567b9 100644
@@ -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"
arm/aarch64/kvm.c +5 -0
index 49e1dd31..d03a27f2 100644
@@ -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);

arm/aarch64/kvm.c fail: · line 63

Quoted 10 lines
arm/aarch64/kvm.c +10 -0
+	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
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

author maintainer
#6 lore raw

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

In reply to 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 <[email protected]>
---
 arm/aarch64/kvm.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)
arm/aarch64/kvm.c +19 -1
index d03a27f2..4e66a22e 100644
@@ -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);
 }
Signature · 6 lines
2.30.2

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
#7 lore raw

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

Marc,

Quoted 35 lines · 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 <[email protected]>
---
 arm/aarch64/kvm.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)
arm/aarch64/kvm.c +7 -1
index d03a27f2..4e66a22e 100644
@@ -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)

arm/aarch64/kvm.c int kvm__arch_get_ipa_limit(struct kvm *kvm) · line 78

Quoted 10 lines
arm/aarch64/kvm.c +10 -0
+               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

Quoted 10 lines
arm/aarch64/kvm.c +2 -1
+
+       return KVM_VM_TYPE_ARM_IPA_SIZE(ipa_bits);
 }
--

2.30.2

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

author maintainer
#8 lore raw

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

Quoted 46 lines · Oliver Upton

Marc,

Quoted 42 lines · 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 <[email protected]>
---
 arm/aarch64/kvm.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)
arm/aarch64/kvm.c +14 -1
index d03a27f2..4e66a22e 100644
@@ -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);
Quoted 8 lines

arm/aarch64/kvm.c int kvm__arch_get_ipa_limit(struct kvm *kvm) · line 78

Quoted 3 lines
arm/aarch64/kvm.c +3 -0
+
+       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.
Signature · 5 lines
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Reviewed-by
#9 lore raw

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

Quoted 58 lines · Marc Zyngier
Quoted 46 lines · Oliver Upton

Marc,

Quoted 42 lines · 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 <[email protected]>
---
 arm/aarch64/kvm.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)
arm/aarch64/kvm.c +14 -1
index d03a27f2..4e66a22e 100644
@@ -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);
Quoted 8 lines

arm/aarch64/kvm.c int kvm__arch_get_ipa_limit(struct kvm *kvm) · line 78

Quoted 3 lines
arm/aarch64/kvm.c +3 -0
+
+       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?

Quoted 1 line

That's consistent with what "the other VMM" does as well.

Lol

Quoted 2 lines

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 <[email protected]>

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Reviewed-by on v2 3/3
#10 lore raw

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

In reply to Marc Zyngier

On Sun, 22 Aug 2021 16:25:26 +0100
Marc Zyngier <[email protected]> wrote:

Hi Marc,

Quoted 5 lines

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?

Quoted 20 lines
Signed-off-by Marc Zyngier <[email protected]>
---
 arm/aarch64/kvm.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)
arm/aarch64/kvm.c +1 -0
index d03a27f2..4e66a22e 100644
@@ -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)

arm/aarch64/kvm.c int kvm__arch_get_ipa_limit(struct kvm *kvm) · line 69

Quoted 10 lines
arm/aarch64/kvm.c +7 -1
 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 <[email protected]>

Cheers,
Andre

arm/aarch64/kvm.c int kvm__arch_get_ipa_limit(struct kvm *kvm) · line 81

Quoted 12 lines
arm/aarch64/kvm.c +11 -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);
 }

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

author maintainer
#11 lore raw

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

Quoted 12 lines · Andre Przywara

On Sun, 22 Aug 2021 16:25:26 +0100
Marc Zyngier <[email protected]> wrote:

Hi Marc,

Quoted 5 lines

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.

Quoted 8 lines

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.

Quoted 21 lines
Quoted 21 lines
Signed-off-by Marc Zyngier <[email protected]>
---
 arm/aarch64/kvm.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)
arm/aarch64/kvm.c +1 -0
index d03a27f2..4e66a22e 100644
@@ -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)
Quoted 13 lines

arm/aarch64/kvm.c int kvm__arch_get_ipa_limit(struct kvm *kvm) · line 69

Quoted 10 lines
arm/aarch64/kvm.c +7 -1
 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.

Quoted 4 lines

Just a nit anyway, the patch looks correct otherwise:

Reviewed-by Andre Przywara <[email protected]>

Thanks,

	M.
Signature · 5 lines
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Applied
#12 lore raw
In reply to Marc Zyngier
Quoted 6 lines · 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.

Quoted 8 lines

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,

Signature · 9 lines
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm