Message ID | 20191024081813.2115-7-david@gibson.dropbear.id.au |
---|---|
State | New |
Headers | show |
Series | [PULL,01/28] xive: Make some device types not user creatable | expand |
On 24/10/2019 10:17, David Gibson wrote: > From: Greg Kurz <groug@kaod.org> > > Support for setting VSMT is available in KVM since linux-4.13. Most distros > that support KVM on POWER already have it. It thus seem reasonable enough > to have the default machine to set VSMT to smp_threads. > > This brings contiguous VCPU ids and thus brings their upper bound down to > the machine's max_cpus. This is especially useful for XIVE KVM devices, > which may thus allocate only one VP descriptor per VCPU. > > Signed-off-by: Greg Kurz <groug@kaod.org> > Message-Id: <157010411885.246126.12610015369068227139.stgit@bahia.lan> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/ppc/spapr.c | 7 ++++++- > include/hw/ppc/spapr.h | 1 + > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 4eb97d3a9b..428b834f30 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2496,6 +2496,7 @@ static CPUArchId *spapr_find_cpu_slot(MachineState *ms, uint32_t id, int *idx) > static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp) > { > MachineState *ms = MACHINE(spapr); > + SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr); > Error *local_err = NULL; > bool vsmt_user = !!spapr->vsmt; > int kvm_smt = kvmppc_smt_threads(); > @@ -2522,7 +2523,7 @@ static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp) > goto out; > } > /* In this case, spapr->vsmt has been set by the command line */ > - } else { > + } else if (!smc->smp_threads_vsmt) { > /* > * Default VSMT value is tricky, because we need it to be as > * consistent as possible (for migration), but this requires > @@ -2531,6 +2532,8 @@ static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp) > * overwhelmingly common case in production systems. > */ > spapr->vsmt = MAX(8, smp_threads); > + } else { > + spapr->vsmt = smp_threads; > } > > /* KVM: If necessary, set the SMT mode: */ > @@ -4438,6 +4441,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) > smc->irq = &spapr_irq_dual; > smc->dr_phb_enabled = true; > smc->linux_pci_probe = true; > + smc->smp_threads_vsmt = true; > } > > static const TypeInfo spapr_machine_info = { > @@ -4505,6 +4509,7 @@ static void spapr_machine_4_1_class_options(MachineClass *mc) > > spapr_machine_4_2_class_options(mc); > smc->linux_pci_probe = false; > + smc->smp_threads_vsmt = false; > compat_props_add(mc->compat_props, hw_compat_4_1, hw_compat_4_1_len); > compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat)); > } > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index cbd1a4c9f3..2009eb64f9 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -122,6 +122,7 @@ struct SpaprMachineClass { > bool broken_host_serial_model; /* present real host info to the guest */ > bool pre_4_1_migration; /* don't migrate hpt-max-page-size */ > bool linux_pci_probe; > + bool smp_threads_vsmt; /* set VSMT to smp_threads by default */ > > void (*phb_placement)(SpaprMachineState *spapr, uint32_t index, > uint64_t *buid, hwaddr *pio, > This patch breaks tests/migration-test on P8 host with kernel older than 4.3 because it tries by default to set the VSMT to 1. qemu-system-ppc64: Failed to set KVM's VSMT mode to 1 (errno -22) On PPC, a VM with 1 threads/core on a host with 8 threads/core requires the use of VSMT mode 1. This KVM seems to be too old to support VSMT. As this is clearly intentional, is there a way to fix migration-test? Thanks, Laurent
On Fri, Nov 08, 2019 at 02:11:03PM +0100, Laurent Vivier wrote: > On 24/10/2019 10:17, David Gibson wrote: > > From: Greg Kurz <groug@kaod.org> > > > > Support for setting VSMT is available in KVM since linux-4.13. Most distros > > that support KVM on POWER already have it. It thus seem reasonable enough > > to have the default machine to set VSMT to smp_threads. > > > > This brings contiguous VCPU ids and thus brings their upper bound down to > > the machine's max_cpus. This is especially useful for XIVE KVM devices, > > which may thus allocate only one VP descriptor per VCPU. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > Message-Id: <157010411885.246126.12610015369068227139.stgit@bahia.lan> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > --- > > hw/ppc/spapr.c | 7 ++++++- > > include/hw/ppc/spapr.h | 1 + > > 2 files changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 4eb97d3a9b..428b834f30 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -2496,6 +2496,7 @@ static CPUArchId *spapr_find_cpu_slot(MachineState *ms, uint32_t id, int *idx) > > static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp) > > { > > MachineState *ms = MACHINE(spapr); > > + SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr); > > Error *local_err = NULL; > > bool vsmt_user = !!spapr->vsmt; > > int kvm_smt = kvmppc_smt_threads(); > > @@ -2522,7 +2523,7 @@ static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp) > > goto out; > > } > > /* In this case, spapr->vsmt has been set by the command line */ > > - } else { > > + } else if (!smc->smp_threads_vsmt) { > > /* > > * Default VSMT value is tricky, because we need it to be as > > * consistent as possible (for migration), but this requires > > @@ -2531,6 +2532,8 @@ static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp) > > * overwhelmingly common case in production systems. > > */ > > spapr->vsmt = MAX(8, smp_threads); > > + } else { > > + spapr->vsmt = smp_threads; > > } > > > > /* KVM: If necessary, set the SMT mode: */ > > @@ -4438,6 +4441,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) > > smc->irq = &spapr_irq_dual; > > smc->dr_phb_enabled = true; > > smc->linux_pci_probe = true; > > + smc->smp_threads_vsmt = true; > > } > > > > static const TypeInfo spapr_machine_info = { > > @@ -4505,6 +4509,7 @@ static void spapr_machine_4_1_class_options(MachineClass *mc) > > > > spapr_machine_4_2_class_options(mc); > > smc->linux_pci_probe = false; > > + smc->smp_threads_vsmt = false; > > compat_props_add(mc->compat_props, hw_compat_4_1, hw_compat_4_1_len); > > compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat)); > > } > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > > index cbd1a4c9f3..2009eb64f9 100644 > > --- a/include/hw/ppc/spapr.h > > +++ b/include/hw/ppc/spapr.h > > @@ -122,6 +122,7 @@ struct SpaprMachineClass { > > bool broken_host_serial_model; /* present real host info to the guest */ > > bool pre_4_1_migration; /* don't migrate hpt-max-page-size */ > > bool linux_pci_probe; > > + bool smp_threads_vsmt; /* set VSMT to smp_threads by default */ > > > > void (*phb_placement)(SpaprMachineState *spapr, uint32_t index, > > uint64_t *buid, hwaddr *pio, > > > > This patch breaks tests/migration-test on P8 host with kernel older than > 4.3 because it tries by default to set the VSMT to 1. > > qemu-system-ppc64: Failed to set KVM's VSMT mode to 1 (errno -22) > On PPC, a VM with 1 threads/core on a host with 8 threads/core requires > the use of VSMT mode 1. > This KVM seems to be too old to support VSMT. > > As this is clearly intentional, is there a way to fix migration-test? Hrm. I believe the argument for this was that the broken kernels were old enough we didn't care. What platform are you testing on where you're hitting this?
On 08/11/2019 15:26, David Gibson wrote: > On Fri, Nov 08, 2019 at 02:11:03PM +0100, Laurent Vivier wrote: >> On 24/10/2019 10:17, David Gibson wrote: >>> From: Greg Kurz <groug@kaod.org> >>> >>> Support for setting VSMT is available in KVM since linux-4.13. Most distros >>> that support KVM on POWER already have it. It thus seem reasonable enough >>> to have the default machine to set VSMT to smp_threads. >>> >>> This brings contiguous VCPU ids and thus brings their upper bound down to >>> the machine's max_cpus. This is especially useful for XIVE KVM devices, >>> which may thus allocate only one VP descriptor per VCPU. >>> >>> Signed-off-by: Greg Kurz <groug@kaod.org> >>> Message-Id: <157010411885.246126.12610015369068227139.stgit@bahia.lan> >>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> >>> --- >>> hw/ppc/spapr.c | 7 ++++++- >>> include/hw/ppc/spapr.h | 1 + >>> 2 files changed, 7 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >>> index 4eb97d3a9b..428b834f30 100644 >>> --- a/hw/ppc/spapr.c >>> +++ b/hw/ppc/spapr.c >>> @@ -2496,6 +2496,7 @@ static CPUArchId *spapr_find_cpu_slot(MachineState *ms, uint32_t id, int *idx) >>> static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp) >>> { >>> MachineState *ms = MACHINE(spapr); >>> + SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr); >>> Error *local_err = NULL; >>> bool vsmt_user = !!spapr->vsmt; >>> int kvm_smt = kvmppc_smt_threads(); >>> @@ -2522,7 +2523,7 @@ static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp) >>> goto out; >>> } >>> /* In this case, spapr->vsmt has been set by the command line */ >>> - } else { >>> + } else if (!smc->smp_threads_vsmt) { >>> /* >>> * Default VSMT value is tricky, because we need it to be as >>> * consistent as possible (for migration), but this requires >>> @@ -2531,6 +2532,8 @@ static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp) >>> * overwhelmingly common case in production systems. >>> */ >>> spapr->vsmt = MAX(8, smp_threads); >>> + } else { >>> + spapr->vsmt = smp_threads; >>> } >>> >>> /* KVM: If necessary, set the SMT mode: */ >>> @@ -4438,6 +4441,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) >>> smc->irq = &spapr_irq_dual; >>> smc->dr_phb_enabled = true; >>> smc->linux_pci_probe = true; >>> + smc->smp_threads_vsmt = true; >>> } >>> >>> static const TypeInfo spapr_machine_info = { >>> @@ -4505,6 +4509,7 @@ static void spapr_machine_4_1_class_options(MachineClass *mc) >>> >>> spapr_machine_4_2_class_options(mc); >>> smc->linux_pci_probe = false; >>> + smc->smp_threads_vsmt = false; >>> compat_props_add(mc->compat_props, hw_compat_4_1, hw_compat_4_1_len); >>> compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat)); >>> } >>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h >>> index cbd1a4c9f3..2009eb64f9 100644 >>> --- a/include/hw/ppc/spapr.h >>> +++ b/include/hw/ppc/spapr.h >>> @@ -122,6 +122,7 @@ struct SpaprMachineClass { >>> bool broken_host_serial_model; /* present real host info to the guest */ >>> bool pre_4_1_migration; /* don't migrate hpt-max-page-size */ >>> bool linux_pci_probe; >>> + bool smp_threads_vsmt; /* set VSMT to smp_threads by default */ >>> >>> void (*phb_placement)(SpaprMachineState *spapr, uint32_t index, >>> uint64_t *buid, hwaddr *pio, >>> >> >> This patch breaks tests/migration-test on P8 host with kernel older than >> 4.3 because it tries by default to set the VSMT to 1. >> >> qemu-system-ppc64: Failed to set KVM's VSMT mode to 1 (errno -22) >> On PPC, a VM with 1 threads/core on a host with 8 threads/core requires >> the use of VSMT mode 1. >> This KVM seems to be too old to support VSMT. >> >> As this is clearly intentional, is there a way to fix migration-test? > > Hrm. I believe the argument for this was that the broken kernels were > old enough we didn't care. What platform are you testing on where > you're hitting this? > I'm going to propose a patch to fix this problem. (it was on RHEL7) Thanks, Laurent
On 08/11/2019 16:34, Laurent Vivier wrote: > On 08/11/2019 15:26, David Gibson wrote: >> On Fri, Nov 08, 2019 at 02:11:03PM +0100, Laurent Vivier wrote: >>> On 24/10/2019 10:17, David Gibson wrote: >>>> From: Greg Kurz <groug@kaod.org> >>>> >>>> Support for setting VSMT is available in KVM since linux-4.13. Most distros >>>> that support KVM on POWER already have it. It thus seem reasonable enough >>>> to have the default machine to set VSMT to smp_threads. >>>> >>>> This brings contiguous VCPU ids and thus brings their upper bound down to >>>> the machine's max_cpus. This is especially useful for XIVE KVM devices, >>>> which may thus allocate only one VP descriptor per VCPU. >>>> >>>> Signed-off-by: Greg Kurz <groug@kaod.org> >>>> Message-Id: <157010411885.246126.12610015369068227139.stgit@bahia.lan> >>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> >>>> --- >>>> hw/ppc/spapr.c | 7 ++++++- >>>> include/hw/ppc/spapr.h | 1 + >>>> 2 files changed, 7 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >>>> index 4eb97d3a9b..428b834f30 100644 >>>> --- a/hw/ppc/spapr.c >>>> +++ b/hw/ppc/spapr.c >>>> @@ -2496,6 +2496,7 @@ static CPUArchId *spapr_find_cpu_slot(MachineState *ms, uint32_t id, int *idx) >>>> static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp) >>>> { >>>> MachineState *ms = MACHINE(spapr); >>>> + SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr); >>>> Error *local_err = NULL; >>>> bool vsmt_user = !!spapr->vsmt; >>>> int kvm_smt = kvmppc_smt_threads(); >>>> @@ -2522,7 +2523,7 @@ static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp) >>>> goto out; >>>> } >>>> /* In this case, spapr->vsmt has been set by the command line */ >>>> - } else { >>>> + } else if (!smc->smp_threads_vsmt) { >>>> /* >>>> * Default VSMT value is tricky, because we need it to be as >>>> * consistent as possible (for migration), but this requires >>>> @@ -2531,6 +2532,8 @@ static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp) >>>> * overwhelmingly common case in production systems. >>>> */ >>>> spapr->vsmt = MAX(8, smp_threads); >>>> + } else { >>>> + spapr->vsmt = smp_threads; >>>> } >>>> >>>> /* KVM: If necessary, set the SMT mode: */ >>>> @@ -4438,6 +4441,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) >>>> smc->irq = &spapr_irq_dual; >>>> smc->dr_phb_enabled = true; >>>> smc->linux_pci_probe = true; >>>> + smc->smp_threads_vsmt = true; >>>> } >>>> >>>> static const TypeInfo spapr_machine_info = { >>>> @@ -4505,6 +4509,7 @@ static void spapr_machine_4_1_class_options(MachineClass *mc) >>>> >>>> spapr_machine_4_2_class_options(mc); >>>> smc->linux_pci_probe = false; >>>> + smc->smp_threads_vsmt = false; >>>> compat_props_add(mc->compat_props, hw_compat_4_1, hw_compat_4_1_len); >>>> compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat)); >>>> } >>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h >>>> index cbd1a4c9f3..2009eb64f9 100644 >>>> --- a/include/hw/ppc/spapr.h >>>> +++ b/include/hw/ppc/spapr.h >>>> @@ -122,6 +122,7 @@ struct SpaprMachineClass { >>>> bool broken_host_serial_model; /* present real host info to the guest */ >>>> bool pre_4_1_migration; /* don't migrate hpt-max-page-size */ >>>> bool linux_pci_probe; >>>> + bool smp_threads_vsmt; /* set VSMT to smp_threads by default */ >>>> >>>> void (*phb_placement)(SpaprMachineState *spapr, uint32_t index, >>>> uint64_t *buid, hwaddr *pio, >>>> >>> >>> This patch breaks tests/migration-test on P8 host with kernel older than >>> 4.3 because it tries by default to set the VSMT to 1. >>> >>> qemu-system-ppc64: Failed to set KVM's VSMT mode to 1 (errno -22) >>> On PPC, a VM with 1 threads/core on a host with 8 threads/core requires >>> the use of VSMT mode 1. >>> This KVM seems to be too old to support VSMT. >>> >>> As this is clearly intentional, is there a way to fix migration-test? >> >> Hrm. I believe the argument for this was that the broken kernels were >> old enough we didn't care. What platform are you testing on where >> you're hitting this? >> > > I'm going to propose a patch to fix this problem. > (it was on RHEL7) Patch sent: [PATCH] spapr: Fix VSMT mode when it is not supported by the kernel Message-Id: <20191108154035.12913-1-lvivier@redhat.com> Thanks, Laurent
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 4eb97d3a9b..428b834f30 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -2496,6 +2496,7 @@ static CPUArchId *spapr_find_cpu_slot(MachineState *ms, uint32_t id, int *idx) static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp) { MachineState *ms = MACHINE(spapr); + SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr); Error *local_err = NULL; bool vsmt_user = !!spapr->vsmt; int kvm_smt = kvmppc_smt_threads(); @@ -2522,7 +2523,7 @@ static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp) goto out; } /* In this case, spapr->vsmt has been set by the command line */ - } else { + } else if (!smc->smp_threads_vsmt) { /* * Default VSMT value is tricky, because we need it to be as * consistent as possible (for migration), but this requires @@ -2531,6 +2532,8 @@ static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp) * overwhelmingly common case in production systems. */ spapr->vsmt = MAX(8, smp_threads); + } else { + spapr->vsmt = smp_threads; } /* KVM: If necessary, set the SMT mode: */ @@ -4438,6 +4441,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) smc->irq = &spapr_irq_dual; smc->dr_phb_enabled = true; smc->linux_pci_probe = true; + smc->smp_threads_vsmt = true; } static const TypeInfo spapr_machine_info = { @@ -4505,6 +4509,7 @@ static void spapr_machine_4_1_class_options(MachineClass *mc) spapr_machine_4_2_class_options(mc); smc->linux_pci_probe = false; + smc->smp_threads_vsmt = false; compat_props_add(mc->compat_props, hw_compat_4_1, hw_compat_4_1_len); compat_props_add(mc->compat_props, compat, G_N_ELEMENTS(compat)); } diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h index cbd1a4c9f3..2009eb64f9 100644 --- a/include/hw/ppc/spapr.h +++ b/include/hw/ppc/spapr.h @@ -122,6 +122,7 @@ struct SpaprMachineClass { bool broken_host_serial_model; /* present real host info to the guest */ bool pre_4_1_migration; /* don't migrate hpt-max-page-size */ bool linux_pci_probe; + bool smp_threads_vsmt; /* set VSMT to smp_threads by default */ void (*phb_placement)(SpaprMachineState *spapr, uint32_t index, uint64_t *buid, hwaddr *pio,