Message-ID: <87ilq55swj.wl-maz@kernel.org>
Date:   Mon, 16 May 2022 17:44:28 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     Raghavendra Rao Ananta <rananta@...gle.com>
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 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;
 	}
-- 
2.34.1


-- 
Without deviation from the norm, progress is not possible.

Powered by blists - more mailing lists