From: Quentin Perret <qperret@google.com>
To: Will Deacon <will@kernel.org>
Cc: android-kvm@google.com, catalin.marinas@arm.com,
mate.toth-pal@arm.com, seanjc@google.com, tabba@google.com,
linux-kernel@vger.kernel.org, robh+dt@kernel.org,
linux-arm-kernel@lists.infradead.org, maz@kernel.org,
kernel-team@android.com, kvmarm@lists.cs.columbia.edu
Subject: Re: [PATCH v4 30/34] KVM: arm64: Add kvm_pgtable_stage2_find_range()
Date: Fri, 12 Mar 2021 05:32:13 +0000 [thread overview]
Message-ID: <YEr83QKZnEd4a6Ba@google.com> (raw)
In-Reply-To: <20210311190406.GB31586@willie-the-truck>
On Thursday 11 Mar 2021 at 19:04:07 (+0000), Will Deacon wrote:
> On Wed, Mar 10, 2021 at 05:57:47PM +0000, Quentin Perret wrote:
> > Since the host stage 2 will be identity mapped, and since it will own
> > most of memory, it would preferable for performance to try and use large
> > block mappings whenever that is possible. To ease this, introduce a new
> > helper in the KVM page-table code which allows to search for large
> > ranges of available IPA space. This will be used in the host memory
> > abort path to greedily idmap large portion of the PA space.
> >
> > Signed-off-by: Quentin Perret <qperret@google.com>
> > ---
> > arch/arm64/include/asm/kvm_pgtable.h | 30 ++++++++++
> > arch/arm64/kvm/hyp/pgtable.c | 90 +++++++++++++++++++++++++++-
> > 2 files changed, 117 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
> > index b09af4612656..477bf10c48a9 100644
> > --- a/arch/arm64/include/asm/kvm_pgtable.h
> > +++ b/arch/arm64/include/asm/kvm_pgtable.h
> > @@ -94,6 +94,16 @@ enum kvm_pgtable_prot {
> > #define PAGE_HYP_RO (KVM_PGTABLE_PROT_R)
> > #define PAGE_HYP_DEVICE (PAGE_HYP | KVM_PGTABLE_PROT_DEVICE)
> >
> > +/**
> > + * struct kvm_mem_range - Range of Intermediate Physical Addresses
> > + * @start: Start of the range.
> > + * @end: End of the range.
> > + */
> > +struct kvm_mem_range {
> > + u64 start;
> > + u64 end;
> > +};
> > +
> > /**
> > * enum kvm_pgtable_walk_flags - Flags to control a depth-first page-table walk.
> > * @KVM_PGTABLE_WALK_LEAF: Visit leaf entries, including invalid
> > @@ -398,4 +408,24 @@ int kvm_pgtable_stage2_flush(struct kvm_pgtable *pgt, u64 addr, u64 size);
> > int kvm_pgtable_walk(struct kvm_pgtable *pgt, u64 addr, u64 size,
> > struct kvm_pgtable_walker *walker);
> >
> > +/**
> > + * kvm_pgtable_stage2_find_range() - Find a range of Intermediate Physical
> > + * Addresses with compatible permission
> > + * attributes.
> > + * @pgt: Page-table structure initialised by kvm_pgtable_stage2_init().
> > + * @addr: Address that must be covered by the range.
> > + * @prot: Protection attributes that the range must be compatible with.
> > + * @range: Range structure used to limit the search space at call time and
> > + * that will hold the result.
> > + *
> > + * The offset of @addr within a page is ignored. An existing mapping is defined
> > + * as compatible with @prot if it is invalid and not owned by another entity, or
> > + * if its permission attributes are strictly similar to @prot and it has no
> > + * software bits set.
>
> nit: I think the 'or' is ambigious here, as it makes it sound like an
> invalid entry that _is_ owned by another entity is compatible if the
> permissions attribute match. How about:
>
> | An IPA is compatible with prot iff its corresponding stage-2 page-table
> | entry has default ownership and, if valid, is mapped with protection
> | attributes identical to @prot.
Works for me.
> > + *
> > + * Return: 0 on success, negative error code on failure.
> > + */
> > +int kvm_pgtable_stage2_find_range(struct kvm_pgtable *pgt, u64 addr,
> > + enum kvm_pgtable_prot prot,
> > + struct kvm_mem_range *range);
> > #endif /* __ARM64_KVM_PGTABLE_H__ */
> > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> > index c16e0306dd9a..f20287bb3e41 100644
> > --- a/arch/arm64/kvm/hyp/pgtable.c
> > +++ b/arch/arm64/kvm/hyp/pgtable.c
> > @@ -48,6 +48,8 @@
> > KVM_PTE_LEAF_ATTR_LO_S2_S2AP_W | \
> > KVM_PTE_LEAF_ATTR_HI_S2_XN)
> >
> > +#define KVM_PTE_LEAF_ATTR_S2_RES GENMASK(58, 55)
>
> Please make this IGN instead of RES, to match the architecture (and because
> RES is usually used in the context of RES0 or RES1).
Ack, maybe I'll even say 'IGNORED' in full to match the Arm ARM -- IGN
sounds like an acronym otherwise.
> > #define KVM_INVALID_PTE_OWNER_MASK GENMASK(63, 32)
> >
> > struct kvm_pgtable_walk_data {
> > @@ -69,10 +71,8 @@ static u64 kvm_granule_size(u32 level)
> > return BIT(kvm_granule_shift(level));
> > }
> >
> > -static bool kvm_block_mapping_supported(u64 addr, u64 end, u64 phys, u32 level)
> > +static bool kvm_level_support_block_mappings(u32 level)
> > {
> > - u64 granule = kvm_granule_size(level);
> > -
> > /*
> > * Reject invalid block mappings and don't bother with 4TB mappings for
> > * 52-bit PAs.
> > @@ -80,6 +80,16 @@ static bool kvm_block_mapping_supported(u64 addr, u64 end, u64 phys, u32 level)
> > if (level == 0 || (PAGE_SIZE != SZ_4K && level == 1))
> > return false;
> >
> > + return true;
>
> return !(level == 0 || (PAGE_SIZE != SZ_4K && level == 1));
>
> > +static bool kvm_block_mapping_supported(u64 addr, u64 end, u64 phys, u32 level)
> > +{
> > + u64 granule = kvm_granule_size(level);
> > +
> > + if (!kvm_level_support_block_mappings(level))
> > + return false;
> > +
> > if (granule > (end - addr))
> > return false;
> >
> > @@ -1042,3 +1052,77 @@ void kvm_pgtable_stage2_destroy(struct kvm_pgtable *pgt)
> > pgt->mm_ops->free_pages_exact(pgt->pgd, pgd_sz);
> > pgt->pgd = NULL;
> > }
> > +
> > +#define KVM_PTE_LEAF_S2_COMPAT_MASK (KVM_PTE_LEAF_ATTR_S2_PERMS | \
> > + KVM_PTE_LEAF_ATTR_LO_S2_MEMATTR | \
> > + KVM_PTE_LEAF_ATTR_S2_RES)
> > +
> > +static int stage2_check_permission_walker(u64 addr, u64 end, u32 level,
> > + kvm_pte_t *ptep,
> > + enum kvm_pgtable_walk_flags flag,
> > + void * const arg)
> > +{
> > + kvm_pte_t old_attr, pte = *ptep, *new_attr = arg;
> > +
> > + /*
> > + * Compatible mappings are either invalid and owned by the page-table
> > + * owner (whose id is 0), or valid with matching permission attributes.
> > + */
> > + if (kvm_pte_valid(pte)) {
> > + old_attr = pte & KVM_PTE_LEAF_S2_COMPAT_MASK;
> > + if (old_attr != *new_attr)
> > + return -EEXIST;
> > + } else if (pte) {
> > + return -EEXIST;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int kvm_pgtable_stage2_find_range(struct kvm_pgtable *pgt, u64 addr,
> > + enum kvm_pgtable_prot prot,
> > + struct kvm_mem_range *range)
> > +{
> > + kvm_pte_t attr;
> > + struct kvm_pgtable_walker check_perm_walker = {
> > + .cb = stage2_check_permission_walker,
> > + .flags = KVM_PGTABLE_WALK_LEAF,
> > + .arg = &attr,
> > + };
> > + u64 granule, start, end;
> > + u32 level;
> > + int ret;
> > +
> > + ret = stage2_set_prot_attr(prot, &attr);
> > + if (ret)
> > + return ret;
> > + attr &= KVM_PTE_LEAF_S2_COMPAT_MASK;
> > +
> > + for (level = pgt->start_level; level < KVM_PGTABLE_MAX_LEVELS; level++) {
> > + granule = kvm_granule_size(level);
> > + start = ALIGN_DOWN(addr, granule);
> > + end = start + granule;
> > +
> > + if (!kvm_level_support_block_mappings(level))
> > + continue;
> > +
> > + if (start < range->start || range->end < end)
> > + continue;
> > +
> > + /*
> > + * Check the presence of existing mappings with incompatible
> > + * permissions within the current block range, and try one level
> > + * deeper if one is found.
> > + */
> > + ret = kvm_pgtable_walk(pgt, start, granule, &check_perm_walker);
> > + if (ret != -EEXIST)
> > + break;
> > + }
>
> Can you write this as a:
>
> do {
> ...
> } while (level < KVM_PGTABLE_MAX_LEVELS && ret == -EEXIST);
>
> loop?
I tried it but found it a little less pretty -- the pre-assignment of
level and the increment at the end make it really feel like a for loop
to me:
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -1098,26 +1098,23 @@ int kvm_pgtable_stage2_find_range(struct kvm_pgtable *pgt, u64 addr,
return ret;
attr &= KVM_PTE_LEAF_S2_COMPAT_MASK;
- for (level = pgt->start_level; level < KVM_PGTABLE_MAX_LEVELS; level++) {
+ ret = -EEXIST;
+ level = pgt->start_level;
+ do {
granule = kvm_granule_size(level);
start = ALIGN_DOWN(addr, granule);
end = start + granule;
- if (!kvm_level_support_block_mappings(level))
- continue;
-
- if (start < range->start || range->end < end)
- continue;
-
/*
* Check the presence of existing mappings with incompatible
* permissions within the current block range, and try one level
* deeper if one is found.
*/
- ret = kvm_pgtable_walk(pgt, start, granule, &check_perm_walker);
- if (ret != -EEXIST)
- break;
- }
+ if (kvm_level_support_block_mappings(level) && start >= range->start && range->end >= end)
+ ret = kvm_pgtable_walk(pgt, start, granule, &check_perm_walker);
+
+ level++;
+ } while (level < KVM_PGTABLE_MAX_LEVELS && ret == -EEXIST);
if (!ret) {
I'm guessing you're not a fan of the break case in existing loop?
Another option could be:
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -1098,7 +1098,7 @@ int kvm_pgtable_stage2_find_range(struct kvm_pgtable *pgt, u64 addr,
return ret;
attr &= KVM_PTE_LEAF_S2_COMPAT_MASK;
- for (level = pgt->start_level; level < KVM_PGTABLE_MAX_LEVELS; level++) {
+ for (ret = -EEXIST, level = pgt->start_level; level < KVM_PGTABLE_MAX_LEVELS && ret == -EEXIST; level++) {
granule = kvm_granule_size(level);
start = ALIGN_DOWN(addr, granule);
end = start + granule;
@@ -1115,8 +1115,6 @@ int kvm_pgtable_stage2_find_range(struct kvm_pgtable *pgt, u64 addr,
* deeper if one is found.
*/
ret = kvm_pgtable_walk(pgt, start, granule, &check_perm_walker);
- if (ret != -EEXIST)
- break;
}
if (!ret) {
No strong opinion either way. Thoughts?
Thanks,
Quentin
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
next prev parent reply other threads:[~2021-03-12 5:32 UTC|newest]
Thread overview: 59+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-10 17:57 [PATCH v4 00/34] KVM: arm64: A stage 2 for the host Quentin Perret
2021-03-10 17:57 ` [PATCH v4 01/34] arm64: lib: Annotate {clear, copy}_page() as position-independent Quentin Perret
2021-03-10 17:57 ` [PATCH v4 02/34] KVM: arm64: Link position-independent string routines into .hyp.text Quentin Perret
2021-03-10 17:57 ` [PATCH v4 03/34] arm64: kvm: Add standalone ticket spinlock implementation for use at hyp Quentin Perret
2021-03-10 17:57 ` [PATCH v4 04/34] KVM: arm64: Initialize kvm_nvhe_init_params early Quentin Perret
2021-03-10 17:57 ` [PATCH v4 05/34] KVM: arm64: Avoid free_page() in page-table allocator Quentin Perret
2021-03-10 17:57 ` [PATCH v4 06/34] KVM: arm64: Factor memory allocation out of pgtable.c Quentin Perret
2021-03-11 16:09 ` Will Deacon
2021-03-10 17:57 ` [PATCH v4 07/34] KVM: arm64: Introduce a BSS section for use at Hyp Quentin Perret
2021-03-10 17:57 ` [PATCH v4 08/34] KVM: arm64: Make kvm_call_hyp() a function call " Quentin Perret
2021-03-10 17:57 ` [PATCH v4 09/34] KVM: arm64: Allow using kvm_nvhe_sym() in hyp code Quentin Perret
2021-03-10 17:57 ` [PATCH v4 10/34] KVM: arm64: Introduce an early Hyp page allocator Quentin Perret
2021-03-10 17:57 ` [PATCH v4 11/34] KVM: arm64: Stub CONFIG_DEBUG_LIST at Hyp Quentin Perret
2021-03-11 16:11 ` Will Deacon
2021-03-10 17:57 ` [PATCH v4 12/34] KVM: arm64: Introduce a Hyp buddy page allocator Quentin Perret
2021-03-11 16:14 ` Will Deacon
2021-03-10 17:57 ` [PATCH v4 13/34] KVM: arm64: Enable access to sanitized CPU features at EL2 Quentin Perret
2021-03-11 19:36 ` Will Deacon
2021-03-12 6:34 ` Quentin Perret
2021-03-12 9:25 ` Will Deacon
2021-03-10 17:57 ` [PATCH v4 14/34] KVM: arm64: Factor out vector address calculation Quentin Perret
2021-03-10 17:57 ` [PATCH v4 15/34] arm64: asm: Provide set_sctlr_el2 macro Quentin Perret
2021-03-11 16:22 ` Will Deacon
2021-03-10 17:57 ` [PATCH v4 16/34] KVM: arm64: Prepare the creation of s1 mappings at EL2 Quentin Perret
2021-03-11 16:21 ` Will Deacon
2021-03-10 17:57 ` [PATCH v4 17/34] KVM: arm64: Elevate hypervisor mappings creation " Quentin Perret
2021-03-11 17:28 ` Will Deacon
2021-03-10 17:57 ` [PATCH v4 18/34] KVM: arm64: Use kvm_arch for stage 2 pgtable Quentin Perret
2021-03-10 17:57 ` [PATCH v4 19/34] KVM: arm64: Use kvm_arch in kvm_s2_mmu Quentin Perret
2021-03-10 17:57 ` [PATCH v4 20/34] KVM: arm64: Set host stage 2 using kvm_nvhe_init_params Quentin Perret
2021-03-10 17:57 ` [PATCH v4 21/34] KVM: arm64: Refactor kvm_arm_setup_stage2() Quentin Perret
2021-03-10 17:57 ` [PATCH v4 22/34] KVM: arm64: Refactor __load_guest_stage2() Quentin Perret
2021-03-10 17:57 ` [PATCH v4 23/34] KVM: arm64: Refactor __populate_fault_info() Quentin Perret
2021-03-10 17:57 ` [PATCH v4 24/34] KVM: arm64: Make memcache anonymous in pgtable allocator Quentin Perret
2021-03-10 17:57 ` [PATCH v4 25/34] KVM: arm64: Reserve memory for host stage 2 Quentin Perret
2021-03-10 17:57 ` [PATCH v4 26/34] KVM: arm64: Sort the hypervisor memblocks Quentin Perret
2021-03-10 17:57 ` [PATCH v4 27/34] KVM: arm64: Always zero invalid PTEs Quentin Perret
2021-03-11 17:33 ` Will Deacon
2021-03-12 9:15 ` Quentin Perret
2021-03-10 17:57 ` [PATCH v4 28/34] KVM: arm64: Use page-table to track page ownership Quentin Perret
2021-03-11 18:38 ` Will Deacon
2021-03-12 6:23 ` Quentin Perret
2021-03-12 9:32 ` Will Deacon
2021-03-12 10:13 ` Quentin Perret
2021-03-12 11:18 ` Will Deacon
2021-03-12 11:45 ` Quentin Perret
2021-03-10 17:57 ` [PATCH v4 29/34] KVM: arm64: Refactor stage2_map_set_prot_attr() Quentin Perret
2021-03-11 18:48 ` Will Deacon
2021-03-12 5:10 ` Quentin Perret
2021-03-10 17:57 ` [PATCH v4 30/34] KVM: arm64: Add kvm_pgtable_stage2_find_range() Quentin Perret
2021-03-11 19:04 ` Will Deacon
2021-03-12 5:32 ` Quentin Perret [this message]
2021-03-12 9:40 ` Will Deacon
2021-03-10 17:57 ` [PATCH v4 31/34] KVM: arm64: Wrap the host with a stage 2 Quentin Perret
2021-03-11 19:09 ` Will Deacon
2021-03-10 17:57 ` [PATCH v4 32/34] KVM: arm64: Page-align the .hyp sections Quentin Perret
2021-03-10 17:57 ` [PATCH v4 33/34] KVM: arm64: Disable PMU support in protected mode Quentin Perret
2021-03-10 17:57 ` [PATCH v4 34/34] KVM: arm64: Protect the .hyp sections from the host Quentin Perret
2021-03-11 19:17 ` Will Deacon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YEr83QKZnEd4a6Ba@google.com \
--to=qperret@google.com \
--cc=android-kvm@google.com \
--cc=catalin.marinas@arm.com \
--cc=kernel-team@android.com \
--cc=kvmarm@lists.cs.columbia.edu \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mate.toth-pal@arm.com \
--cc=maz@kernel.org \
--cc=robh+dt@kernel.org \
--cc=seanjc@google.com \
--cc=tabba@google.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).