[+ Andrew for the selftest part.]
Hi Oliver,
On Wed, 18 Aug 2021 09:50:43 +0100,
Oliver Upton <oupton@xxxxxxxxxx> wrote:
>
> The CPU_ON PSCI call requires careful coordination between vCPUs in KVM,
> as it allows callers to send a payload (pc, context id) to another vCPU
> to start execution. There are a couple of races in the handling of
> CPU_ON:
>
> - KVM uses the kvm->lock to serialize the write-side of a vCPU's reset
> state. However, kvm_vcpu_reset() doesn't take the lock on the
> read-size, meaning the vCPU could be reset with interleaved state
> from two separate CPU_ON calls.
>
> - If a targeted vCPU never enters the guest again (say, the VMM was
> getting ready to migrate), then the reset payload is never actually
> folded in to the vCPU's registers. Despite this, the calling vCPU has
> already made the target runnable. Migrating the target vCPU at this
> time will result in execution from its old PC, not execution coming
> out of the reset state at the requested address.
>
> Patch 1 addresses the read-side race in KVM's CPU_ON implementation.
>
> Patch 2 fixes the KVM/VMM race by resetting a vCPU (if requested)
> whenever the VMM tries to read out its registers. Gross, but it avoids
> exposing the vcpu_reset_state structure through some other UAPI. That is
> undesirable, as we really are only trying to paper over the
> implementation details of PSCI in KVM.
>
> Patch 3 is unrelated, and is based on my own reading of the PSCI
> specification. In short, if you invoke PSCI_ON from AArch64, then you
> must set the Aff3 bits. This is impossible if you use the 32 bit
> function, since the arguments are only 32 bits. Just return
> INVALID_PARAMS to the guest in this case.
Overall, this looks pretty good, and I only have a few nits on the
first three patches. I'll let Andrew comment on the selftest, which
looks OK to me at least on the surface.
>
> This series cleanly applies to kvm-arm/next at the following commit:
>
> ae280335cdb5 ("Merge branch kvm-arm64/mmu/el2-tracking into kvmarm-master/next")
>
Another nit: In the future, please consider basing your series on a
stable tag (such as v5.14-rc4), as kvmarm/next is a bit of a moving
target (the individual commits are stable, but the merge commits
aren't). Basing something off -next should be reserved for the cases
where you are fixing something that is only broken there.
Thanks,
M.
> The series was tested with the included KVM selftest on an Ampere Mt.
> Jade system. Broken behavior was verified using the same test on
> kvm-arm/next, sans this series.
>
> Oliver Upton (4):
> KVM: arm64: Fix read-side race on updates to vcpu reset state
> KVM: arm64: Handle PSCI resets before userspace touches vCPU state
> KVM: arm64: Enforce reserved bits for PSCI target affinities
> selftests: KVM: Introduce psci_cpu_on_test
>
> arch/arm64/kvm/arm.c | 9 ++
> arch/arm64/kvm/psci.c | 20 ++-
> arch/arm64/kvm/reset.c | 16 ++-
> tools/testing/selftests/kvm/.gitignore | 1 +
> tools/testing/selftests/kvm/Makefile | 1 +
> .../selftests/kvm/aarch64/psci_cpu_on_test.c | 121 ++++++++++++++++++
> .../selftests/kvm/include/aarch64/processor.h | 3 +
> 7 files changed, 162 insertions(+), 9 deletions(-)
> create mode 100644 tools/testing/selftests/kvm/aarch64/psci_cpu_on_test.c
>
--
Without deviation from the norm, progress is not possible.
