[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH RFC 3/4] PPC: KVM: Support machine option to set V
From: |
Greg Kurz |
Subject: |
Re: [Qemu-ppc] [PATCH RFC 3/4] PPC: KVM: Support machine option to set VSMT mode |
Date: |
Wed, 28 Jun 2017 15:16:12 +0200 |
On Tue, 27 Jun 2017 10:22:55 +1000
Sam Bobroff <address@hidden> wrote:
> Currently, on Power9, the default value for KVM_CAP_PPC_SMT is one, so
> VMs cannot currently use more than one thread/core. Also, for Power7
> and above, when creating VCPUs on PPC KVM and the VM uses less
> threads/core than the host, QEMU must skip some VCPU IDs so that VCPUs
> are grouped correctly in VCOREs. This is because KVM divides the VCPU
> ID by the number of host threads/core to determine the VCORE ID.
>
> PPC KVM will soon allow writing to KVM_CAP_PPC_SMT to set the Virtual
> SMT mode. If N is successfully written:
> * KVM will use VCPU ID / N to determine the VCORE ID.
> * KVM will support the VM using N threads/core.
>
> To exploit this, allow the VSMT mode to be set on the command line.
> If it is not given, choose a default value that is compatible with
> hosts without this feature. This allows:
> * VCPU IDs to be allocated without skipping.
> * Power9 hosts to run VMs with more than one thread/core.
> * Migration between hosts with different but compatible threads/core.
>
> Signed-off-by: Sam Bobroff <address@hidden>
> ---
> hw/ppc/spapr.c | 84
> ++++++++++++++++++++++++++++++++++++++++++++-
> target/ppc/kvm.c | 5 +++
> target/ppc/kvm_ppc.h | 1 +
> target/ppc/translate_init.c | 23 ++++---------
> 4 files changed, 95 insertions(+), 18 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b33dc1b5c0..c9ec1e87a1 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -26,6 +26,7 @@
> */
> #include "qemu/osdep.h"
> #include "qapi/error.h"
> +#include "qapi/visitor.h"
> #include "sysemu/sysemu.h"
> #include "sysemu/numa.h"
> #include "hw/hw.h"
> @@ -2035,6 +2036,60 @@ static void spapr_init_cpus(sPAPRMachineState *spapr)
> g_free(type);
> }
>
> +static void ppc_set_vsmt_mode(sPAPRMachineState *spapr)
> +{
> + bool vsmt_user = !!spapr->vsmt;
> + int kvm_smt = (kvm_enabled() ? kvmppc_get_smt_threads() : 0);
> + int ret;
> +
> + if (!is_power_of_2(smp_threads)) {
> + error_report("Cannot support %d threads/core on PPC,"
> + " because it must be a power of 2.", smp_threads);
> + exit(1);
> + }
> + if (!kvm_enabled() && (smp_threads > 1)) {
> + error_report("Cannot support more than 1 thread/core on PPC with
> TCG.");
> + exit(1);
> + }
Cool to see these checks being moved to the machine code. :)
> +
> + /* Detemine the VSMT mode to use: */
> + if (vsmt_user) {
> + if (!is_power_of_2(spapr->vsmt)) {
> + error_report("Cannot support VSMT mode %d"
> + " because it must be a power of 2.", spapr->vsmt);
> + goto error;
> + }
> + if (spapr->vsmt < smp_threads) {
> + error_report("Cannot support VSMT mode %d"
> + " because it must be >= threads/core (%d).",
> + spapr->vsmt, smp_threads);
> + goto error;
> + }
> + } else {
> + spapr->vsmt = MAX(kvm_smt, smp_threads);
> + }
> +
> + if (kvm_enabled() && (spapr->vsmt != kvmppc_get_smt_threads())) {
> + ret = kvmppc_set_smt_threads(spapr->vsmt);
> + if (ret) {
> + error_report("Failed to set KVM's VSMT mode to %d, errno %d.",
> + spapr->vsmt, ret);
> + if (!vsmt_user) {
> + error_report("(On PPC, a VM with %d threads/core on a host"
> + " with %d threads/core requires the use of a"
> + " VSMT mode >= %d.)",
> + smp_threads, kvm_smt, spapr->vsmt);
> + }
> + goto error;
> + }
> + }
> + /* else TCG: nothing to do currently */
> + return;
> +error:
> + exit(1);
> +
> +}
> +
> /* pSeries LPAR / sPAPR hardware init */
> static void ppc_spapr_init(MachineState *machine)
> {
> @@ -2133,6 +2188,8 @@ static void ppc_spapr_init(MachineState *machine)
>
> ppc_cpu_parse_features(machine->cpu_model);
>
> + ppc_set_vsmt_mode(spapr);
> +
> spapr_init_cpus(spapr);
>
> if (kvm_enabled()) {
> @@ -2483,13 +2540,32 @@ static void spapr_set_modern_hotplug_events(Object
> *obj, bool value,
> spapr->use_hotplug_event_source = value;
> }
>
> +static void spapr_get_vsmt(Object *obj, Visitor *v, const char *name,
> + void *opaque, Error **errp)
> +{
> + visit_type_uint32(v, name, (uint32_t *)opaque, errp);
> +}
> +
> +static void spapr_set_vsmt(Object *obj, Visitor *v, const char *name,
> + void *opaque, Error **errp)
> +{
> + Error *local_err = NULL;
> +
> + visit_type_uint32(v, name, (uint32_t *)opaque, &local_err);
> + if (local_err) {
> + goto out;
> + }
> +out:
> + error_propagate(errp, local_err);
> +}
> +
> static void spapr_machine_initfn(Object *obj)
> {
> sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
>
> spapr->htab_fd = -1;
> spapr->use_hotplug_event_source = true;
> - spapr->vsmt = (kvm_enabled() ? kvmppc_get_smt_threads() : 1);
> + spapr->vsmt = 0;
> object_property_add_str(obj, "kvm-type",
> spapr_get_kvm_type, spapr_set_kvm_type, NULL);
> object_property_set_description(obj, "kvm-type",
> @@ -2504,6 +2580,11 @@ static void spapr_machine_initfn(Object *obj)
> " place of standard EPOW events when
> possible"
> " (required for memory hot-unplug
> support)",
> NULL);
> + object_property_add(obj, "vsmt", "uint32", spapr_get_vsmt,
> + spapr_set_vsmt, NULL, &spapr->vsmt, NULL);
> + object_property_set_description(obj, "vsmt",
> + "Virtual SMT: KVM behaves as if this
> were"
> + " the host's SMT mode", NULL);
> }
>
> static void spapr_machine_finalizefn(Object *obj)
> @@ -3643,3 +3724,4 @@ static void spapr_machine_register_types(void)
> }
>
> type_init(spapr_machine_register_types)
> +
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 74dfab95eb..da562fb100 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2121,6 +2121,11 @@ int kvmppc_get_smt_threads(void)
> return kvm_vm_check_extension(kvm_state, KVM_CAP_PPC_SMT);
> }
>
> +int kvmppc_set_smt_threads(int vsmt)
> +{
> + return kvm_vm_enable_cap(kvm_state, KVM_CAP_PPC_SMT, 0, vsmt, 0);
> +}
> +
> #ifdef TARGET_PPC64
> off_t kvmppc_alloc_rma(void **rma)
> {
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index cdc1407462..5c21ce668c 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -29,6 +29,7 @@ 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_get_smt_threads(void);
> +int kvmppc_set_smt_threads(int vsmt);
> 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);
> diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> index 0d3da9d746..51cc2cb031 100644
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -33,6 +33,9 @@
> #include "hw/qdev-properties.h"
> #include "hw/ppc/ppc.h"
> #include "mmu-book3s-v3.h"
> +#if !defined(CONFIG_USER_ONLY)
> +#include "hw/ppc/spapr.h"
> +#endif
>
> //#define PPC_DUMP_CPU
> //#define PPC_DEBUG_SPR
> @@ -9827,21 +9830,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_get_smt_threads();
> -#endif
> -
> -#if !defined(CONFIG_USER_ONLY)
> - if (smp_threads > max_smt) {
> - error_setg(errp, "Cannot support more than %d threads on PPC with
> %s",
> - max_smt, kvm_enabled() ? "KVM" : "TCG");
> - return;
> - }
> - if (!is_power_of_2(smp_threads)) {
> - error_setg(errp, "Cannot support %d threads on PPC with %s, "
> - "threads count must be a power of 2.",
> - smp_threads, kvm_enabled() ? "KVM" : "TCG");
> - return;
> - }
> + int vsmt = SPAPR_MACHINE(qdev_get_machine())->vsmt;
Hmm... it doesn't look right for CPU code to rely on machine stuff.
> #endif
>
> cpu_exec_realizefn(cs, &local_err);
> @@ -9851,14 +9840,14 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error
> **errp)
> }
>
> #if !defined(CONFIG_USER_ONLY)
> - cpu->cpu_dt_id = (cs->cpu_index / smp_threads) * max_smt
> + cpu->cpu_dt_id = (cs->cpu_index / smp_threads) * vsmt
> + (cs->cpu_index % smp_threads);
>
> if (kvm_enabled() && !kvm_vcpu_id_is_valid(cpu->cpu_dt_id)) {
> error_setg(errp, "Can't create CPU with id %d in KVM",
> cpu->cpu_dt_id);
> error_append_hint(errp, "Adjust the number of cpus to %d "
> "or try to raise the number of threads per core\n",
> - cpu->cpu_dt_id * smp_threads / max_smt);
> + cpu->cpu_dt_id * smp_threads / vsmt);
Moreover vsmt is only needed to compute cpu_dt_id which is also a PAPR
abstraction actually. I remember David mentioning that he would rather
like cpu->cpu_dt_id to be dropped and replaced by a helper in the machine
code.
> return;
> }
> #endif
pgpmYHBo7fLvJ.pgp
Description: OpenPGP digital signature