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

[Cédric Le Goater]

On 5/4/20 1:30 PM, Greg Kurz wrote:
> 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 ?

So we could use the command line options : 

 -cpu POWER8 -machine pseries,pvr=off

instead of

 -cpu host -machine pseries,max-cpu-compat=POWER8  

is that it ? 

I am not sure I get it either.


>> 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");
>> +        }

we would still have the error above even if pvr=off ? 

C. 

>> +        /*
>> +         * 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) {
> 
>