Re: [PATCH v2 4/6] arm64/kvm: Avoid invalid physical addresses to signal owner updates
From: Will Deacon <will@kernel.org>
Date: 2024-12-11 22:14:38
Also in:
lkml
Hi Ard, On Thu, Dec 05, 2024 at 04:02:34PM +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, and these 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. Cc: Quentin Perret <redacted> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- arch/arm64/kvm/hyp/pgtable.c | 33 ++++++-------------- 1 file changed, 10 insertions(+), 23 deletions(-)
Sorry that I didn't reply again on v1, but I have an annoying request that would make this a little easier for me to follow (since I'm tainted with the pKVM stack in Android that we're gradually landing upstream):
quoted hunk ↗ jump to hunk
diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c index 40bd55966540..0569e1d97c38 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;
Can you rename this to "annotation", please? We'll eventually land other types of invalid pte than ownership (e.g. MMIO_GUARD) and, given that the ownership walker is caught by the 'force_pte' flag, it's a little more generic. Again, apologies I didn't ask for this earlier. Will