Re: [PATCH v7 0/9] KVM: arm64: Add support for hypercall services selection

From: Raghavendra Rao Ananta
Date: Mon May 16 2022 - 14:31:43 EST

Next message: Matthew Rosato: "Re: [PATCH v7 17/22] vfio-pci/zdev: add open/close device hooks"
Previous message: Peter Zijlstra: "Re: [RFC PATCH v2 20/21] x86: Add support for CONFIG_CFI_CLANG"
In reply to: Marc Zyngier: "Re: [PATCH v7 0/9] KVM: arm64: Add support for hypercall services selection"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, May 16, 2022 at 9:44 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> On Tue, 03 May 2022 22:09:29 +0100,
> Raghavendra Rao Ananta <rananta@xxxxxxxxxx> wrote:
> >
> > On Tue, May 3, 2022 at 1:33 PM Marc Zyngier <maz@xxxxxxxxxx> wrote:
> > >
> > > On Tue, 03 May 2022 19:49:13 +0100,
> > > Raghavendra Rao Ananta <rananta@xxxxxxxxxx> wrote:
> > > >
> > > > Hi Marc,
> > > >
> > > > On Tue, May 3, 2022 at 10:24 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Tue, 03 May 2022 00:38:44 +0100,
> > > > > Raghavendra Rao Ananta <rananta@xxxxxxxxxx> 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@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> ---
> 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.

Next message: Matthew Rosato: "Re: [PATCH v7 17/22] vfio-pci/zdev: add open/close device hooks"
Previous message: Peter Zijlstra: "Re: [RFC PATCH v2 20/21] x86: Add support for CONFIG_CFI_CLANG"
In reply to: Marc Zyngier: "Re: [PATCH v7 0/9] KVM: arm64: Add support for hypercall services selection"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]