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 wrote:
>
> On Tue, May 3, 2022 at 1:33 PM Marc Zyngier wrote:
> >
> > On Tue, 03 May 2022 19:49:13 +0100,
> > Raghavendra Rao Ananta wrote:
> > >
> > > Hi Marc,
> > >
> > > On Tue, May 3, 2022 at 10:24 AM Marc Zyngier wrote:
> > > >
> > > > On Tue, 03 May 2022 00:38:44 +0100,
> > > > Raghavendra Rao Ananta 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
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
---
arch/arm64/kvm/hypercalls.c | 3 ++-
1 file
Re: [PATCH v7 0/9] KVM: arm64: Add support for hypercall services selection
On Mon, 2 May 2022 23:38:44 +, 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. -- Without deviation from the norm, progress is not possible. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v7 0/9] KVM: arm64: Add support for hypercall services selection
On Tue, 03 May 2022 19:49:13 +0100, Raghavendra Rao Ananta wrote: > > Hi Marc, > > On Tue, May 3, 2022 at 10:24 AM Marc Zyngier wrote: > > > > On Tue, 03 May 2022 00:38:44 +0100, > > Raghavendra Rao Ananta 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. -- Without deviation from the norm, progress is not possible. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v7 0/9] KVM: arm64: Add support for hypercall services selection
On Tue, 03 May 2022 00:38:44 +0100, Raghavendra Rao Ananta 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 -- Without deviation from the norm, progress is not possible. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
