| Message ID | 20241111083544.1845845-12-ardb+git@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: Clean up and simplify PA space size handling | expand |
On Mon, Nov 11, 2024 at 09:35:48AM +0100, Ard Biesheuvel wrote: > From: Ard Biesheuvel <ardb@kernel.org> > > The pKVM stage2 mapping code relies on an invalid physical address to > signal to the internal API that only the owner_id fields of descriptors > should be updated, which are stored in the high bits of invalid > descriptors covering memory that has been donated to protected guests, > and is therefore unmapped from the host stage-2 page tables. > > Given that these invalid PAs are never stored into the descriptors, it > is better to rely on an explicit flag, to clarify the API and to avoid > confusion regarding whether or not the output address of a descriptor > can ever be invalid to begin with (which is not the case with LPA2). > > That removes a dependency on the logic that reasons about the maximum PA > range, which differs on LPA2 capable CPUs based on whether LPA2 is > enabled or not, and will be further clarified in subsequent patches. > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > arch/arm64/kvm/hyp/pgtable.c | 37 ++++++++------------ > 1 file changed, 14 insertions(+), 23 deletions(-) > > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c > index b11bcebac908..4bf618b2cba7 100644 > --- a/arch/arm64/kvm/hyp/pgtable.c > +++ b/arch/arm64/kvm/hyp/pgtable.c > @@ -35,14 +35,6 @@ static bool kvm_pgtable_walk_skip_cmo(const struct kvm_pgtable_visit_ctx *ctx) > return unlikely(ctx->flags & KVM_PGTABLE_WALK_SKIP_CMO); > } > > -static bool kvm_phys_is_valid(u64 phys) > -{ > - u64 parange_max = kvm_get_parange_max(); > - u8 shift = id_aa64mmfr0_parange_to_phys_shift(parange_max); > - > - return phys < BIT(shift); > -} > - > static bool kvm_block_mapping_supported(const struct kvm_pgtable_visit_ctx *ctx, u64 phys) > { > u64 granule = kvm_granule_size(ctx->level); > @@ -53,7 +45,7 @@ static bool kvm_block_mapping_supported(const struct kvm_pgtable_visit_ctx *ctx, > if (granule > (ctx->end - ctx->addr)) > return false; > > - if (kvm_phys_is_valid(phys) && !IS_ALIGNED(phys, granule)) > + if (!IS_ALIGNED(phys, granule)) > return false; > > return IS_ALIGNED(ctx->addr, granule); > @@ -587,6 +579,9 @@ struct stage2_map_data { > > /* Force mappings to page granularity */ > bool force_pte; > + > + /* Walk should update owner_id only */ > + bool owner_update; > }; > > u64 kvm_get_vtcr(u64 mmfr0, u64 mmfr1, u32 phys_shift) > @@ -885,18 +880,7 @@ static u64 stage2_map_walker_phys_addr(const struct kvm_pgtable_visit_ctx *ctx, > { > u64 phys = data->phys; > > - /* > - * Stage-2 walks to update ownership data are communicated to the map > - * walker using an invalid PA. Avoid offsetting an already invalid PA, > - * which could overflow and make the address valid again. > - */ > - if (!kvm_phys_is_valid(phys)) > - return phys; > - > - /* > - * Otherwise, work out the correct PA based on how far the walk has > - * gotten. > - */ > + /* Work out the correct PA based on how far the walk has gotten */ > return phys + (ctx->addr - ctx->start); > } > > @@ -908,6 +892,13 @@ static bool stage2_leaf_mapping_allowed(const struct kvm_pgtable_visit_ctx *ctx, > if (data->force_pte && ctx->level < KVM_PGTABLE_LAST_LEVEL) > return false; > > + /* > + * Pass a value that is aligned to any block size when updating > + * only the owner_id on invalid mappings. > + */ > + if (data->owner_update) > + phys = 0; > + Hmm, isn't this a change in behaviour? Previously we would always store the owner annotations at the leaf, but now this will take place at a higher-level. I think that probably goes horribly wrong if we later try to change the owner for a sub-range; the block will be replaced with a table and the old ownership information will be lost rather than propagated to the leaves. In other words, I think we should return false here. Will
On Mon, 11 Nov 2024 at 18:27, Will Deacon <will@kernel.org> wrote: > > On Mon, Nov 11, 2024 at 09:35:48AM +0100, Ard Biesheuvel wrote: > > From: Ard Biesheuvel <ardb@kernel.org> > > > > The pKVM stage2 mapping code relies on an invalid physical address to > > signal to the internal API that only the owner_id fields of descriptors > > should be updated, which are stored in the high bits of invalid > > descriptors covering memory that has been donated to protected guests, > > and is therefore unmapped from the host stage-2 page tables. > > > > Given that these invalid PAs are never stored into the descriptors, it > > is better to rely on an explicit flag, to clarify the API and to avoid > > confusion regarding whether or not the output address of a descriptor > > can ever be invalid to begin with (which is not the case with LPA2). > > > > That removes a dependency on the logic that reasons about the maximum PA > > range, which differs on LPA2 capable CPUs based on whether LPA2 is > > enabled or not, and will be further clarified in subsequent patches. > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > --- > > arch/arm64/kvm/hyp/pgtable.c | 37 ++++++++------------ > > 1 file changed, 14 insertions(+), 23 deletions(-) > > > > diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c > > index b11bcebac908..4bf618b2cba7 100644 > > --- a/arch/arm64/kvm/hyp/pgtable.c > > +++ b/arch/arm64/kvm/hyp/pgtable.c > > @@ -35,14 +35,6 @@ static bool kvm_pgtable_walk_skip_cmo(const struct kvm_pgtable_visit_ctx *ctx) > > return unlikely(ctx->flags & KVM_PGTABLE_WALK_SKIP_CMO); > > } > > > > -static bool kvm_phys_is_valid(u64 phys) > > -{ > > - u64 parange_max = kvm_get_parange_max(); > > - u8 shift = id_aa64mmfr0_parange_to_phys_shift(parange_max); > > - > > - return phys < BIT(shift); > > -} > > - > > static bool kvm_block_mapping_supported(const struct kvm_pgtable_visit_ctx *ctx, u64 phys) > > { > > u64 granule = kvm_granule_size(ctx->level); > > @@ -53,7 +45,7 @@ static bool kvm_block_mapping_supported(const struct kvm_pgtable_visit_ctx *ctx, > > if (granule > (ctx->end - ctx->addr)) > > return false; > > > > - if (kvm_phys_is_valid(phys) && !IS_ALIGNED(phys, granule)) > > + if (!IS_ALIGNED(phys, granule)) > > return false; > > > > return IS_ALIGNED(ctx->addr, granule); > > @@ -587,6 +579,9 @@ struct stage2_map_data { > > > > /* Force mappings to page granularity */ > > bool force_pte; > > + > > + /* Walk should update owner_id only */ > > + bool owner_update; > > }; > > > > u64 kvm_get_vtcr(u64 mmfr0, u64 mmfr1, u32 phys_shift) > > @@ -885,18 +880,7 @@ static u64 stage2_map_walker_phys_addr(const struct kvm_pgtable_visit_ctx *ctx, > > { > > u64 phys = data->phys; > > > > - /* > > - * Stage-2 walks to update ownership data are communicated to the map > > - * walker using an invalid PA. Avoid offsetting an already invalid PA, > > - * which could overflow and make the address valid again. > > - */ > > - if (!kvm_phys_is_valid(phys)) > > - return phys; > > - > > - /* > > - * Otherwise, work out the correct PA based on how far the walk has > > - * gotten. > > - */ > > + /* Work out the correct PA based on how far the walk has gotten */ > > return phys + (ctx->addr - ctx->start); > > } > > > > @@ -908,6 +892,13 @@ static bool stage2_leaf_mapping_allowed(const struct kvm_pgtable_visit_ctx *ctx, > > if (data->force_pte && ctx->level < KVM_PGTABLE_LAST_LEVEL) > > return false; > > > > + /* > > + * Pass a value that is aligned to any block size when updating > > + * only the owner_id on invalid mappings. > > + */ > > + if (data->owner_update) > > + phys = 0; > > + > > Hmm, isn't this a change in behaviour? Previously we would always store > the owner annotations at the leaf, but now this will take place at a > higher-level. I think that probably goes horribly wrong if we later > try to change the owner for a sub-range; the block will be replaced with > a table and the old ownership information will be lost rather than > propagated to the leaves. > The data->force_pte check preceding this will take care of that, afaict.
diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c index b11bcebac908..4bf618b2cba7 100644 --- a/arch/arm64/kvm/hyp/pgtable.c +++ b/arch/arm64/kvm/hyp/pgtable.c @@ -35,14 +35,6 @@ static bool kvm_pgtable_walk_skip_cmo(const struct kvm_pgtable_visit_ctx *ctx) return unlikely(ctx->flags & KVM_PGTABLE_WALK_SKIP_CMO); } -static bool kvm_phys_is_valid(u64 phys) -{ - u64 parange_max = kvm_get_parange_max(); - u8 shift = id_aa64mmfr0_parange_to_phys_shift(parange_max); - - return phys < BIT(shift); -} - static bool kvm_block_mapping_supported(const struct kvm_pgtable_visit_ctx *ctx, u64 phys) { u64 granule = kvm_granule_size(ctx->level); @@ -53,7 +45,7 @@ static bool kvm_block_mapping_supported(const struct kvm_pgtable_visit_ctx *ctx, if (granule > (ctx->end - ctx->addr)) return false; - if (kvm_phys_is_valid(phys) && !IS_ALIGNED(phys, granule)) + if (!IS_ALIGNED(phys, granule)) return false; return IS_ALIGNED(ctx->addr, granule); @@ -587,6 +579,9 @@ struct stage2_map_data { /* Force mappings to page granularity */ bool force_pte; + + /* Walk should update owner_id only */ + bool owner_update; }; u64 kvm_get_vtcr(u64 mmfr0, u64 mmfr1, u32 phys_shift) @@ -885,18 +880,7 @@ static u64 stage2_map_walker_phys_addr(const struct kvm_pgtable_visit_ctx *ctx, { u64 phys = data->phys; - /* - * Stage-2 walks to update ownership data are communicated to the map - * walker using an invalid PA. Avoid offsetting an already invalid PA, - * which could overflow and make the address valid again. - */ - if (!kvm_phys_is_valid(phys)) - return phys; - - /* - * Otherwise, work out the correct PA based on how far the walk has - * gotten. - */ + /* Work out the correct PA based on how far the walk has gotten */ return phys + (ctx->addr - ctx->start); } @@ -908,6 +892,13 @@ static bool stage2_leaf_mapping_allowed(const struct kvm_pgtable_visit_ctx *ctx, if (data->force_pte && ctx->level < KVM_PGTABLE_LAST_LEVEL) return false; + /* + * Pass a value that is aligned to any block size when updating + * only the owner_id on invalid mappings. + */ + if (data->owner_update) + phys = 0; + return kvm_block_mapping_supported(ctx, phys); } @@ -923,7 +914,7 @@ static int stage2_map_walker_try_leaf(const struct kvm_pgtable_visit_ctx *ctx, if (!stage2_leaf_mapping_allowed(ctx, data)) return -E2BIG; - if (kvm_phys_is_valid(phys)) + if (!data->owner_update) new = kvm_init_valid_leaf_pte(phys, data->attr, ctx->level); else new = kvm_init_invalid_leaf_owner(data->owner_id); @@ -1085,11 +1076,11 @@ int kvm_pgtable_stage2_set_owner(struct kvm_pgtable *pgt, u64 addr, u64 size, { int ret; struct stage2_map_data map_data = { - .phys = KVM_PHYS_INVALID, .mmu = pgt->mmu, .memcache = mc, .owner_id = owner_id, .force_pte = true, + .owner_update = true, }; struct kvm_pgtable_walker walker = { .cb = stage2_map_walker,