Re: [Qemu-ppc] [PATCH RFC 2/4] PPC: KVM: Move kvm_cap_smt into sPAPRMachineState

[Sam Bobroff]

On Wed, Jun 28, 2017 at 10:18:44AM +0200, Greg Kurz wrote:
> On Tue, 27 Jun 2017 10:22:37 +1000
> Sam Bobroff <address@hidden> wrote:
> 
> > PPC QEMU currenlty tracks the host's SMT mode via a KVM capability
> > (KVM_CAP_PPC_SMT) and this value is cached in target/ppc/kvm.c.
> > 
> > This patch moves that tracking into the sPAPRMachineState without
> > functional changes (and changes the name of kvmppc_cap_smt() to
> > kvmppc_get_cap_smt() to make it clear that it now fetches directly).
> > 
> > This will facilitate further work introducing virtual SMT modes.
> > 
> > Signed-off-by: Sam Bobroff <address@hidden>
> > ---
> >  hw/ppc/spapr.c              | 14 ++++++++------
> >  include/hw/ppc/spapr.h      |  1 +
> >  target/ppc/kvm.c            |  6 ++----
> >  target/ppc/kvm_ppc.h        |  7 +------
> >  target/ppc/translate_init.c |  2 +-
> >  5 files changed, 13 insertions(+), 17 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index ede5167bc0..b33dc1b5c0 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -285,7 +285,7 @@ static int spapr_fixup_cpu_dt(void *fdt, 
> > sPAPRMachineState *spapr)
> >      int ret = 0, offset, cpus_offset;
> >      CPUState *cs;
> >      char cpu_model[32];
> > -    int smt = kvmppc_smt_threads();
> > +    int smt = spapr->vsmt;
> >      uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)};
> >  
> >      CPU_FOREACH(cs) {
> > @@ -563,7 +563,7 @@ static void spapr_populate_cpus_dt_node(void *fdt, 
> > sPAPRMachineState *spapr)
> >      CPUState *cs;
> >      int cpus_offset;
> >      char *nodename;
> > -    int smt = kvmppc_smt_threads();
> > +    int smt = spapr->vsmt;
> >  
> >      cpus_offset = fdt_add_subnode(fdt, 0, "cpus");
> >      _FDT(cpus_offset);
> > @@ -979,7 +979,7 @@ static void *spapr_build_fdt(sPAPRMachineState *spapr,
> >      void *fdt;
> >      sPAPRPHBState *phb;
> >      char *buf;
> > -    int smt = kvmppc_smt_threads();
> > +    int smt = spapr->vsmt;
> >  
> >      fdt = g_malloc0(FDT_MAX_SIZE);
> >      _FDT((fdt_create_empty_tree(fdt, FDT_MAX_SIZE)));
> > @@ -1975,7 +1975,7 @@ static void spapr_init_cpus(sPAPRMachineState *spapr)
> >      MachineState *machine = MACHINE(spapr);
> >      MachineClass *mc = MACHINE_GET_CLASS(machine);
> >      char *type = spapr_get_cpu_core_type(machine->cpu_model);
> > -    int smt = kvmppc_smt_threads();
> > +    int smt = spapr->vsmt;
> >      const CPUArchIdList *possible_cpus;
> >      int boot_cores_nr = smp_cpus / smp_threads;
> >      int i;
> > @@ -2489,6 +2489,7 @@ static void spapr_machine_initfn(Object *obj)
> >  
> >      spapr->htab_fd = -1;
> >      spapr->use_hotplug_event_source = true;
> > +    spapr->vsmt = (kvm_enabled() ? kvmppc_get_smt_threads() : 1);
> 
> Parenthesitis ? :)

Yep ;-)

> >      object_property_add_str(obj, "kvm-type",
> >                              spapr_get_kvm_type, spapr_set_kvm_type, NULL);
> >      object_property_set_description(obj, "kvm-type",
> > @@ -2827,11 +2828,12 @@ static
> >  void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState 
> > *dev,
> >                                 Error **errp)
> >  {
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
> >      int index;
> >      sPAPRDRConnector *drc;
> >      Error *local_err = NULL;
> >      CPUCore *cc = CPU_CORE(dev);
> > -    int smt = kvmppc_smt_threads();
> > +    int smt = spapr->vsmt;
> >  
> >      if (!spapr_find_cpu_slot(MACHINE(hotplug_dev), cc->core_id, &index)) {
> >          error_setg(errp, "Unable to find CPU core with core-id: %d",
> > @@ -2867,7 +2869,7 @@ static void spapr_core_plug(HotplugHandler 
> > *hotplug_dev, DeviceState *dev,
> >      Error *local_err = NULL;
> >      void *fdt = NULL;
> >      int fdt_offset = 0;
> > -    int smt = kvmppc_smt_threads();
> > +    int smt = spapr->vsmt;
> >      CPUArchId *core_slot;
> >      int index;
> >  
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index f973b02845..1ddbe0b81a 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -90,6 +90,7 @@ struct sPAPRMachineState {
> >      sPAPROptionVector *ov5_cas;     /* negotiated (via CAS) option vectors 
> > */
> >      bool cas_reboot;
> >      bool cas_legacy_guest_workaround;
> > +    uint32_t vsmt;                  /* Virtual SMT mode (KVM's "core 
> > stride") */
> >  
> >      Notifier epow_notifier;
> >      QTAILQ_HEAD(, sPAPREventLogEntry) pending_events;
> > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> > index f2f7c531bc..74dfab95eb 100644
> > --- a/target/ppc/kvm.c
> > +++ b/target/ppc/kvm.c
> > @@ -72,7 +72,6 @@ static int cap_interrupt_unset = false;
> >  static int cap_interrupt_level = false;
> >  static int cap_segstate;
> >  static int cap_booke_sregs;
> > -static int cap_ppc_smt;
> >  static int cap_ppc_rma;
> >  static int cap_spapr_tce;
> >  static int cap_spapr_tce_64;
> > @@ -127,7 +126,6 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
> >      cap_interrupt_level = kvm_check_extension(s, KVM_CAP_PPC_IRQ_LEVEL);
> >      cap_segstate = kvm_check_extension(s, KVM_CAP_PPC_SEGSTATE);
> >      cap_booke_sregs = kvm_check_extension(s, KVM_CAP_PPC_BOOKE_SREGS);
> > -    cap_ppc_smt = kvm_check_extension(s, KVM_CAP_PPC_SMT);
> >      cap_ppc_rma = kvm_check_extension(s, KVM_CAP_PPC_RMA);
> >      cap_spapr_tce = kvm_check_extension(s, KVM_CAP_SPAPR_TCE);
> >      cap_spapr_tce_64 = kvm_check_extension(s, KVM_CAP_SPAPR_TCE_64);
> > @@ -2118,9 +2116,9 @@ void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int 
> > mpic_proxy)
> >      }
> >  }
> >  
> > -int kvmppc_smt_threads(void)
> > +int kvmppc_get_smt_threads(void)
> >  {
> > -    return cap_ppc_smt ? cap_ppc_smt : 1;
> > +    return kvm_vm_check_extension(kvm_state, KVM_CAP_PPC_SMT);
> >  }
> >  
> >  #ifdef TARGET_PPC64
> > diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> > index eab7c8fdb3..cdc1407462 100644
> > --- a/target/ppc/kvm_ppc.h
> > +++ b/target/ppc/kvm_ppc.h
> > @@ -28,7 +28,7 @@ void kvmppc_enable_clear_ref_mod_hcalls(void);
> >  void kvmppc_set_papr(PowerPCCPU *cpu);
> >  int kvmppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr);
> >  void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy);
> > -int kvmppc_smt_threads(void);
> > +int kvmppc_get_smt_threads(void);
> >  int kvmppc_clear_tsr_bits(PowerPCCPU *cpu, uint32_t tsr_bits);
> >  int kvmppc_or_tsr_bits(PowerPCCPU *cpu, uint32_t tsr_bits);
> >  int kvmppc_set_tcr(PowerPCCPU *cpu);
> > @@ -138,11 +138,6 @@ static inline void kvmppc_set_mpic_proxy(PowerPCCPU 
> > *cpu, int mpic_proxy)
> >  {
> >  }
> >  
> > -static inline int kvmppc_smt_threads(void)
> > -{
> > -    return 1;
> > -}
> > -
> 
> Don't we need a stub for kvmppc_get_smt_threads() if !CONFIG_KVM ?

I was hoping we wouldn't! I thought it better to handle the TCG/non-KVM
case explicitly (see below).

> >  static inline int kvmppc_or_tsr_bits(PowerPCCPU *cpu, uint32_t tsr_bits)
> >  {
> >      return 0;
> > diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> > index 56a0ab22cf..0d3da9d746 100644
> > --- a/target/ppc/translate_init.c
> > +++ b/target/ppc/translate_init.c
> > @@ -9827,7 +9827,7 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error 
> > **errp)
> >      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> >      Error *local_err = NULL;
> >  #if !defined(CONFIG_USER_ONLY)
> > -    int max_smt = kvmppc_smt_threads();
> > +    int max_smt = kvmppc_get_smt_threads();
> 
> And so we will use KVM_CAP_PPC_SMT even in TCG mode ?
> 
> Maybe the helper should do the following:
> 
> int kvmppc_get_smt_threads(void)
> {
>     return kvm_enabled() ? kvm_vm_check_extension(kvm_state, KVM_CAP_PPC_SMT) 
> : 1;
> }

Ah, no, it wasn't my intention to use it for TCG: I just made a
mistake here. At this point spapr->vsmt has been set up and it should
just be used here (which is done in the next patch). I agree that it is
a bit awkward to use spapr here but as you also noticed that I'll
address that in my next reply :-)

> >  #endif
> >  
> >  #if !defined(CONFIG_USER_ONLY)
> 

Thanks for the review!
Sam.