Message-ID: <CAJHc60zLt2ZtCR2=DgtS3XK8e7vQrocEO2rinW_hMb4o4-WhTg@mail.gmail.com>
Date:   Mon, 16 May 2022 11:30:48 -0700
From:   Raghavendra Rao Ananta <rananta@...gle.com>
To:     Marc Zyngier <maz@...nel.org>
Cc:     Andrew Jones <drjones@...hat.com>,
        James Morse <james.morse@....com>,
        Alexandru Elisei <alexandru.elisei@....com>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>, Peter Shier <pshier@...gle.com>,
        Ricardo Koller <ricarkol@...gle.com>,
        Oliver Upton <oupton@...gle.com>,
        Reiji Watanabe <reijiw@...gle.com>,
        Jing Zhang <jingzhangos@...gle.com>,
        linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.cs.columbia.edu,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [PATCH v7 0/9] KVM: arm64: Add support for hypercall services selection

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

Powered by blists - more mailing lists