| Message ID | 20220819174659.2427983-4-vannapurve@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | selftests: KVM: selftests for fd-based private memory | expand |
On Fri, Aug 19, 2022, Vishal Annapurve wrote: > Add a helper to query guest physical address for ucall pool > so that guest can mark the page as accessed shared or private. > > Signed-off-by: Vishal Annapurve <vannapurve@google.com> > --- This should be handled by the SEV series[*]. Can you provide feedback on that series if having a generic way to map the ucall address as shared won't work? [*] https://lore.kernel.org/all/20220829171021.701198-1-pgonda@google.com
On Fri, Oct 7, 2022 at 1:32 AM Sean Christopherson <seanjc@google.com> wrote: > > On Fri, Aug 19, 2022, Vishal Annapurve wrote: > > Add a helper to query guest physical address for ucall pool > > so that guest can mark the page as accessed shared or private. > > > > Signed-off-by: Vishal Annapurve <vannapurve@google.com> > > --- > > This should be handled by the SEV series[*]. Can you provide feedback on that > series if having a generic way to map the ucall address as shared won't work? > > [*] https://lore.kernel.org/all/20220829171021.701198-1-pgonda@google.com Based on the SEV series you referred to, selftests are capable of accessing ucall pool memory by having encryption bit cleared (as set by guest pagetables) as allowed by generic API vm_vaddr_alloc_shared. This change is needed in the context of fd based private memory where guest (specifically non-confidential/sev guests) code in the selftests will have to explicitly indicate that ucall pool address range will be accessed by guest as shared.
On Fri, Oct 14, 2022, Vishal Annapurve wrote: > On Fri, Oct 7, 2022 at 1:32 AM Sean Christopherson <seanjc@google.com> wrote: > > > > On Fri, Aug 19, 2022, Vishal Annapurve wrote: > > > Add a helper to query guest physical address for ucall pool > > > so that guest can mark the page as accessed shared or private. > > > > > > Signed-off-by: Vishal Annapurve <vannapurve@google.com> > > > --- > > > > This should be handled by the SEV series[*]. Can you provide feedback on that > > series if having a generic way to map the ucall address as shared won't work? > > > > [*] https://lore.kernel.org/all/20220829171021.701198-1-pgonda@google.com > > Based on the SEV series you referred to, selftests are capable of > accessing ucall pool memory by having encryption bit cleared (as set > by guest pagetables) as allowed by generic API vm_vaddr_alloc_shared. > This change is needed in the context of fd based private memory where > guest (specifically non-confidential/sev guests) code in the selftests > will have to explicitly indicate that ucall pool address range will be > accessed by guest as shared. Ah, right, the conversion needs an explicit hypercall, which gets downright annoying because auto-converting shared pages would effectivfely require injecting code into the start of every guest. Ha! I think we got too fancy. This is purely for testing UPM, not any kind of trust model, i.e. there's no need for KVM to treat userspace as untrusted. Rather than jump through hoops just to let the guest dictate private vs. shared, simply "trust" userspace when determining whether a page should be mapped private. Then the selftests can invoke the repurposed KVM_MEMORY_ENCRYPT_(UN)REG_REGION ioctls as appropriate when allocating/remapping guest private memory. E.g. on top of UPM v8, I think the test hook boils down to: diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c index d68944f07b4b..d42d0e6bdd8c 100644 --- a/arch/x86/kvm/mmu/mmu.c +++ b/arch/x86/kvm/mmu/mmu.c @@ -4279,6 +4279,9 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault fault->gfn = fault->addr >> PAGE_SHIFT; fault->slot = kvm_vcpu_gfn_to_memslot(vcpu, fault->gfn); + fault->is_private = IS_ENABLED(CONFIG_KVM_PRIVATE_MEM_TESTING) && + kvm_slot_can_be_private(fault->slot) && + kvm_mem_is_private(vcpu->kvm, fault->gfn); if (page_fault_handle_page_track(vcpu, fault)) return RET_PF_EMULATE; diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 8ffd4607c7d8..0dc5d0bf647c 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -1653,7 +1653,7 @@ static void kvm_replace_memslot(struct kvm *kvm, bool __weak kvm_arch_has_private_mem(struct kvm *kvm) { - return false; + return IS_ENABLED(CONFIG_KVM_PRIVATE_MEM_TESTING); } static int check_memory_region_flags(struct kvm *kvm,
On Sat, Oct 15, 2022 at 12:17 AM Sean Christopherson <seanjc@google.com> wrote: > > On Fri, Oct 14, 2022, Vishal Annapurve wrote: > > On Fri, Oct 7, 2022 at 1:32 AM Sean Christopherson <seanjc@google.com> wrote: > > > > > > On Fri, Aug 19, 2022, Vishal Annapurve wrote: > > > > Add a helper to query guest physical address for ucall pool > > > > so that guest can mark the page as accessed shared or private. > > > > > > > > Signed-off-by: Vishal Annapurve <vannapurve@google.com> > > > > --- > > > > > > This should be handled by the SEV series[*]. Can you provide feedback on that > > > series if having a generic way to map the ucall address as shared won't work? > > > > > > [*] https://lore.kernel.org/all/20220829171021.701198-1-pgonda@google.com > > > > Based on the SEV series you referred to, selftests are capable of > > accessing ucall pool memory by having encryption bit cleared (as set > > by guest pagetables) as allowed by generic API vm_vaddr_alloc_shared. > > This change is needed in the context of fd based private memory where > > guest (specifically non-confidential/sev guests) code in the selftests > > will have to explicitly indicate that ucall pool address range will be > > accessed by guest as shared. > > Ah, right, the conversion needs an explicit hypercall, which gets downright > annoying because auto-converting shared pages would effectivfely require injecting > code into the start of every guest. > Ack. > Ha! I think we got too fancy. This is purely for testing UPM, not any kind of > trust model, i.e. there's no need for KVM to treat userspace as untrusted. Rather > than jump through hoops just to let the guest dictate private vs. shared, simply > "trust" userspace when determining whether a page should be mapped private. Then > the selftests can invoke the repurposed KVM_MEMORY_ENCRYPT_(UN)REG_REGION ioctls > as appropriate when allocating/remapping guest private memory. > > E.g. on top of UPM v8, I think the test hook boils down to: > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c > index d68944f07b4b..d42d0e6bdd8c 100644 > --- a/arch/x86/kvm/mmu/mmu.c > +++ b/arch/x86/kvm/mmu/mmu.c > @@ -4279,6 +4279,9 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault > > fault->gfn = fault->addr >> PAGE_SHIFT; > fault->slot = kvm_vcpu_gfn_to_memslot(vcpu, fault->gfn); > + fault->is_private = IS_ENABLED(CONFIG_KVM_PRIVATE_MEM_TESTING) && > + kvm_slot_can_be_private(fault->slot) && > + kvm_mem_is_private(vcpu->kvm, fault->gfn); > > if (page_fault_handle_page_track(vcpu, fault)) > return RET_PF_EMULATE; > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 8ffd4607c7d8..0dc5d0bf647c 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -1653,7 +1653,7 @@ static void kvm_replace_memslot(struct kvm *kvm, > > bool __weak kvm_arch_has_private_mem(struct kvm *kvm) > { > - return false; > + return IS_ENABLED(CONFIG_KVM_PRIVATE_MEM_TESTING); > } > > static int check_memory_region_flags(struct kvm *kvm, This is much sleeker and will avoid hacking KVM for testing. Only caveat here is that these tests will not be able to exercise implicit conversion path if we go this route.
On Mon, Oct 17, 2022, Vishal Annapurve wrote: > This is much sleeker and will avoid hacking KVM for testing. Only > caveat here is that these tests will not be able to exercise implicit > conversion path if we go this route. Yeah, I think that's a perfectly fine tradeoff. Implicit conversion isn't strictly a UPM feature, e.g. if TDX and SNP "architecturally" disallowed implicit conversions, then KVM wouldn't need to support implicit conversions at all, i.e. that testing can be punted to SNP and/or TDX selftests.
On Mon, Oct 17, 2022 at 11:38 PM Sean Christopherson <seanjc@google.com> wrote: > > On Mon, Oct 17, 2022, Vishal Annapurve wrote: > > This is much sleeker and will avoid hacking KVM for testing. Only > > caveat here is that these tests will not be able to exercise implicit > > conversion path if we go this route. > > Yeah, I think that's a perfectly fine tradeoff. Implicit conversion isn't strictly > a UPM feature, e.g. if TDX and SNP "architecturally" disallowed implicit conversions, > then KVM wouldn't need to support implicit conversions at all, i.e. that testing can > be punted to SNP and/or TDX selftests. Ack. Will address this feedback in the next series.
diff --git a/tools/testing/selftests/kvm/include/ucall_common.h b/tools/testing/selftests/kvm/include/ucall_common.h index 279bbab011c7..2c6e5c4df012 100644 --- a/tools/testing/selftests/kvm/include/ucall_common.h +++ b/tools/testing/selftests/kvm/include/ucall_common.h @@ -31,6 +31,8 @@ void ucall_arch_uninit(struct kvm_vm *vm); void ucall_arch_do_ucall(vm_vaddr_t uc); void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu); +vm_paddr_t get_ucall_pool_paddr(void); + void ucall(uint64_t cmd, int nargs, ...); uint64_t get_ucall(struct kvm_vcpu *vcpu, struct ucall *uc); diff --git a/tools/testing/selftests/kvm/lib/ucall_common.c b/tools/testing/selftests/kvm/lib/ucall_common.c index 5a15fa39cd51..4d2abef8ee77 100644 --- a/tools/testing/selftests/kvm/lib/ucall_common.c +++ b/tools/testing/selftests/kvm/lib/ucall_common.c @@ -11,6 +11,7 @@ struct ucall_header { static bool use_ucall_pool; static struct ucall_header *ucall_pool; +static vm_paddr_t ucall_page_paddr; void ucall_init(struct kvm_vm *vm, void *arg) { @@ -35,7 +36,10 @@ void ucall_init(struct kvm_vm *vm, void *arg) } ucall_pool = (struct ucall_header *)vaddr; + ucall_page_paddr = addr_gva2gpa(vm, vaddr); sync_global_to_guest(vm, ucall_pool); + sync_global_to_guest(vm, ucall_page_paddr); + printf("ucall_page_paddr 0x%lx\n", ucall_page_paddr); out: ucall_arch_init(vm, arg); @@ -54,6 +58,14 @@ void ucall_uninit(struct kvm_vm *vm) ucall_arch_uninit(vm); } +vm_paddr_t get_ucall_pool_paddr(void) +{ + if (!use_ucall_pool) + return 0; + + return ucall_page_paddr; +} + static struct ucall *ucall_alloc(void) { struct ucall *uc = NULL;
Add a helper to query guest physical address for ucall pool so that guest can mark the page as accessed shared or private. Signed-off-by: Vishal Annapurve <vannapurve@google.com> --- tools/testing/selftests/kvm/include/ucall_common.h | 2 ++ tools/testing/selftests/kvm/lib/ucall_common.c | 12 ++++++++++++ 2 files changed, 14 insertions(+)