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

[David Gibson]

On Tue, May 05, 2020 at 04:18:40PM +1000, Alexey Kardashevskiy wrote:
> 
> 
> 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.

I'm not really sure what you mean by that.  The exact cpu model can be
relevant for pseries in certain edge cases, although in most cases
it's the compat mode that will matter in practice.

> So, is this a no? Thanks,

Yes, although not really for the reasons above.  As you say this is
just for the handful of people who have never-released machines, which
makes this some ugly and confusing cruft for nearly everyone.  So, I'm
more inclined to say that if you happen to have one of those machines,
you get to apply the workaround patch yourself.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature