| Message ID | 20210719104735.3681732-7-qperret@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Track shared pages at EL2 in protected mode | expand |
Hi Quentin, On Mon, Jul 19, 2021 at 11:47 AM Quentin Perret <qperret@google.com> wrote: > > The current hypervisor stage-1 mapping code doesn't allow changing an > existing valid mapping. Relax this condition by allowing changes that > only target ignored bits, as that will soon be needed to annotate shared > pages. > > Signed-off-by: Quentin Perret <qperret@google.com> > --- > arch/arm64/kvm/hyp/pgtable.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c > index a0ac8c2bc174..34cf67997a82 100644 > --- a/arch/arm64/kvm/hyp/pgtable.c > +++ b/arch/arm64/kvm/hyp/pgtable.c > @@ -362,6 +362,17 @@ static int hyp_set_prot_attr(enum kvm_pgtable_prot prot, kvm_pte_t *ptep) > return 0; > } > > +static bool hyp_pte_needs_update(kvm_pte_t old, kvm_pte_t new) > +{ > + if (old == new) > + return false; > + > + if (!kvm_pte_valid(old)) > + return true; > + > + return !WARN_ON((old ^ new) & ~KVM_PTE_LEAF_ATTR_IGNORED); Wouldn't this return false if both ignored and non-ignored bits were different, or is that not possible (judging by the WARN_ON)? If it is, then it would need an update, wouldn't it? Thanks, /fuad > +} > + > static bool hyp_map_walker_try_leaf(u64 addr, u64 end, u32 level, > kvm_pte_t *ptep, struct hyp_map_data *data) > { > @@ -371,9 +382,12 @@ static bool hyp_map_walker_try_leaf(u64 addr, u64 end, u32 level, > if (!kvm_block_mapping_supported(addr, end, phys, level)) > return false; > > - /* Tolerate KVM recreating the exact same mapping */ > + /* > + * Tolerate KVM recreating the exact same mapping, or changing ignored > + * bits. > + */ > new = kvm_init_valid_leaf_pte(phys, data->attr, level); > - if (old != new && !WARN_ON(kvm_pte_valid(old))) > + if (hyp_pte_needs_update(old, new)) > smp_store_release(ptep, new); > > data->phys += granule; > -- > 2.32.0.402.g57bb445576-goog >
Hi Fuad, On Tuesday 20 Jul 2021 at 11:17:03 (+0100), Fuad Tabba wrote: > Hi Quentin, > > > On Mon, Jul 19, 2021 at 11:47 AM Quentin Perret <qperret@google.com> wrote: > > > > The current hypervisor stage-1 mapping code doesn't allow changing an > > existing valid mapping. Relax this condition by allowing changes that > > only target ignored bits, as that will soon be needed to annotate shared > > pages. > > > > Signed-off-by: Quentin Perret <qperret@google.com> > > --- > > arch/arm64/kvm/hyp/pgtable.c | 18 ++++++++++++++++-- > > 1 file changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c > > index a0ac8c2bc174..34cf67997a82 100644 > > --- a/arch/arm64/kvm/hyp/pgtable.c > > +++ b/arch/arm64/kvm/hyp/pgtable.c > > @@ -362,6 +362,17 @@ static int hyp_set_prot_attr(enum kvm_pgtable_prot prot, kvm_pte_t *ptep) > > return 0; > > } > > > > +static bool hyp_pte_needs_update(kvm_pte_t old, kvm_pte_t new) > > +{ > > + if (old == new) > > + return false; > > + > > + if (!kvm_pte_valid(old)) > > + return true; > > + > > + return !WARN_ON((old ^ new) & ~KVM_PTE_LEAF_ATTR_IGNORED); > > Wouldn't this return false if both ignored and non-ignored bits were > different, or is that not possible (judging by the WARN_ON)? Correct, but that is intentional, see below ;) > If it is, then it would need an update, wouldn't it? Maybe, but if you look at what the existing code does, we do skip the update if the old mapping is valid and not equal to new. So I kept the behaviour as close as possible to this -- if you change any bits outside of SW bits you get a WARN and we skip the update, as we already do today. But if you touch only SW bits and nothing else, then I let the update go through. That said, I don't think warning and then proceeding to update would be terribly wrong, it's just that a change of behaviour felt a bit unnecessary for this particular patch. Thanks, Quentin
Hi Quentin, On Tue, Jul 20, 2021 at 11:30 AM 'Quentin Perret' via kernel-team <kernel-team@android.com> wrote: > > Hi Fuad, > > On Tuesday 20 Jul 2021 at 11:17:03 (+0100), Fuad Tabba wrote: > > Hi Quentin, > > > > > > On Mon, Jul 19, 2021 at 11:47 AM Quentin Perret <qperret@google.com> wrote: > > > > > > The current hypervisor stage-1 mapping code doesn't allow changing an > > > existing valid mapping. Relax this condition by allowing changes that > > > only target ignored bits, as that will soon be needed to annotate shared > > > pages. > > > > > > Signed-off-by: Quentin Perret <qperret@google.com> > > > --- > > > arch/arm64/kvm/hyp/pgtable.c | 18 ++++++++++++++++-- > > > 1 file changed, 16 insertions(+), 2 deletions(-) > > > > > > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c > > > index a0ac8c2bc174..34cf67997a82 100644 > > > --- a/arch/arm64/kvm/hyp/pgtable.c > > > +++ b/arch/arm64/kvm/hyp/pgtable.c > > > @@ -362,6 +362,17 @@ static int hyp_set_prot_attr(enum kvm_pgtable_prot prot, kvm_pte_t *ptep) > > > return 0; > > > } > > > > > > +static bool hyp_pte_needs_update(kvm_pte_t old, kvm_pte_t new) > > > +{ > > > + if (old == new) > > > + return false; > > > + > > > + if (!kvm_pte_valid(old)) > > > + return true; > > > + > > > + return !WARN_ON((old ^ new) & ~KVM_PTE_LEAF_ATTR_IGNORED); > > > > Wouldn't this return false if both ignored and non-ignored bits were > > different, or is that not possible (judging by the WARN_ON)? > > Correct, but that is intentional, see below ;) > > > If it is, then it would need an update, wouldn't it? > > Maybe, but if you look at what the existing code does, we do skip the > update if the old mapping is valid and not equal to new. So I kept the > behaviour as close as possible to this -- if you change any bits outside > of SW bits you get a WARN and we skip the update, as we already do > today. But if you touch only SW bits and nothing else, then I let the > update go through. > > That said, I don't think warning and then proceeding to update would be > terribly wrong, it's just that a change of behaviour felt a bit > unnecessary for this particular patch. Thanks for the clarification. It makes sense to preserve the existing behavior, but I was wondering if a comment would be good, describing what merits a "needs update"? Cheers, /fuad > Thanks, > Quentin > > -- > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com. >
On Tuesday 20 Jul 2021 at 11:59:27 (+0100), Fuad Tabba wrote: > Thanks for the clarification. It makes sense to preserve the existing > behavior, but I was wondering if a comment would be good, describing > what merits a "needs update"? Sure thing, I'll add something for v2. Cheers, Quentin
diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c index a0ac8c2bc174..34cf67997a82 100644 --- a/arch/arm64/kvm/hyp/pgtable.c +++ b/arch/arm64/kvm/hyp/pgtable.c @@ -362,6 +362,17 @@ static int hyp_set_prot_attr(enum kvm_pgtable_prot prot, kvm_pte_t *ptep) return 0; } +static bool hyp_pte_needs_update(kvm_pte_t old, kvm_pte_t new) +{ + if (old == new) + return false; + + if (!kvm_pte_valid(old)) + return true; + + return !WARN_ON((old ^ new) & ~KVM_PTE_LEAF_ATTR_IGNORED); +} + static bool hyp_map_walker_try_leaf(u64 addr, u64 end, u32 level, kvm_pte_t *ptep, struct hyp_map_data *data) { @@ -371,9 +382,12 @@ static bool hyp_map_walker_try_leaf(u64 addr, u64 end, u32 level, if (!kvm_block_mapping_supported(addr, end, phys, level)) return false; - /* Tolerate KVM recreating the exact same mapping */ + /* + * Tolerate KVM recreating the exact same mapping, or changing ignored + * bits. + */ new = kvm_init_valid_leaf_pte(phys, data->attr, level); - if (old != new && !WARN_ON(kvm_pte_valid(old))) + if (hyp_pte_needs_update(old, new)) smp_store_release(ptep, new); data->phys += granule;
The current hypervisor stage-1 mapping code doesn't allow changing an existing valid mapping. Relax this condition by allowing changes that only target ignored bits, as that will soon be needed to annotate shared pages. Signed-off-by: Quentin Perret <qperret@google.com> --- arch/arm64/kvm/hyp/pgtable.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)