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

[Alexey Kardashevskiy]

On 05/05/2020 15:50, David Gibson wrote:
> On Tue, May 05, 2020 at 10:56:17AM +1000, Alexey Kardashevskiy wrote:
>>
>>
>> On 04/05/2020 21:30, 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 ?
>>
>> I answered that part in another mail in this thread, shortly this is to
>> make QEMU work with HV KVM on unknown-to-QEMU CPU family (0x004f0000).
>>
>>
>>>
>>>> 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.
>>
>> The only thing kvm_arch_sync_sregs() does is setting PVR, and it does
>> not do even that for Book3E so there is dependency already, mine at
>> least explicit :)
>>
>> I am really not sure what setting PVR buys us, KVM PR will take
>> anything,
> 
> PR will take anything, but it changes the behaviour.  Basically PR
> will try to match its behaviour to the PVR you specify.  It can't
> entirely succeeed in all cases, of course, but that's PR for you.
>
> From an API point of view, the idea is that setting the PVR here
> specifies the model you want the vcpu to appear to be.  But, KVM is
> free to say "can't do that".


Except pseries does not care what the CPU appears to be and uses
KVM_REG_PPC_ARCH_COMPAT instead.

So, is this a no? Thanks,


> 
>> KVM HV will only take the same family PVR and reject others
>> while it could still run more configurations, let's say -cpu POWER8 on
>> actual POWER9 machine. Dunno. The capability seems a harmless way of
>> relaxing this restriction. Thanks,
>>
>>
>>
>>
>>>
>>>>      /* 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) {
>>>
>>
> 

-- 
Alexey