| Message ID | 20220502233853.1233742-1-rananta@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | KVM: arm64: Add support for hypercall services selection | expand |
On Tue, 03 May 2022 00:38:44 +0100, Raghavendra Rao Ananta <rananta@google.com> wrote: > > Hello, > > Continuing the discussion from [1], the series tries to add support > for the userspace to elect the hypercall services that it wishes > to expose to the guest, rather than the guest discovering them > unconditionally. The idea employed by the series was taken from > [1] as suggested by Marc Z. As it took some time to get there, and that there was still a bunch of things to address, I've taken the liberty to apply my own fixes to the series. Please have a look at [1], and let me know if you're OK with the result. If you are, I'll merge the series for 5.19. Thanks, M. [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/hcall-selection
Hi Marc, On Tue, May 3, 2022 at 10:24 AM Marc Zyngier <maz@kernel.org> wrote: > > On Tue, 03 May 2022 00:38:44 +0100, > Raghavendra Rao Ananta <rananta@google.com> wrote: > > > > Hello, > > > > Continuing the discussion from [1], the series tries to add support > > for the userspace to elect the hypercall services that it wishes > > to expose to the guest, rather than the guest discovering them > > unconditionally. The idea employed by the series was taken from > > [1] as suggested by Marc Z. > > As it took some time to get there, and that there was still a bunch of > things to address, I've taken the liberty to apply my own fixes to the > series. > > Please have a look at [1], and let me know if you're OK with the > result. If you are, I'll merge the series for 5.19. > > Thanks, > > M. > Thank you for speeding up the process; appreciate it. However, the series's selftest patches have a dependency on Oliver's PSCI_SYSTEM_SUSPEND's selftest patches [1][2]. Can we pull them in too? aarch64/hypercalls.c: In function ‘guest_test_hvc’: aarch64/hypercalls.c:95:30: error: storage size of ‘res’ isn’t known 95 | struct arm_smccc_res res; | ^~~ aarch64/hypercalls.c:103:17: warning: implicit declaration of function ‘smccc_hvc’ [-Wimplicit-function-declaration] 103 | smccc_hvc(hc_info->func_id, hc_info->arg1, 0, 0, 0, 0, 0, 0, &res); | ^~~~~~~~~ Also, just a couple of readability nits in the fixed version: 1. Patch-2/9, hypercall.c:kvm_hvc_call_default_allowed(), in the 'default' case, do you think we should probably add a small comment that mentions we are checking for func_id in the PSCI range? 2. Patch-2/9, arm_hypercall.h, clear all the macros in this patch itself instead of doing it in increments (unless there's some reason that I'm missing)? Regards, Raghavendra [1]: https://lore.kernel.org/all/20220409184549.1681189-10-oupton@google.com/ [2]: https://lore.kernel.org/all/20220409184549.1681189-11-oupton@google.com/ > [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/hcall-selection > > -- > Without deviation from the norm, progress is not possible.
On Tue, 03 May 2022 19:49:13 +0100, Raghavendra Rao Ananta <rananta@google.com> wrote: > > Hi Marc, > > On Tue, May 3, 2022 at 10:24 AM Marc Zyngier <maz@kernel.org> wrote: > > > > On Tue, 03 May 2022 00:38:44 +0100, > > Raghavendra Rao Ananta <rananta@google.com> wrote: > > > > > > Hello, > > > > > > Continuing the discussion from [1], the series tries to add support > > > for the userspace to elect the hypercall services that it wishes > > > to expose to the guest, rather than the guest discovering them > > > unconditionally. The idea employed by the series was taken from > > > [1] as suggested by Marc Z. > > > > As it took some time to get there, and that there was still a bunch of > > things to address, I've taken the liberty to apply my own fixes to the > > series. > > > > Please have a look at [1], and let me know if you're OK with the > > result. If you are, I'll merge the series for 5.19. > > > > Thanks, > > > > M. > > > Thank you for speeding up the process; appreciate it. However, the > series's selftest patches have a dependency on Oliver's > PSCI_SYSTEM_SUSPEND's selftest patches [1][2]. Can we pull them in > too? Urgh... I guess this is the time to set some ground rules: - Please don't introduce dependencies between series, that's unmanageable. I really need to see each series independently, and if there is a merge conflict, that's my job to fix (and I don't really mind). - If there is a dependency between series, please post a version of the required patches as a prefix to your series, assuming this prefix is itself standalone. If it isn't, then something really is wrong, and the series should be resplit. - You also should be basing your series on an *official* tag from Linus' tree (preferably -rc1, -rc2 or -rc3), and not something random like any odd commit from the KVM tree (I had conflicts while applying this on -rc3, probably due to the non-advertised dependency on Oliver's series). > > aarch64/hypercalls.c: In function ‘guest_test_hvc’: > aarch64/hypercalls.c:95:30: error: storage size of ‘res’ isn’t known > 95 | struct arm_smccc_res res; > | ^~~ > aarch64/hypercalls.c:103:17: warning: implicit declaration of function > ‘smccc_hvc’ [-Wimplicit-function-declaration] > 103 | smccc_hvc(hc_info->func_id, hc_info->arg1, 0, > 0, 0, 0, 0, 0, &res); > | ^~~~~~~~~ > I've picked the two patches, which means they will most likely appear twice in the history. In the future, please reach out so that we can organise this better. > Also, just a couple of readability nits in the fixed version: > > 1. Patch-2/9, hypercall.c:kvm_hvc_call_default_allowed(), in the > 'default' case, do you think we should probably add a small comment > that mentions we are checking for func_id in the PSCI range? Dumped a one-liner there. > 2. Patch-2/9, arm_hypercall.h, clear all the macros in this patch > itself instead of doing it in increments (unless there's some reason > that I'm missing)? Ah, rebasing leftovers, now gone. I've pushed an updated branch again, please have a look. M.
On Tue, May 3, 2022 at 1:33 PM Marc Zyngier <maz@kernel.org> wrote: > > On Tue, 03 May 2022 19:49:13 +0100, > Raghavendra Rao Ananta <rananta@google.com> wrote: > > > > Hi Marc, > > > > On Tue, May 3, 2022 at 10:24 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > On Tue, 03 May 2022 00:38:44 +0100, > > > Raghavendra Rao Ananta <rananta@google.com> wrote: > > > > > > > > Hello, > > > > > > > > Continuing the discussion from [1], the series tries to add support > > > > for the userspace to elect the hypercall services that it wishes > > > > to expose to the guest, rather than the guest discovering them > > > > unconditionally. The idea employed by the series was taken from > > > > [1] as suggested by Marc Z. > > > > > > As it took some time to get there, and that there was still a bunch of > > > things to address, I've taken the liberty to apply my own fixes to the > > > series. > > > > > > Please have a look at [1], and let me know if you're OK with the > > > result. If you are, I'll merge the series for 5.19. > > > > > > Thanks, > > > > > > M. > > > > > Thank you for speeding up the process; appreciate it. However, the > > series's selftest patches have a dependency on Oliver's > > PSCI_SYSTEM_SUSPEND's selftest patches [1][2]. Can we pull them in > > too? > > Urgh... I guess this is the time to set some ground rules: > > - Please don't introduce dependencies between series, that's > unmanageable. I really need to see each series independently, and if > there is a merge conflict, that's my job to fix (and I don't really > mind). > > - If there is a dependency between series, please post a version of > the required patches as a prefix to your series, assuming this > prefix is itself standalone. If it isn't, then something really is > wrong, and the series should be resplit. > > - You also should be basing your series on an *official* tag from > Linus' tree (preferably -rc1, -rc2 or -rc3), and not something > random like any odd commit from the KVM tree (I had conflicts while > applying this on -rc3, probably due to the non-advertised dependency > on Oliver's series). > Thanks for picking the dependency patches. I'll keep these mind the next time I push changes. > > > > aarch64/hypercalls.c: In function ‘guest_test_hvc’: > > aarch64/hypercalls.c:95:30: error: storage size of ‘res’ isn’t known > > 95 | struct arm_smccc_res res; > > | ^~~ > > aarch64/hypercalls.c:103:17: warning: implicit declaration of function > > ‘smccc_hvc’ [-Wimplicit-function-declaration] > > 103 | smccc_hvc(hc_info->func_id, hc_info->arg1, 0, > > 0, 0, 0, 0, 0, &res); > > | ^~~~~~~~~ > > > > I've picked the two patches, which means they will most likely appear > twice in the history. In the future, please reach out so that we can > organise this better. > > > Also, just a couple of readability nits in the fixed version: > > > > 1. Patch-2/9, hypercall.c:kvm_hvc_call_default_allowed(), in the > > 'default' case, do you think we should probably add a small comment > > that mentions we are checking for func_id in the PSCI range? > > Dumped a one-liner there. > > > 2. Patch-2/9, arm_hypercall.h, clear all the macros in this patch > > itself instead of doing it in increments (unless there's some reason > > that I'm missing)? > > Ah, rebasing leftovers, now gone. > > I've pushed an updated branch again, please have a look. > Thanks for addressing these. The series looks good now. Regards, Raghavendra > M. > > -- > Without deviation from the norm, progress is not possible.
On Tue, May 03, 2022 at 09:33:40PM +0100, Marc Zyngier wrote: > On Tue, 03 May 2022 19:49:13 +0100, > Raghavendra Rao Ananta <rananta@google.com> wrote: > > > > Hi Marc, > > > > On Tue, May 3, 2022 at 10:24 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > On Tue, 03 May 2022 00:38:44 +0100, > > > Raghavendra Rao Ananta <rananta@google.com> wrote: > > > > > > > > Hello, > > > > > > > > Continuing the discussion from [1], the series tries to add support > > > > for the userspace to elect the hypercall services that it wishes > > > > to expose to the guest, rather than the guest discovering them > > > > unconditionally. The idea employed by the series was taken from > > > > [1] as suggested by Marc Z. > > > > > > As it took some time to get there, and that there was still a bunch of > > > things to address, I've taken the liberty to apply my own fixes to the > > > series. > > > > > > Please have a look at [1], and let me know if you're OK with the > > > result. If you are, I'll merge the series for 5.19. > > > > > > Thanks, > > > > > > M. > > > > > Thank you for speeding up the process; appreciate it. However, the > > series's selftest patches have a dependency on Oliver's > > PSCI_SYSTEM_SUSPEND's selftest patches [1][2]. Can we pull them in > > too? Posted, BTW. http://lore.kernel.org/kvmarm/20220504032446.4133305-1-oupton@google.com > > 2. Patch-2/9, arm_hypercall.h, clear all the macros in this patch > > itself instead of doing it in increments (unless there's some reason > > that I'm missing)? > > Ah, rebasing leftovers, now gone. > > I've pushed an updated branch again, please have a look. Series looks good with your additions. For the pile: Reviewed-by: Oliver Upton <oupton@google.com>
On Mon, 2 May 2022 23:38:44 +0000, Raghavendra Rao Ananta wrote: > Continuing the discussion from [1], the series tries to add support > for the userspace to elect the hypercall services that it wishes > to expose to the guest, rather than the guest discovering them > unconditionally. The idea employed by the series was taken from > [1] as suggested by Marc Z. > > In a broad sense, the concept is similar to the current implementation > of PSCI interface- create a 'firmware psuedo-register' to handle the > firmware revisions. The series extends this idea to all the other > hypercalls such as TRNG (True Random Number Generator), PV_TIME > (Paravirtualized Time), and PTP (Precision Time protocol). > > [...] Applied to next, thanks! [1/9] KVM: arm64: Factor out firmware register handling from psci.c commit: 85fbe08e4da862dc64fc10071c4a03e51b6361d0 [2/9] KVM: arm64: Setup a framework for hypercall bitmap firmware registers commit: 05714cab7d63b189894235cf310fae7d6ffc2e9b [3/9] KVM: arm64: Add standard hypervisor firmware register commit: 428fd6788d4d0e0d390de9fb4486be3c1187310d [4/9] KVM: arm64: Add vendor hypervisor firmware register commit: b22216e1a617ca55b41337cd1e057ebc784a65d4 [5/9] Docs: KVM: Rename psci.rst to hypercalls.rst commit: f1ced23a9be5727c6f4cac0e2262c5411038952f [6/9] Docs: KVM: Add doc for the bitmap firmware registers commit: fa246c68a04d46c7af6953b47dba7e16d24efbe2 [7/9] tools: Import ARM SMCCC definitions commit: ea733263949646700977feeb662a92703f514351 [8/9] selftests: KVM: aarch64: Introduce hypercall ABI test commit: 5ca24697d54027c1c94c94a5b920a75448108ed0 [9/9] selftests: KVM: aarch64: Add the bitmap firmware registers to get-reg-list commit: 920f4a55fdaa6f68b31c50cca6e51fecac5857a0 Cheers, M.
On Tue, 03 May 2022 22:09:29 +0100, Raghavendra Rao Ananta <rananta@google.com> wrote: > > On Tue, May 3, 2022 at 1:33 PM Marc Zyngier <maz@kernel.org> wrote: > > > > On Tue, 03 May 2022 19:49:13 +0100, > > Raghavendra Rao Ananta <rananta@google.com> wrote: > > > > > > Hi Marc, > > > > > > On Tue, May 3, 2022 at 10:24 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > On Tue, 03 May 2022 00:38:44 +0100, > > > > Raghavendra Rao Ananta <rananta@google.com> wrote: > > > > > > > > > > Hello, > > > > > > > > > > Continuing the discussion from [1], the series tries to add support > > > > > for the userspace to elect the hypercall services that it wishes > > > > > to expose to the guest, rather than the guest discovering them > > > > > unconditionally. The idea employed by the series was taken from > > > > > [1] as suggested by Marc Z. > > > > > > > > As it took some time to get there, and that there was still a bunch of > > > > things to address, I've taken the liberty to apply my own fixes to the > > > > series. > > > > > > > > Please have a look at [1], and let me know if you're OK with the > > > > result. If you are, I'll merge the series for 5.19. > > > > > > > > Thanks, > > > > > > > > M. > > > > > > > Thank you for speeding up the process; appreciate it. However, the > > > series's selftest patches have a dependency on Oliver's > > > PSCI_SYSTEM_SUSPEND's selftest patches [1][2]. Can we pull them in > > > too? > > > > Urgh... I guess this is the time to set some ground rules: > > > > - Please don't introduce dependencies between series, that's > > unmanageable. I really need to see each series independently, and if > > there is a merge conflict, that's my job to fix (and I don't really > > mind). > > > > - If there is a dependency between series, please post a version of > > the required patches as a prefix to your series, assuming this > > prefix is itself standalone. If it isn't, then something really is > > wrong, and the series should be resplit. > > > > - You also should be basing your series on an *official* tag from > > Linus' tree (preferably -rc1, -rc2 or -rc3), and not something > > random like any odd commit from the KVM tree (I had conflicts while > > applying this on -rc3, probably due to the non-advertised dependency > > on Oliver's series). > > > Thanks for picking the dependency patches. I'll keep these mind the > next time I push changes. > > > > > > > aarch64/hypercalls.c: In function ‘guest_test_hvc’: > > > aarch64/hypercalls.c:95:30: error: storage size of ‘res’ isn’t known > > > 95 | struct arm_smccc_res res; > > > | ^~~ > > > aarch64/hypercalls.c:103:17: warning: implicit declaration of function > > > ‘smccc_hvc’ [-Wimplicit-function-declaration] > > > 103 | smccc_hvc(hc_info->func_id, hc_info->arg1, 0, > > > 0, 0, 0, 0, 0, &res); > > > | ^~~~~~~~~ > > > > > > > I've picked the two patches, which means they will most likely appear > > twice in the history. In the future, please reach out so that we can > > organise this better. > > > > > Also, just a couple of readability nits in the fixed version: > > > > > > 1. Patch-2/9, hypercall.c:kvm_hvc_call_default_allowed(), in the > > > 'default' case, do you think we should probably add a small comment > > > that mentions we are checking for func_id in the PSCI range? > > > > Dumped a one-liner there. > > > > > 2. Patch-2/9, arm_hypercall.h, clear all the macros in this patch > > > itself instead of doing it in increments (unless there's some reason > > > that I'm missing)? > > > > Ah, rebasing leftovers, now gone. > > > > I've pushed an updated branch again, please have a look. > > > Thanks for addressing these. The series looks good now. Except it doesn't. I introduced a bug by overly simplifying kvm_arm_set_fw_reg_bmap(), as we have to allow userspace writing the *same* value. As it turns out, QEMU restores all the registers on each reboot. Which as the vcpus have all run. This in turns triggers another issue in QEMU, which instead of taking the hint ans stopping there, sends all the vcpus into the guest in one go with random states... Crap happens. I'll wear a brown paper bag for the rest of the day and add the following patch to the branch. Thanks, M. From 528ada2811ba0bb2b2db5bf0f829b48c50f3c13c Mon Sep 17 00:00:00 2001 From: Marc Zyngier <maz@kernel.org> Date: Mon, 16 May 2022 17:32:54 +0100 Subject: [PATCH] KVM: arm64: Fix hypercall bitmap writeback when vcpus have already run We generally want to disallow hypercall bitmaps being changed once vcpus have already run. But we must allow the write if the written value is unchanged so that userspace can rewrite the register file on reboot, for example. Without this, a QEMU-based VM will fail to reboot correctly. The original code was correct, and it is me that introduced the regression. Fixes: 05714cab7d63 ("KVM: arm64: Setup a framework for hypercall bitmap firmware registers") Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/kvm/hypercalls.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c index ccbd3cefb91a..c9f401fa01a9 100644 --- a/arch/arm64/kvm/hypercalls.c +++ b/arch/arm64/kvm/hypercalls.c @@ -379,7 +379,8 @@ static int kvm_arm_set_fw_reg_bmap(struct kvm_vcpu *vcpu, u64 reg_id, u64 val) mutex_lock(&kvm->lock); - if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &kvm->arch.flags)) { + if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &kvm->arch.flags) && + val != *fw_reg_bmap) { ret = -EBUSY; goto out; }
On Mon, May 16, 2022 at 9:44 AM Marc Zyngier <maz@kernel.org> wrote: > > On Tue, 03 May 2022 22:09:29 +0100, > Raghavendra Rao Ananta <rananta@google.com> wrote: > > > > On Tue, May 3, 2022 at 1:33 PM Marc Zyngier <maz@kernel.org> wrote: > > > > > > On Tue, 03 May 2022 19:49:13 +0100, > > > Raghavendra Rao Ananta <rananta@google.com> wrote: > > > > > > > > Hi Marc, > > > > > > > > On Tue, May 3, 2022 at 10:24 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > > > On Tue, 03 May 2022 00:38:44 +0100, > > > > > Raghavendra Rao Ananta <rananta@google.com> wrote: > > > > > > > > > > > > Hello, > > > > > > > > > > > > Continuing the discussion from [1], the series tries to add support > > > > > > for the userspace to elect the hypercall services that it wishes > > > > > > to expose to the guest, rather than the guest discovering them > > > > > > unconditionally. The idea employed by the series was taken from > > > > > > [1] as suggested by Marc Z. > > > > > > > > > > As it took some time to get there, and that there was still a bunch of > > > > > things to address, I've taken the liberty to apply my own fixes to the > > > > > series. > > > > > > > > > > Please have a look at [1], and let me know if you're OK with the > > > > > result. If you are, I'll merge the series for 5.19. > > > > > > > > > > Thanks, > > > > > > > > > > M. > > > > > > > > > Thank you for speeding up the process; appreciate it. However, the > > > > series's selftest patches have a dependency on Oliver's > > > > PSCI_SYSTEM_SUSPEND's selftest patches [1][2]. Can we pull them in > > > > too? > > > > > > Urgh... I guess this is the time to set some ground rules: > > > > > > - Please don't introduce dependencies between series, that's > > > unmanageable. I really need to see each series independently, and if > > > there is a merge conflict, that's my job to fix (and I don't really > > > mind). > > > > > > - If there is a dependency between series, please post a version of > > > the required patches as a prefix to your series, assuming this > > > prefix is itself standalone. If it isn't, then something really is > > > wrong, and the series should be resplit. > > > > > > - You also should be basing your series on an *official* tag from > > > Linus' tree (preferably -rc1, -rc2 or -rc3), and not something > > > random like any odd commit from the KVM tree (I had conflicts while > > > applying this on -rc3, probably due to the non-advertised dependency > > > on Oliver's series). > > > > > Thanks for picking the dependency patches. I'll keep these mind the > > next time I push changes. > > > > > > > > > > aarch64/hypercalls.c: In function ‘guest_test_hvc’: > > > > aarch64/hypercalls.c:95:30: error: storage size of ‘res’ isn’t known > > > > 95 | struct arm_smccc_res res; > > > > | ^~~ > > > > aarch64/hypercalls.c:103:17: warning: implicit declaration of function > > > > ‘smccc_hvc’ [-Wimplicit-function-declaration] > > > > 103 | smccc_hvc(hc_info->func_id, hc_info->arg1, 0, > > > > 0, 0, 0, 0, 0, &res); > > > > | ^~~~~~~~~ > > > > > > > > > > I've picked the two patches, which means they will most likely appear > > > twice in the history. In the future, please reach out so that we can > > > organise this better. > > > > > > > Also, just a couple of readability nits in the fixed version: > > > > > > > > 1. Patch-2/9, hypercall.c:kvm_hvc_call_default_allowed(), in the > > > > 'default' case, do you think we should probably add a small comment > > > > that mentions we are checking for func_id in the PSCI range? > > > > > > Dumped a one-liner there. > > > > > > > 2. Patch-2/9, arm_hypercall.h, clear all the macros in this patch > > > > itself instead of doing it in increments (unless there's some reason > > > > that I'm missing)? > > > > > > Ah, rebasing leftovers, now gone. > > > > > > I've pushed an updated branch again, please have a look. > > > > > Thanks for addressing these. The series looks good now. > > Except it doesn't. > > I introduced a bug by overly simplifying kvm_arm_set_fw_reg_bmap(), as > we have to allow userspace writing the *same* value. As it turns out, > QEMU restores all the registers on each reboot. Which as the vcpus > have all run. This in turns triggers another issue in QEMU, which > instead of taking the hint ans stopping there, sends all the vcpus > into the guest in one go with random states... Crap happens. > > I'll wear a brown paper bag for the rest of the day and add the > following patch to the branch. > > Thanks, > > M. > > From 528ada2811ba0bb2b2db5bf0f829b48c50f3c13c Mon Sep 17 00:00:00 2001 > From: Marc Zyngier <maz@kernel.org> > Date: Mon, 16 May 2022 17:32:54 +0100 > Subject: [PATCH] KVM: arm64: Fix hypercall bitmap writeback when vcpus have > already run > > We generally want to disallow hypercall bitmaps being changed > once vcpus have already run. But we must allow the write if > the written value is unchanged so that userspace can rewrite > the register file on reboot, for example. > > Without this, a QEMU-based VM will fail to reboot correctly. > > The original code was correct, and it is me that introduced > the regression. > > Fixes: 05714cab7d63 ("KVM: arm64: Setup a framework for hypercall bitmap firmware registers") > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/kvm/hypercalls.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c > index ccbd3cefb91a..c9f401fa01a9 100644 > --- a/arch/arm64/kvm/hypercalls.c > +++ b/arch/arm64/kvm/hypercalls.c > @@ -379,7 +379,8 @@ static int kvm_arm_set_fw_reg_bmap(struct kvm_vcpu *vcpu, u64 reg_id, u64 val) > > mutex_lock(&kvm->lock); > > - if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &kvm->arch.flags)) { > + if (test_bit(KVM_ARCH_FLAG_HAS_RAN_ONCE, &kvm->arch.flags) && > + val != *fw_reg_bmap) { > ret = -EBUSY; > goto out; > } Ha, not your fault. We have been going back and forth on this aspect of the design. Thanks for correcting this. However, since this changes the API behavior, I think we may have to amend the documentation as well: diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst index b5ccec4572d7..ab04979bf104 100644 --- a/Documentation/virt/kvm/api.rst +++ b/Documentation/virt/kvm/api.rst @@ -2615,7 +2615,8 @@ KVM_SET_ONE_REG. Note: These registers are immutable once any of the vCPUs of the VM has run at least once. A KVM_SET_ONE_REG in such a scenario will return -a -EBUSY to userspace. +a -EBUSY to userspace if there's an update in the bitmap. If there's no +change in the value, it'll simply return a 0. (See Documentation/virt/kvm/arm/hypercalls.rst for more details.) diff --git a/Documentation/virt/kvm/arm/hypercalls.rst b/Documentation/virt/kvm/arm/hypercalls.rst index 3e23084644ba..275f4dbe5792 100644 --- a/Documentation/virt/kvm/arm/hypercalls.rst +++ b/Documentation/virt/kvm/arm/hypercalls.rst @@ -91,9 +91,10 @@ desired bitmap back via SET_ONE_REG. The features for the registers that are untouched, probably because userspace isn't aware of them, will be exposed as is to the guest. -Note that KVM will not allow the userspace to configure the registers -anymore once any of the vCPUs has run at least once. Instead, it will -return a -EBUSY. +Note that KVM will not allow the userspace to update the registers +with a new value anymore once any of the vCPUs has run at least once, +and will return a -EBUSY. However, a 'write' of the previously configured +value is allowed and returns a 0. The pseudo-firmware bitmap register are as follows: @@ -129,10 +130,10 @@ The pseudo-firmware bitmap register are as follows: Errors: - ======= ============================================================= + ======= =============================================================== -ENOENT Unknown register accessed. - -EBUSY Attempt a 'write' to the register after the VM has started. + -EBUSY Attempt to update the register bitmap after the VM has started. -EINVAL Invalid bitmap written to the register. - ======= ============================================================= + ======= ================================================================ .. [1] https://developer.arm.com/-/media/developer/pdf/ARM_DEN_0070A_Firmware_interfaces_for_mitigating_CVE-2017-5715.pdf Thank you. Raghavendra > -- > 2.34.1 > > > -- > Without deviation from the norm, progress is not possible.
Hello, Continuing the discussion from [1], the series tries to add support for the userspace to elect the hypercall services that it wishes to expose to the guest, rather than the guest discovering them unconditionally. The idea employed by the series was taken from [1] as suggested by Marc Z. In a broad sense, the concept is similar to the current implementation of PSCI interface- create a 'firmware psuedo-register' to handle the firmware revisions. The series extends this idea to all the other hypercalls such as TRNG (True Random Number Generator), PV_TIME (Paravirtualized Time), and PTP (Precision Time protocol). For better categorization and future scaling, these firmware registers are categorized based on the service call owners. Also, unlike the existing firmware psuedo-registers, they hold the features supported in the form of a bitmap. During the VM initialization, the registers holds an upper-limit of the features supported by each one of them. It's expected that the userspace discover the features provided by each register via GET_ONE_REG, and writeback the desired values using SET_ONE_REG. KVM allows this modification only until the VM has started. Some of the standard function-ids, such as ARM_SMCCC_VERSION_FUNC_ID, need not be associated with a feature bit. For such ids, the series introduced an allowed-list (in kvm_hvc_call_default_allowed()), that holds all such ids. As a result, the functions that are not elected by userspace, or if they are not a part of this allowed-list, will be denied for when the guests invoke them. Older VMMs can simply ignore this interface and the hypercall services will be exposed unconditionally to the guests, thus ensuring backward compatibility. The patches are based off of mainline kernel 5.18-rc5, with the selftest patches from [2] applied. Patch-1 factors out the non-PSCI related interface from psci.c to hypercalls.c, as the series would extend the list in the upcoming patches. Patch-2 sets up the framework for the bitmap firmware psuedo-registers. It includes read/write support for the registers, and a helper to check if a particular hypercall service is supported for the guest. It also adds the register KVM_REG_ARM_STD_HYP_BMAP to support ARM's standard secure services. Patch-3 introduces the firmware register, KVM_REG_ARM_STD_HYP_BMAP, which holds the standard hypervisor services (such as PV_TIME). Patch-4 introduces the firmware register, KVM_REG_ARM_VENDOR_HYP_BMAP, which holds the vendor specific hypercall services. Patch-5,6 Add the necessary documentation for the newly added firmware registers. Patch-7 imports the SMCCC definitions from linux/arm-smccc.h into tools/ for further use in selftests. Patch-8 adds the selftest to test the guest (using 'hvc') and userspace interfaces (SET/GET_ONE_REG). Patch-9 adds these firmware registers into the get-reg-list selftest. [1]: https://lore.kernel.org/kvmarm/874kbcpmlq.wl-maz@kernel.org/T/ [2]: https://lore.kernel.org/all/20220409184549.1681189-1-oupton@google.com/ Regards, Raghavendra v6 -> v7: Addressed the comments by Gavin and Reiji: - kvm_arm_set_fw_reg_bmap() is optimzed to avoid unnecessary serialization of kvm->lock by checking for the KVM_ARCH_FLAG_HAS_RAN_ONCE flag or for a bitmap update before taking the lock (Reiji). - kvm_psci_func_id_is_valid() avoids depending on *vcpu to figure out if the PSCI version is 0.1. Instead, it checks the same using KVM_PSCI_FN() for range 0 to 3 (Reiji). - Fixed typos and comments (Gavin). v5 -> v6: Addressed the comments by Marc and Gavin: - Bitmaps are represented using 'unsigned long' inctead of 'u64' (Marc). - Replaced the array holding the allowed-list, hvc_func_default_allowed_list[], which looked up the func_id using a loop, with a switch-case statement (Marc). - kvm_arm_set_fw_reg_bmap() now always returns -EBUSY for any 'write' of the bitmap value after the VM has started running. Documentation is adjusted accordingly (Marc). - kvm_psci_func_id_is_valid() is moved from an inline function to kvm/psci.c (Marc). - Merged ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID into bit-0 of the vendor hypervisor firmware register (Gavin). - Macro optimizations and replace arg0 with arg1 (to comply with KVM convention) in hypercalls.c selftest (Gavin). - Dropped the patch v5 10/10 (Add KVM_REG_ARM_FW_REG(3) to get-reg-list) as it was already uploaded by Andrew. - Fixed typos v4 -> v5: Addressed comments by Oliver (thank you!): - Rebased the series to accommodate ARM_SMCCC_ARCH_WORKAROUND_3 and PSCI 1.1 changes, and capturing VM's first run. - Removed the patches related to register scoping (v4 02/13 and 03/13). I plan to re-introduce them in its own series. - Dropped the patch that captures VM's first run. - Moved the bitmap feature firmware registers to its own CORPOC space (0x0016). - Move the KVM_REG_ARM_*_BIT_MAX definitions from uapi header to internal header (arm_hypercalls.h). - Renamed the hypercall descriptor to 'struct kvm_smccc_features', and kvm_hvc_call_supported() to kvm_hvc_call_allowed(). - Introduced an allowed-list to hold the function-ids that aren't represented by feature-bits. - Introduced kvm_psci_func_id_is_valid() to check if a given function-id is a valid PSCI id, which is used in kvm_hvc_call_allowed(). - Introduced KVM_REG_ARM_VENDOR_HYP_BIT_FUNC_FEAT as bit-0 of KVM_REG_ARM_VENDOR_HYP_BMAP register and KVM_REG_ARM_VENDOR_HYP_BIT_PTP is moved to bit-1. - Updated the arm-smccc.h import to include the definition of ARM_SMCCC_ARCH_WORKAROUND_3. - Introduced the KVM_REG_ARM_FW_FEAT_BMAP COPROC definition to get-reg-list selftest. - Created a new patch to include KVM_REG_ARM_FW_REG(3) in get-reg-list. v3 -> v4 Addressed comments and took suggestions by Reiji, Oliver, Marc, Sean and Jim: - Renamed and moved the VM has run once check to arm64. - Introduced the capability to dynamically modify the register encodings to include the scope information. - Replaced mutex_lock with READ_ONCE and WRITE_ONCE when the bitmaps are accessed. - The hypercalls selftest re-runs with KVM_CAP_ARM_REG_SCOPE enabled. v2 -> v3 Addressed comments by Marc and Andrew: - Dropped kvm_vcpu_has_run_once() implementation. - Redifined kvm_vm_has_run_once() as kvm_vm_has_started() in the core KVM code that introduces a new field, 'vm_started', to track this. - KVM_CAP_ARM_HVC_FW_REG_BMAP returns the number of psuedo-firmware bitmap registers upon a 'read'. Support for 'write' removed. - Removed redundant spinlock, 'fw_reg_bmap_enabled' fields from the hypercall descriptor structure. - A separate sub-struct to hold the bitmap info is removed. The bitmap info is directly stored in the hypercall descriptor structure (struct kvm_hvc_desc). v1 -> v2 Addressed comments by Oliver (thanks!): - Introduced kvm_vcpu_has_run_once() and kvm_vm_has_run_once() in the core kvm code, rather than relying on ARM specific vcpu->arch.has_run_once. - Writing to KVM_REG_ARM_PSCI_VERSION is done in hypercalls.c itself, rather than separating out to psci.c. - Introduced KVM_CAP_ARM_HVC_FW_REG_BMAP to enable the extension. - Tracks the register accesses from VMM to decide whether to sanitize a register or not, as opposed to sanitizing upon the first 'write' in v1. - kvm_hvc_call_supported() is implemented using a direct switch-case statement, instead of looping over all the registers to pick the register for the function-id. - Replaced the register bit definitions with #defines, instead of enums. - Removed the patch v1-06/08 that imports the firmware register definitions as it's not needed. - Separated out the documentations in its own patch, and the renaming of hypercalls.rst to psci.rst into another patch. - Add the new firmware registers to get-reg-list KVM selftest. v1: https://lore.kernel.org/kvmarm/20211102002203.1046069-1-rananta@google.com/ v2: https://lore.kernel.org/kvmarm/20211113012234.1443009-1-rananta@google.com/ v3: https://lore.kernel.org/linux-arm-kernel/20220104194918.373612-1-rananta@google.com/ v4: https://lore.kernel.org/lkml/20220224172559.4170192-1-rananta@google.com/ v5: https://lore.kernel.org/lkml/20220407011605.1966778-1-rananta@google.com/ v6: https://lore.kernel.org/kvmarm/20220423000328.2103733-1-rananta@google.com/ Raghavendra Rao Ananta (9): KVM: arm64: Factor out firmware register handling from psci.c KVM: arm64: Setup a framework for hypercall bitmap firmware registers KVM: arm64: Add standard hypervisor firmware register KVM: arm64: Add vendor hypervisor firmware register Docs: KVM: Rename psci.rst to hypercalls.rst Docs: KVM: Add doc for the bitmap firmware registers tools: Import ARM SMCCC definitions selftests: KVM: aarch64: Introduce hypercall ABI test selftests: KVM: aarch64: Add the bitmap firmware registers to get-reg-list Documentation/virt/kvm/api.rst | 16 + Documentation/virt/kvm/arm/hypercalls.rst | 135 +++++++ Documentation/virt/kvm/arm/psci.rst | 77 ---- arch/arm64/include/asm/kvm_host.h | 16 + arch/arm64/include/uapi/asm/kvm.h | 16 + arch/arm64/kvm/arm.c | 1 + arch/arm64/kvm/guest.c | 10 +- arch/arm64/kvm/hypercalls.c | 325 ++++++++++++++++- arch/arm64/kvm/psci.c | 186 +--------- include/kvm/arm_hypercalls.h | 17 + include/kvm/arm_psci.h | 9 +- tools/include/linux/arm-smccc.h | 193 ++++++++++ tools/testing/selftests/kvm/.gitignore | 1 + tools/testing/selftests/kvm/Makefile | 1 + .../selftests/kvm/aarch64/get-reg-list.c | 8 + .../selftests/kvm/aarch64/hypercalls.c | 336 ++++++++++++++++++ 16 files changed, 1078 insertions(+), 269 deletions(-) create mode 100644 Documentation/virt/kvm/arm/hypercalls.rst delete mode 100644 Documentation/virt/kvm/arm/psci.rst create mode 100644 tools/include/linux/arm-smccc.h create mode 100644 tools/testing/selftests/kvm/aarch64/hypercalls.c