Re: [PATCH 0/4] KVM: arm64: Fix some races in CPU_ON PSCI call

[Marc Zyngier <maz@kernel.org>]

[+ Andrew for the selftest part.]

Hi Oliver,

On Wed, 18 Aug 2021 09:50:43 +0100,
Oliver Upton <oupton@google.com> 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.