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

[Alexey Kardashevskiy]

On 04/05/2020 21:38, Cédric Le Goater wrote:
> 
> 
> 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 ? 

This does not work for my case.

> 
> I am not sure I get it either.


QEMU has to know the host CPU family to work with HV KVM: QEMU reads
pvr, picks a CPU class and goes from there, the max-compat applies lot
later and does not affect the CPU class which QEMU chooses.

Now I have a machine ("swift") with 0x004F1100 and QEMU has no idea what
0x004Fxxxx is (but I know it is POWER9) and there is currently no way to
bypass the PVR check which QEMU requests from KVM which KVM PR passes
but KVM HV fails as it has to match. Adding a new CPU family to the
upstream QEMU makes no sense as there will only be a handful of those
ever. And forcing PVR does not buy us that much anyway.




> 
> 
>>> 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 ?


Yup. We would still print a message but continue, for the moment it is a
debugging tool. Thanks,



> 
> 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) {
>>
>>

-- 
Alexey