From: Michael Roth <michael.roth@amd.com> To: Sean Christopherson <seanjc@google.com> Cc: <linux-kselftest@vger.kernel.org>, <kvm@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <x86@kernel.org>, Nathan Tempelman <natet@google.com>, Marc Orr <marcorr@google.com>, Steve Rutherford <srutherford@google.com>, Mingwei Zhang <mizhang@google.com>, Brijesh Singh <brijesh.singh@amd.com>, Tom Lendacky <thomas.lendacky@amd.com>, Varad Gautam <varad.gautam@suse.com>, Shuah Khan <shuah@kernel.org>, Vitaly Kuznetsov <vkuznets@redhat.com>, David Woodhouse <dwmw@amazon.co.uk>, Ricardo Koller <ricarkol@google.com>, Jim Mattson <jmattson@google.com>, Joerg Roedel <joro@8bytes.org>, Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>, "H . Peter Anvin" <hpa@zytor.com>, Christian Borntraeger <borntraeger@linux.ibm.com>, "Janosch Frank" <frankja@linux.ibm.com>, David Hildenbrand <david@redhat.com>, "Claudio Imbrenda" <imbrenda@linux.ibm.com>, Marc Zyngier <maz@kernel.org>, James Morse <james.morse@arm.com>, Alexandru Elisei <alexandru.elisei@arm.com>, "Suzuki K Poulose" <suzuki.poulose@arm.com>, <kvmarm@lists.cs.columbia.edu> Subject: Re: [RFC PATCH 00/10] KVM: selftests: Add support for test-selectable ucall implementations Date: Wed, 5 Jan 2022 15:35:19 -0600 [thread overview] Message-ID: <20220105213519.g746jzf756nax562@amd.com> (raw) In-Reply-To: <YdX0SRoBXReggrVA@google.com> On Wed, Jan 05, 2022 at 07:40:57PM +0000, Sean Christopherson wrote: > On Wed, Jan 05, 2022, Michael Roth wrote: > > On Wed, Jan 05, 2022 at 05:43:21PM +0000, Sean Christopherson wrote: > > > Because it uses multiple VMs, and my rough sketch only allows for a single VM to > > > use ucall. Though I suppose we could simply keep appending to the ucall list for > > > every VM. The requirement would then be that all VMs are of the same type, i.e. > > > utilize the same ucall_ops. > > > > Hmm, maybe I misread your patch. Not supporting multiple VMs was the > > reason I gave up on having the ucall structs allocated on-demand and > > went with requiring them to be passed as arguments to ucall(). > > > > I thought with your patch you had solved that by having each vm have it's > > own pool, via vm->ucall_list, and then mapping each pool into each guest > > separately via: > > > > ucall_init(vm): > > ucall_list = vm->ucall_list > > sync_global_to_guest(ucall_list). > > > > then as long as that ucall_init() is done *after* the guest calls > > kvm_vm_elf_load(), it will end up with a 'ucall_list' global that points > > to it's own specific vm->ucall_list. Then on the test side it doesn't > > matter what the 'ucall_list' global is currently set to since you have > > the GPA and know what vm exited. > > > > Or am I missing something there? > > Ha, that was not at all intented. But yes, it should work. I'd rather be lucky > than good? :) > > > Although even if that is the case, now that we're proposing doing the > > ucall_init() inside vm_create(), then we run the risk of a test calling > > kvm_vm_elf_load() after, which might clobber the guest's copy of > > ucall_list global if ucall_init() had since been called for another VM. > > But that could maybe be worked around by having whatever vm_create() > > variant we use also do the kvm_vm_elf_load() unconditionally as part of > > creation. > > Will sync_global_to_guest() even work as intended if kvm_vm_elf_load() hasn't > been called? If not, then sync_global_{to,from}_guest() should really assert if > the test hasn't been loaded. Yah, seems like it would get clobbered by kvm_vm_elf_load() later. And can't think of any good reason to use sync_global_to_guest() without also needing kvm_vm_elf_load() at some point, so makes sense to enforce it. > > As for ucall_init(), I think the best approach would be to make kvm_vm_elf_load() > a static and replace all calls with: > > kvm_vm_load_guest(vm); > > where its implementation is: > > void kvm_vm_load_guest(struct kvm_vm *vm) > { > kvm_vm_elf_load(vm, program_invocation_name); > > ucall_init(vm); > } > > The logic being that if a test creates a VM but never loads any code into the guest, > e.g. kvm_create_max_vcpus, then it _can't_ make ucalls. Makes sense. And if different ops are needed for vmgexit()/tdcall() it could be something like (if based on patches 1-5 of this series, and extending vm_guest_mode as you suggested earlier): void kvm_vm_load_guest(struct kvm_vm *vm) { kvm_vm_elf_load(vm, program_invocation_name); if (vm->mode == VM_MODE_SEV) ucall_init_ops(vm, ucall_ops_pio_vmgexit); else (vm->vm_type == VM_MODE_TDX) ucall_init_ops(vm, ucall_ops_pio_tdcall); else ucall_init_ops(vm, ucall_ops_pio); Shame we have to update all the kvm_vm_elf_load() call-sites, but they'd end up potentially breaking things if left as-is anyway. Were you planning on sending patches for these changes, or should I incorporate your prototype and take a stab at the other changes as part of v2 of this series?
WARNING: multiple messages have this Message-ID (diff)
From: Michael Roth <michael.roth@amd.com> To: Sean Christopherson <seanjc@google.com> Cc: Brijesh Singh <brijesh.singh@amd.com>, kvm@vger.kernel.org, David Hildenbrand <david@redhat.com>, Marc Orr <marcorr@google.com>, linux-kselftest@vger.kernel.org, "H . Peter Anvin" <hpa@zytor.com>, Claudio Imbrenda <imbrenda@linux.ibm.com>, Shuah Khan <shuah@kernel.org>, kvmarm@lists.cs.columbia.edu, Nathan Tempelman <natet@google.com>, Janosch Frank <frankja@linux.ibm.com>, Marc Zyngier <maz@kernel.org>, Joerg Roedel <joro@8bytes.org>, x86@kernel.org, Ingo Molnar <mingo@redhat.com>, Mingwei Zhang <mizhang@google.com>, Christian Borntraeger <borntraeger@linux.ibm.com>, Tom Lendacky <thomas.lendacky@amd.com>, Borislav Petkov <bp@alien8.de>, Thomas Gleixner <tglx@linutronix.de>, Varad Gautam <varad.gautam@suse.com>, Jim Mattson <jmattson@google.com>, Steve Rutherford <srutherford@google.com>, linux-kernel@vger.kernel.org, Vitaly Kuznetsov <vkuznets@redhat.com>, David Woodhouse <dwmw@amazon.co.uk> Subject: Re: [RFC PATCH 00/10] KVM: selftests: Add support for test-selectable ucall implementations Date: Wed, 5 Jan 2022 15:35:19 -0600 [thread overview] Message-ID: <20220105213519.g746jzf756nax562@amd.com> (raw) In-Reply-To: <YdX0SRoBXReggrVA@google.com> On Wed, Jan 05, 2022 at 07:40:57PM +0000, Sean Christopherson wrote: > On Wed, Jan 05, 2022, Michael Roth wrote: > > On Wed, Jan 05, 2022 at 05:43:21PM +0000, Sean Christopherson wrote: > > > Because it uses multiple VMs, and my rough sketch only allows for a single VM to > > > use ucall. Though I suppose we could simply keep appending to the ucall list for > > > every VM. The requirement would then be that all VMs are of the same type, i.e. > > > utilize the same ucall_ops. > > > > Hmm, maybe I misread your patch. Not supporting multiple VMs was the > > reason I gave up on having the ucall structs allocated on-demand and > > went with requiring them to be passed as arguments to ucall(). > > > > I thought with your patch you had solved that by having each vm have it's > > own pool, via vm->ucall_list, and then mapping each pool into each guest > > separately via: > > > > ucall_init(vm): > > ucall_list = vm->ucall_list > > sync_global_to_guest(ucall_list). > > > > then as long as that ucall_init() is done *after* the guest calls > > kvm_vm_elf_load(), it will end up with a 'ucall_list' global that points > > to it's own specific vm->ucall_list. Then on the test side it doesn't > > matter what the 'ucall_list' global is currently set to since you have > > the GPA and know what vm exited. > > > > Or am I missing something there? > > Ha, that was not at all intented. But yes, it should work. I'd rather be lucky > than good? :) > > > Although even if that is the case, now that we're proposing doing the > > ucall_init() inside vm_create(), then we run the risk of a test calling > > kvm_vm_elf_load() after, which might clobber the guest's copy of > > ucall_list global if ucall_init() had since been called for another VM. > > But that could maybe be worked around by having whatever vm_create() > > variant we use also do the kvm_vm_elf_load() unconditionally as part of > > creation. > > Will sync_global_to_guest() even work as intended if kvm_vm_elf_load() hasn't > been called? If not, then sync_global_{to,from}_guest() should really assert if > the test hasn't been loaded. Yah, seems like it would get clobbered by kvm_vm_elf_load() later. And can't think of any good reason to use sync_global_to_guest() without also needing kvm_vm_elf_load() at some point, so makes sense to enforce it. > > As for ucall_init(), I think the best approach would be to make kvm_vm_elf_load() > a static and replace all calls with: > > kvm_vm_load_guest(vm); > > where its implementation is: > > void kvm_vm_load_guest(struct kvm_vm *vm) > { > kvm_vm_elf_load(vm, program_invocation_name); > > ucall_init(vm); > } > > The logic being that if a test creates a VM but never loads any code into the guest, > e.g. kvm_create_max_vcpus, then it _can't_ make ucalls. Makes sense. And if different ops are needed for vmgexit()/tdcall() it could be something like (if based on patches 1-5 of this series, and extending vm_guest_mode as you suggested earlier): void kvm_vm_load_guest(struct kvm_vm *vm) { kvm_vm_elf_load(vm, program_invocation_name); if (vm->mode == VM_MODE_SEV) ucall_init_ops(vm, ucall_ops_pio_vmgexit); else (vm->vm_type == VM_MODE_TDX) ucall_init_ops(vm, ucall_ops_pio_tdcall); else ucall_init_ops(vm, ucall_ops_pio); Shame we have to update all the kvm_vm_elf_load() call-sites, but they'd end up potentially breaking things if left as-is anyway. Were you planning on sending patches for these changes, or should I incorporate your prototype and take a stab at the other changes as part of v2 of this series? _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
next prev parent reply other threads:[~2022-01-05 21:35 UTC|newest] Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-12-10 16:46 [RFC PATCH 00/10] KVM: selftests: Add support for test-selectable ucall implementations Michael Roth 2021-12-10 16:46 ` Michael Roth 2021-12-10 16:46 ` [PATCH RFC 01/10] kvm: selftests: move base kvm_util.h declarations to kvm_util_base.h Michael Roth 2021-12-10 16:46 ` Michael Roth 2021-12-10 16:46 ` [PATCH RFC 02/10] kvm: selftests: move ucall declarations into ucall_common.h Michael Roth 2021-12-10 16:46 ` Michael Roth 2021-12-25 9:11 ` Andrew Jones 2021-12-25 9:11 ` Andrew Jones 2021-12-10 16:46 ` [PATCH RFC 03/10] kvm: selftests: introduce ucall_ops for test/arch-specific ucall implementations Michael Roth 2021-12-10 16:46 ` Michael Roth 2021-12-10 16:46 ` [PATCH RFC 04/10] kvm: arm64: selftests: use ucall_ops to define default ucall implementation Michael Roth 2021-12-10 16:46 ` Michael Roth 2021-12-10 16:46 ` [PATCH RFC 05/10] kvm: s390: " Michael Roth 2021-12-10 16:46 ` Michael Roth 2021-12-10 16:46 ` [PATCH RFC 06/10] kvm: selftests: add ucall interfaces based around shared memory Michael Roth 2021-12-10 16:46 ` Michael Roth 2021-12-10 16:46 ` [PATCH RFC 07/10] kvm: selftests: add ucall_shared ops for PIO Michael Roth 2021-12-10 16:46 ` Michael Roth 2021-12-10 16:46 ` [PATCH RFC 08/10] kvm: selftests: introduce ucall implementation based on halt instructions Michael Roth 2021-12-10 16:46 ` Michael Roth 2021-12-10 16:46 ` [PATCH RFC 09/10] kvm: selftests: add GUEST_SHARED_* macros for shared ucall implementations Michael Roth 2021-12-10 16:46 ` Michael Roth 2021-12-10 16:46 ` [PATCH RFC 10/10] kvm: selftests: add ucall_test to test various ucall functionality Michael Roth 2021-12-10 16:46 ` Michael Roth 2021-12-22 14:46 ` [RFC PATCH 00/10] KVM: selftests: Add support for test-selectable ucall implementations Paolo Bonzini 2021-12-22 14:46 ` Paolo Bonzini 2021-12-30 21:11 ` Sean Christopherson 2021-12-30 21:11 ` Sean Christopherson 2022-01-04 23:35 ` Michael Roth 2022-01-04 23:35 ` Michael Roth 2022-01-05 0:17 ` Sean Christopherson 2022-01-05 0:17 ` Sean Christopherson 2022-01-05 17:02 ` Michael Roth 2022-01-05 17:02 ` Michael Roth 2022-01-05 17:43 ` Sean Christopherson 2022-01-05 17:43 ` Sean Christopherson 2022-01-05 19:11 ` Michael Roth 2022-01-05 19:11 ` Michael Roth 2022-01-05 19:40 ` Sean Christopherson 2022-01-05 19:40 ` Sean Christopherson 2022-01-05 21:35 ` Michael Roth [this message] 2022-01-05 21:35 ` Michael Roth 2022-01-05 22:02 ` Sean Christopherson 2022-01-05 22:02 ` Sean Christopherson 2022-01-05 22:32 ` Michael Roth 2022-01-05 22:32 ` Michael Roth
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20220105213519.g746jzf756nax562@amd.com \ --to=michael.roth@amd.com \ --cc=alexandru.elisei@arm.com \ --cc=borntraeger@linux.ibm.com \ --cc=bp@alien8.de \ --cc=brijesh.singh@amd.com \ --cc=david@redhat.com \ --cc=dwmw@amazon.co.uk \ --cc=frankja@linux.ibm.com \ --cc=hpa@zytor.com \ --cc=imbrenda@linux.ibm.com \ --cc=james.morse@arm.com \ --cc=jmattson@google.com \ --cc=joro@8bytes.org \ --cc=kvm@vger.kernel.org \ --cc=kvmarm@lists.cs.columbia.edu \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-kselftest@vger.kernel.org \ --cc=marcorr@google.com \ --cc=maz@kernel.org \ --cc=mingo@redhat.com \ --cc=mizhang@google.com \ --cc=natet@google.com \ --cc=ricarkol@google.com \ --cc=seanjc@google.com \ --cc=shuah@kernel.org \ --cc=srutherford@google.com \ --cc=suzuki.poulose@arm.com \ --cc=tglx@linutronix.de \ --cc=thomas.lendacky@amd.com \ --cc=varad.gautam@suse.com \ --cc=vkuznets@redhat.com \ --cc=x86@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.