Re: [PATCH v2 07/16] arm/arm64: KVM: Add PSCI version selection API

[Andrew Jones]

On Mon, Jan 29, 2018 at 05:45:50PM +0000, Marc Zyngier wrote:
> Although we've implemented PSCI 1.0 and 1.1, nothing can select them
> Since all the new PSCI versions are backward compatible, we decide to
> default to the latest version of the PSCI implementation. This is no
> different from doing a firmware upgrade on KVM.
> 
> But in order to give a chance to hypothetical badly implemented guests
> that would have a fit by discovering something other than PSCI 0.2,
> let's provide a new API that allows userspace to pick one particular
> version of the API.
> 
> This is implemented as a new class of "firmware" registers, where
> we expose the PSCI version. This allows the PSCI version to be
> save/restored as part of a guest migration, and also set to
> any supported version if the guest requires it.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> ---
>  Documentation/virtual/kvm/api.txt      |  3 +-
>  Documentation/virtual/kvm/arm/psci.txt | 28 ++++++++++++++
>  arch/arm/include/asm/kvm_host.h        |  3 ++
>  arch/arm/include/uapi/asm/kvm.h        |  6 +++
>  arch/arm/kvm/guest.c                   | 13 +++++++
>  arch/arm64/include/asm/kvm_host.h      |  3 ++
>  arch/arm64/include/uapi/asm/kvm.h      |  6 +++
>  arch/arm64/kvm/guest.c                 | 14 ++++++-
>  include/kvm/arm_psci.h                 |  9 +++++
>  virt/kvm/arm/psci.c                    | 68 +++++++++++++++++++++++++++++++++-
>  10 files changed, 149 insertions(+), 4 deletions(-)
>  create mode 100644 Documentation/virtual/kvm/arm/psci.txt
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 57d3ee9e4bde..334905202141 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2493,7 +2493,8 @@ Possible features:
>  	  and execute guest code when KVM_RUN is called.
>  	- KVM_ARM_VCPU_EL1_32BIT: Starts the CPU in a 32bit mode.
>  	  Depends on KVM_CAP_ARM_EL1_32BIT (arm64 only).
> -	- KVM_ARM_VCPU_PSCI_0_2: Emulate PSCI v0.2 for the CPU.
> +	- KVM_ARM_VCPU_PSCI_0_2: Emulate PSCI v0.2 (or a future revision
> +          backward compatible with v0.2) for the CPU.
>  	  Depends on KVM_CAP_ARM_PSCI_0_2.
>  	- KVM_ARM_VCPU_PMU_V3: Emulate PMUv3 for the CPU.
>  	  Depends on KVM_CAP_ARM_PMU_V3.
> diff --git a/Documentation/virtual/kvm/arm/psci.txt b/Documentation/virtual/kvm/arm/psci.txt
> new file mode 100644
> index 000000000000..2e49a4e9f084
> --- /dev/null
> +++ b/Documentation/virtual/kvm/arm/psci.txt
> @@ -0,0 +1,28 @@
> +KVM implements the PSCI (Power State Coordination Interface)
> +specification in order to provide services such as CPU on/off, reset
> +and power-off to the guest.
> +
> +The PSCI specification is regularly updated to provide new features,
> +and KVM implements these updates if they make sense from a virtualization
> +point of view.
> +
> +This means that a guest booted on two different versions of KVM can
> +observe two different "firmware" revisions. This could cause issues if
> +a given guest is tied to a particular PSCI revision (unlikely), or if
> +a migration causes a different PSCI version to be exposed out of the
> +blue to an unsuspecting guest.
> +
> +In order to remedy this situation, KVM exposes a set of "firmware
> +pseuodo-registers" that can be manipulated using the GET/SET_ONE_REG
> +interface. These registers can be saved/restored by userspace, and set
> +to a convenient value if required.

Userspace can save/restore any state it deems necessary. Is there another
reason to invent a pseudo register? Considering the PSCI version is VM
state, then maybe a VM "device" attribute[*], like s390 use, would fit
better. Or, if keeping it VCPU state has some benefit, then we already
have VCPU device attribute support for ARM that we could extend with
another attribute. An advantage of using the device-attr API is that it
has KVM_HAS_DEVICE_ATTR, allowing the new attribute support to be probed.
How should userspace check if the pseudo register is supported? Maybe
by just failing with NOT_SUPPORTED to get/set it?

[*] Documentation/virtual/kvm/devices/vm.txt

> +
> +The following register is defined:
> +
> +* KVM_REG_ARM_PSCI_VERSION:
> +
> +  - Only valid if the vcpu has KVM_ARM_VCPU_PSCI_0_2 feature set
> +  - Returns the current PSCI version on GET_ONE_REG
> +  - Allows any supported PSCI version compatible with v0.2 to be set
> +    with SET_ONE_REG
> +  - Affects the whole VM (even if the register view is per-vcpu)


> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index acbf9ec7b396..e9d57060d88c 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -75,6 +75,9 @@ struct kvm_arch {
>  	/* Interrupt controller */
>  	struct vgic_dist	vgic;
>  	int max_vcpus;
> +
> +	/* Mandated version of PSCI */
> +	u32 psci_version;
>  };
>  
>  #define KVM_NR_MEM_OBJS     40
> diff --git a/arch/arm/include/uapi/asm/kvm.h b/arch/arm/include/uapi/asm/kvm.h
> index 6edd177bb1c7..47dfc99f5cd0 100644
> --- a/arch/arm/include/uapi/asm/kvm.h
> +++ b/arch/arm/include/uapi/asm/kvm.h
> @@ -186,6 +186,12 @@ struct kvm_arch_memory_slot {
>  #define KVM_REG_ARM_VFP_FPINST		0x1009
>  #define KVM_REG_ARM_VFP_FPINST2		0x100A
>  
> +/* KVM-as-firmware specific pseudo-registers */
> +#define KVM_REG_ARM_FW			(0x0014 << KVM_REG_ARM_COPROC_SHIFT)

Is this 0x14 documented somewhere? I'm just curious how the space for
pseudo registers is reserved. Is there a common place that we should
document that 0x14 is for the firmware (if it's not documented there
already)?

Thanks,
drew