Re: [REPOST PATCH] arm/arm64: KVM: Add PSCI version selection API

[Marc Zyngier]

On Fri, 02 Mar 2018 10:44:48 +0000,
Auger Eric wrote:
> 
> Hi Marc,
> 
> On 15/02/18 18:58, Marc Zyngier wrote:
> > Although we've implemented PSCI 0.1, 0.2 and 1.0, we expose either 0.1
> > or 1.0 to a guest, defaulting to the latest version of the PSCI
> > implementation that is compatible with the requested version. 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.
> 
> I understand the get/set is called as part of the migration process.
> So my understanding is the benefit of this series is migration fails in
> those cases:
> 
> >=0.2 source -> 0.1 destination
> 0.1 source -> >=0.2 destination

It also fails in the case where you migrate a 1.0 guest to something
that cannot support it.

> However in the above mentioned use case where the guest would absolutely
> need a 0.2 and not a 1.0 I don't get on which event the userspace would
> do a set to force 0.2?

You'd have to know that your guest cannot support 0.2, and force 0.2
by doing a SET_ONE_REG(KVM_REG_ARM_PSCI_VERSION, PSCI_VERSION(0, 2)).

Or am I missing what you're asking for?

> > 
> > 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>
> > ---
> > This is a repost of this proposal for a firmware selection API, as
> > there was some discussion during the merging window. Now that the core
> > variant-2 stuff is merged, we can have a go at more
> > bikesheding. Please open fire!
> > 
> >  Documentation/virtual/kvm/api.txt      |  9 ++++-
> >  Documentation/virtual/kvm/arm/psci.txt | 30 +++++++++++++++++
> >  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                 | 16 +++++++--
> >  virt/kvm/arm/psci.c                    | 60 ++++++++++++++++++++++++++++++++++
> >  10 files changed, 156 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..d2b41e1608b4 100644
> > --- a/Documentation/virtual/kvm/api.txt
> > +++ b/Documentation/virtual/kvm/api.txt
> > @@ -1943,6 +1943,9 @@ ARM 32-bit VFP control registers have the following id bit patterns:
> >  ARM 64-bit FP registers have the following id bit patterns:
> >    0x4030 0000 0012 0 <regno:12>
> >  
> > +ARM firmware pseudo-registers have the following bit pattern:
> s/bit pattern/id bit patterns?
> > +  0x4030 0000 0014 <regno:16>
> > +
> >  
> >  arm64 registers are mapped using the lower 32 bits. The upper 16 of
> >  that is the register group type, or coprocessor number:
> > @@ -1959,6 +1962,9 @@ arm64 CCSIDR registers are demultiplexed by CSSELR value:
> >  arm64 system registers have the following id bit patterns:
> >    0x6030 0000 0013 <op0:2> <op1:3> <crn:4> <crm:4> <op2:3>
> >  
> > +arm64 firmware pseudo-registers have the following bit pattern:
> s/bit pattern/id bit patterns?
> > +  0x6030 0000 0014 <regno:16>
> > +
> >  
> >  MIPS registers are mapped using the lower 32 bits.  The upper 16 of that is
> >  the register group type:
> > @@ -2493,7 +2499,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..aafdab887b04
> > --- /dev/null
> > +++ b/Documentation/virtual/kvm/arm/psci.txt
> > @@ -0,0 +1,30 @@
> > +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
> > +pseudo-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.
> > +
> > +The following register is defined:
> > +
> > +* KVM_REG_ARM_PSCI_VERSION:
> > +
> > +  - Only valid if the vcpu has the KVM_ARM_VCPU_PSCI_0_2 feature set
> > +    (and thus has already been initialized)
> > +  - Returns the current PSCI version on GET_ONE_REG (defaulting to the
> > +    highest PSCI version implemented by KVM and compatible with v0.2)
> backwards compatible?
> > +  - Allows any PSCI version implemented by KVM and compatible with
> backwards compatible?
> > +    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 ef54013b5b9f..6c05e3b13081 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)
> > +#define KVM_REG_ARM_FW_REG(r)		(KVM_REG_ARM | KVM_REG_SIZE_U64 | \
> > +					 KVM_REG_ARM_FW | ((r) & 0xffff))
> > +#define KVM_REG_ARM_PSCI_VERSION	KVM_REG_ARM_FW_REG(0)
> 
> #define KVM_ARM_VCPU_PSCI_0_2           2 /* CPU uses PSCI v0.2 */
> Comment may deserve an upgrade too

I'm not sure we want this. KVM_REG_PSCI_VERSION takes a PSCI version
number, as indicated in the PSCI spec. You should use
PSCI_VERSION(0,2), as defined in include/uapi/linux/psci.h.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.