| Message ID | 20230113035000.480021-2-ricarkol@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Eager Huge-page splitting for dirty-logging | expand |
On Thu, Jan 12, 2023 at 7:50 PM Ricardo Koller <ricarkol@google.com> wrote: > > Add a flag to kvm_pgtable_visit_ctx, KVM_PGTABLE_WALK_REMOVED, to > indicate that the walk is on a removed table not accesible to the HW > page-table walker. Then use it to avoid doing break-before-make or > performing CMOs (Cache Maintenance Operations) when mapping a removed Nit: Should this say unmapping? Or are we actually going to use this to map memory ? > table. This is safe as these removed tables are not visible to the HW > page-table walker. This will be used in a subsequent commit for ...
On Mon, Jan 23, 2023 at 04:51:16PM -0800, Ben Gardon wrote: > On Thu, Jan 12, 2023 at 7:50 PM Ricardo Koller <ricarkol@google.com> wrote: > > > > Add a flag to kvm_pgtable_visit_ctx, KVM_PGTABLE_WALK_REMOVED, to > > indicate that the walk is on a removed table not accesible to the HW > > page-table walker. Then use it to avoid doing break-before-make or > > performing CMOs (Cache Maintenance Operations) when mapping a removed > > Nit: Should this say unmapping? Or are we actually going to use this > to map memory ? I think the *_REMOVED term feels weird as it relates to constructing a page table. It'd be better if we instead added flags to describe the operations we intend to elide (i.e. CMOs and TLBIs). That way the implementation is generic enough that we can repurpose it for other use cases. -- Thanks, Oliver
On Mon, Jan 23, 2023 at 04:51:16PM -0800, Ben Gardon wrote: > On Thu, Jan 12, 2023 at 7:50 PM Ricardo Koller <ricarkol@google.com> wrote: > > > > Add a flag to kvm_pgtable_visit_ctx, KVM_PGTABLE_WALK_REMOVED, to > > indicate that the walk is on a removed table not accesible to the HW > > page-table walker. Then use it to avoid doing break-before-make or > > performing CMOs (Cache Maintenance Operations) when mapping a removed > > Nit: Should this say unmapping? Or are we actually going to use this > to map memory ? As you mentioned in the next commit, this will be clearer if I use "unliked" instead of "removed". Might end up rewriting this message as well, as I will be using Oliver's suggestion of using multiple flags, one for each operation to elide. > > > table. This is safe as these removed tables are not visible to the HW > > page-table walker. This will be used in a subsequent commit for > ...
On Tue, Jan 24, 2023 at 12:56:11AM +0000, Oliver Upton wrote: > On Mon, Jan 23, 2023 at 04:51:16PM -0800, Ben Gardon wrote: > > On Thu, Jan 12, 2023 at 7:50 PM Ricardo Koller <ricarkol@google.com> wrote: > > > > > > Add a flag to kvm_pgtable_visit_ctx, KVM_PGTABLE_WALK_REMOVED, to > > > indicate that the walk is on a removed table not accesible to the HW > > > page-table walker. Then use it to avoid doing break-before-make or > > > performing CMOs (Cache Maintenance Operations) when mapping a removed > > > > Nit: Should this say unmapping? Or are we actually going to use this > > to map memory ? > > I think the *_REMOVED term feels weird as it relates to constructing a > page table. It'd be better if we instead added flags to describe the > operations we intend to elide (i.e. CMOs and TLBIs). What about KVM_PGTABLE_WALK_ELIDE_BBM and KVM_PGTABLE_WALK_ELIDE_CMO? > That way the > implementation is generic enough that we can repurpose it for other use > cases. Aha, good point. I actually have a use case for it (FEAT_BBM). > > -- > Thanks, > Oliver
On Tue, Jan 24, 2023 at 8:32 AM Ricardo Koller <ricarkol@google.com> wrote: > > On Tue, Jan 24, 2023 at 12:56:11AM +0000, Oliver Upton wrote: > > On Mon, Jan 23, 2023 at 04:51:16PM -0800, Ben Gardon wrote: > > > On Thu, Jan 12, 2023 at 7:50 PM Ricardo Koller <ricarkol@google.com> wrote: > > > > > > > > Add a flag to kvm_pgtable_visit_ctx, KVM_PGTABLE_WALK_REMOVED, to > > > > indicate that the walk is on a removed table not accesible to the HW > > > > page-table walker. Then use it to avoid doing break-before-make or > > > > performing CMOs (Cache Maintenance Operations) when mapping a removed > > > > > > Nit: Should this say unmapping? Or are we actually going to use this > > > to map memory ? > > > > I think the *_REMOVED term feels weird as it relates to constructing a > > page table. It'd be better if we instead added flags to describe the > > operations we intend to elide (i.e. CMOs and TLBIs). > > What about KVM_PGTABLE_WALK_ELIDE_BBM and KVM_PGTABLE_WALK_ELIDE_CMO? I like this, but please don't use elide in the code. I'm all for vocabulary, but that's not a common enough word to expect everyone to know. Perhaps just SKIP? > > > That way the > > implementation is generic enough that we can repurpose it for other use > > cases. > > Aha, good point. I actually have a use case for it (FEAT_BBM). > > > > > -- > > Thanks, > > Oliver
On Tue, Jan 24, 2023 at 10:00 AM Ben Gardon <bgardon@google.com> wrote: > > On Tue, Jan 24, 2023 at 8:32 AM Ricardo Koller <ricarkol@google.com> wrote: > > > > On Tue, Jan 24, 2023 at 12:56:11AM +0000, Oliver Upton wrote: > > > On Mon, Jan 23, 2023 at 04:51:16PM -0800, Ben Gardon wrote: > > > > On Thu, Jan 12, 2023 at 7:50 PM Ricardo Koller <ricarkol@google.com> wrote: > > > > > > > > > > Add a flag to kvm_pgtable_visit_ctx, KVM_PGTABLE_WALK_REMOVED, to > > > > > indicate that the walk is on a removed table not accesible to the HW > > > > > page-table walker. Then use it to avoid doing break-before-make or > > > > > performing CMOs (Cache Maintenance Operations) when mapping a removed > > > > > > > > Nit: Should this say unmapping? Or are we actually going to use this > > > > to map memory ? > > > > > > I think the *_REMOVED term feels weird as it relates to constructing a > > > page table. It'd be better if we instead added flags to describe the > > > operations we intend to elide (i.e. CMOs and TLBIs). > > > > What about KVM_PGTABLE_WALK_ELIDE_BBM and KVM_PGTABLE_WALK_ELIDE_CMO? > > I like this, but please don't use elide in the code. I'm all for > vocabulary, but that's not a common enough word to expect everyone to > know. Perhaps just SKIP? No problem, SKIP should be fine. > > > > > > That way the > > > implementation is generic enough that we can repurpose it for other use > > > cases. > > > > Aha, good point. I actually have a use case for it (FEAT_BBM). > > > > > > > > -- > > > Thanks, > > > Oliver
diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h index 63f81b27a4e3..84a271647007 100644 --- a/arch/arm64/include/asm/kvm_pgtable.h +++ b/arch/arm64/include/asm/kvm_pgtable.h @@ -188,12 +188,15 @@ typedef bool (*kvm_pgtable_force_pte_cb_t)(u64 addr, u64 end, * children. * @KVM_PGTABLE_WALK_SHARED: Indicates the page-tables may be shared * with other software walkers. + * @KVM_PGTABLE_WALK_REMOVED: Indicates the page-tables are + * removed: not visible to the HW walker. */ enum kvm_pgtable_walk_flags { KVM_PGTABLE_WALK_LEAF = BIT(0), KVM_PGTABLE_WALK_TABLE_PRE = BIT(1), KVM_PGTABLE_WALK_TABLE_POST = BIT(2), KVM_PGTABLE_WALK_SHARED = BIT(3), + KVM_PGTABLE_WALK_REMOVED = BIT(4), }; struct kvm_pgtable_visit_ctx { @@ -215,6 +218,11 @@ static inline bool kvm_pgtable_walk_shared(const struct kvm_pgtable_visit_ctx *c return ctx->flags & KVM_PGTABLE_WALK_SHARED; } +static inline bool kvm_pgtable_walk_removed(const struct kvm_pgtable_visit_ctx *ctx) +{ + return ctx->flags & KVM_PGTABLE_WALK_REMOVED; +} + /** * struct kvm_pgtable_walker - Hook into a page-table walk. * @cb: Callback function to invoke during the walk. diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c index b11cf2c618a6..87fd40d09056 100644 --- a/arch/arm64/kvm/hyp/pgtable.c +++ b/arch/arm64/kvm/hyp/pgtable.c @@ -717,14 +717,17 @@ static bool stage2_try_break_pte(const struct kvm_pgtable_visit_ctx *ctx, if (!stage2_try_set_pte(ctx, KVM_INVALID_PTE_LOCKED)) return false; - /* - * Perform the appropriate TLB invalidation based on the evicted pte - * value (if any). - */ - if (kvm_pte_table(ctx->old, ctx->level)) - kvm_call_hyp(__kvm_tlb_flush_vmid, mmu); - else if (kvm_pte_valid(ctx->old)) - kvm_call_hyp(__kvm_tlb_flush_vmid_ipa, mmu, ctx->addr, ctx->level); + if (!kvm_pgtable_walk_removed(ctx)) { + /* + * Perform the appropriate TLB invalidation based on the + * evicted pte value (if any). + */ + if (kvm_pte_table(ctx->old, ctx->level)) + kvm_call_hyp(__kvm_tlb_flush_vmid, mmu); + else if (kvm_pte_valid(ctx->old)) + kvm_call_hyp(__kvm_tlb_flush_vmid_ipa, mmu, + ctx->addr, ctx->level); + } if (stage2_pte_is_counted(ctx->old)) mm_ops->put_page(ctx->ptep); @@ -808,11 +811,13 @@ static int stage2_map_walker_try_leaf(const struct kvm_pgtable_visit_ctx *ctx, return -EAGAIN; /* Perform CMOs before installation of the guest stage-2 PTE */ - if (mm_ops->dcache_clean_inval_poc && stage2_pte_cacheable(pgt, new)) + if (!kvm_pgtable_walk_removed(ctx) && mm_ops->dcache_clean_inval_poc && + stage2_pte_cacheable(pgt, new)) mm_ops->dcache_clean_inval_poc(kvm_pte_follow(new, mm_ops), - granule); + granule); - if (mm_ops->icache_inval_pou && stage2_pte_executable(new)) + if (!kvm_pgtable_walk_removed(ctx) && mm_ops->icache_inval_pou && + stage2_pte_executable(new)) mm_ops->icache_inval_pou(kvm_pte_follow(new, mm_ops), granule); stage2_make_pte(ctx, new);
Add a flag to kvm_pgtable_visit_ctx, KVM_PGTABLE_WALK_REMOVED, to indicate that the walk is on a removed table not accesible to the HW page-table walker. Then use it to avoid doing break-before-make or performing CMOs (Cache Maintenance Operations) when mapping a removed table. This is safe as these removed tables are not visible to the HW page-table walker. This will be used in a subsequent commit for replacing huge-page block PTEs into tables of 4K PTEs. Signed-off-by: Ricardo Koller <ricarkol@google.com> --- arch/arm64/include/asm/kvm_pgtable.h | 8 ++++++++ arch/arm64/kvm/hyp/pgtable.c | 27 ++++++++++++++++----------- 2 files changed, 24 insertions(+), 11 deletions(-)