Hi all,
This series is a collection of various fixes and cleanups for KVM/arm64
when running in nVHE protected mode. The first two patches are real
fixes/improvements, the following two are minor cleanups, and the last
two help satisfy my paranoia so they're certainly optional.
Cheers!
Quentin
Quentin Perret (6):
KVM: arm64: pkvm: Fix hyp_pool max order
KVM: arm64: pkvm: Disable GICv2 support
KVM: arm64: Make the hyp memory pool static
KVM: arm64: Make __io_map_base static
KVM: arm64: pkvm: Stub io map functions
KVM: arm64: pkvm: Make kvm_host_owns_hyp_mappings() robust to VHE
arch/arm64/kvm/hyp/include/nvhe/mm.h | 2 --
arch/arm64/kvm/hyp/nvhe/mm.c | 3 ++-
arch/arm64/kvm/hyp/nvhe/page_alloc.c | 2 +-
arch/arm64/kvm/hyp/nvhe/setup.c | 2 +-
arch/arm64/kvm/mmu.c | 9 +++++++++
arch/arm64/kvm/vgic/vgic-v2.c | 5 +++++
arch/arm64/kvm/vgic/vgic-v3.c | 2 +-
7 files changed, 19 insertions(+), 6 deletions(-)
--
2.34.1.400.ga245620fadb-goog
The EL2 page allocator in protected mode maintains a per-pool max order
value to optimize allocations when the memory region it covers is small.
However, the max order value is currently under-estimated whenever the
number of pages in the region is a power of two. Fix the estimation.
Signed-off-by: Quentin Perret <[email protected]>
---
arch/arm64/kvm/hyp/nvhe/page_alloc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/kvm/hyp/nvhe/page_alloc.c b/arch/arm64/kvm/hyp/nvhe/page_alloc.c
index 0bd7701ad1df..543cad6c376a 100644
--- a/arch/arm64/kvm/hyp/nvhe/page_alloc.c
+++ b/arch/arm64/kvm/hyp/nvhe/page_alloc.c
@@ -241,7 +241,7 @@ int hyp_pool_init(struct hyp_pool *pool, u64 pfn, unsigned int nr_pages,
int i;
hyp_spin_lock_init(&pool->lock);
- pool->max_order = min(MAX_ORDER, get_order(nr_pages << PAGE_SHIFT));
+ pool->max_order = min(MAX_ORDER, get_order((nr_pages + 1) << PAGE_SHIFT));
for (i = 0; i < pool->max_order; i++)
INIT_LIST_HEAD(&pool->free_area[i]);
pool->range_start = phys;
--
2.34.1.400.ga245620fadb-goog
GICv2 requires having device mappings in guests and the hypervisor,
which is incompatible with the current pKVM EL2 page ownership model
which only covers memory. While it would be desirable to support pKVM
with GICv2, this will require a lot more work, so let's make the
current assumption clear until then.
Co-developed-by: Marc Zyngier <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
Signed-off-by: Quentin Perret <[email protected]>
---
arch/arm64/kvm/vgic/vgic-v2.c | 5 +++++
arch/arm64/kvm/vgic/vgic-v3.c | 2 +-
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/kvm/vgic/vgic-v2.c b/arch/arm64/kvm/vgic/vgic-v2.c
index 95a18cec14a3..8e337a0d7817 100644
--- a/arch/arm64/kvm/vgic/vgic-v2.c
+++ b/arch/arm64/kvm/vgic/vgic-v2.c
@@ -345,6 +345,11 @@ int vgic_v2_probe(const struct gic_kvm_info *info)
int ret;
u32 vtr;
+ if (is_protected_kvm_enabled()) {
+ kvm_err("GICv2 not supported in protected mode\n");
+ return -ENXIO;
+ }
+
if (!info->vctrl.start) {
kvm_err("GICH not present in the firmware table\n");
return -ENXIO;
diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
index 04f62c4b07fb..debad4e6e6c9 100644
--- a/arch/arm64/kvm/vgic/vgic-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-v3.c
@@ -651,7 +651,7 @@ int vgic_v3_probe(const struct gic_kvm_info *info)
} else if (!PAGE_ALIGNED(info->vcpu.start)) {
pr_warn("GICV physical address 0x%llx not page aligned\n",
(unsigned long long)info->vcpu.start);
- } else {
+ } else if (kvm_get_mode() != KVM_MODE_PROTECTED) {
kvm_vgic_global_state.vcpu_base = info->vcpu.start;
kvm_vgic_global_state.can_emulate_gicv2 = true;
ret = kvm_register_vgic_device(KVM_DEV_TYPE_ARM_VGIC_V2);
--
2.34.1.400.ga245620fadb-goog
The hyp memory pool struct is sized to fit exactly the needs of the
hypervisor stage-1 page-table allocator, so it is important it is not
used for anything else. As it is currently used only from setup.c,
reduce its visibility by marking it static.
Signed-off-by: Quentin Perret <[email protected]>
---
arch/arm64/kvm/hyp/include/nvhe/mm.h | 1 -
arch/arm64/kvm/hyp/nvhe/setup.c | 2 +-
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/arm64/kvm/hyp/include/nvhe/mm.h b/arch/arm64/kvm/hyp/include/nvhe/mm.h
index ef6a58a04235..3f60d6cc5368 100644
--- a/arch/arm64/kvm/hyp/include/nvhe/mm.h
+++ b/arch/arm64/kvm/hyp/include/nvhe/mm.h
@@ -12,7 +12,6 @@
extern struct kvm_pgtable pkvm_pgtable;
extern hyp_spinlock_t pkvm_pgd_lock;
-extern struct hyp_pool hpool;
extern u64 __io_map_base;
int hyp_create_idmap(u32 hyp_va_bits);
diff --git a/arch/arm64/kvm/hyp/nvhe/setup.c b/arch/arm64/kvm/hyp/nvhe/setup.c
index 51e68a040d8a..e31149965204 100644
--- a/arch/arm64/kvm/hyp/nvhe/setup.c
+++ b/arch/arm64/kvm/hyp/nvhe/setup.c
@@ -18,7 +18,6 @@
#include <nvhe/mm.h>
#include <nvhe/trap_handler.h>
-struct hyp_pool hpool;
unsigned long hyp_nr_cpus;
#define hyp_percpu_size ((unsigned long)__per_cpu_end - \
@@ -28,6 +27,7 @@ static void *vmemmap_base;
static void *hyp_pgt_base;
static void *host_s2_pgt_base;
static struct kvm_pgtable_mm_ops pkvm_pgtable_mm_ops;
+static struct hyp_pool hpool;
static int divide_memory_pool(void *virt, unsigned long size)
{
--
2.34.1.400.ga245620fadb-goog
The __io_map_base variable is used at EL2 to track the end of the
hypervisor's "private" VA range in nVHE protected mode. However it
doesn't need to be used outside of mm.c, so let's make it static to keep
all the hyp VA allocation logic in one place.
Signed-off-by: Quentin Perret <[email protected]>
---
arch/arm64/kvm/hyp/include/nvhe/mm.h | 1 -
arch/arm64/kvm/hyp/nvhe/mm.c | 3 ++-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/kvm/hyp/include/nvhe/mm.h b/arch/arm64/kvm/hyp/include/nvhe/mm.h
index 3f60d6cc5368..2d08510c6cc1 100644
--- a/arch/arm64/kvm/hyp/include/nvhe/mm.h
+++ b/arch/arm64/kvm/hyp/include/nvhe/mm.h
@@ -12,7 +12,6 @@
extern struct kvm_pgtable pkvm_pgtable;
extern hyp_spinlock_t pkvm_pgd_lock;
-extern u64 __io_map_base;
int hyp_create_idmap(u32 hyp_va_bits);
int hyp_map_vectors(void);
diff --git a/arch/arm64/kvm/hyp/nvhe/mm.c b/arch/arm64/kvm/hyp/nvhe/mm.c
index 9e0ff5a700dd..526a7d6fa86f 100644
--- a/arch/arm64/kvm/hyp/nvhe/mm.c
+++ b/arch/arm64/kvm/hyp/nvhe/mm.c
@@ -19,11 +19,12 @@
struct kvm_pgtable pkvm_pgtable;
hyp_spinlock_t pkvm_pgd_lock;
-u64 __io_map_base;
struct memblock_region hyp_memory[HYP_MEMBLOCK_REGIONS];
unsigned int hyp_memblock_nr;
+static u64 __io_map_base;
+
static int __pkvm_create_mappings(unsigned long start, unsigned long size,
unsigned long phys, enum kvm_pgtable_prot prot)
{
--
2.34.1.400.ga245620fadb-goog
Now that GICv2 is disabled in nVHE protected mode there should be no
other reason for the host to use create_hyp_io_mappings() or
kvm_phys_addr_ioremap(). Add sanity checks to make sure that assumption
remains true looking forward.
Signed-off-by: Quentin Perret <[email protected]>
---
arch/arm64/kvm/mmu.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 326cdfec74a1..605c104eb030 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -407,6 +407,9 @@ int create_hyp_io_mappings(phys_addr_t phys_addr, size_t size,
unsigned long addr;
int ret;
+ if (is_protected_kvm_enabled())
+ return -EPERM;
+
*kaddr = ioremap(phys_addr, size);
if (!*kaddr)
return -ENOMEM;
@@ -650,6 +653,9 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
KVM_PGTABLE_PROT_R |
(writable ? KVM_PGTABLE_PROT_W : 0);
+ if (is_protected_kvm_enabled())
+ return -EPERM;
+
size += offset_in_page(guest_ipa);
guest_ipa &= PAGE_MASK;
--
2.34.1.400.ga245620fadb-goog
The kvm_host_owns_hyp_mappings() function should return true if and only
if the host kernel is responsible for creating the hypervisor stage-1
mappings. That is only possible in standard non-VHE mode, or during boot
in protected nVHE mode. But either way, non of this makes sense in VHE,
so make sure to catch this case as well, hence making the function
return sensible values in any context (VHE or not).
Suggested-by: Marc Zyngier <[email protected]>
Signed-off-by: Quentin Perret <[email protected]>
---
arch/arm64/kvm/mmu.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 605c104eb030..ea840fa223b5 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -239,6 +239,9 @@ void free_hyp_pgds(void)
static bool kvm_host_owns_hyp_mappings(void)
{
+ if (is_kernel_in_hyp_mode())
+ return false;
+
if (static_branch_likely(&kvm_protected_mode_initialized))
return false;
--
2.34.1.400.ga245620fadb-goog
Reviewed-by: Andrew Walbran <[email protected]>
On Wed, 8 Dec 2021 at 15:23, 'Quentin Perret' via kernel-team
<[email protected]> wrote:
>
> The hyp memory pool struct is sized to fit exactly the needs of the
> hypervisor stage-1 page-table allocator, so it is important it is not
> used for anything else. As it is currently used only from setup.c,
> reduce its visibility by marking it static.
>
> Signed-off-by: Quentin Perret <[email protected]>
> ---
> arch/arm64/kvm/hyp/include/nvhe/mm.h | 1 -
> arch/arm64/kvm/hyp/nvhe/setup.c | 2 +-
> 2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kvm/hyp/include/nvhe/mm.h b/arch/arm64/kvm/hyp/include/nvhe/mm.h
> index ef6a58a04235..3f60d6cc5368 100644
> --- a/arch/arm64/kvm/hyp/include/nvhe/mm.h
> +++ b/arch/arm64/kvm/hyp/include/nvhe/mm.h
> @@ -12,7 +12,6 @@
>
> extern struct kvm_pgtable pkvm_pgtable;
> extern hyp_spinlock_t pkvm_pgd_lock;
> -extern struct hyp_pool hpool;
> extern u64 __io_map_base;
>
> int hyp_create_idmap(u32 hyp_va_bits);
> diff --git a/arch/arm64/kvm/hyp/nvhe/setup.c b/arch/arm64/kvm/hyp/nvhe/setup.c
> index 51e68a040d8a..e31149965204 100644
> --- a/arch/arm64/kvm/hyp/nvhe/setup.c
> +++ b/arch/arm64/kvm/hyp/nvhe/setup.c
> @@ -18,7 +18,6 @@
> #include <nvhe/mm.h>
> #include <nvhe/trap_handler.h>
>
> -struct hyp_pool hpool;
> unsigned long hyp_nr_cpus;
>
> #define hyp_percpu_size ((unsigned long)__per_cpu_end - \
> @@ -28,6 +27,7 @@ static void *vmemmap_base;
> static void *hyp_pgt_base;
> static void *host_s2_pgt_base;
> static struct kvm_pgtable_mm_ops pkvm_pgtable_mm_ops;
> +static struct hyp_pool hpool;
>
> static int divide_memory_pool(void *virt, unsigned long size)
> {
> --
> 2.34.1.400.ga245620fadb-goog
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
>
On Wed, Dec 08, 2021 at 03:22:54PM +0000, Quentin Perret wrote:
> The EL2 page allocator in protected mode maintains a per-pool max order
> value to optimize allocations when the memory region it covers is small.
> However, the max order value is currently under-estimated whenever the
> number of pages in the region is a power of two. Fix the estimation.
>
> Signed-off-by: Quentin Perret <[email protected]>
> ---
> arch/arm64/kvm/hyp/nvhe/page_alloc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Acked-by: Will Deacon <[email protected]>
Will
On Wed, Dec 08, 2021 at 03:22:55PM +0000, Quentin Perret wrote:
> GICv2 requires having device mappings in guests and the hypervisor,
> which is incompatible with the current pKVM EL2 page ownership model
> which only covers memory. While it would be desirable to support pKVM
> with GICv2, this will require a lot more work, so let's make the
> current assumption clear until then.
>
> Co-developed-by: Marc Zyngier <[email protected]>
> Signed-off-by: Marc Zyngier <[email protected]>
> Signed-off-by: Quentin Perret <[email protected]>
> ---
> arch/arm64/kvm/vgic/vgic-v2.c | 5 +++++
> arch/arm64/kvm/vgic/vgic-v3.c | 2 +-
> 2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-v2.c b/arch/arm64/kvm/vgic/vgic-v2.c
> index 95a18cec14a3..8e337a0d7817 100644
> --- a/arch/arm64/kvm/vgic/vgic-v2.c
> +++ b/arch/arm64/kvm/vgic/vgic-v2.c
> @@ -345,6 +345,11 @@ int vgic_v2_probe(const struct gic_kvm_info *info)
> int ret;
> u32 vtr;
>
> + if (is_protected_kvm_enabled()) {
> + kvm_err("GICv2 not supported in protected mode\n");
> + return -ENXIO;
> + }
> +
> if (!info->vctrl.start) {
> kvm_err("GICH not present in the firmware table\n");
> return -ENXIO;
> diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
> index 04f62c4b07fb..debad4e6e6c9 100644
> --- a/arch/arm64/kvm/vgic/vgic-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-v3.c
> @@ -651,7 +651,7 @@ int vgic_v3_probe(const struct gic_kvm_info *info)
> } else if (!PAGE_ALIGNED(info->vcpu.start)) {
> pr_warn("GICV physical address 0x%llx not page aligned\n",
> (unsigned long long)info->vcpu.start);
> - } else {
> + } else if (kvm_get_mode() != KVM_MODE_PROTECTED) {
> kvm_vgic_global_state.vcpu_base = info->vcpu.start;
> kvm_vgic_global_state.can_emulate_gicv2 = true;
> ret = kvm_register_vgic_device(KVM_DEV_TYPE_ARM_VGIC_V2);
Acked-by: Will Deacon <[email protected]>
Will
On Wed, Dec 08, 2021 at 03:22:57PM +0000, Quentin Perret wrote:
> The __io_map_base variable is used at EL2 to track the end of the
> hypervisor's "private" VA range in nVHE protected mode. However it
> doesn't need to be used outside of mm.c, so let's make it static to keep
> all the hyp VA allocation logic in one place.
>
> Signed-off-by: Quentin Perret <[email protected]>
> ---
> arch/arm64/kvm/hyp/include/nvhe/mm.h | 1 -
> arch/arm64/kvm/hyp/nvhe/mm.c | 3 ++-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kvm/hyp/include/nvhe/mm.h b/arch/arm64/kvm/hyp/include/nvhe/mm.h
> index 3f60d6cc5368..2d08510c6cc1 100644
> --- a/arch/arm64/kvm/hyp/include/nvhe/mm.h
> +++ b/arch/arm64/kvm/hyp/include/nvhe/mm.h
> @@ -12,7 +12,6 @@
>
> extern struct kvm_pgtable pkvm_pgtable;
> extern hyp_spinlock_t pkvm_pgd_lock;
> -extern u64 __io_map_base;
>
> int hyp_create_idmap(u32 hyp_va_bits);
> int hyp_map_vectors(void);
> diff --git a/arch/arm64/kvm/hyp/nvhe/mm.c b/arch/arm64/kvm/hyp/nvhe/mm.c
> index 9e0ff5a700dd..526a7d6fa86f 100644
> --- a/arch/arm64/kvm/hyp/nvhe/mm.c
> +++ b/arch/arm64/kvm/hyp/nvhe/mm.c
> @@ -19,11 +19,12 @@
>
> struct kvm_pgtable pkvm_pgtable;
> hyp_spinlock_t pkvm_pgd_lock;
> -u64 __io_map_base;
>
> struct memblock_region hyp_memory[HYP_MEMBLOCK_REGIONS];
> unsigned int hyp_memblock_nr;
>
> +static u64 __io_map_base;
> +
> static int __pkvm_create_mappings(unsigned long start, unsigned long size,
> unsigned long phys, enum kvm_pgtable_prot prot)
Acked-by: Will Deacon <[email protected]>
Will
On Wed, Dec 08, 2021 at 03:22:58PM +0000, Quentin Perret wrote:
> Now that GICv2 is disabled in nVHE protected mode there should be no
> other reason for the host to use create_hyp_io_mappings() or
> kvm_phys_addr_ioremap(). Add sanity checks to make sure that assumption
> remains true looking forward.
>
> Signed-off-by: Quentin Perret <[email protected]>
> ---
> arch/arm64/kvm/mmu.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 326cdfec74a1..605c104eb030 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -407,6 +407,9 @@ int create_hyp_io_mappings(phys_addr_t phys_addr, size_t size,
> unsigned long addr;
> int ret;
>
> + if (is_protected_kvm_enabled())
> + return -EPERM;
Looks like the vGIC is the only caller, so no need to worry about anybody
relying on *kaddr being zeroed on failure.
Acked-by: Will Deacon <[email protected]>
Will
On Wed, Dec 08, 2021 at 03:22:59PM +0000, Quentin Perret wrote:
> The kvm_host_owns_hyp_mappings() function should return true if and only
> if the host kernel is responsible for creating the hypervisor stage-1
> mappings. That is only possible in standard non-VHE mode, or during boot
> in protected nVHE mode. But either way, non of this makes sense in VHE,
Typo: non => none
> so make sure to catch this case as well, hence making the function
> return sensible values in any context (VHE or not).
>
> Suggested-by: Marc Zyngier <[email protected]>
> Signed-off-by: Quentin Perret <[email protected]>
> ---
> arch/arm64/kvm/mmu.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 605c104eb030..ea840fa223b5 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -239,6 +239,9 @@ void free_hyp_pgds(void)
>
> static bool kvm_host_owns_hyp_mappings(void)
> {
> + if (is_kernel_in_hyp_mode())
> + return false;
Acked-by: Will Deacon <[email protected]>
Will
On Wed, 8 Dec 2021 15:22:53 +0000, Quentin Perret wrote:
> This series is a collection of various fixes and cleanups for KVM/arm64
> when running in nVHE protected mode. The first two patches are real
> fixes/improvements, the following two are minor cleanups, and the last
> two help satisfy my paranoia so they're certainly optional.
>
> Cheers!
> Quentin
>
> [...]
Applied to next, thanks!
[1/6] KVM: arm64: pkvm: Fix hyp_pool max order
commit: 34b43a8849229e8363c19236ecdf463b7a89d085
[2/6] KVM: arm64: pkvm: Disable GICv2 support
commit: a770ee80e66270a7df183dda5ad6df4e8c8ab615
[3/6] KVM: arm64: Make the hyp memory pool static
commit: 53a563b01fa2ae2376a0b7d547f26a0ae9c78b5c
[4/6] KVM: arm64: Make __io_map_base static
commit: 473a3efbafaa9ffd06c8b8f653f24c97b5ac3ff0
[5/6] KVM: arm64: pkvm: Stub io map functions
commit: bff01cb6b1bf68052739eb6155132f7d6d974208
[6/6] KVM: arm64: pkvm: Make kvm_host_owns_hyp_mappings() robust to VHE
commit: 64a1fbda59f4b14adde7f21cda687e2b9703b7bb
Cheers,
M.
--
Without deviation from the norm, progress is not possible.