| Message ID | 20210715163159.1480168-4-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: MMIO guard PV services | expand |
On Thursday 15 Jul 2021 at 17:31:46 (+0100), Marc Zyngier wrote: > @@ -815,7 +807,7 @@ int kvm_pgtable_stage2_set_owner(struct kvm_pgtable *pgt, u64 addr, u64 size, > .arg = &map_data, > }; > > - if (owner_id > KVM_MAX_OWNER_ID) > + if (!annotation || (annotation & PTE_VALID)) > return -EINVAL; Why do you consider annotation==0 invalid? The assumption so far has been that the owner_id for the host is 0, so annotating a range with 0s should be a valid operation -- this will be required when e.g. transferring ownership of a page back to the host. > > ret = kvm_pgtable_walk(pgt, addr, size, &walker); > -- > 2.30.2 >
On Tue, 20 Jul 2021 11:09:21 +0100, Quentin Perret <qperret@google.com> wrote: > > On Thursday 15 Jul 2021 at 17:31:46 (+0100), Marc Zyngier wrote: > > @@ -815,7 +807,7 @@ int kvm_pgtable_stage2_set_owner(struct kvm_pgtable *pgt, u64 addr, u64 size, > > .arg = &map_data, > > }; > > > > - if (owner_id > KVM_MAX_OWNER_ID) > > + if (!annotation || (annotation & PTE_VALID)) > > return -EINVAL; > > Why do you consider annotation==0 invalid? The assumption so far has > been that the owner_id for the host is 0, so annotating a range with 0s > should be a valid operation -- this will be required when e.g. > transferring ownership of a page back to the host. How do you then distinguish it from an empty entry that doesn't map to anything at all? M.
On Tuesday 20 Jul 2021 at 11:21:17 (+0100), Marc Zyngier wrote: > On Tue, 20 Jul 2021 11:09:21 +0100, > Quentin Perret <qperret@google.com> wrote: > > > > On Thursday 15 Jul 2021 at 17:31:46 (+0100), Marc Zyngier wrote: > > > @@ -815,7 +807,7 @@ int kvm_pgtable_stage2_set_owner(struct kvm_pgtable *pgt, u64 addr, u64 size, > > > .arg = &map_data, > > > }; > > > > > > - if (owner_id > KVM_MAX_OWNER_ID) > > > + if (!annotation || (annotation & PTE_VALID)) > > > return -EINVAL; > > > > Why do you consider annotation==0 invalid? The assumption so far has > > been that the owner_id for the host is 0, so annotating a range with 0s > > should be a valid operation -- this will be required when e.g. > > transferring ownership of a page back to the host. > > How do you then distinguish it from an empty entry that doesn't map to > anything at all? You don't, but that's beauty of it :) The host starts with a PGD full of zeroes, which in terms of ownership means that it owns the entire (I)PA space. And it loses ownership of a page only when we explicitly annotate it with an owner id != 0.
On Tue, 20 Jul 2021 11:38:17 +0100, Quentin Perret <qperret@google.com> wrote: > > On Tuesday 20 Jul 2021 at 11:21:17 (+0100), Marc Zyngier wrote: > > On Tue, 20 Jul 2021 11:09:21 +0100, > > Quentin Perret <qperret@google.com> wrote: > > > > > > On Thursday 15 Jul 2021 at 17:31:46 (+0100), Marc Zyngier wrote: > > > > @@ -815,7 +807,7 @@ int kvm_pgtable_stage2_set_owner(struct kvm_pgtable *pgt, u64 addr, u64 size, > > > > .arg = &map_data, > > > > }; > > > > > > > > - if (owner_id > KVM_MAX_OWNER_ID) > > > > + if (!annotation || (annotation & PTE_VALID)) > > > > return -EINVAL; > > > > > > Why do you consider annotation==0 invalid? The assumption so far has > > > been that the owner_id for the host is 0, so annotating a range with 0s > > > should be a valid operation -- this will be required when e.g. > > > transferring ownership of a page back to the host. > > > > How do you then distinguish it from an empty entry that doesn't map to > > anything at all? > > You don't, but that's beauty of it :) > > The host starts with a PGD full of zeroes, which in terms of ownership > means that it owns the entire (I)PA space. And it loses ownership of a > page only when we explicitly annotate it with an owner id != 0. Right. But this scheme doesn't apply to the guests, does it? Don't we need something that is non-null to preserve the table refcounting? Thanks, M.
On Tuesday 20 Jul 2021 at 12:20:58 (+0100), Marc Zyngier wrote: > On Tue, 20 Jul 2021 11:38:17 +0100, > Quentin Perret <qperret@google.com> wrote: > > > > On Tuesday 20 Jul 2021 at 11:21:17 (+0100), Marc Zyngier wrote: > > > On Tue, 20 Jul 2021 11:09:21 +0100, > > > Quentin Perret <qperret@google.com> wrote: > > > > > > > > On Thursday 15 Jul 2021 at 17:31:46 (+0100), Marc Zyngier wrote: > > > > > @@ -815,7 +807,7 @@ int kvm_pgtable_stage2_set_owner(struct kvm_pgtable *pgt, u64 addr, u64 size, > > > > > .arg = &map_data, > > > > > }; > > > > > > > > > > - if (owner_id > KVM_MAX_OWNER_ID) > > > > > + if (!annotation || (annotation & PTE_VALID)) > > > > > return -EINVAL; > > > > > > > > Why do you consider annotation==0 invalid? The assumption so far has > > > > been that the owner_id for the host is 0, so annotating a range with 0s > > > > should be a valid operation -- this will be required when e.g. > > > > transferring ownership of a page back to the host. > > > > > > How do you then distinguish it from an empty entry that doesn't map to > > > anything at all? > > > > You don't, but that's beauty of it :) > > > > The host starts with a PGD full of zeroes, which in terms of ownership > > means that it owns the entire (I)PA space. And it loses ownership of a > > page only when we explicitly annotate it with an owner id != 0. > > Right. But this scheme doesn't apply to the guests, does it? Right, the meaning of a NULL PTE in guests will clearly be something different, but I guess the interpretation of what invalid mappings mean is up to the caller. > Don't we > need something that is non-null to preserve the table refcounting? Sure, but do we care? If the table entry gets zeroed we're then basically using an 'invalid block' mapping to annotate the entire block range with '0', whatever that means. For guests it won't mean much, but for the host that would mean sole ownership of the entire range.
On Tue, 20 Jul 2021 12:36:21 +0100, Quentin Perret <qperret@google.com> wrote: > > On Tuesday 20 Jul 2021 at 12:20:58 (+0100), Marc Zyngier wrote: > > On Tue, 20 Jul 2021 11:38:17 +0100, > > Quentin Perret <qperret@google.com> wrote: > > > > > > On Tuesday 20 Jul 2021 at 11:21:17 (+0100), Marc Zyngier wrote: > > > > On Tue, 20 Jul 2021 11:09:21 +0100, > > > > Quentin Perret <qperret@google.com> wrote: > > > > > > > > > > On Thursday 15 Jul 2021 at 17:31:46 (+0100), Marc Zyngier wrote: > > > > > > @@ -815,7 +807,7 @@ int kvm_pgtable_stage2_set_owner(struct kvm_pgtable *pgt, u64 addr, u64 size, > > > > > > .arg = &map_data, > > > > > > }; > > > > > > > > > > > > - if (owner_id > KVM_MAX_OWNER_ID) > > > > > > + if (!annotation || (annotation & PTE_VALID)) > > > > > > return -EINVAL; > > > > > > > > > > Why do you consider annotation==0 invalid? The assumption so far has > > > > > been that the owner_id for the host is 0, so annotating a range with 0s > > > > > should be a valid operation -- this will be required when e.g. > > > > > transferring ownership of a page back to the host. > > > > > > > > How do you then distinguish it from an empty entry that doesn't map to > > > > anything at all? > > > > > > You don't, but that's beauty of it :) > > > > > > The host starts with a PGD full of zeroes, which in terms of ownership > > > means that it owns the entire (I)PA space. And it loses ownership of a > > > page only when we explicitly annotate it with an owner id != 0. > > > > Right. But this scheme doesn't apply to the guests, does it? > > Right, the meaning of a NULL PTE in guests will clearly be something > different, but I guess the interpretation of what invalid mappings mean > is up to the caller. > > > Don't we > > need something that is non-null to preserve the table refcounting? > > Sure, but do we care? If the table entry gets zeroed we're then > basically using an 'invalid block' mapping to annotate the entire block > range with '0', whatever that means. For guests it won't mean much, but > for the host that would mean sole ownership of the entire range. I see. You let the refcount drop to 0, unmap the table and let transfer the 0 annotation one level up, covering the whole block. I guess I'll revert back to allowing 0, but I'd like to make sure we don't do that for guests unless we actually tear down the address space (checking for KVM_PGTABLE_S2_IDMAP should work). Thanks, M.
diff --git a/arch/arm64/include/asm/kvm_pgtable.h b/arch/arm64/include/asm/kvm_pgtable.h index f004c0115d89..9579e8c2793b 100644 --- a/arch/arm64/include/asm/kvm_pgtable.h +++ b/arch/arm64/include/asm/kvm_pgtable.h @@ -274,14 +274,16 @@ int kvm_pgtable_stage2_map(struct kvm_pgtable *pgt, u64 addr, u64 size, void *mc); /** - * kvm_pgtable_stage2_set_owner() - Unmap and annotate pages in the IPA space to - * track ownership. + * kvm_pgtable_stage2_annotate() - Unmap and annotate pages in the IPA space + * to track ownership (and more). * @pgt: Page-table structure initialised by kvm_pgtable_stage2_init*(). * @addr: Base intermediate physical address to annotate. * @size: Size of the annotated range. * @mc: Cache of pre-allocated and zeroed memory from which to allocate * page-table pages. - * @owner_id: Unique identifier for the owner of the page. + * @annotation: A 63 bit value that will be stored in the page tables. + * @annotation[0] must be 0, and @annotation[63:1] is stored + * in the page tables. @annotation as a whole must not be 0. * * By default, all page-tables are owned by identifier 0. This function can be * used to mark portions of the IPA space as owned by other entities. When a @@ -290,8 +292,8 @@ int kvm_pgtable_stage2_map(struct kvm_pgtable *pgt, u64 addr, u64 size, * * Return: 0 on success, negative error code on failure. */ -int kvm_pgtable_stage2_set_owner(struct kvm_pgtable *pgt, u64 addr, u64 size, - void *mc, u8 owner_id); +int kvm_pgtable_stage2_annotate(struct kvm_pgtable *pgt, u64 addr, u64 size, + void *mc, kvm_pte_t annotation); /** * kvm_pgtable_stage2_unmap() - Remove a mapping from a guest stage-2 page-table. diff --git a/arch/arm64/kvm/hyp/nvhe/mem_protect.c b/arch/arm64/kvm/hyp/nvhe/mem_protect.c index d938ce95d3bd..ffe482c3b818 100644 --- a/arch/arm64/kvm/hyp/nvhe/mem_protect.c +++ b/arch/arm64/kvm/hyp/nvhe/mem_protect.c @@ -245,6 +245,15 @@ static int host_stage2_idmap(u64 addr) return ret; } +#define KVM_INVALID_PTE_OWNER_MASK GENMASK(63, 56) +#define KVM_MAX_OWNER_ID 1 + +static kvm_pte_t kvm_init_invalid_leaf_owner(u8 owner_id) +{ + BUG_ON(owner_id > KVM_MAX_OWNER_ID); + return FIELD_PREP(KVM_INVALID_PTE_OWNER_MASK, owner_id); +} + int __pkvm_mark_hyp(phys_addr_t start, phys_addr_t end) { int ret; @@ -257,8 +266,9 @@ int __pkvm_mark_hyp(phys_addr_t start, phys_addr_t end) return -EINVAL; hyp_spin_lock(&host_kvm.lock); - ret = kvm_pgtable_stage2_set_owner(&host_kvm.pgt, start, end - start, - &host_s2_pool, pkvm_hyp_id); + ret = kvm_pgtable_stage2_annotate(&host_kvm.pgt, start, end - start, + &host_s2_pool, + kvm_init_invalid_leaf_owner(pkvm_hyp_id)); hyp_spin_unlock(&host_kvm.lock); return ret != -EAGAIN ? ret : 0; diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c index a5874ebd0354..a065f6d960af 100644 --- a/arch/arm64/kvm/hyp/pgtable.c +++ b/arch/arm64/kvm/hyp/pgtable.c @@ -50,9 +50,6 @@ #define KVM_PTE_LEAF_ATTR_S2_IGNORED GENMASK(58, 55) -#define KVM_INVALID_PTE_OWNER_MASK GENMASK(63, 56) -#define KVM_MAX_OWNER_ID 1 - struct kvm_pgtable_walk_data { struct kvm_pgtable *pgt; struct kvm_pgtable_walker *walker; @@ -206,11 +203,6 @@ static kvm_pte_t kvm_init_valid_leaf_pte(u64 pa, kvm_pte_t attr, u32 level) return pte; } -static kvm_pte_t kvm_init_invalid_leaf_owner(u8 owner_id) -{ - return FIELD_PREP(KVM_INVALID_PTE_OWNER_MASK, owner_id); -} - static int kvm_pgtable_visitor_cb(struct kvm_pgtable_walk_data *data, u64 addr, u32 level, kvm_pte_t *ptep, enum kvm_pgtable_walk_flags flag) @@ -466,7 +458,7 @@ void kvm_pgtable_hyp_destroy(struct kvm_pgtable *pgt) struct stage2_map_data { u64 phys; kvm_pte_t attr; - u8 owner_id; + u64 annotation; kvm_pte_t *anchor; kvm_pte_t *childp; @@ -603,7 +595,7 @@ static int stage2_map_walker_try_leaf(u64 addr, u64 end, u32 level, if (kvm_phys_is_valid(phys)) new = kvm_init_valid_leaf_pte(phys, data->attr, level); else - new = kvm_init_invalid_leaf_owner(data->owner_id); + new = data->annotation; if (stage2_pte_is_counted(old)) { /* @@ -796,8 +788,8 @@ int kvm_pgtable_stage2_map(struct kvm_pgtable *pgt, u64 addr, u64 size, return ret; } -int kvm_pgtable_stage2_set_owner(struct kvm_pgtable *pgt, u64 addr, u64 size, - void *mc, u8 owner_id) +int kvm_pgtable_stage2_annotate(struct kvm_pgtable *pgt, u64 addr, u64 size, + void *mc, kvm_pte_t annotation) { int ret; struct stage2_map_data map_data = { @@ -805,7 +797,7 @@ int kvm_pgtable_stage2_set_owner(struct kvm_pgtable *pgt, u64 addr, u64 size, .mmu = pgt->mmu, .memcache = mc, .mm_ops = pgt->mm_ops, - .owner_id = owner_id, + .annotation = annotation, }; struct kvm_pgtable_walker walker = { .cb = stage2_map_walker, @@ -815,7 +807,7 @@ int kvm_pgtable_stage2_set_owner(struct kvm_pgtable *pgt, u64 addr, u64 size, .arg = &map_data, }; - if (owner_id > KVM_MAX_OWNER_ID) + if (!annotation || (annotation & PTE_VALID)) return -EINVAL; ret = kvm_pgtable_walk(pgt, addr, size, &walker);
kvm_pgtable_stage2_set_owner() could be generalised into a way to store up to 63 bits in the page tables, as long as we don't set bit 0. Let's just do that. Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/include/asm/kvm_pgtable.h | 12 +++++++----- arch/arm64/kvm/hyp/nvhe/mem_protect.c | 14 ++++++++++++-- arch/arm64/kvm/hyp/pgtable.c | 20 ++++++-------------- 3 files changed, 25 insertions(+), 21 deletions(-)