Hi all,
This is v3 of the patch series previously posted here:
https://lore.kernel.org/kvmarm/20210726092905.2198501-1-qperret@google.com/
This series aims to improve how the nVHE hypervisor tracks ownership of
memory pages when running in protected mode ("kvm-arm.mode=protected" on
the kernel command line).
The main issue with the existing ownership tracking code is that it is
completely binary: a page is either owned by an entity (e.g. the host)
or not. However, we'll need something smarter to track shared pages, as
is needed for virtio, or even just host/hypervisor communications.
This series introduces a few changes to the kvm page-table library to
allow annotating shared pages in ignored bits (a.k.a. software bits) of
leaf entries, and makes use of that infrastructure to track all pages
that are shared between the host and the hypervisor. We will obviously
want to apply the same treatment to guest stage-2 page-tables, but that
is not really possible to do until EL2 manages them directly, so I'll
keep that for another series.
The series is based on the kvmarm/fixes branch, and has been tested on
AML-S905X-CC (Le Potato) and using various Qemu configurations.
Changes since v2:
- Renamed and refactored the find_range() path for host memory aborts;
- Added hyp_assert_lock_held() using Will's hyp_spin_is_locked()
helper, and sprinkled a few of them throughout the series;
- Changed how host stage-2 mappings are adjusted after __pkvm_init() by
walking the hyp stage-1 instead of relying on the host calling
__pkvm_mark_hyp.
Changes since v1:
- Changed the 'share' hypercall to accept a single page at a time;
- Dropped the patch allowing to continue stage-2 map when hitting the
EAGAIN case;
- Dropped some of the custom pgtable walkers and used Marc's get_leaf()
patch instead;
- Changed pgtable API to manipulate SW bits directly rather than
specifying shared pages;
- Added comments and documentations all over;
- Cleanups and small refactoring.
Thanks,
Quentin
Marc Zyngier (1):
KVM: arm64: Introduce helper to retrieve a PTE and its level
Quentin Perret (19):
KVM: arm64: Introduce hyp_assert_lock_held()
KVM: arm64: Provide the host_stage2_try() helper macro
KVM: arm64: Expose page-table helpers
KVM: arm64: Optimize host memory aborts
KVM: arm64: Rename KVM_PTE_LEAF_ATTR_S2_IGNORED
KVM: arm64: Don't overwrite software bits with owner id
KVM: arm64: Tolerate re-creating hyp mappings to set software bits
KVM: arm64: Enable forcing page-level stage-2 mappings
KVM: arm64: Allow populating software bits
KVM: arm64: Add helpers to tag shared pages in SW bits
KVM: arm64: Expose host stage-2 manipulation helpers
KVM: arm64: Expose pkvm_hyp_id
KVM: arm64: Introduce addr_is_memory()
KVM: arm64: Enable retrieving protections attributes of PTEs
KVM: arm64: Mark host bss and rodata section as shared
KVM: arm64: Remove __pkvm_mark_hyp
KVM: arm64: Refactor protected nVHE stage-1 locking
KVM: arm64: Restrict EL2 stage-1 changes in protected mode
KVM: arm64: Make __pkvm_create_mappings static
Will Deacon (1):
KVM: arm64: Add hyp_spin_is_locked() for basic locking assertions at
EL2
arch/arm64/include/asm/kvm_asm.h | 5 +-
arch/arm64/include/asm/kvm_pgtable.h | 166 ++++++++----
arch/arm64/kvm/Kconfig | 9 +
arch/arm64/kvm/arm.c | 46 ----
arch/arm64/kvm/hyp/include/nvhe/mem_protect.h | 33 ++-
arch/arm64/kvm/hyp/include/nvhe/mm.h | 3 +-
arch/arm64/kvm/hyp/include/nvhe/spinlock.h | 25 ++
arch/arm64/kvm/hyp/nvhe/hyp-main.c | 20 +-
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 221 ++++++++++++++--
arch/arm64/kvm/hyp/nvhe/mm.c | 22 +-
arch/arm64/kvm/hyp/nvhe/setup.c | 82 +++++-
arch/arm64/kvm/hyp/pgtable.c | 247 +++++++++---------
arch/arm64/kvm/mmu.c | 28 +-
13 files changed, 625 insertions(+), 282 deletions(-)
--
2.32.0.432.gabb21c7263-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Will Deacon <will@kernel.org>
Introduce hyp_spin_is_locked() so that functions can easily assert that
a given lock is held (albeit possibly by another CPU!) without having to
drag full lockdep support up to EL2.
Signed-off-by: Will Deacon <will@kernel.org>
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/kvm/hyp/include/nvhe/spinlock.h | 8 ++++++++
1 file changed, 8 insertions(+)
Introduce a poor man's lockdep implementation at EL2 which allows to
BUG() whenever a hyp spinlock is not held when it should. Hide this
feature behind a new Kconfig option that targets the EL2 object
specifically, instead of piggy backing on the existing CONFIG_LOCKDEP.
EL2 cannot WARN() cleanly to report locking issues, hence BUG() is the
only option and it is not clear whether we want this widely enabled.
This is most likely going to be useful for local testing until the EL2
WARN() situation has improved.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/kvm/Kconfig | 9 +++++++++
arch/arm64/kvm/hyp/include/nvhe/spinlock.h | 17 +++++++++++++++++
2 files changed, 26 insertions(+)
We currently unmap all MMIO mappings from the host stage-2 to recycle
the pages whenever we run out. In order to make this pattern easy to
re-use from other places, factor the logic out into a dedicated macro.
While at it, apply the macro for the kvm_pgtable_stage2_set_owner()
calls. They're currently only called early on and are guaranteed to
succeed, but making them robust to the -ENOMEM case doesn't hurt and
will avoid painful debugging sessions later on.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 40 +++++++++++++++------------
1 file changed, 22 insertions(+), 18 deletions(-)
From: Marc Zyngier <maz@kernel.org>
It is becoming a common need to fetch the PTE for a given address
together with its level. Add such a helper.
Signed-off-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/include/asm/kvm_pgtable.h | 19 ++++++++++++++
arch/arm64/kvm/hyp/pgtable.c | 39 ++++++++++++++++++++++++++++
2 files changed, 58 insertions(+)
The KVM pgtable API exposes the kvm_pgtable_walk() function to allow
the definition of walkers outside of pgtable.c. However, it is not easy
to implement any of those walkers without some of the low-level helpers.
Move some of them to the header file to allow re-use from other places.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/include/asm/kvm_pgtable.h | 40 ++++++++++++++++++++++++++++
arch/arm64/kvm/hyp/pgtable.c | 39 ---------------------------
2 files changed, 40 insertions(+), 39 deletions(-)
The kvm_pgtable_stage2_find_range() function is used in the host memory
abort path to try and look for the largest block mapping that can be
used to map the faulting address. In order to do so, the function
currently walks the stage-2 page-table and looks for existing
incompatible mappings within the range of the largest possible block.
If incompatible mappings are found, it tries the same procedure again,
but using a smaller block range, and repeats until a matching range is
found (potentially up to page granularity). While this approach has
benefits (mostly in the fact that it proactively coalesces host stage-2
mappings), it can be slow if the ranges are fragmented, and it isn't
optimized to deal with CPUs faulting on the same IPA as all of them will
do all the work every time.
To avoid these issues, remove kvm_pgtable_stage2_find_range(), and walk
the page-table only once in the host_mem_abort() path to find the
closest leaf to the input address. With this, use the corresponding
range if it is invalid and not owned by another entity. If a valid leaf
is found, return -EAGAIN similar to what is done in the
kvm_pgtable_stage2_map() path to optimize concurrent faults.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/include/asm/kvm_pgtable.h | 30 -----------
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 45 +++++++++++++++-
arch/arm64/kvm/hyp/pgtable.c | 74 ---------------------------
3 files changed, 44 insertions(+), 105 deletions(-)
The ignored bits for both stage-1 and stage-2 page and block
descriptors are in [55:58], so rename KVM_PTE_LEAF_ATTR_S2_IGNORED to
make it applicable to both. And while at it, since these bits are more
commonly known as 'software' bits, rename accordingly.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/kvm/hyp/pgtable.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
We will soon start annotating page-tables with new flags to track shared
pages and such, and we will do so in valid mappings using software bits
in the PTEs, as provided by the architecture. However, it is possible
that we will need to use those flags to annotate invalid mappings as
well in the future, similar to what we do to track page ownership in the
host stage-2.
In order to facilitate the annotation of invalid mappings with such
flags, it would be preferable to re-use the same bits as for valid
mappings (bits [58-55]), but these are currently used for ownership
encoding. Since we have plenty of bits left to use in invalid
mappings, move the ownership bits further down the PTE to avoid the
conflict.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/kvm/hyp/pgtable.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
The current hypervisor stage-1 mapping code doesn't allow changing an
existing valid mapping. Relax this condition by allowing changes that
only target software bits, as that will soon be needed to annotate shared
pages.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/kvm/hyp/pgtable.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
Much of the stage-2 manipulation logic relies on being able to destroy
block mappings if e.g. installing a smaller mapping in the range. The
rationale for this behaviour is that stage-2 mappings can always be
re-created lazily. However, this gets more complicated when the stage-2
page-table is used to store metadata about the underlying pages. In such
cases, destroying a block mapping may lead to losing part of the state,
and confuse the user of those metadata (such as the hypervisor in nVHE
protected mode).
To avoid this, introduce a callback function in the pgtable struct which
is called during all map operations to determine whether the mappings
can use blocks, or should be forced to page granularity. This is used by
the hypervisor when creating the host stage-2 to force page-level
mappings when using non-default protection attributes.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/include/asm/kvm_pgtable.h | 65 ++++++++++++++++-----------
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 30 +++++++++++--
arch/arm64/kvm/hyp/pgtable.c | 29 +++++++++---
3 files changed, 91 insertions(+), 33 deletions(-)
Introduce infrastructure allowing to manipulate software bits in stage-1
and stage-2 page-tables using additional entries in the kvm_pgtable_prot
enum.
This is heavily inspired by Marc's implementation of a similar feature
in the NV patch series, but adapted to allow stage-1 changes as well:
https://lore.kernel.org/kvmarm/20210510165920.1913477-56-maz@kernel.org/
Suggested-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/include/asm/kvm_pgtable.h | 12 +++++++++++-
arch/arm64/kvm/hyp/pgtable.c | 5 +++++
2 files changed, 16 insertions(+), 1 deletion(-)
We will soon start annotating shared pages in page-tables in nVHE
protected mode. Define all the states in which a page can be (owned,
shared and owned, shared and borrowed), and provide helpers allowing to
convert this into SW bits annotations using the matching prot
attributes.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/kvm/hyp/include/nvhe/mem_protect.h | 26 +++++++++++++++++++
1 file changed, 26 insertions(+)
We will need to manipulate the host stage-2 page-table from outside
mem_protect.c soon. Introduce two functions allowing this, and make
them usable to users of mem_protect.h.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/kvm/hyp/include/nvhe/mem_protect.h | 2 ++
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 17 ++++++++++++++++-
2 files changed, 18 insertions(+), 1 deletion(-)
Introduce helper functions in the KVM stage-2 and stage-1 page-table
manipulation library allowing to retrieve the enum kvm_pgtable_prot of a
PTE. This will be useful to implement custom walkers outside of
pgtable.c.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/include/asm/kvm_pgtable.h | 20 +++++++++++++++
arch/arm64/kvm/hyp/pgtable.c | 37 ++++++++++++++++++++++++++++
2 files changed, 57 insertions(+)
As the hypervisor maps the host's .bss and .rodata sections in its
stage-1, make sure to tag them as shared in hyp and host page-tables.
But since the hypervisor relies on the presence of these mappings, we
cannot let the host in complete control of the memory regions -- it
must not unshare or donate them to another entity for example. To
prevent this, let's transfer the ownership of those ranges to the
hypervisor itself, and share the pages back with the host.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/kvm/hyp/nvhe/setup.c | 82 +++++++++++++++++++++++++++++----
1 file changed, 74 insertions(+), 8 deletions(-)
@@ -58,6 +58,7 @@ static int recreate_hyp_mappings(phys_addr_t phys, unsigned long size,{void*start,*end,*virt=hyp_phys_to_virt(phys);unsignedlongpgt_size=hyp_s1_pgtable_pages()<<PAGE_SHIFT;+enumkvm_pgtable_protprot;intret,i;/* Recreate the hyp page-table using the early page allocator */
@@ -83,10 +84,6 @@ static int recreate_hyp_mappings(phys_addr_t phys, unsigned long size,if(ret)returnret;-ret=pkvm_create_mappings(__start_rodata,__end_rodata,PAGE_HYP_RO);-if(ret)-returnret;-ret=pkvm_create_mappings(__hyp_rodata_start,__hyp_rodata_end,PAGE_HYP_RO);if(ret)returnret;
@@ -95,10 +92,6 @@ static int recreate_hyp_mappings(phys_addr_t phys, unsigned long size,if(ret)returnret;-ret=pkvm_create_mappings(__hyp_bss_end,__bss_stop,PAGE_HYP_RO);-if(ret)-returnret;-ret=pkvm_create_mappings(virt,virt+size,PAGE_HYP);if(ret)returnret;
@@ -117,6 +110,24 @@ static int recreate_hyp_mappings(phys_addr_t phys, unsigned long size,returnret;}+/*+*Mapthehost's.bssand.rodatasectionsROinthehypervisor,but+*transfertheownerhsipfromthehosttothehypervisoritselfto+*makesureitcan'tbedonatedorsharedwithanotherentity.+*+*Theownershiptranstionrequiresmatchingchangesinthehost+*stage-2.Thiswilldonelater(seefinalize_host_mappings())oncethe+*hyp_vmemmapisaddressable.+*/+prot=pkvm_mkstate(PAGE_HYP_RO,PKVM_PAGE_SHARED_OWNED);+ret=pkvm_create_mappings(__start_rodata,__end_rodata,prot);+if(ret)+returnret;++ret=pkvm_create_mappings(__hyp_bss_end,__bss_stop,prot);+if(ret)+returnret;+return0;}
Introduce a helper usable in nVHE protected mode to check whether a
physical address is in a RAM region or not.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/kvm/hyp/include/nvhe/mem_protect.h | 1 +
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 7 +++++++
2 files changed, 8 insertions(+)
Now that we mark memory owned by the hypervisor in the host stage-2
during __pkvm_init(), we no longer need to rely on the host to
explicitly mark the hyp sections later on.
Remove the __pkvm_mark_hyp() hypercall altogether.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/include/asm/kvm_asm.h | 3 +-
arch/arm64/kvm/arm.c | 46 -------------------
arch/arm64/kvm/hyp/include/nvhe/mem_protect.h | 1 -
arch/arm64/kvm/hyp/nvhe/hyp-main.c | 9 ----
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 19 --------
5 files changed, 1 insertion(+), 77 deletions(-)
The host kernel is currently able to change EL2 stage-1 mappings without
restrictions thanks to the __pkvm_create_mappings() hypercall. But in a
world where the host is no longer part of the TCB, this clearly poses a
problem.
To fix this, introduce a new hypercall to allow the host to share a
physical memory page with the hypervisor, and remove the
__pkvm_create_mappings() variant. The new hypercall implements
ownership and permission checks before allowing the sharing operation,
and it annotates the shared page in the hypervisor stage-1 and host
stage-2 page-tables.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/include/asm/kvm_asm.h | 2 +-
arch/arm64/kvm/hyp/include/nvhe/mem_protect.h | 1 +
arch/arm64/kvm/hyp/nvhe/hyp-main.c | 11 +--
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 89 +++++++++++++++++++
arch/arm64/kvm/mmu.c | 28 +++++-
5 files changed, 119 insertions(+), 12 deletions(-)
@@ -259,10 +259,8 @@ static int __create_hyp_mappings(unsigned long start, unsigned long size,{interr;-if(!kvm_host_owns_hyp_mappings()){-returnkvm_call_hyp_nvhe(__pkvm_create_mappings,-start,size,phys,prot);-}+if(WARN_ON(!kvm_host_owns_hyp_mappings()))+return-EINVAL;mutex_lock(&kvm_hyp_pgd_mutex);err=kvm_pgtable_hyp_map(hyp_pgtable,start,size,phys,prot);
Refactor the hypervisor stage-1 locking in nVHE protected mode to expose
a new pkvm_create_mappings_locked() function. This will be used in later
patches to allow walking and changing the hypervisor stage-1 without
releasing the lock.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/kvm/hyp/include/nvhe/mm.h | 1 +
arch/arm64/kvm/hyp/nvhe/mm.c | 18 ++++++++++++++++--
2 files changed, 17 insertions(+), 2 deletions(-)
The __pkvm_create_mappings() function is no longer used outside of
nvhe/mm.c, make it static.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/kvm/hyp/include/nvhe/mm.h | 2 --
arch/arm64/kvm/hyp/nvhe/mm.c | 4 ++--
2 files changed, 2 insertions(+), 4 deletions(-)
Hi Quentin.
On Thu, Jul 29, 2021 at 3:28 PM Quentin Perret [off-list ref] wrote:
quoted hunk
We currently unmap all MMIO mappings from the host stage-2 to recycle
the pages whenever we run out. In order to make this pattern easy to
re-use from other places, factor the logic out into a dedicated macro.
While at it, apply the macro for the kvm_pgtable_stage2_set_owner()
calls. They're currently only called early on and are guaranteed to
succeed, but making them robust to the -ENOMEM case doesn't hurt and
will avoid painful debugging sessions later on.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 40 +++++++++++++++------------
1 file changed, 22 insertions(+), 18 deletions(-)
@@ -223,22 +242,7 @@ static int host_stage2_idmap(u64 addr) if (ret) goto unlock;- ret = __host_stage2_idmap(range.start, range.end, prot);- if (ret != -ENOMEM)- goto unlock;-- /*- * The pool has been provided with enough pages to cover all of memory- * with page granularity, but it is difficult to know how much of the- * MMIO range we will need to cover upfront, so we may need to 'recycle'- * the pages if we run out.- */- ret = host_stage2_unmap_dev_all();- if (ret)- goto unlock;-- ret = __host_stage2_idmap(range.start, range.end, prot);-+ ret = host_stage2_try(__host_stage2_idmap, range.start, range.end, prot); unlock: hyp_spin_unlock(&host_kvm.lock);
@@ -257,8 +261,8 @@ int __pkvm_mark_hyp(phys_addr_t start, phys_addr_t end) return -EINVAL; hyp_spin_lock(&host_kvm.lock);- ret = kvm_pgtable_stage2_set_owner(&host_kvm.pgt, start, end - start,- &host_s2_pool, pkvm_hyp_id);+ ret = host_stage2_try(kvm_pgtable_stage2_set_owner, &host_kvm.pgt,+ start, end - start, &host_s2_pool, pkvm_hyp_id); hyp_spin_unlock(&host_kvm.lock); return ret != -EAGAIN ? ret : 0;--
Hi Quentin,
On Thu, Jul 29, 2021 at 3:28 PM Quentin Perret [off-list ref] wrote:
The kvm_pgtable_stage2_find_range() function is used in the host memory
abort path to try and look for the largest block mapping that can be
used to map the faulting address. In order to do so, the function
currently walks the stage-2 page-table and looks for existing
incompatible mappings within the range of the largest possible block.
If incompatible mappings are found, it tries the same procedure again,
but using a smaller block range, and repeats until a matching range is
found (potentially up to page granularity). While this approach has
benefits (mostly in the fact that it proactively coalesces host stage-2
mappings), it can be slow if the ranges are fragmented, and it isn't
optimized to deal with CPUs faulting on the same IPA as all of them will
do all the work every time.
To avoid these issues, remove kvm_pgtable_stage2_find_range(), and walk
the page-table only once in the host_mem_abort() path to find the
closest leaf to the input address. With this, use the corresponding
range if it is invalid and not owned by another entity. If a valid leaf
is found, return -EAGAIN similar to what is done in the
kvm_pgtable_stage2_map() path to optimize concurrent faults.
Signed-off-by: Quentin Perret <redacted>
Reviewing the code it seems to work as described, with the lock
assertion ensuring that the caller knows which lock to hold.
Reviewed-by: Fuad Tabba <redacted>
Thanks,
/fuad
Hi Quentin,
On Thu, Jul 29, 2021 at 3:28 PM Quentin Perret [off-list ref] wrote:
The ignored bits for both stage-1 and stage-2 page and block
descriptors are in [55:58], so rename KVM_PTE_LEAF_ATTR_S2_IGNORED to
make it applicable to both. And while at it, since these bits are more
commonly known as 'software' bits, rename accordingly.
As in the Armv8-A Address Translation spec.
Reviewed-by: Fuad Tabba <redacted>
Thanks,
/fuad
Hi Quentin,
On Thu, Jul 29, 2021 at 3:28 PM Quentin Perret [off-list ref] wrote:
quoted hunk
We will soon start annotating page-tables with new flags to track shared
pages and such, and we will do so in valid mappings using software bits
in the PTEs, as provided by the architecture. However, it is possible
that we will need to use those flags to annotate invalid mappings as
well in the future, similar to what we do to track page ownership in the
host stage-2.
In order to facilitate the annotation of invalid mappings with such
flags, it would be preferable to re-use the same bits as for valid
mappings (bits [58-55]), but these are currently used for ownership
encoding. Since we have plenty of bits left to use in invalid
mappings, move the ownership bits further down the PTE to avoid the
conflict.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/kvm/hyp/pgtable.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Hi Quentin,
On Thu, Jul 29, 2021 at 3:28 PM Quentin Perret [off-list ref] wrote:
quoted hunk
Much of the stage-2 manipulation logic relies on being able to destroy
block mappings if e.g. installing a smaller mapping in the range. The
rationale for this behaviour is that stage-2 mappings can always be
re-created lazily. However, this gets more complicated when the stage-2
page-table is used to store metadata about the underlying pages. In such
cases, destroying a block mapping may lead to losing part of the state,
and confuse the user of those metadata (such as the hypervisor in nVHE
protected mode).
To avoid this, introduce a callback function in the pgtable struct which
is called during all map operations to determine whether the mappings
can use blocks, or should be forced to page granularity. This is used by
the hypervisor when creating the host stage-2 to force page-level
mappings when using non-default protection attributes.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/include/asm/kvm_pgtable.h | 65 ++++++++++++++++-----------
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 30 +++++++++++--
arch/arm64/kvm/hyp/pgtable.c | 29 +++++++++---
3 files changed, 91 insertions(+), 33 deletions(-)
I wonder if it would be useful to add a couple of other aliases for
default memory and default mmio protections, e.g.,
#define KVM_PGTABLE_PROT_MEM KVM_PGTABLE_PROT_RWX
#define KVM_PGTABLE_PROT_MMIO KVM_PGTABLE_PROT_RW
I think that using these below, e.g., host_stage2_force_pte_cb(),
might make it clearer and answer comments you had in earlier patches
about why "RWX" for memory.
+typedef bool (*kvm_pgtable_force_pte_cb_t)(u64 addr, u64 end,
+ enum kvm_pgtable_prot prot);
+
+/**
+ * struct kvm_pgtable - KVM page-table.
+ * @ia_bits: Maximum input address size, in bits.
+ * @start_level: Level at which the page-table walk starts.
+ * @pgd: Pointer to the first top-level entry of the page-table.
+ * @mm_ops: Memory management callbacks.
+ * @mmu: Stage-2 KVM MMU struct. Unused for stage-1 page-tables.
+ * @flags: Stage-2 page-table flags.
+ * @force_pte_cb: Callback function used during map operations to decide
+ * whether block mappings can be used to map the given IPA
+ * range.
+ */
nit: I think it might be clearer (and probably not longer) to rephrase
to describe in terms of the return value of the callback, e.g., "...
function that returns true if page level mappings must be used instead
of block mappings."
int kvm_host_prepare_stage2(void *pgt_pool_base)
{
struct kvm_s2_mmu *mmu = &host_kvm.arch.mmu;
@@ -101,8 +102,9 @@ int kvm_host_prepare_stage2(void *pgt_pool_base) if (ret) return ret;- ret = kvm_pgtable_stage2_init_flags(&host_kvm.pgt, &host_kvm.arch,- &host_kvm.mm_ops, KVM_HOST_S2_FLAGS);+ ret = __kvm_pgtable_stage2_init(&host_kvm.pgt, &host_kvm.arch,+ &host_kvm.mm_ops, KVM_HOST_S2_FLAGS,+ host_stage2_force_pte_cb); if (ret) return ret;
@@ -270,9 +272,31 @@ static int host_stage2_adjust_range(u64 addr, struct kvm_mem_range *range) return 0; }+static bool host_stage2_force_pte_cb(u64 addr, u64 end, enum kvm_pgtable_prot prot)+{+ /*+ * Block mappings must be used with care in the host stage-2 as a+ * kvm_pgtable_stage2_map() operation targeting a page in the range of+ * an existing block will delete the block under the assumption that+ * mappings in the rest of the block range can always be rebuilt lazily.+ * That assumption is correct for the host stage-2 with RWX mappings+ * targeting memory or RW mappings targeting MMIO ranges (see+ * host_stage2_idmap() below which implements some of the host memory+ * abort logic). However, this is not safe for any other mappings where+ * the host stage-2 page-table is in fact the only place where this+ * state is stored. In all those cases, it is safer to use page-level+ * mappings, hence avoiding to lose the state because of side-effects in+ * kvm_pgtable_stage2_map().+ */+ if (range_is_memory(addr, end))+ return prot != KVM_PGTABLE_PROT_RWX;+ else+ return prot != KVM_PGTABLE_PROT_RW;+}
Just checking, I don't think that it's possible for the range to be
big enough to somehow include both memory and mmio, neither now nor in
future use cases, is it?
Hi Quentin,
On Thu, Jul 29, 2021 at 3:28 PM Quentin Perret [off-list ref] wrote:
quoted hunk
The current hypervisor stage-1 mapping code doesn't allow changing an
existing valid mapping. Relax this condition by allowing changes that
only target software bits, as that will soon be needed to annotate shared
pages.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/kvm/hyp/pgtable.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
Hi Quentin,
On Thu, Jul 29, 2021 at 3:28 PM Quentin Perret [off-list ref] wrote:
quoted hunk
We will soon start annotating shared pages in page-tables in nVHE
protected mode. Define all the states in which a page can be (owned,
shared and owned, shared and borrowed), and provide helpers allowing to
convert this into SW bits annotations using the matching prot
attributes.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/kvm/hyp/include/nvhe/mem_protect.h | 26 +++++++++++++++++++
1 file changed, 26 insertions(+)
Hi Quentin,
On Thu, Jul 29, 2021 at 3:28 PM Quentin Perret [off-list ref] wrote:
quoted hunk
We will need to manipulate the host stage-2 page-table from outside
mem_protect.c soon. Introduce two functions allowing this, and make
them usable to users of mem_protect.h.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/kvm/hyp/include/nvhe/mem_protect.h | 2 ++
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 17 ++++++++++++++++-
2 files changed, 18 insertions(+), 1 deletion(-)
This is a potential issue elsewhere as well, but all functions in
kvm_pgtable.h, including kvm_pgtable_stage2_set_owner, specify an
address range via address and size. The two you have introduced here
take a start and an end. I'm not sure if making these two consistent
with the ones in kvm_pgtable.h would be good, or would just complicate
things in other places.
Thanks,
/fuad
Hi Quentin.
On Thu, Jul 29, 2021 at 3:28 PM Quentin Perret [off-list ref] wrote:
quoted hunk
Introduce a helper usable in nVHE protected mode to check whether a
physical address is in a RAM region or not.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/kvm/hyp/include/nvhe/mem_protect.h | 1 +
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 7 +++++++
2 files changed, 8 insertions(+)
I'm just wondering about the naming of the function. I understand what
you're trying to achieve with it, but an address without a unit that
conveys size or type seems to be missing something. Would
memregion_addr_is_memory or something like that be a better
description, since it is what find_mem_range finds?
Thanks,
/fuad
quoted hunk
int host_stage2_idmap_locked(u64 start, u64 end, enum kvm_pgtable_prot prot);
int host_stage2_set_owner_locked(u64 start, u64 end, u8 owner_id);
int kvm_host_prepare_stage2(void *pgt_pool_base);
Hi Quentin,
On Thu, Jul 29, 2021 at 3:29 PM Quentin Perret [off-list ref] wrote:
quoted hunk
Introduce helper functions in the KVM stage-2 and stage-1 page-table
manipulation library allowing to retrieve the enum kvm_pgtable_prot of a
PTE. This will be useful to implement custom walkers outside of
pgtable.c.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/include/asm/kvm_pgtable.h | 20 +++++++++++++++
arch/arm64/kvm/hyp/pgtable.c | 37 ++++++++++++++++++++++++++++
2 files changed, 57 insertions(+)
Hi Quentin,
On Thu, Jul 29, 2021 at 3:29 PM Quentin Perret [off-list ref] wrote:
quoted hunk
As the hypervisor maps the host's .bss and .rodata sections in its
stage-1, make sure to tag them as shared in hyp and host page-tables.
But since the hypervisor relies on the presence of these mappings, we
cannot let the host in complete control of the memory regions -- it
must not unshare or donate them to another entity for example. To
prevent this, let's transfer the ownership of those ranges to the
hypervisor itself, and share the pages back with the host.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/kvm/hyp/nvhe/setup.c | 82 +++++++++++++++++++++++++++++----
1 file changed, 74 insertions(+), 8 deletions(-)
@@ -58,6 +58,7 @@ static int recreate_hyp_mappings(phys_addr_t phys, unsigned long size,{void*start,*end,*virt=hyp_phys_to_virt(phys);unsignedlongpgt_size=hyp_s1_pgtable_pages()<<PAGE_SHIFT;+enumkvm_pgtable_protprot;intret,i;/* Recreate the hyp page-table using the early page allocator */
@@ -83,10 +84,6 @@ static int recreate_hyp_mappings(phys_addr_t phys, unsigned long size,if(ret)returnret;-ret=pkvm_create_mappings(__start_rodata,__end_rodata,PAGE_HYP_RO);-if(ret)-returnret;-ret=pkvm_create_mappings(__hyp_rodata_start,__hyp_rodata_end,PAGE_HYP_RO);if(ret)returnret;
@@ -95,10 +92,6 @@ static int recreate_hyp_mappings(phys_addr_t phys, unsigned long size,if(ret)returnret;-ret=pkvm_create_mappings(__hyp_bss_end,__bss_stop,PAGE_HYP_RO);-if(ret)-returnret;-ret=pkvm_create_mappings(virt,virt+size,PAGE_HYP);if(ret)returnret;
@@ -117,6 +110,24 @@ static int recreate_hyp_mappings(phys_addr_t phys, unsigned long size,returnret;}+/*+*Mapthehost's.bssand.rodatasectionsROinthehypervisor,but+*transfertheownerhsipfromthehosttothehypervisoritselfto+*makesureitcan'tbedonatedorsharedwithanotherentity.
nit: ownerhsip -> ownership
+ *
+ * The ownership transtion requires matching changes in the host
nit: transtion -> transition
+ * stage-2. This will done later (see finalize_host_mappings()) once the
nit: will done -> will be done
+ * hyp_vmemmap is addressable.
+ */
+ prot = pkvm_mkstate(PAGE_HYP_RO, PKVM_PAGE_SHARED_OWNED);
+ ret = pkvm_create_mappings(__start_rodata, __end_rodata, prot);
+ if (ret)
+ return ret;
+
+ ret = pkvm_create_mappings(__hyp_bss_end, __bss_stop, prot);
nit: for clarity, I wonder if it might be good to create an alias of
__hyp_bss_end as __bss_start or something. When it's been moved here,
it sticks out a bit more and makes the reader wonder about the
significance of __hyp_bss_end.
I know that it's not in scope here, but I'm wondering whether we
should be checking for KVM_PTE_TYPE_PAGE instead of the level. Maybe
it would be good to have a helper somewhere for all these checks both
for clarity and to ensure that nothing has gone wrong with the pte.
Hi Quentin,
On Thu, Jul 29, 2021 at 3:29 PM Quentin Perret [off-list ref] wrote:
quoted hunk
Refactor the hypervisor stage-1 locking in nVHE protected mode to expose
a new pkvm_create_mappings_locked() function. This will be used in later
patches to allow walking and changing the hypervisor stage-1 without
releasing the lock.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/kvm/hyp/include/nvhe/mm.h | 1 +
arch/arm64/kvm/hyp/nvhe/mm.c | 18 ++++++++++++++++--
2 files changed, 17 insertions(+), 2 deletions(-)
I'm wondering whether this patch should also refactor
__pkvm_create_mappings. It doesn't quite do the exact same thing and
has different parameters.
Thanks,
/fuad
int hyp_back_vmemmap(phys_addr_t phys, unsigned long size, phys_addr_t back)
{
unsigned long start, end;
--
2.32.0.432.gabb21c7263-goog
@@ -338,6 +338,95 @@ static int host_stage2_idmap(u64 addr)returnret;}+staticinlineboolcheck_prot(enumkvm_pgtable_protprot,+enumkvm_pgtable_protrequired,+enumkvm_pgtable_protdenied)+{+return(prot&(required|denied))==required;+}++int__pkvm_host_share_hyp(u64pfn)+{+phys_addr_taddr=hyp_pfn_to_phys(pfn);+enumkvm_pgtable_protprot,cur;+void*virt=__hyp_va(addr);+enumpkvm_page_statestate;+kvm_pte_tpte;+u32level;+intret;++if(!range_is_memory(addr,addr+PAGE_SIZE))+return-EINVAL;++hyp_spin_lock(&host_kvm.lock);+hyp_spin_lock(&pkvm_pgd_lock);++ret=kvm_pgtable_get_leaf(&host_kvm.pgt,addr,&pte,&level);+if(ret)+gotounlock;+if(!pte)+gotomap_shared;
Should this check whether kvm_pte_valid as well, is that guaranteed to
always be the case, or implicitly handled later?
+
+ /*
+ * Check attributes in the host stage-2 PTE. We need the page to be:
+ * - mapped RWX as we're sharing memory;
+ * - not borrowed, as that implies absence of ownership.
+ * Otherwise, we can't let it got through
+ */
+ cur = kvm_pgtable_stage2_pte_prot(pte);
+ prot = pkvm_mkstate(0, PKVM_PAGE_SHARED_BORROWED);
+ if (!check_prot(cur, KVM_PGTABLE_PROT_RWX, prot)) {
+ ret = -EPERM;
+ goto unlock;
+ }
+
+ state = pkvm_getstate(cur);
+ if (state == PKVM_PAGE_OWNED)
+ goto map_shared;
+
+ /*
+ * Tolerate double-sharing the same page, but this requires
+ * cross-checking the hypervisor stage-1.
+ */
+ if (state != PKVM_PAGE_SHARED_OWNED) {
+ ret = -EPERM;
+ goto unlock;
+ }
+
+ ret = kvm_pgtable_get_leaf(&pkvm_pgtable, (u64)virt, &pte, &level);
+ if (ret)
+ goto unlock;
+
+ /*
+ * If the page has been shared with the hypervisor, it must be
+ * SHARED_BORROWED already.
+ */
This comment confused me at first, but then I realized it's referring
to the page from the hyp's point of view. Could you add something to
the comment to that effect?
It might also make it easier to follow if the variables could be
annotated to specify whether cur, state, and prot are the host's or
hyps (and not reuse the same one for both).
quoted hunk
+ cur = kvm_pgtable_hyp_pte_prot(pte);
+ prot = pkvm_mkstate(PAGE_HYP, PKVM_PAGE_SHARED_BORROWED);
+ if (!check_prot(cur, prot, ~prot))
+ ret = EPERM;
+ goto unlock;
+
+map_shared:
+ /*
+ * If the page is not yet shared, adjust mappings in both page-tables
+ * while both locks are held.
+ */
+ prot = pkvm_mkstate(PAGE_HYP, PKVM_PAGE_SHARED_BORROWED);
+ ret = pkvm_create_mappings_locked(virt, virt + PAGE_SIZE, prot);
+ BUG_ON(ret);
+
+ prot = pkvm_mkstate(KVM_PGTABLE_PROT_RWX, PKVM_PAGE_SHARED_OWNED);
+ ret = host_stage2_idmap_locked(addr, addr + PAGE_SIZE, prot);
+ BUG_ON(ret);
+
+unlock:
+ hyp_spin_unlock(&pkvm_pgd_lock);
+ hyp_spin_unlock(&host_kvm.lock);
+
+ return ret;
+}
+
void handle_host_mem_abort(struct kvm_cpu_context *host_ctxt)
{
struct kvm_vcpu_fault_info fault;
@@ -259,10 +259,8 @@ static int __create_hyp_mappings(unsigned long start, unsigned long size,{interr;-if(!kvm_host_owns_hyp_mappings()){-returnkvm_call_hyp_nvhe(__pkvm_create_mappings,-start,size,phys,prot);-}+if(WARN_ON(!kvm_host_owns_hyp_mappings()))+return-EINVAL;mutex_lock(&kvm_hyp_pgd_mutex);err=kvm_pgtable_hyp_map(hyp_pgtable,start,size,phys,prot);
I guess we don't expect this to happen often, but I wonder if it would
be better to have the looping in the hyp call rather than here, to
reduce the number of hyp calls when sharing.
Thanks,
/fuad
quoted hunk
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
/**
* create_hyp_mappings - duplicate a kernel virtual address range in Hyp mode
* @from: The virtual kernel start address of the range
@@ -302,6 +315,13 @@ int create_hyp_mappings(void *from, void *to, enum kvm_pgtable_prot prot) if (is_kernel_in_hyp_mode()) return 0;+ if (!kvm_host_owns_hyp_mappings()) {+ if (WARN_ON(prot != PAGE_HYP))+ return -EPERM;+ return pkvm_share_hyp(kvm_kaddr_to_phys(from),+ kvm_kaddr_to_phys(to));+ }+ start = start & PAGE_MASK; end = PAGE_ALIGN(end);--
Hi Fuad,
On Monday 02 Aug 2021 at 11:49:28 (+0200), Fuad Tabba wrote:
On Thu, Jul 29, 2021 at 3:28 PM Quentin Perret [off-list ref] wrote:
quoted
Much of the stage-2 manipulation logic relies on being able to destroy
block mappings if e.g. installing a smaller mapping in the range. The
rationale for this behaviour is that stage-2 mappings can always be
re-created lazily. However, this gets more complicated when the stage-2
page-table is used to store metadata about the underlying pages. In such
cases, destroying a block mapping may lead to losing part of the state,
and confuse the user of those metadata (such as the hypervisor in nVHE
protected mode).
To avoid this, introduce a callback function in the pgtable struct which
is called during all map operations to determine whether the mappings
can use blocks, or should be forced to page granularity. This is used by
the hypervisor when creating the host stage-2 to force page-level
mappings when using non-default protection attributes.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/include/asm/kvm_pgtable.h | 65 ++++++++++++++++-----------
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 30 +++++++++++--
arch/arm64/kvm/hyp/pgtable.c | 29 +++++++++---
3 files changed, 91 insertions(+), 33 deletions(-)
I wonder if it would be useful to add a couple of other aliases for
default memory and default mmio protections, e.g.,
#define KVM_PGTABLE_PROT_MEM KVM_PGTABLE_PROT_RWX
#define KVM_PGTABLE_PROT_MMIO KVM_PGTABLE_PROT_RW
I think that using these below, e.g., host_stage2_force_pte_cb(),
might make it clearer and answer comments you had in earlier patches
about why "RWX" for memory.
Sure I can add something. I'll probably call them something else than
KVM_PGTABLE_PROT_{MEM,MMIO} though, just to make it clear this is all
specific to the host stage-2 stuff and not a general requirement of the
pgtable code to map things like this.
quoted
+typedef bool (*kvm_pgtable_force_pte_cb_t)(u64 addr, u64 end,
+ enum kvm_pgtable_prot prot);
+
+/**
+ * struct kvm_pgtable - KVM page-table.
+ * @ia_bits: Maximum input address size, in bits.
+ * @start_level: Level at which the page-table walk starts.
+ * @pgd: Pointer to the first top-level entry of the page-table.
+ * @mm_ops: Memory management callbacks.
+ * @mmu: Stage-2 KVM MMU struct. Unused for stage-1 page-tables.
+ * @flags: Stage-2 page-table flags.
+ * @force_pte_cb: Callback function used during map operations to decide
+ * whether block mappings can be used to map the given IPA
+ * range.
+ */
nit: I think it might be clearer (and probably not longer) to rephrase
to describe in terms of the return value of the callback, e.g., "...
function that returns true if page level mappings must be used instead
of block mappings."
int kvm_host_prepare_stage2(void *pgt_pool_base)
{
struct kvm_s2_mmu *mmu = &host_kvm.arch.mmu;
@@ -101,8 +102,9 @@ int kvm_host_prepare_stage2(void *pgt_pool_base) if (ret) return ret;- ret = kvm_pgtable_stage2_init_flags(&host_kvm.pgt, &host_kvm.arch,- &host_kvm.mm_ops, KVM_HOST_S2_FLAGS);+ ret = __kvm_pgtable_stage2_init(&host_kvm.pgt, &host_kvm.arch,+ &host_kvm.mm_ops, KVM_HOST_S2_FLAGS,+ host_stage2_force_pte_cb); if (ret) return ret;
@@ -270,9 +272,31 @@ static int host_stage2_adjust_range(u64 addr, struct kvm_mem_range *range) return 0; }+static bool host_stage2_force_pte_cb(u64 addr, u64 end, enum kvm_pgtable_prot prot)+{+ /*+ * Block mappings must be used with care in the host stage-2 as a+ * kvm_pgtable_stage2_map() operation targeting a page in the range of+ * an existing block will delete the block under the assumption that+ * mappings in the rest of the block range can always be rebuilt lazily.+ * That assumption is correct for the host stage-2 with RWX mappings+ * targeting memory or RW mappings targeting MMIO ranges (see+ * host_stage2_idmap() below which implements some of the host memory+ * abort logic). However, this is not safe for any other mappings where+ * the host stage-2 page-table is in fact the only place where this+ * state is stored. In all those cases, it is safer to use page-level+ * mappings, hence avoiding to lose the state because of side-effects in+ * kvm_pgtable_stage2_map().+ */+ if (range_is_memory(addr, end))+ return prot != KVM_PGTABLE_PROT_RWX;+ else+ return prot != KVM_PGTABLE_PROT_RW;+}
Just checking, I don't think that it's possible for the range to be
big enough to somehow include both memory and mmio, neither now nor in
future use cases, is it?
That really shouldn't be the case no -- the host_stage2_idmap() function
tries hard to respect that, so I figured as long as these two are
consistent we should be fine.
I'm not sure I understand why checking the level is necessary. Can
there be block mapping at the last possible level?
That's probably just a matter of naming, but this function is in fact
called at every level, just like kvm_block_mapping_supported() was
before. And we rely on it returning true at the last level, so I need to
do that check here.
Maybe renaming this stage2_leaf_mapping_allowed() would clarify?
Thanks,
Quentin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Monday 02 Aug 2021 at 13:13:20 (+0200), Fuad Tabba wrote:
Hi Quentin,
On Thu, Jul 29, 2021 at 3:28 PM Quentin Perret [off-list ref] wrote:
quoted
We will need to manipulate the host stage-2 page-table from outside
mem_protect.c soon. Introduce two functions allowing this, and make
them usable to users of mem_protect.h.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/kvm/hyp/include/nvhe/mem_protect.h | 2 ++
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 17 ++++++++++++++++-
2 files changed, 18 insertions(+), 1 deletion(-)
This is a potential issue elsewhere as well, but all functions in
kvm_pgtable.h, including kvm_pgtable_stage2_set_owner, specify an
address range via address and size. The two you have introduced here
take a start and an end. I'm not sure if making these two consistent
with the ones in kvm_pgtable.h would be good, or would just complicate
things in other places.
Good point, and it looks like specifying these two with start-size
parameters would simplify the callers a tiny bit as well, so I'll fold
that in v4.
Thanks,
Quentin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Monday 02 Aug 2021 at 16:52:31 (+0200), Fuad Tabba wrote:
Hi Quentin.
On Thu, Jul 29, 2021 at 3:28 PM Quentin Perret [off-list ref] wrote:
quoted
Introduce a helper usable in nVHE protected mode to check whether a
physical address is in a RAM region or not.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/kvm/hyp/include/nvhe/mem_protect.h | 1 +
arch/arm64/kvm/hyp/nvhe/mem_protect.c | 7 +++++++
2 files changed, 8 insertions(+)
I'm just wondering about the naming of the function. I understand what
you're trying to achieve with it, but an address without a unit that
conveys size or type seems to be missing something. Would
Well it does have a type no? I was hopping this would make it clear what
it actually does.
memregion_addr_is_memory or something like that be a better
description, since it is what find_mem_range finds?
I think the callers shouldn't need to care about the implementation
details though. This just replies to the question 'is this physical
address in RAM range or not?'. And I could actually imagine that we
would change the implementation some day to avoid the binary search, but
the users probably don't need to care.
Thanks,
Quentin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Monday 02 Aug 2021 at 16:52:49 (+0200), Fuad Tabba wrote:
Hi Quentin,
On Thu, Jul 29, 2021 at 3:29 PM Quentin Perret [off-list ref] wrote:
quoted
Introduce helper functions in the KVM stage-2 and stage-1 page-table
manipulation library allowing to retrieve the enum kvm_pgtable_prot of a
PTE. This will be useful to implement custom walkers outside of
pgtable.c.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/include/asm/kvm_pgtable.h | 20 +++++++++++++++
arch/arm64/kvm/hyp/pgtable.c | 37 ++++++++++++++++++++++++++++
2 files changed, 57 insertions(+)
On Tuesday 03 Aug 2021 at 07:02:42 (+0200), Fuad Tabba wrote:
Hi Quentin,
On Thu, Jul 29, 2021 at 3:29 PM Quentin Perret [off-list ref] wrote:
quoted
As the hypervisor maps the host's .bss and .rodata sections in its
stage-1, make sure to tag them as shared in hyp and host page-tables.
But since the hypervisor relies on the presence of these mappings, we
cannot let the host in complete control of the memory regions -- it
must not unshare or donate them to another entity for example. To
prevent this, let's transfer the ownership of those ranges to the
hypervisor itself, and share the pages back with the host.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/kvm/hyp/nvhe/setup.c | 82 +++++++++++++++++++++++++++++----
1 file changed, 74 insertions(+), 8 deletions(-)
@@ -58,6 +58,7 @@ static int recreate_hyp_mappings(phys_addr_t phys, unsigned long size,{void*start,*end,*virt=hyp_phys_to_virt(phys);unsignedlongpgt_size=hyp_s1_pgtable_pages()<<PAGE_SHIFT;+enumkvm_pgtable_protprot;intret,i;/* Recreate the hyp page-table using the early page allocator */
@@ -83,10 +84,6 @@ static int recreate_hyp_mappings(phys_addr_t phys, unsigned long size,if(ret)returnret;-ret=pkvm_create_mappings(__start_rodata,__end_rodata,PAGE_HYP_RO);-if(ret)-returnret;-ret=pkvm_create_mappings(__hyp_rodata_start,__hyp_rodata_end,PAGE_HYP_RO);if(ret)returnret;
@@ -95,10 +92,6 @@ static int recreate_hyp_mappings(phys_addr_t phys, unsigned long size,if(ret)returnret;-ret=pkvm_create_mappings(__hyp_bss_end,__bss_stop,PAGE_HYP_RO);-if(ret)-returnret;-ret=pkvm_create_mappings(virt,virt+size,PAGE_HYP);if(ret)returnret;
@@ -117,6 +110,24 @@ static int recreate_hyp_mappings(phys_addr_t phys, unsigned long size,returnret;}+/*+*Mapthehost's.bssand.rodatasectionsROinthehypervisor,but+*transfertheownerhsipfromthehosttothehypervisoritselfto+*makesureitcan'tbedonatedorsharedwithanotherentity.
nit: ownerhsip -> ownership
quoted
+ *
+ * The ownership transtion requires matching changes in the host
nit: transtion -> transition
quoted
+ * stage-2. This will done later (see finalize_host_mappings()) once the
nit: will done -> will be done
Urgh, I clearly went too fast writing this, thanks!
quoted
+ * hyp_vmemmap is addressable.
+ */
+ prot = pkvm_mkstate(PAGE_HYP_RO, PKVM_PAGE_SHARED_OWNED);
+ ret = pkvm_create_mappings(__start_rodata, __end_rodata, prot);
+ if (ret)
+ return ret;
+
+ ret = pkvm_create_mappings(__hyp_bss_end, __bss_stop, prot);
nit: for clarity, I wonder if it might be good to create an alias of
__hyp_bss_end as __bss_start or something. When it's been moved here,
it sticks out a bit more and makes the reader wonder about the
significance of __hyp_bss_end.
I understand what you mean, but I'm not sure this aliasing is really
going to clarify things much. We have a comment in arm.c (see
init_hyp_mode()) to explain exactly why we're doing this, so maybe it
would be worth adding it here too. WDYT?
I know that it's not in scope here, but I'm wondering whether we
should be checking for KVM_PTE_TYPE_PAGE instead of the level. Maybe
Well these would check different things no?
it would be good to have a helper somewhere for all these checks both
for clarity and to ensure that nothing has gone wrong with the pte.
The reason I need this check is just to make sure the call to
host_stage2_idmap_locked() further down is correct with a hardcoded
PAGE_SIZE size. The alternative would be to not be lazy and actually
compute the current granule size based on the level and use that, as
that would make this code robust to using block mappings at EL2 stage-1
in the future.
And I'll fix this up for v4.
Cheers,
Quentin
Hey Fuad,
On Tuesday 03 Aug 2021 at 07:31:03 (+0200), Fuad Tabba wrote:
Hi Quentin,
On Thu, Jul 29, 2021 at 3:29 PM Quentin Perret [off-list ref] wrote:
quoted
Refactor the hypervisor stage-1 locking in nVHE protected mode to expose
a new pkvm_create_mappings_locked() function. This will be used in later
patches to allow walking and changing the hypervisor stage-1 without
releasing the lock.
Signed-off-by: Quentin Perret <redacted>
---
arch/arm64/kvm/hyp/include/nvhe/mm.h | 1 +
arch/arm64/kvm/hyp/nvhe/mm.c | 18 ++++++++++++++++--
2 files changed, 17 insertions(+), 2 deletions(-)
I'm wondering whether this patch should also refactor
__pkvm_create_mappings. It doesn't quite do the exact same thing and
has different parameters.
Sorry, not sure I'm understanding your suggestion here. What do you
think should be done to __pkvm_create_mappings?
Cheers,
Quentin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
@@ -338,6 +338,95 @@ static int host_stage2_idmap(u64 addr)returnret;}+staticinlineboolcheck_prot(enumkvm_pgtable_protprot,+enumkvm_pgtable_protrequired,+enumkvm_pgtable_protdenied)+{+return(prot&(required|denied))==required;+}++int__pkvm_host_share_hyp(u64pfn)+{+phys_addr_taddr=hyp_pfn_to_phys(pfn);+enumkvm_pgtable_protprot,cur;+void*virt=__hyp_va(addr);+enumpkvm_page_statestate;+kvm_pte_tpte;+u32level;+intret;++if(!range_is_memory(addr,addr+PAGE_SIZE))+return-EINVAL;++hyp_spin_lock(&host_kvm.lock);+hyp_spin_lock(&pkvm_pgd_lock);++ret=kvm_pgtable_get_leaf(&host_kvm.pgt,addr,&pte,&level);+if(ret)+gotounlock;+if(!pte)+gotomap_shared;
Should this check whether kvm_pte_valid as well, is that guaranteed to
always be the case, or implicitly handled later?
Yep, this is implicitly handled by kvm_pgtable_stage2_pte_prot() which
is guaranteed not to return KVM_PGTABLE_PROT_RWX for an invalid mapping.
quoted
+
+ /*
+ * Check attributes in the host stage-2 PTE. We need the page to be:
+ * - mapped RWX as we're sharing memory;
+ * - not borrowed, as that implies absence of ownership.
+ * Otherwise, we can't let it got through
+ */
+ cur = kvm_pgtable_stage2_pte_prot(pte);
+ prot = pkvm_mkstate(0, PKVM_PAGE_SHARED_BORROWED);
+ if (!check_prot(cur, KVM_PGTABLE_PROT_RWX, prot)) {
+ ret = -EPERM;
+ goto unlock;
+ }
+
+ state = pkvm_getstate(cur);
+ if (state == PKVM_PAGE_OWNED)
+ goto map_shared;
+
+ /*
+ * Tolerate double-sharing the same page, but this requires
+ * cross-checking the hypervisor stage-1.
+ */
+ if (state != PKVM_PAGE_SHARED_OWNED) {
+ ret = -EPERM;
+ goto unlock;
+ }
+
+ ret = kvm_pgtable_get_leaf(&pkvm_pgtable, (u64)virt, &pte, &level);
+ if (ret)
+ goto unlock;
+
+ /*
+ * If the page has been shared with the hypervisor, it must be
+ * SHARED_BORROWED already.
+ */
This comment confused me at first, but then I realized it's referring
to the page from the hyp's point of view. Could you add something to
the comment to that effect?
Sure thing.
It might also make it easier to follow if the variables could be
annotated to specify whether cur, state, and prot are the host's or
hyps (and not reuse the same one for both).
quoted
+ cur = kvm_pgtable_hyp_pte_prot(pte);
+ prot = pkvm_mkstate(PAGE_HYP, PKVM_PAGE_SHARED_BORROWED);
+ if (!check_prot(cur, prot, ~prot))
+ ret = EPERM;
+ goto unlock;
+
+map_shared:
+ /*
+ * If the page is not yet shared, adjust mappings in both page-tables
+ * while both locks are held.
+ */
+ prot = pkvm_mkstate(PAGE_HYP, PKVM_PAGE_SHARED_BORROWED);
+ ret = pkvm_create_mappings_locked(virt, virt + PAGE_SIZE, prot);
+ BUG_ON(ret);
+
+ prot = pkvm_mkstate(KVM_PGTABLE_PROT_RWX, PKVM_PAGE_SHARED_OWNED);
+ ret = host_stage2_idmap_locked(addr, addr + PAGE_SIZE, prot);
+ BUG_ON(ret);
+
+unlock:
+ hyp_spin_unlock(&pkvm_pgd_lock);
+ hyp_spin_unlock(&host_kvm.lock);
+
+ return ret;
+}
+
void handle_host_mem_abort(struct kvm_cpu_context *host_ctxt)
{
struct kvm_vcpu_fault_info fault;
@@ -259,10 +259,8 @@ static int __create_hyp_mappings(unsigned long start, unsigned long size,{interr;-if(!kvm_host_owns_hyp_mappings()){-returnkvm_call_hyp_nvhe(__pkvm_create_mappings,-start,size,phys,prot);-}+if(WARN_ON(!kvm_host_owns_hyp_mappings()))+return-EINVAL;mutex_lock(&kvm_hyp_pgd_mutex);err=kvm_pgtable_hyp_map(hyp_pgtable,start,size,phys,prot);
I guess we don't expect this to happen often, but I wonder if it would
be better to have the looping in the hyp call rather than here, to
reduce the number of hyp calls when sharing.
Yes, I was wondering the same thing, but ended up doing the looping here
to avoid spending long periods of time in a non-preemptible state at
EL2. Probably doesn't make a big difference for now, but it might if we
ever need to share large memory regions.
Cheers,
Quentin
Thanks,
/fuad
quoted
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
/**
* create_hyp_mappings - duplicate a kernel virtual address range in Hyp mode
* @from: The virtual kernel start address of the range
@@ -302,6 +315,13 @@ int create_hyp_mappings(void *from, void *to, enum kvm_pgtable_prot prot) if (is_kernel_in_hyp_mode()) return 0;+ if (!kvm_host_owns_hyp_mappings()) {+ if (WARN_ON(prot != PAGE_HYP))+ return -EPERM;+ return pkvm_share_hyp(kvm_kaddr_to_phys(from),+ kvm_kaddr_to_phys(to));+ }+ start = start & PAGE_MASK; end = PAGE_ALIGN(end);--
I'm not sure I understand why checking the level is necessary. Can
there be block mapping at the last possible level?
That's probably just a matter of naming, but this function is in fact
called at every level, just like kvm_block_mapping_supported() was
before. And we rely on it returning true at the last level, so I need to
do that check here.
Maybe renaming this stage2_leaf_mapping_allowed() would clarify?
+int pkvm_create_mappings(void *from, void *to, enum kvm_pgtable_prot prot)
+{
+ int ret;
+
+ hyp_spin_lock(&pkvm_pgd_lock);
+ ret = pkvm_create_mappings_locked(from, to, prot);
+ hyp_spin_unlock(&pkvm_pgd_lock);
+
+ return ret;
+}
+
I'm wondering whether this patch should also refactor
__pkvm_create_mappings. It doesn't quite do the exact same thing and
has different parameters.
Sorry, not sure I'm understanding your suggestion here. What do you
think should be done to __pkvm_create_mappings?
Sorry, my comment wasn't very clear, and "refactor" is the wrong word.
I think it should probably be renamed, because __pkvm_create_mappings
isn't called by pkvm_create_mappings nor by
pkvm_create_mappings_locked. It also has different parameters and
behaves slightly differently.
Thanks,
/fuad
+ ret = pkvm_create_mappings(__hyp_bss_end, __bss_stop, prot);
nit: for clarity, I wonder if it might be good to create an alias of
__hyp_bss_end as __bss_start or something. When it's been moved here,
it sticks out a bit more and makes the reader wonder about the
significance of __hyp_bss_end.
I understand what you mean, but I'm not sure this aliasing is really
going to clarify things much. We have a comment in arm.c (see
init_hyp_mode()) to explain exactly why we're doing this, so maybe it
would be worth adding it here too. WDYT?
Not sure to be honest. Comments are good, until they're stale, and
replicating the comment increases the odds of that happening. No
strong opinion either way.
I know that it's not in scope here, but I'm wondering whether we
should be checking for KVM_PTE_TYPE_PAGE instead of the level. Maybe
Well these would check different things no?
quoted
it would be good to have a helper somewhere for all these checks both
for clarity and to ensure that nothing has gone wrong with the pte.
The reason I need this check is just to make sure the call to
host_stage2_idmap_locked() further down is correct with a hardcoded
PAGE_SIZE size. The alternative would be to not be lazy and actually
compute the current granule size based on the level and use that, as
that would make this code robust to using block mappings at EL2 stage-1
in the future.
And I'll fix this up for v4.