| Message ID | 20211201170411.1561936-4-qperret@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Introduce kvm_share_hyp() | expand |
On Wed, Dec 01, 2021 at 05:03:57PM +0000, Quentin Perret wrote: > To prepare the ground for allowing hyp stage-1 mappings to be removed at > run-time, update the KVM page-table code to maintain a correct refcount > using the ->{get,put}_page() function callbacks. > > Signed-off-by: Quentin Perret <qperret@google.com> > --- > arch/arm64/kvm/hyp/pgtable.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c > index f8ceebe4982e..768a58835153 100644 > --- a/arch/arm64/kvm/hyp/pgtable.c > +++ b/arch/arm64/kvm/hyp/pgtable.c > @@ -408,8 +408,10 @@ static bool hyp_map_walker_try_leaf(u64 addr, u64 end, u32 level, > return false; > > new = kvm_init_valid_leaf_pte(phys, data->attr, level); > - if (hyp_pte_needs_update(old, new)) > + if (hyp_pte_needs_update(old, new)) { > smp_store_release(ptep, new); > + data->mm_ops->get_page(ptep); In the case where we're just updating software bits for a valid pte, doesn't this result in us taking a spurious reference to the page? > @@ -482,8 +485,16 @@ static int hyp_free_walker(u64 addr, u64 end, u32 level, kvm_pte_t *ptep, > enum kvm_pgtable_walk_flags flag, void * const arg) > { > struct kvm_pgtable_mm_ops *mm_ops = arg; > + kvm_pte_t pte = *ptep; > + > + if (!kvm_pte_valid(pte)) > + return 0; > + > + mm_ops->put_page(ptep); > + > + if (kvm_pte_table(pte, level)) > + mm_ops->put_page(kvm_pte_follow(pte, mm_ops)); > > - mm_ops->put_page((void *)kvm_pte_follow(*ptep, mm_ops)); > return 0; This looks pretty similar to the stage-2 walker now, but given how small the functions are, I'm not sure we'd really gain much by abstracting the "pte counted" check. Will
On Thursday 09 Dec 2021 at 10:29:24 (+0000), Will Deacon wrote: > On Wed, Dec 01, 2021 at 05:03:57PM +0000, Quentin Perret wrote: > > To prepare the ground for allowing hyp stage-1 mappings to be removed at > > run-time, update the KVM page-table code to maintain a correct refcount > > using the ->{get,put}_page() function callbacks. > > > > Signed-off-by: Quentin Perret <qperret@google.com> > > --- > > arch/arm64/kvm/hyp/pgtable.c | 17 ++++++++++++++--- > > 1 file changed, 14 insertions(+), 3 deletions(-) > > > > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c > > index f8ceebe4982e..768a58835153 100644 > > --- a/arch/arm64/kvm/hyp/pgtable.c > > +++ b/arch/arm64/kvm/hyp/pgtable.c > > @@ -408,8 +408,10 @@ static bool hyp_map_walker_try_leaf(u64 addr, u64 end, u32 level, > > return false; > > > > new = kvm_init_valid_leaf_pte(phys, data->attr, level); > > - if (hyp_pte_needs_update(old, new)) > > + if (hyp_pte_needs_update(old, new)) { > > smp_store_release(ptep, new); > > + data->mm_ops->get_page(ptep); > > In the case where we're just updating software bits for a valid pte, doesn't > this result in us taking a spurious reference to the page? Ahem, yes, that is the case. I ended up with the below diff to fix it, which I intend to fold in the next version: diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c index 6ad4cb2d6947..e2047d3f05a2 100644 --- a/arch/arm64/kvm/hyp/pgtable.c +++ b/arch/arm64/kvm/hyp/pgtable.c @@ -383,21 +383,6 @@ enum kvm_pgtable_prot kvm_pgtable_hyp_pte_prot(kvm_pte_t pte) return prot; } -static bool hyp_pte_needs_update(kvm_pte_t old, kvm_pte_t new) -{ - /* - * Tolerate KVM recreating the exact same mapping, or changing software - * bits if the existing mapping was valid. - */ - if (old == new) - return false; - - if (!kvm_pte_valid(old)) - return true; - - return !WARN_ON((old ^ new) & ~KVM_PTE_LEAF_ATTR_HI_SW); -} - static bool hyp_map_walker_try_leaf(u64 addr, u64 end, u32 level, kvm_pte_t *ptep, struct hyp_map_data *data) { @@ -407,13 +392,16 @@ static bool hyp_map_walker_try_leaf(u64 addr, u64 end, u32 level, if (!kvm_block_mapping_supported(addr, end, phys, level)) return false; + data->phys += granule; new = kvm_init_valid_leaf_pte(phys, data->attr, level); - if (hyp_pte_needs_update(old, new)) { - smp_store_release(ptep, new); + if (old == new) + return true; + else if (!kvm_pte_valid(old)) data->mm_ops->get_page(ptep); - } + else if (WARN_ON((old ^ new) & ~KVM_PTE_LEAF_ATTR_HI_SW)) + return false; - data->phys += granule; + smp_store_release(ptep, new); return true; }
On Fri, Dec 10, 2021 at 02:34:16PM +0000, Quentin Perret wrote: > On Thursday 09 Dec 2021 at 10:29:24 (+0000), Will Deacon wrote: > > On Wed, Dec 01, 2021 at 05:03:57PM +0000, Quentin Perret wrote: > > > To prepare the ground for allowing hyp stage-1 mappings to be removed at > > > run-time, update the KVM page-table code to maintain a correct refcount > > > using the ->{get,put}_page() function callbacks. > > > > > > Signed-off-by: Quentin Perret <qperret@google.com> > > > --- > > > arch/arm64/kvm/hyp/pgtable.c | 17 ++++++++++++++--- > > > 1 file changed, 14 insertions(+), 3 deletions(-) > > > > > > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c > > > index f8ceebe4982e..768a58835153 100644 > > > --- a/arch/arm64/kvm/hyp/pgtable.c > > > +++ b/arch/arm64/kvm/hyp/pgtable.c > > > @@ -408,8 +408,10 @@ static bool hyp_map_walker_try_leaf(u64 addr, u64 end, u32 level, > > > return false; > > > > > > new = kvm_init_valid_leaf_pte(phys, data->attr, level); > > > - if (hyp_pte_needs_update(old, new)) > > > + if (hyp_pte_needs_update(old, new)) { > > > smp_store_release(ptep, new); > > > + data->mm_ops->get_page(ptep); > > > > In the case where we're just updating software bits for a valid pte, doesn't > > this result in us taking a spurious reference to the page? > > Ahem, yes, that is the case. I ended up with the below diff to fix it, > which I intend to fold in the next version: > > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c > index 6ad4cb2d6947..e2047d3f05a2 100644 > --- a/arch/arm64/kvm/hyp/pgtable.c > +++ b/arch/arm64/kvm/hyp/pgtable.c > @@ -383,21 +383,6 @@ enum kvm_pgtable_prot kvm_pgtable_hyp_pte_prot(kvm_pte_t pte) > return prot; > } > > -static bool hyp_pte_needs_update(kvm_pte_t old, kvm_pte_t new) > -{ > - /* > - * Tolerate KVM recreating the exact same mapping, or changing software > - * bits if the existing mapping was valid. > - */ > - if (old == new) > - return false; > - > - if (!kvm_pte_valid(old)) > - return true; > - > - return !WARN_ON((old ^ new) & ~KVM_PTE_LEAF_ATTR_HI_SW); > -} > - > static bool hyp_map_walker_try_leaf(u64 addr, u64 end, u32 level, > kvm_pte_t *ptep, struct hyp_map_data *data) > { > @@ -407,13 +392,16 @@ static bool hyp_map_walker_try_leaf(u64 addr, u64 end, u32 level, > if (!kvm_block_mapping_supported(addr, end, phys, level)) > return false; > > + data->phys += granule; > new = kvm_init_valid_leaf_pte(phys, data->attr, level); > - if (hyp_pte_needs_update(old, new)) { > - smp_store_release(ptep, new); > + if (old == new) > + return true; > + else if (!kvm_pte_valid(old)) (nit: clearer to drop the 'else' part here) > data->mm_ops->get_page(ptep); Ok, so this works because new is always valid. LGTM. Will
diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c index f8ceebe4982e..768a58835153 100644 --- a/arch/arm64/kvm/hyp/pgtable.c +++ b/arch/arm64/kvm/hyp/pgtable.c @@ -408,8 +408,10 @@ static bool hyp_map_walker_try_leaf(u64 addr, u64 end, u32 level, return false; new = kvm_init_valid_leaf_pte(phys, data->attr, level); - if (hyp_pte_needs_update(old, new)) + if (hyp_pte_needs_update(old, new)) { smp_store_release(ptep, new); + data->mm_ops->get_page(ptep); + } data->phys += granule; return true; @@ -433,6 +435,7 @@ static int hyp_map_walker(u64 addr, u64 end, u32 level, kvm_pte_t *ptep, return -ENOMEM; kvm_set_table_pte(ptep, childp, mm_ops); + mm_ops->get_page(ptep); return 0; } @@ -482,8 +485,16 @@ static int hyp_free_walker(u64 addr, u64 end, u32 level, kvm_pte_t *ptep, enum kvm_pgtable_walk_flags flag, void * const arg) { struct kvm_pgtable_mm_ops *mm_ops = arg; + kvm_pte_t pte = *ptep; + + if (!kvm_pte_valid(pte)) + return 0; + + mm_ops->put_page(ptep); + + if (kvm_pte_table(pte, level)) + mm_ops->put_page(kvm_pte_follow(pte, mm_ops)); - mm_ops->put_page((void *)kvm_pte_follow(*ptep, mm_ops)); return 0; } @@ -491,7 +502,7 @@ void kvm_pgtable_hyp_destroy(struct kvm_pgtable *pgt) { struct kvm_pgtable_walker walker = { .cb = hyp_free_walker, - .flags = KVM_PGTABLE_WALK_TABLE_POST, + .flags = KVM_PGTABLE_WALK_LEAF | KVM_PGTABLE_WALK_TABLE_POST, .arg = pgt->mm_ops, };
To prepare the ground for allowing hyp stage-1 mappings to be removed at run-time, update the KVM page-table code to maintain a correct refcount using the ->{get,put}_page() function callbacks. Signed-off-by: Quentin Perret <qperret@google.com> --- arch/arm64/kvm/hyp/pgtable.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)