Re: [PATCH qemu] spapr: Add PVR setting capability

[Greg Kurz]

On Fri, 17 Apr 2020 14:11:05 +1000
Alexey Kardashevskiy <address@hidden> wrote:

> At the moment the VCPU init sequence includes setting PVR which in case of
> KVM-HV only checks if it matches the hardware PVR mask as PVR cannot be
> virtualized by the hardware. In order to cope with various CPU revisions
> only top 16bit of PVR are checked which works for minor revision updates.
> 
> However in every CPU generation starting POWER7 (at least) there were CPUs
> supporting the (almost) same POWER ISA level but having different top
> 16bits of PVR - POWER7+, POWER8E, POWER8NVL; this time we got POWER9+
> with a new PVR family. We would normally add the PVR mask for the new one
> too, the problem with it is that although the physical machines exist,
> P9+ is not going to be released as a product, and this situation is likely
> to repeat in the future.
> 
> Instead of adding every new CPU family in QEMU, this adds a new sPAPR
> machine capability to force PVR setting/checking. It is "on" by default
> to preserve the existing behavior. When "off", it is the user's
> responsibility to specify the correct CPU.
> 

I don't quite understand the motivation for this... what does this
buy us ?

> Signed-off-by: Alexey Kardashevskiy <address@hidden>
> ---
>  include/hw/ppc/spapr.h |  5 ++++-
>  hw/ppc/spapr.c         |  1 +
>  hw/ppc/spapr_caps.c    | 18 ++++++++++++++++++
>  target/ppc/kvm.c       | 16 ++++++++++++++--
>  4 files changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index e579eaf28c05..5ccac4d56871 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -81,8 +81,10 @@ typedef enum {
>  #define SPAPR_CAP_CCF_ASSIST            0x09
>  /* Implements PAPR FWNMI option */
>  #define SPAPR_CAP_FWNMI                 0x0A
> +/* Implements PAPR PVR option */
> +#define SPAPR_CAP_PVR                   0x0B
>  /* Num Caps */
> -#define SPAPR_CAP_NUM                   (SPAPR_CAP_FWNMI + 1)
> +#define SPAPR_CAP_NUM                   (SPAPR_CAP_PVR + 1)
>  
>  /*
>   * Capability Values
> @@ -912,6 +914,7 @@ extern const VMStateDescription 
> vmstate_spapr_cap_nested_kvm_hv;
>  extern const VMStateDescription vmstate_spapr_cap_large_decr;
>  extern const VMStateDescription vmstate_spapr_cap_ccf_assist;
>  extern const VMStateDescription vmstate_spapr_cap_fwnmi;
> +extern const VMStateDescription vmstate_spapr_cap_pvr;
>  
>  static inline uint8_t spapr_get_cap(SpaprMachineState *spapr, int cap)
>  {
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 841b5ec59b12..ecc74c182b9f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4535,6 +4535,7 @@ static void spapr_machine_class_init(ObjectClass *oc, 
> void *data)
>      smc->default_caps.caps[SPAPR_CAP_LARGE_DECREMENTER] = SPAPR_CAP_ON;
>      smc->default_caps.caps[SPAPR_CAP_CCF_ASSIST] = SPAPR_CAP_ON;
>      smc->default_caps.caps[SPAPR_CAP_FWNMI] = SPAPR_CAP_ON;
> +    smc->default_caps.caps[SPAPR_CAP_PVR] = SPAPR_CAP_ON;
>      spapr_caps_add_properties(smc, &error_abort);
>      smc->irq = &spapr_irq_dual;
>      smc->dr_phb_enabled = true;
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index eb54f9422722..398b72b77f9f 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -525,6 +525,14 @@ static void cap_fwnmi_apply(SpaprMachineState *spapr, 
> uint8_t val,
>      }
>  }
>  
> +static void cap_pvr_apply(SpaprMachineState *spapr, uint8_t val, Error 
> **errp)
> +{
> +    if (val) {
> +        return;
> +    }
> +    warn_report("If you're uing kvm-hv.ko, only \"-cpu host\" is supported");
> +}
> +
>  SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>      [SPAPR_CAP_HTM] = {
>          .name = "htm",
> @@ -633,6 +641,15 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>          .type = "bool",
>          .apply = cap_fwnmi_apply,
>      },
> +    [SPAPR_CAP_PVR] = {
> +        .name = "pvr",
> +        .description = "Enforce PVR in KVM",
> +        .index = SPAPR_CAP_PVR,
> +        .get = spapr_cap_get_bool,
> +        .set = spapr_cap_set_bool,
> +        .type = "bool",
> +        .apply = cap_pvr_apply,
> +    },
>  };
>  
>  static SpaprCapabilities default_caps_with_cpu(SpaprMachineState *spapr,
> @@ -773,6 +790,7 @@ SPAPR_CAP_MIG_STATE(nested_kvm_hv, 
> SPAPR_CAP_NESTED_KVM_HV);
>  SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER);
>  SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST);
>  SPAPR_CAP_MIG_STATE(fwnmi, SPAPR_CAP_FWNMI);
> +SPAPR_CAP_MIG_STATE(pvr, SPAPR_CAP_PVR);
>  
>  void spapr_caps_init(SpaprMachineState *spapr)
>  {
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 03d0667e8f94..a4adc29b6522 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -466,15 +466,27 @@ int kvm_arch_init_vcpu(CPUState *cs)
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>      CPUPPCState *cenv = &cpu->env;
>      int ret;
> +    SpaprMachineState *spapr;
>  

We generally try to avoid adding such explicit dependencies to the
machine code within the target directory... A virtual hypervisor
hook could possibly do the trick but this would require to set
PowerPCCPU::vhyp before kvm_arch_init_vcpu() gets called, eg.
when the vCPU is created in spapr_create_vcpu() rather than
when it gets realized.

>      /* Synchronize sregs with kvm */
>      ret = kvm_arch_sync_sregs(cpu);
>      if (ret) {
>          if (ret == -EINVAL) {
>              error_report("Register sync failed... If you're using kvm-hv.ko,"
> -                         " only \"-cpu host\" is possible");
> +                         " only \"-cpu host\" is supported");
> +        }
> +        /*
> +         * The user chose not to set PVR which makes sense if we are running
> +         * on a CPU with known ISA level but unknown PVR.
> +         */
> +        spapr = (SpaprMachineState *)
> +            object_dynamic_cast(OBJECT(qdev_get_machine()), 
> TYPE_SPAPR_MACHINE);
> +
> +        if (spapr && spapr->eff.caps[SPAPR_CAP_PVR] == SPAPR_CAP_OFF) {
> +            ret = 0;
> +        } else {
> +            return ret;
>          }
> -        return ret;
>      }
>  
>      switch (cenv->mmu_model) {