All of lore.kernel.org
 help / color / mirror / Atom feed
From: Fuad Tabba <tabba@google.com>
To: Quentin Perret <qperret@google.com>
Cc: maz@kernel.org, james.morse@arm.com, alexandru.elisei@arm.com,
	suzuki.poulose@arm.com, catalin.marinas@arm.com, will@kernel.org,
	linux-arm-kernel@lists.infradead.org,
	kvmarm@lists.cs.columbia.edu, linux-kernel@vger.kernel.org,
	ardb@kernel.org, qwandor@google.com, dbrazdil@google.com,
	kernel-team@android.com
Subject: Re: [PATCH v3 06/21] KVM: arm64: Optimize host memory aborts
Date: Mon, 2 Aug 2021 11:37:01 +0200	[thread overview]
Message-ID: <CA+EHjTz20cf78XtAEgTZ4sqaL=N3fY0hd=_=rZge53nUv=vsjQ@mail.gmail.com> (raw)
In-Reply-To: <20210729132818.4091769-7-qperret@google.com>

Hi Quentin,

On Thu, Jul 29, 2021 at 3:28 PM Quentin Perret <qperret@google.com> 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 <qperret@google.com>

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 <tabba@google.com>

Thanks,
/fuad


> ---
>  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(-)
>
> diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
> index 6938eac72c1f..83c5c97d9eac 100644
> --- a/arch/arm64/include/asm/kvm_pgtable.h
> +++ b/arch/arm64/include/asm/kvm_pgtable.h
> @@ -154,16 +154,6 @@ 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
> @@ -490,24 +480,4 @@ int kvm_pgtable_walk(struct kvm_pgtable *pgt, u64 addr, u64 size,
>   */
>  int kvm_pgtable_get_leaf(struct kvm_pgtable *pgt, u64 addr,
>                          kvm_pte_t *ptep, u32 *level);
> -
> -/**
> - * 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 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.
> - *
> - * 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/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> index 74280a753efb..2148d3968aa5 100644
> --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> @@ -159,6 +159,11 @@ static int host_stage2_unmap_dev_all(void)
>         return kvm_pgtable_stage2_unmap(pgt, addr, BIT(pgt->ia_bits) - addr);
>  }
>
> +struct kvm_mem_range {
> +       u64 start;
> +       u64 end;
> +};
> +
>  static bool find_mem_range(phys_addr_t addr, struct kvm_mem_range *range)
>  {
>         int cur, left = 0, right = hyp_memblock_nr;
> @@ -227,6 +232,44 @@ static inline int __host_stage2_idmap(u64 start, u64 end,
>                 __ret;                                                  \
>          })
>
> +static inline bool range_included(struct kvm_mem_range *child,
> +                                 struct kvm_mem_range *parent)
> +{
> +       return parent->start <= child->start && child->end <= parent->end;
> +}
> +
> +static int host_stage2_adjust_range(u64 addr, struct kvm_mem_range *range)
> +{
> +       struct kvm_mem_range cur;
> +       kvm_pte_t pte;
> +       u32 level;
> +       int ret;
> +
> +       hyp_assert_lock_held(&host_kvm.lock);
> +       ret = kvm_pgtable_get_leaf(&host_kvm.pgt, addr, &pte, &level);
> +       if (ret)
> +               return ret;
> +
> +       if (kvm_pte_valid(pte))
> +               return -EAGAIN;
> +
> +       if (pte)
> +               return -EPERM;
> +
> +       do {
> +               u64 granule = kvm_granule_size(level);
> +               cur.start = ALIGN_DOWN(addr, granule);
> +               cur.end = cur.start + granule;
> +               level++;
> +       } while ((level < KVM_PGTABLE_MAX_LEVELS) &&
> +                       !(kvm_level_supports_block_mapping(level) &&
> +                         range_included(&cur, range)));
> +
> +       *range = cur;
> +
> +       return 0;
> +}
> +
>  static int host_stage2_idmap(u64 addr)
>  {
>         enum kvm_pgtable_prot prot = KVM_PGTABLE_PROT_R | KVM_PGTABLE_PROT_W;
> @@ -238,7 +281,7 @@ static int host_stage2_idmap(u64 addr)
>                 prot |= KVM_PGTABLE_PROT_X;
>
>         hyp_spin_lock(&host_kvm.lock);
> -       ret = kvm_pgtable_stage2_find_range(&host_kvm.pgt, addr, prot, &range);
> +       ret = host_stage2_adjust_range(addr, &range);
>         if (ret)
>                 goto unlock;
>
> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> index 49d768b92997..4dff2ad39ee4 100644
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c
> @@ -1102,77 +1102,3 @@ 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_IGNORED)
> -
> -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(pgt, 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_supports_block_mapping(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 (!ret) {
> -               range->start = start;
> -               range->end = end;
> -       }
> -
> -       return ret;
> -}
> --
> 2.32.0.432.gabb21c7263-goog
>

WARNING: multiple messages have this Message-ID (diff)
From: Fuad Tabba <tabba@google.com>
To: Quentin Perret <qperret@google.com>
Cc: kernel-team@android.com, qwandor@google.com, maz@kernel.org,
	linux-kernel@vger.kernel.org, catalin.marinas@arm.com,
	will@kernel.org, kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3 06/21] KVM: arm64: Optimize host memory aborts
Date: Mon, 2 Aug 2021 11:37:01 +0200	[thread overview]
Message-ID: <CA+EHjTz20cf78XtAEgTZ4sqaL=N3fY0hd=_=rZge53nUv=vsjQ@mail.gmail.com> (raw)
In-Reply-To: <20210729132818.4091769-7-qperret@google.com>

Hi Quentin,

On Thu, Jul 29, 2021 at 3:28 PM Quentin Perret <qperret@google.com> 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 <qperret@google.com>

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 <tabba@google.com>

Thanks,
/fuad


> ---
>  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(-)
>
> diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
> index 6938eac72c1f..83c5c97d9eac 100644
> --- a/arch/arm64/include/asm/kvm_pgtable.h
> +++ b/arch/arm64/include/asm/kvm_pgtable.h
> @@ -154,16 +154,6 @@ 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
> @@ -490,24 +480,4 @@ int kvm_pgtable_walk(struct kvm_pgtable *pgt, u64 addr, u64 size,
>   */
>  int kvm_pgtable_get_leaf(struct kvm_pgtable *pgt, u64 addr,
>                          kvm_pte_t *ptep, u32 *level);
> -
> -/**
> - * 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 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.
> - *
> - * 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/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> index 74280a753efb..2148d3968aa5 100644
> --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> @@ -159,6 +159,11 @@ static int host_stage2_unmap_dev_all(void)
>         return kvm_pgtable_stage2_unmap(pgt, addr, BIT(pgt->ia_bits) - addr);
>  }
>
> +struct kvm_mem_range {
> +       u64 start;
> +       u64 end;
> +};
> +
>  static bool find_mem_range(phys_addr_t addr, struct kvm_mem_range *range)
>  {
>         int cur, left = 0, right = hyp_memblock_nr;
> @@ -227,6 +232,44 @@ static inline int __host_stage2_idmap(u64 start, u64 end,
>                 __ret;                                                  \
>          })
>
> +static inline bool range_included(struct kvm_mem_range *child,
> +                                 struct kvm_mem_range *parent)
> +{
> +       return parent->start <= child->start && child->end <= parent->end;
> +}
> +
> +static int host_stage2_adjust_range(u64 addr, struct kvm_mem_range *range)
> +{
> +       struct kvm_mem_range cur;
> +       kvm_pte_t pte;
> +       u32 level;
> +       int ret;
> +
> +       hyp_assert_lock_held(&host_kvm.lock);
> +       ret = kvm_pgtable_get_leaf(&host_kvm.pgt, addr, &pte, &level);
> +       if (ret)
> +               return ret;
> +
> +       if (kvm_pte_valid(pte))
> +               return -EAGAIN;
> +
> +       if (pte)
> +               return -EPERM;
> +
> +       do {
> +               u64 granule = kvm_granule_size(level);
> +               cur.start = ALIGN_DOWN(addr, granule);
> +               cur.end = cur.start + granule;
> +               level++;
> +       } while ((level < KVM_PGTABLE_MAX_LEVELS) &&
> +                       !(kvm_level_supports_block_mapping(level) &&
> +                         range_included(&cur, range)));
> +
> +       *range = cur;
> +
> +       return 0;
> +}
> +
>  static int host_stage2_idmap(u64 addr)
>  {
>         enum kvm_pgtable_prot prot = KVM_PGTABLE_PROT_R | KVM_PGTABLE_PROT_W;
> @@ -238,7 +281,7 @@ static int host_stage2_idmap(u64 addr)
>                 prot |= KVM_PGTABLE_PROT_X;
>
>         hyp_spin_lock(&host_kvm.lock);
> -       ret = kvm_pgtable_stage2_find_range(&host_kvm.pgt, addr, prot, &range);
> +       ret = host_stage2_adjust_range(addr, &range);
>         if (ret)
>                 goto unlock;
>
> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> index 49d768b92997..4dff2ad39ee4 100644
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c
> @@ -1102,77 +1102,3 @@ 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_IGNORED)
> -
> -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(pgt, 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_supports_block_mapping(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 (!ret) {
> -               range->start = start;
> -               range->end = end;
> -       }
> -
> -       return ret;
> -}
> --
> 2.32.0.432.gabb21c7263-goog
>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

WARNING: multiple messages have this Message-ID (diff)
From: Fuad Tabba <tabba@google.com>
To: Quentin Perret <qperret@google.com>
Cc: maz@kernel.org, james.morse@arm.com, alexandru.elisei@arm.com,
	 suzuki.poulose@arm.com, catalin.marinas@arm.com,
	will@kernel.org,  linux-arm-kernel@lists.infradead.org,
	kvmarm@lists.cs.columbia.edu,  linux-kernel@vger.kernel.org,
	ardb@kernel.org, qwandor@google.com,  dbrazdil@google.com,
	kernel-team@android.com
Subject: Re: [PATCH v3 06/21] KVM: arm64: Optimize host memory aborts
Date: Mon, 2 Aug 2021 11:37:01 +0200	[thread overview]
Message-ID: <CA+EHjTz20cf78XtAEgTZ4sqaL=N3fY0hd=_=rZge53nUv=vsjQ@mail.gmail.com> (raw)
In-Reply-To: <20210729132818.4091769-7-qperret@google.com>

Hi Quentin,

On Thu, Jul 29, 2021 at 3:28 PM Quentin Perret <qperret@google.com> 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 <qperret@google.com>

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 <tabba@google.com>

Thanks,
/fuad


> ---
>  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(-)
>
> diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h
> index 6938eac72c1f..83c5c97d9eac 100644
> --- a/arch/arm64/include/asm/kvm_pgtable.h
> +++ b/arch/arm64/include/asm/kvm_pgtable.h
> @@ -154,16 +154,6 @@ 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
> @@ -490,24 +480,4 @@ int kvm_pgtable_walk(struct kvm_pgtable *pgt, u64 addr, u64 size,
>   */
>  int kvm_pgtable_get_leaf(struct kvm_pgtable *pgt, u64 addr,
>                          kvm_pte_t *ptep, u32 *level);
> -
> -/**
> - * 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 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.
> - *
> - * 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/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> index 74280a753efb..2148d3968aa5 100644
> --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c
> @@ -159,6 +159,11 @@ static int host_stage2_unmap_dev_all(void)
>         return kvm_pgtable_stage2_unmap(pgt, addr, BIT(pgt->ia_bits) - addr);
>  }
>
> +struct kvm_mem_range {
> +       u64 start;
> +       u64 end;
> +};
> +
>  static bool find_mem_range(phys_addr_t addr, struct kvm_mem_range *range)
>  {
>         int cur, left = 0, right = hyp_memblock_nr;
> @@ -227,6 +232,44 @@ static inline int __host_stage2_idmap(u64 start, u64 end,
>                 __ret;                                                  \
>          })
>
> +static inline bool range_included(struct kvm_mem_range *child,
> +                                 struct kvm_mem_range *parent)
> +{
> +       return parent->start <= child->start && child->end <= parent->end;
> +}
> +
> +static int host_stage2_adjust_range(u64 addr, struct kvm_mem_range *range)
> +{
> +       struct kvm_mem_range cur;
> +       kvm_pte_t pte;
> +       u32 level;
> +       int ret;
> +
> +       hyp_assert_lock_held(&host_kvm.lock);
> +       ret = kvm_pgtable_get_leaf(&host_kvm.pgt, addr, &pte, &level);
> +       if (ret)
> +               return ret;
> +
> +       if (kvm_pte_valid(pte))
> +               return -EAGAIN;
> +
> +       if (pte)
> +               return -EPERM;
> +
> +       do {
> +               u64 granule = kvm_granule_size(level);
> +               cur.start = ALIGN_DOWN(addr, granule);
> +               cur.end = cur.start + granule;
> +               level++;
> +       } while ((level < KVM_PGTABLE_MAX_LEVELS) &&
> +                       !(kvm_level_supports_block_mapping(level) &&
> +                         range_included(&cur, range)));
> +
> +       *range = cur;
> +
> +       return 0;
> +}
> +
>  static int host_stage2_idmap(u64 addr)
>  {
>         enum kvm_pgtable_prot prot = KVM_PGTABLE_PROT_R | KVM_PGTABLE_PROT_W;
> @@ -238,7 +281,7 @@ static int host_stage2_idmap(u64 addr)
>                 prot |= KVM_PGTABLE_PROT_X;
>
>         hyp_spin_lock(&host_kvm.lock);
> -       ret = kvm_pgtable_stage2_find_range(&host_kvm.pgt, addr, prot, &range);
> +       ret = host_stage2_adjust_range(addr, &range);
>         if (ret)
>                 goto unlock;
>
> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> index 49d768b92997..4dff2ad39ee4 100644
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c
> @@ -1102,77 +1102,3 @@ 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_IGNORED)
> -
> -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(pgt, 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_supports_block_mapping(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 (!ret) {
> -               range->start = start;
> -               range->end = end;
> -       }
> -
> -       return ret;
> -}
> --
> 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

  reply	other threads:[~2021-08-02  9:37 UTC|newest]

Thread overview: 135+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-29 13:27 [PATCH v3 00/21] Track shared pages at EL2 in protected mode Quentin Perret
2021-07-29 13:27 ` Quentin Perret
2021-07-29 13:27 ` Quentin Perret
2021-07-29 13:27 ` [PATCH v3 01/21] KVM: arm64: Add hyp_spin_is_locked() for basic locking assertions at EL2 Quentin Perret
2021-07-29 13:27   ` Quentin Perret
2021-07-29 13:27   ` Quentin Perret
2021-07-29 13:27 ` [PATCH v3 02/21] KVM: arm64: Introduce hyp_assert_lock_held() Quentin Perret
2021-07-29 13:27   ` Quentin Perret
2021-07-29 13:27   ` Quentin Perret
2021-07-29 13:28 ` [PATCH v3 03/21] KVM: arm64: Provide the host_stage2_try() helper macro Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-08-02  9:36   ` Fuad Tabba
2021-08-02  9:36     ` Fuad Tabba
2021-08-02  9:36     ` Fuad Tabba
2021-07-29 13:28 ` [PATCH v3 04/21] KVM: arm64: Introduce helper to retrieve a PTE and its level Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-07-29 13:28 ` [PATCH v3 05/21] KVM: arm64: Expose page-table helpers Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-07-29 13:28 ` [PATCH v3 06/21] KVM: arm64: Optimize host memory aborts Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-08-02  9:37   ` Fuad Tabba [this message]
2021-08-02  9:37     ` Fuad Tabba
2021-08-02  9:37     ` Fuad Tabba
2021-07-29 13:28 ` [PATCH v3 07/21] KVM: arm64: Rename KVM_PTE_LEAF_ATTR_S2_IGNORED Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-08-02  9:37   ` Fuad Tabba
2021-08-02  9:37     ` Fuad Tabba
2021-08-02  9:37     ` Fuad Tabba
2021-07-29 13:28 ` [PATCH v3 08/21] KVM: arm64: Don't overwrite software bits with owner id Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-08-02  9:38   ` Fuad Tabba
2021-08-02  9:38     ` Fuad Tabba
2021-08-02  9:38     ` Fuad Tabba
2021-07-29 13:28 ` [PATCH v3 09/21] KVM: arm64: Tolerate re-creating hyp mappings to set software bits Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-08-02  9:50   ` Fuad Tabba
2021-08-02  9:50     ` Fuad Tabba
2021-08-02  9:50     ` Fuad Tabba
2021-07-29 13:28 ` [PATCH v3 10/21] KVM: arm64: Enable forcing page-level stage-2 mappings Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-08-02  9:49   ` Fuad Tabba
2021-08-02  9:49     ` Fuad Tabba
2021-08-02  9:49     ` Fuad Tabba
2021-08-03 10:13     ` Quentin Perret
2021-08-03 10:13       ` Quentin Perret
2021-08-03 10:13       ` Quentin Perret
2021-08-03 10:43       ` Fuad Tabba
2021-08-03 10:43         ` Fuad Tabba
2021-08-03 10:43         ` Fuad Tabba
2021-07-29 13:28 ` [PATCH v3 11/21] KVM: arm64: Allow populating software bits Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-07-29 13:28 ` [PATCH v3 12/21] KVM: arm64: Add helpers to tag shared pages in SW bits Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-08-02 10:30   ` Fuad Tabba
2021-08-02 10:30     ` Fuad Tabba
2021-08-02 10:30     ` Fuad Tabba
2021-07-29 13:28 ` [PATCH v3 13/21] KVM: arm64: Expose host stage-2 manipulation helpers Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-08-02 11:13   ` Fuad Tabba
2021-08-02 11:13     ` Fuad Tabba
2021-08-02 11:13     ` Fuad Tabba
2021-08-03 10:20     ` Quentin Perret
2021-08-03 10:20       ` Quentin Perret
2021-08-03 10:20       ` Quentin Perret
2021-07-29 13:28 ` [PATCH v3 14/21] KVM: arm64: Expose pkvm_hyp_id Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-07-29 13:28 ` [PATCH v3 15/21] KVM: arm64: Introduce addr_is_memory() Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-08-02 14:52   ` Fuad Tabba
2021-08-02 14:52     ` Fuad Tabba
2021-08-02 14:52     ` Fuad Tabba
2021-08-03 10:23     ` Quentin Perret
2021-08-03 10:23       ` Quentin Perret
2021-08-03 10:23       ` Quentin Perret
2021-07-29 13:28 ` [PATCH v3 16/21] KVM: arm64: Enable retrieving protections attributes of PTEs Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-08-02 14:52   ` Fuad Tabba
2021-08-02 14:52     ` Fuad Tabba
2021-08-02 14:52     ` Fuad Tabba
2021-08-03 10:24     ` Quentin Perret
2021-08-03 10:24       ` Quentin Perret
2021-08-03 10:24       ` Quentin Perret
2021-07-29 13:28 ` [PATCH v3 17/21] KVM: arm64: Mark host bss and rodata section as shared Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-08-03  5:02   ` Fuad Tabba
2021-08-03  5:02     ` Fuad Tabba
2021-08-03  5:02     ` Fuad Tabba
2021-08-03 10:34     ` Quentin Perret
2021-08-03 10:34       ` Quentin Perret
2021-08-03 10:34       ` Quentin Perret
2021-08-03 10:54       ` Fuad Tabba
2021-08-03 10:54         ` Fuad Tabba
2021-08-03 10:54         ` Fuad Tabba
2021-07-29 13:28 ` [PATCH v3 18/21] KVM: arm64: Remove __pkvm_mark_hyp Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-07-29 13:28 ` [PATCH v3 19/21] KVM: arm64: Refactor protected nVHE stage-1 locking Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-08-03  5:31   ` Fuad Tabba
2021-08-03  5:31     ` Fuad Tabba
2021-08-03  5:31     ` Fuad Tabba
2021-08-03 10:37     ` Quentin Perret
2021-08-03 10:37       ` Quentin Perret
2021-08-03 10:37       ` Quentin Perret
2021-08-03 10:51       ` Fuad Tabba
2021-08-03 10:51         ` Fuad Tabba
2021-08-03 10:51         ` Fuad Tabba
2021-07-29 13:28 ` [PATCH v3 20/21] KVM: arm64: Restrict EL2 stage-1 changes in protected mode Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-08-03  8:22   ` Fuad Tabba
2021-08-03  8:22     ` Fuad Tabba
2021-08-03  8:22     ` Fuad Tabba
2021-08-03 10:43     ` Quentin Perret
2021-08-03 10:43       ` Quentin Perret
2021-08-03 10:43       ` Quentin Perret
2021-07-29 13:28 ` [PATCH v3 21/21] KVM: arm64: Make __pkvm_create_mappings static Quentin Perret
2021-07-29 13:28   ` Quentin Perret
2021-07-29 13:28   ` Quentin Perret

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='CA+EHjTz20cf78XtAEgTZ4sqaL=N3fY0hd=_=rZge53nUv=vsjQ@mail.gmail.com' \
    --to=tabba@google.com \
    --cc=alexandru.elisei@arm.com \
    --cc=ardb@kernel.org \
    --cc=catalin.marinas@arm.com \
    --cc=dbrazdil@google.com \
    --cc=james.morse@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=maz@kernel.org \
    --cc=qperret@google.com \
    --cc=qwandor@google.com \
    --cc=suzuki.poulose@arm.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.