| Message ID | 1462192431-146342-13-git-send-email-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 02, 2016 at 02:33:21PM +0200, Igor Mammedov wrote: > on old machine types CPU hotplug was uncondtionally > enabled since it was introduced, consuming IO ports > and providing AML regardles of whether it was actually > in use or not. Keep it so for 2.6 and older machines. > > New machine types will have an option to turn CPU > hotplug on if it's needed while by default it stays > disabled not consuming extra RAM/IO resources. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> What if people are using "-machine pc -smp N,max_cpus=M"? Shouldn't we at least warning about missing CPU hotplug support when using just "max_cpus" with no "cpu-hotplug=on" with pc-2.7 and newer? Should max_cpus > smp_cpus automatically set cpu-hotplug=on?
On Tue, 10 May 2016 17:24:14 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Mon, May 02, 2016 at 02:33:21PM +0200, Igor Mammedov wrote: > > on old machine types CPU hotplug was uncondtionally > > enabled since it was introduced, consuming IO ports > > and providing AML regardles of whether it was actually > > in use or not. Keep it so for 2.6 and older machines. > > > > New machine types will have an option to turn CPU > > hotplug on if it's needed while by default it stays > > disabled not consuming extra RAM/IO resources. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > What if people are using "-machine pc -smp N,max_cpus=M"? > Shouldn't we at least warning about missing CPU hotplug support > when using just "max_cpus" with no "cpu-hotplug=on" with pc-2.7 > and newer? Yep, I'll add it on next respin. Would hard error better than warning? > Should max_cpus > smp_cpus automatically set > cpu-hotplug=on? I'd prefer dumb explicit feature enablement, as it doesn't put any assumptions on other options and QEMU + mgmt don't have to maintain logic for implicit rules that might enable it. and if I didn't manage to push 'device_add x86cpu' in 2.7 time frame, guess work gets a bit confusing with current cpu-add semantic, consider current: SRC-QEMU -smp 1,maxcpus=2 cpu-add 1 DST-QEMU -smp 2,maxcpus=2 vs would be device_add: SRC-QEMU -smp 1,maxcpus=2 device_add cpu DST-QEMU -smp 1,maxcpus=2 -device cpu so instead of qemu/users guessing, I suggest to make it explictly enabled to get feature (which is mostly optional) or cleanly fail qemu start if confusing options are specified with a clear error message.
On Wed, May 11, 2016 at 03:50:39PM +0200, Igor Mammedov wrote: > On Tue, 10 May 2016 17:24:14 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Mon, May 02, 2016 at 02:33:21PM +0200, Igor Mammedov wrote: > > > on old machine types CPU hotplug was uncondtionally > > > enabled since it was introduced, consuming IO ports > > > and providing AML regardles of whether it was actually > > > in use or not. Keep it so for 2.6 and older machines. > > > > > > New machine types will have an option to turn CPU > > > hotplug on if it's needed while by default it stays > > > disabled not consuming extra RAM/IO resources. > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > What if people are using "-machine pc -smp N,max_cpus=M"? > > Shouldn't we at least warning about missing CPU hotplug support > > when using just "max_cpus" with no "cpu-hotplug=on" with pc-2.7 > > and newer? > Yep, I'll add it on next respin. > Would hard error better than warning? Most people don't need cpu hotplug, attempts to hotplug fail, should be enough. > > Should max_cpus > smp_cpus automatically set > > cpu-hotplug=on? > I'd prefer dumb explicit feature enablement, > as it doesn't put any assumptions on other options and > QEMU + mgmt don't have to maintain logic for implicit > rules that might enable it. > > and if I didn't manage to push 'device_add x86cpu' in 2.7 time frame, > guess work gets a bit confusing with current cpu-add semantic, > consider current: > > SRC-QEMU -smp 1,maxcpus=2 > cpu-add 1 > DST-QEMU -smp 2,maxcpus=2 > > vs would be device_add: > > SRC-QEMU -smp 1,maxcpus=2 > device_add cpu > DST-QEMU -smp 1,maxcpus=2 -device cpu > > so instead of qemu/users guessing, I suggest to make it explictly > enabled to get feature (which is mostly optional) or > cleanly fail qemu start if confusing options are specified > with a clear error message.
On Thu, May 12, 2016 at 12:51:55AM +0300, Michael S. Tsirkin wrote: > On Wed, May 11, 2016 at 03:50:39PM +0200, Igor Mammedov wrote: > > On Tue, 10 May 2016 17:24:14 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Mon, May 02, 2016 at 02:33:21PM +0200, Igor Mammedov wrote: > > > > on old machine types CPU hotplug was uncondtionally > > > > enabled since it was introduced, consuming IO ports > > > > and providing AML regardles of whether it was actually > > > > in use or not. Keep it so for 2.6 and older machines. > > > > > > > > New machine types will have an option to turn CPU > > > > hotplug on if it's needed while by default it stays > > > > disabled not consuming extra RAM/IO resources. > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > > What if people are using "-machine pc -smp N,max_cpus=M"? > > > Shouldn't we at least warning about missing CPU hotplug support > > > when using just "max_cpus" with no "cpu-hotplug=on" with pc-2.7 > > > and newer? > > Yep, I'll add it on next respin. > > Would hard error better than warning? > > Most people don't need cpu hotplug, attempts > to hotplug fail, should be enough. People who don't need CPU hotplug shouldn't be using the max_cpus option. I believe we should at least warn people (early, during initialization) that their configuration don't make sense anymore.
On Wed, May 11, 2016 at 03:50:39PM +0200, Igor Mammedov wrote: > On Tue, 10 May 2016 17:24:14 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Mon, May 02, 2016 at 02:33:21PM +0200, Igor Mammedov wrote: > > > on old machine types CPU hotplug was uncondtionally > > > enabled since it was introduced, consuming IO ports > > > and providing AML regardles of whether it was actually > > > in use or not. Keep it so for 2.6 and older machines. > > > > > > New machine types will have an option to turn CPU > > > hotplug on if it's needed while by default it stays > > > disabled not consuming extra RAM/IO resources. > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > What if people are using "-machine pc -smp N,max_cpus=M"? > > Shouldn't we at least warning about missing CPU hotplug support > > when using just "max_cpus" with no "cpu-hotplug=on" with pc-2.7 > > and newer? > Yep, I'll add it on next respin. > Would hard error better than warning? Not sure. It would break older libvirt versions, wouldn't it? But: isn't the new legacy-cpu-hotplug=false default going to break old libvirt versions anyway? Should we? (CCing libvirt list) > > > Should max_cpus > smp_cpus automatically set > > cpu-hotplug=on? > I'd prefer dumb explicit feature enablement, > as it doesn't put any assumptions on other options and > QEMU + mgmt don't have to maintain logic for implicit > rules that might enable it. > > and if I didn't manage to push 'device_add x86cpu' in 2.7 time frame, > guess work gets a bit confusing with current cpu-add semantic, > consider current: > > SRC-QEMU -smp 1,maxcpus=2 > cpu-add 1 > DST-QEMU -smp 2,maxcpus=2 > > vs would be device_add: > > SRC-QEMU -smp 1,maxcpus=2 > device_add cpu > DST-QEMU -smp 1,maxcpus=2 -device cpu > > so instead of qemu/users guessing, I suggest to make it explictly > enabled to get feature (which is mostly optional) or > cleanly fail qemu start if confusing options are specified > with a clear error message. Agreed we shouldn't encourage people to use the old option to get the new behavior. But I am worried about breaking existing configurations on a machine-type change. Is it possible to avoid that?
On Wed, May 11, 2016 at 08:36:00PM -0300, Eduardo Habkost wrote: > On Wed, May 11, 2016 at 03:50:39PM +0200, Igor Mammedov wrote: > > On Tue, 10 May 2016 17:24:14 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Mon, May 02, 2016 at 02:33:21PM +0200, Igor Mammedov wrote: > > > > on old machine types CPU hotplug was uncondtionally > > > > enabled since it was introduced, consuming IO ports > > > > and providing AML regardles of whether it was actually > > > > in use or not. Keep it so for 2.6 and older machines. > > > > > > > > New machine types will have an option to turn CPU > > > > hotplug on if it's needed while by default it stays > > > > disabled not consuming extra RAM/IO resources. > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > > What if people are using "-machine pc -smp N,max_cpus=M"? > > > Shouldn't we at least warning about missing CPU hotplug support > > > when using just "max_cpus" with no "cpu-hotplug=on" with pc-2.7 > > > and newer? > > Yep, I'll add it on next respin. > > Would hard error better than warning? > > Not sure. It would break older libvirt versions, wouldn't it? > But: isn't the new legacy-cpu-hotplug=false default going to > break old libvirt versions anyway? Should we? > > (CCing libvirt list) Even if not, we should not break scripts people use. Maybe it was a mistake to enable hotplug by default but we can't just remove it now after all this time. Why not have new hotplug on by default as well? If you see value in ability to remove this feature, add an option for that. > > > > > Should max_cpus > smp_cpus automatically set > > > cpu-hotplug=on? > > I'd prefer dumb explicit feature enablement, > > as it doesn't put any assumptions on other options and > > QEMU + mgmt don't have to maintain logic for implicit > > rules that might enable it. > > > > and if I didn't manage to push 'device_add x86cpu' in 2.7 time frame, > > guess work gets a bit confusing with current cpu-add semantic, > > consider current: > > > > SRC-QEMU -smp 1,maxcpus=2 > > cpu-add 1 > > DST-QEMU -smp 2,maxcpus=2 > > > > vs would be device_add: > > > > SRC-QEMU -smp 1,maxcpus=2 > > device_add cpu > > DST-QEMU -smp 1,maxcpus=2 -device cpu > > > > so instead of qemu/users guessing, I suggest to make it explictly > > enabled to get feature (which is mostly optional) or > > cleanly fail qemu start if confusing options are specified > > with a clear error message. > > Agreed we shouldn't encourage people to use the old option to get > the new behavior. > > But I am worried about breaking existing configurations on a > machine-type change. Is it possible to avoid that? > > -- > Eduardo
On Wed, May 11, 2016 at 08:28:59PM -0300, Eduardo Habkost wrote: > On Thu, May 12, 2016 at 12:51:55AM +0300, Michael S. Tsirkin wrote: > > On Wed, May 11, 2016 at 03:50:39PM +0200, Igor Mammedov wrote: > > > On Tue, 10 May 2016 17:24:14 -0300 > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > On Mon, May 02, 2016 at 02:33:21PM +0200, Igor Mammedov wrote: > > > > > on old machine types CPU hotplug was uncondtionally > > > > > enabled since it was introduced, consuming IO ports > > > > > and providing AML regardles of whether it was actually > > > > > in use or not. Keep it so for 2.6 and older machines. > > > > > > > > > > New machine types will have an option to turn CPU > > > > > hotplug on if it's needed while by default it stays > > > > > disabled not consuming extra RAM/IO resources. > > > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > > > > What if people are using "-machine pc -smp N,max_cpus=M"? > > > > Shouldn't we at least warning about missing CPU hotplug support > > > > when using just "max_cpus" with no "cpu-hotplug=on" with pc-2.7 > > > > and newer? > > > Yep, I'll add it on next respin. > > > Would hard error better than warning? > > > > Most people don't need cpu hotplug, attempts > > to hotplug fail, should be enough. > > People who don't need CPU hotplug shouldn't be using the max_cpus > option. I agree. > I believe we should at least warn people (early, during > initialization) that their configuration don't make sense > anymore. I think we should try to keep old command line working if we can. > -- > Eduardo
On Thu, 12 May 2016 10:07:03 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, May 11, 2016 at 08:28:59PM -0300, Eduardo Habkost wrote: > > On Thu, May 12, 2016 at 12:51:55AM +0300, Michael S. Tsirkin wrote: > > > On Wed, May 11, 2016 at 03:50:39PM +0200, Igor Mammedov wrote: > > > > On Tue, 10 May 2016 17:24:14 -0300 > > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > > > On Mon, May 02, 2016 at 02:33:21PM +0200, Igor Mammedov wrote: > > > > > > on old machine types CPU hotplug was uncondtionally > > > > > > enabled since it was introduced, consuming IO ports > > > > > > and providing AML regardles of whether it was actually > > > > > > in use or not. Keep it so for 2.6 and older machines. > > > > > > > > > > > > New machine types will have an option to turn CPU > > > > > > hotplug on if it's needed while by default it stays > > > > > > disabled not consuming extra RAM/IO resources. > > > > > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > > > > > > What if people are using "-machine pc -smp N,max_cpus=M"? > > > > > Shouldn't we at least warning about missing CPU hotplug support > > > > > when using just "max_cpus" with no "cpu-hotplug=on" with pc-2.7 > > > > > and newer? > > > > Yep, I'll add it on next respin. > > > > Would hard error better than warning? > > > > > > Most people don't need cpu hotplug, attempts > > > to hotplug fail, should be enough. > > > > People who don't need CPU hotplug shouldn't be using the max_cpus > > option. > > I agree. > > > I believe we should at least warn people (early, during > > initialization) that their configuration don't make sense > > anymore. > > I think we should try to keep old command line working > if we can. this patch won't break old command line for old machine types, but for new machine type users would need to fix it and be explicit if they want cpu-hotplug. > > > -- > > Eduardo
On Thu, 12 May 2016 10:05:54 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, May 11, 2016 at 08:36:00PM -0300, Eduardo Habkost wrote: > > On Wed, May 11, 2016 at 03:50:39PM +0200, Igor Mammedov wrote: > > > On Tue, 10 May 2016 17:24:14 -0300 > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > On Mon, May 02, 2016 at 02:33:21PM +0200, Igor Mammedov wrote: > > > > > on old machine types CPU hotplug was uncondtionally > > > > > enabled since it was introduced, consuming IO ports > > > > > and providing AML regardles of whether it was actually > > > > > in use or not. Keep it so for 2.6 and older machines. > > > > > > > > > > New machine types will have an option to turn CPU > > > > > hotplug on if it's needed while by default it stays > > > > > disabled not consuming extra RAM/IO resources. > > > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > > > > What if people are using "-machine pc -smp N,max_cpus=M"? > > > > Shouldn't we at least warning about missing CPU hotplug support > > > > when using just "max_cpus" with no "cpu-hotplug=on" with pc-2.7 > > > > and newer? > > > Yep, I'll add it on next respin. > > > Would hard error better than warning? > > > > Not sure. It would break older libvirt versions, wouldn't it? > > But: isn't the new legacy-cpu-hotplug=false default going to > > break old libvirt versions anyway? Should we? if older libvirt are running older machine version, it won't break it. but it would break old libvirt for new machine types. > > > > (CCing libvirt list) > > Even if not, we should not break scripts people use. Maybe it was a > mistake to enable hotplug by default but we can't just remove it now > after all this time. Why not have new hotplug on by default as well? > If you see value in ability to remove this feature, add an option for that. It would save us some space in ACPI tables and IO ports as by default it's not used feature. But if it's preferred to keep cpu-hotplug always on, I'm ok with it too, it's even better as I would be able to drop 3-4 patches from this series. > > > > > > > Should max_cpus > smp_cpus automatically set > > > > cpu-hotplug=on? > > > I'd prefer dumb explicit feature enablement, > > > as it doesn't put any assumptions on other options and > > > QEMU + mgmt don't have to maintain logic for implicit > > > rules that might enable it. > > > > > > and if I didn't manage to push 'device_add x86cpu' in 2.7 time frame, > > > guess work gets a bit confusing with current cpu-add semantic, > > > consider current: > > > > > > SRC-QEMU -smp 1,maxcpus=2 > > > cpu-add 1 > > > DST-QEMU -smp 2,maxcpus=2 > > > > > > vs would be device_add: > > > > > > SRC-QEMU -smp 1,maxcpus=2 > > > device_add cpu > > > DST-QEMU -smp 1,maxcpus=2 -device cpu > > > > > > so instead of qemu/users guessing, I suggest to make it explictly > > > enabled to get feature (which is mostly optional) or > > > cleanly fail qemu start if confusing options are specified > > > with a clear error message. > > > > Agreed we shouldn't encourage people to use the old option to get > > the new behavior. > > > > But I am worried about breaking existing configurations on a > > machine-type change. Is it possible to avoid that? > > > > -- > > Eduardo
On Thu, May 12, 2016 at 12:29:55PM +0200, Igor Mammedov wrote: > On Thu, 12 May 2016 10:07:03 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Wed, May 11, 2016 at 08:28:59PM -0300, Eduardo Habkost wrote: > > > On Thu, May 12, 2016 at 12:51:55AM +0300, Michael S. Tsirkin wrote: > > > > On Wed, May 11, 2016 at 03:50:39PM +0200, Igor Mammedov wrote: > > > > > On Tue, 10 May 2016 17:24:14 -0300 > > > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > > > > > On Mon, May 02, 2016 at 02:33:21PM +0200, Igor Mammedov wrote: > > > > > > > on old machine types CPU hotplug was uncondtionally > > > > > > > enabled since it was introduced, consuming IO ports > > > > > > > and providing AML regardles of whether it was actually > > > > > > > in use or not. Keep it so for 2.6 and older machines. > > > > > > > > > > > > > > New machine types will have an option to turn CPU > > > > > > > hotplug on if it's needed while by default it stays > > > > > > > disabled not consuming extra RAM/IO resources. > > > > > > > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > > > > > > > > What if people are using "-machine pc -smp N,max_cpus=M"? > > > > > > Shouldn't we at least warning about missing CPU hotplug support > > > > > > when using just "max_cpus" with no "cpu-hotplug=on" with pc-2.7 > > > > > > and newer? > > > > > Yep, I'll add it on next respin. > > > > > Would hard error better than warning? > > > > > > > > Most people don't need cpu hotplug, attempts > > > > to hotplug fail, should be enough. > > > > > > People who don't need CPU hotplug shouldn't be using the max_cpus > > > option. > > > > I agree. > > > > > I believe we should at least warn people (early, during > > > initialization) that their configuration don't make sense > > > anymore. > > > > I think we should try to keep old command line working > > if we can. > this patch won't break old command line for old machine types, > but for new machine type users would need to fix it > and be explicit if they want cpu-hotplug. I don't think we should do that, unless users already had time to update their scripts and libvirt had time to implement code supporting the new method. I believe libvirt (and people's scripts) use maxcpus only when they want CPU hotplug, so making max_cpus > smp_cpus enable CPU hotplug implicitly would probably solve the compatibility issue. If we want to deprecate the use of maxcpus to enable CPU hotplug, then we can make it print a warning for a few releases, so people have time to update their code.
On Thu, May 12, 2016 at 07:52:23 -0300, Eduardo Habkost wrote: > I don't think we should do that, unless users already had time to > update their scripts and libvirt had time to implement code > supporting the new method. > > I believe libvirt (and people's scripts) use maxcpus only when > they want CPU hotplug, so making max_cpus > smp_cpus enable CPU > hotplug implicitly would probably solve the compatibility issue. Libvirt uses maxcpus only if the configuration explicitly has less active cpus than the maximum number. This option would be the best IMO. > If we want to deprecate the use of maxcpus to enable CPU hotplug, > then we can make it print a warning for a few releases, so people > have time to update their code. At that point libvirt also needs a way to detect that the new argument is supported by qemu, so we can start passing it on the command line basically every time we now pass 'maxcpus'. The warning will get most probably ignored by people using libvirt as the stdout/err of qemu is not visible to them. Peter
On Thu, 12 May 2016 13:04:23 +0200 Peter Krempa <pkrempa@redhat.com> wrote: > On Thu, May 12, 2016 at 07:52:23 -0300, Eduardo Habkost wrote: > > I don't think we should do that, unless users already had time to > > update their scripts and libvirt had time to implement code > > supporting the new method. > > > > I believe libvirt (and people's scripts) use maxcpus only when > > they want CPU hotplug, so making max_cpus > smp_cpus enable CPU > > hotplug implicitly would probably solve the compatibility issue. > > Libvirt uses maxcpus only if the configuration explicitly has less > active cpus than the maximum number. This option would be the best IMO. > > > If we want to deprecate the use of maxcpus to enable CPU hotplug, > > then we can make it print a warning for a few releases, so people > > have time to update their code. > > At that point libvirt also needs a way to detect that the new argument > is supported by qemu, so we can start passing it on the command line > basically every time we now pass 'maxcpus'. > > The warning will get most probably ignored by people using libvirt as > the stdout/err of qemu is not visible to them. Ok, to make things less complicated I'll drop machine.cpu-hotplug and leave it always enabled as it used to be and as Michael suggested. I'll drop following patches: 12, 13, 14, 20, 23 and respin series > > Peter
On Thu, May 12, 2016 at 15:20:51 +0200, Igor Mammedov wrote: > On Thu, 12 May 2016 13:04:23 +0200 > Peter Krempa <pkrempa@redhat.com> wrote: > > > On Thu, May 12, 2016 at 07:52:23 -0300, Eduardo Habkost wrote: > > > I don't think we should do that, unless users already had time to > > > update their scripts and libvirt had time to implement code > > > supporting the new method. > > > > > > I believe libvirt (and people's scripts) use maxcpus only when > > > they want CPU hotplug, so making max_cpus > smp_cpus enable CPU > > > hotplug implicitly would probably solve the compatibility issue. > > > > Libvirt uses maxcpus only if the configuration explicitly has less > > active cpus than the maximum number. This option would be the best IMO. > > > > > If we want to deprecate the use of maxcpus to enable CPU hotplug, > > > then we can make it print a warning for a few releases, so people > > > have time to update their code. > > > > At that point libvirt also needs a way to detect that the new argument > > is supported by qemu, so we can start passing it on the command line > > basically every time we now pass 'maxcpus'. > > > > The warning will get most probably ignored by people using libvirt as > > the stdout/err of qemu is not visible to them. > Ok, to make things less complicated I'll drop machine.cpu-hotplug > and leave it always enabled as it used to be and as Michael suggested. > > I'll drop following patches: 12, 13, 14, 20, 23 and respin series I actually don't mind disabling it. But I'd be glad if it was based on the max_cpus value as Eduardo suggested. Peter
On Thu, 12 May 2016 15:27:04 +0200 Peter Krempa <pkrempa@redhat.com> wrote: > On Thu, May 12, 2016 at 15:20:51 +0200, Igor Mammedov wrote: > > On Thu, 12 May 2016 13:04:23 +0200 > > Peter Krempa <pkrempa@redhat.com> wrote: > > > > > On Thu, May 12, 2016 at 07:52:23 -0300, Eduardo Habkost wrote: > > > > I don't think we should do that, unless users already had time to > > > > update their scripts and libvirt had time to implement code > > > > supporting the new method. > > > > > > > > I believe libvirt (and people's scripts) use maxcpus only when > > > > they want CPU hotplug, so making max_cpus > smp_cpus enable CPU > > > > hotplug implicitly would probably solve the compatibility issue. > > > > > > Libvirt uses maxcpus only if the configuration explicitly has less > > > active cpus than the maximum number. This option would be the best IMO. > > > > > > > If we want to deprecate the use of maxcpus to enable CPU hotplug, > > > > then we can make it print a warning for a few releases, so people > > > > have time to update their code. > > > > > > At that point libvirt also needs a way to detect that the new argument > > > is supported by qemu, so we can start passing it on the command line > > > basically every time we now pass 'maxcpus'. > > > > > > The warning will get most probably ignored by people using libvirt as > > > the stdout/err of qemu is not visible to them. > > Ok, to make things less complicated I'll drop machine.cpu-hotplug > > and leave it always enabled as it used to be and as Michael suggested. > > > > I'll drop following patches: 12, 13, 14, 20, 23 and respin series > > I actually don't mind disabling it. But I'd be glad if it was based on > the max_cpus value as Eduardo suggested. I've already dropped it, and simplifies series quite a bit. So lets continue without disabling for now. Later we can consider disabling it if we agree on conditions when it should happen. smp_cpus < max_cpus is not a good enough I think: > On Wed, 11 May 2016 15:50:39 +0200 > Igor Mammedov <imammedo@redhat.com> wrote: > > > On Tue, 10 May 2016 17:24:14 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > [...] > > > Should max_cpus > smp_cpus automatically set > > > cpu-hotplug=on? > > I'd prefer dumb explicit feature enablement, > > as it doesn't put any assumptions on other options and > > QEMU + mgmt don't have to maintain logic for implicit > > rules that might enable it. > > > > and if I didn't manage to push 'device_add x86cpu' in 2.7 time frame, > > guess work gets a bit confusing with current cpu-add semantic, > > consider current: > > > > SRC-QEMU -smp 1,maxcpus=2 > > cpu-add 1 > > DST-QEMU -smp 2,maxcpus=2 > > > > vs would be device_add: > > > > SRC-QEMU -smp 1,maxcpus=2 > > device_add cpu > > DST-QEMU -smp 1,maxcpus=2 -device cpu > > > > so instead of qemu/users guessing, I suggest to make it explictly > > enabled to get feature (which is mostly optional) or > > cleanly fail qemu start if confusing options are specified > > with a clear error message.
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c index 1cfe832..b5481d4 100644 --- a/hw/acpi/ich9.c +++ b/hw/acpi/ich9.c @@ -273,8 +273,10 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm, pm->powerdown_notifier.notify = pm_powerdown_req; qemu_register_powerdown_notifier(&pm->powerdown_notifier); - legacy_acpi_cpu_hotplug_init(pci_address_space_io(lpc_pci), - OBJECT(lpc_pci), &pm->gpe_cpu, ICH9_CPU_HOTPLUG_IO_BASE); + if (pm->cpu_hotplug_legacy) { + legacy_acpi_cpu_hotplug_init(pci_address_space_io(lpc_pci), + OBJECT(lpc_pci), &pm->gpe_cpu, ICH9_CPU_HOTPLUG_IO_BASE); + } if (pm->acpi_memory_hotplug.is_enabled) { acpi_memory_hotplug_init(pci_address_space_io(lpc_pci), OBJECT(lpc_pci), @@ -455,7 +457,8 @@ void ich9_pm_device_plug_cb(ICH9LPCPMRegs *pm, DeviceState *dev, Error **errp) object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { acpi_memory_plug_cb(&pm->acpi_regs, pm->irq, &pm->acpi_memory_hotplug, dev, errp); - } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { + } else if (pm->cpu_hotplug_legacy && + object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { legacy_acpi_cpu_plug_cb(&pm->acpi_regs, pm->irq, &pm->gpe_cpu, dev, errp); } else { diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c index 7b5c312..86d68dd 100644 --- a/hw/acpi/piix4.c +++ b/hw/acpi/piix4.c @@ -352,7 +352,8 @@ static void piix4_device_plug_cb(HotplugHandler *hotplug_dev, } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) { acpi_pcihp_device_plug_cb(&s->ar, s->irq, &s->acpi_pci_hotplug, dev, errp); - } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { + } else if (s->cpu_hotplug_legacy && + object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { legacy_acpi_cpu_plug_cb(&s->ar, s->irq, &s->gpe_cpu, dev, errp); } else { error_setg(errp, "acpi: device plug request for not supported device" @@ -571,8 +572,10 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent, acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent, s->use_acpi_pci_hotplug); - legacy_acpi_cpu_hotplug_init(parent, OBJECT(s), &s->gpe_cpu, - PIIX4_CPU_HOTPLUG_IO_BASE); + if (s->cpu_hotplug_legacy) { + legacy_acpi_cpu_hotplug_init(parent, OBJECT(s), &s->gpe_cpu, + PIIX4_CPU_HOTPLUG_IO_BASE); + } if (s->acpi_memory_hotplug.is_enabled) { acpi_memory_hotplug_init(parent, OBJECT(s), &s->acpi_memory_hotplug); diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 4a7eab3..502f1a7 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -93,6 +93,7 @@ typedef struct AcpiPmInfo { uint32_t gpe0_blk; uint32_t gpe0_blk_len; uint32_t io_base; + bool legacy_cpu_hp; uint16_t cpu_hp_io_base; uint16_t mem_hp_io_base; uint16_t mem_hp_io_len; @@ -141,6 +142,9 @@ static void acpi_get_pm_info(AcpiPmInfo *pm) } assert(obj); + pm->legacy_cpu_hp = object_property_get_bool(obj, "cpu-hotplug-legacy", + NULL); + pm->mem_hp_io_base = ACPI_MEMORY_HOTPLUG_BASE; pm->mem_hp_io_len = ACPI_MEMORY_HOTPLUG_IO_LEN; @@ -1933,7 +1937,9 @@ build_dsdt(GArray *table_data, GArray *linker, build_q35_pci0_int(dsdt); } - build_legacy_cpu_hotplug_aml(dsdt, machine, pm->cpu_hp_io_base); + if (pm->legacy_cpu_hp) { + build_legacy_cpu_hotplug_aml(dsdt, machine, pm->cpu_hp_io_base); + } build_memory_hotplug_aml(dsdt, nr_mem, pm->mem_hp_io_base, pm->mem_hp_io_len); diff --git a/include/hw/compat.h b/include/hw/compat.h index 636befe..f8c662d 100644 --- a/include/hw/compat.h +++ b/include/hw/compat.h @@ -2,7 +2,16 @@ #define HW_COMPAT_H #define HW_COMPAT_2_6 \ - /* empty */ + {\ + .driver = "PIIX4_PM",\ + .property = "cpu-hotplug-legacy",\ + .value = "on",\ + },\ + {\ + .driver = "ICH9-LPC",\ + .property = "cpu-hotplug-legacy",\ + .value = "on",\ + },\ #define HW_COMPAT_2_5 \ {\
on old machine types CPU hotplug was uncondtionally enabled since it was introduced, consuming IO ports and providing AML regardles of whether it was actually in use or not. Keep it so for 2.6 and older machines. New machine types will have an option to turn CPU hotplug on if it's needed while by default it stays disabled not consuming extra RAM/IO resources. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- hw/acpi/ich9.c | 9 ++++++--- hw/acpi/piix4.c | 9 ++++++--- hw/i386/acpi-build.c | 8 +++++++- include/hw/compat.h | 11 ++++++++++- 4 files changed, 29 insertions(+), 8 deletions(-)