[Qemu-devel] [PATCH v5 0/2] deprecate incorrect CPUs topology

[Qemu-devel] [PATCH v5 0/2] deprecate incorrect CPUs topology

[Igor Mammedov]

Changelog since v4:
  * extend deprication doc, adding that maxcpus should be multiple of
        present on CLI [sockets/cores/threads] options
        (Eduardo Habkost <ehabkost@redhat.com>)

series bundles together 2 related patches posted separately earlier:
  vl.c deprecate incorrect CPUs topology
  vl:c: make sure that sockets are calculated  correctly in '-smp X' case

Goal is to depricate invalid topologies so we could make sure that topology
configuration is correct by forbidding invalid input once deprecation
period ends.

---
CC: libvir-list@redhat.com
CC: drjones@redhat.com
CC: ehabkost@redhat.com


Igor Mammedov (2):
  vl.c deprecate incorrect CPUs topology
  vl:c: make sure that sockets are calculated correctly in '-smp X' case

 qemu-deprecated.texi | 19 +++++++++++++++++++
 vl.c                 | 12 +++++++++++-
 2 files changed, 30 insertions(+), 1 deletion(-)

-- 
2.7.4

[Qemu-devel] [PATCH v5 1/2] vl.c deprecate incorrect CPUs topology

[Igor Mammedov]

-smp [cpus],sockets/cores/threads[,maxcpus] should describe topology
so that total number of logical CPUs [sockets * cores * threads]
would be equal to [maxcpus], however historically we didn't have
such check in QEMU and it is possible to start VM with an invalid
topology.
Deprecate invalid options combination so we can make sure that
the topology VM started with is always correct in the future.
Users with an invalid sockets/cores/threads/maxcpus values should
fix their CLI to make sure that
   [sockets * cores * threads] == [maxcpus]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v5:
  - extend deprecation doc, adding that maxcpus should be multiple of
    present on CLI [sockets/cores/threads] options
    (Eduardo Habkost <ehabkost@redhat.com>)
v4:
  - missed dot comment, fix it with s/./,/ (Andrew Jones <drjones@redhat.com>)
v3:
  - more spelling fixes (Andrew Jones <drjones@redhat.com>)
  - place deprecation check after (sockets * cores * threads > max_cpus) check
    (Eduardo Habkost <ehabkost@redhat.com>)
v2:
  - spelling&&co fixes (Andrew Jones <drjones@redhat.com>)
---
 qemu-deprecated.texi | 19 +++++++++++++++++++
 vl.c                 |  7 +++++++
 2 files changed, 26 insertions(+)

diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 87212b6..827c3ce 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -159,6 +159,25 @@ The 'file' driver for drives is no longer appropriate for character or host
 devices and will only accept regular files (S_IFREG). The correct driver
 for these file types is 'host_cdrom' or 'host_device' as appropriate.
 
+@subsection -smp X,[socket=a,core=b,thread=c],maxcpus=Y (since 3.1)
+
+CPU topology properties should describe whole machine topology including
+possible CPUs, but historically it was possible to start QEMU with
+an incorrect topology where
+  sockets * cores * threads >= X && X < maxcpus
+which could lead to an incorrect topology enumeration by the guest.
+Support for invalid topologies will be removed, the user must ensure
+topologies described with -smp include all possible cpus, i.e.
+  sockets * cores * threads == maxcpus
+Note: it's assumed that maxcpus value must be multiple of the topology
+options present on command line to avoid creating an invalid topology.
+If maxcpus isn't be multiple of present topology options then the condition
+(sockets * cores * threads == maxcpus) can't be satisfied and it will
+trigger deprecation warning which later will be converted to a error.
+If you get deprecation warning it's recommended to explicitly specify
+a correct topology to make warning go away and ensure that it will
+continue working in the future.
+
 @section QEMU Machine Protocol (QMP) commands
 
 @subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
diff --git a/vl.c b/vl.c
index 5ba06ad..7fd700e 100644
--- a/vl.c
+++ b/vl.c
@@ -1246,6 +1246,13 @@ static void smp_parse(QemuOpts *opts)
             exit(1);
         }
 
+        if (sockets * cores * threads != max_cpus) {
+            warn_report("Invalid CPU topology deprecated: "
+                        "sockets (%u) * cores (%u) * threads (%u) "
+                        "!= maxcpus (%u)",
+                        sockets, cores, threads, max_cpus);
+        }
+
         smp_cpus = cpus;
         smp_cores = cores;
         smp_threads = threads;
-- 
2.7.4

[Qemu-devel] [PATCH v5 2/2] vl:c: make sure that sockets are calculated correctly in '-smp X' case

[Igor Mammedov]

commit
  (5cdc9b76e3 vl.c: Remove dead assignment)
removed sockets calculation when 'sockets' weren't provided on CLI
since there wasn't any users for it back then. Exiting checks
are neither reachable
   } else if (sockets * cores * threads < cpus) {
or nor triggable
   if (sockets * cores * threads > max_cpus)
so we weren't noticing wrong topology since then, since users
recalculate sockets adhoc on their own.

However with deprecation check it becomes noticable, for example
  -smp 2
will start printing warning:
  "warning: Invalid CPU topology deprecated: sockets (1) * cores (1) * threads (1) != maxcpus (2)"
calculating sockets if they weren't specified.

Fix it by returning back sockets calculation if it's omited on CLI.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 vl.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index 7fd700e..333d638 100644
--- a/vl.c
+++ b/vl.c
@@ -1210,11 +1210,14 @@ static void smp_parse(QemuOpts *opts)
 
         /* compute missing values, prefer sockets over cores over threads */
         if (cpus == 0 || sockets == 0) {
-            sockets = sockets > 0 ? sockets : 1;
             cores = cores > 0 ? cores : 1;
             threads = threads > 0 ? threads : 1;
             if (cpus == 0) {
+                sockets = sockets > 0 ? sockets : 1;
                 cpus = cores * threads * sockets;
+            } else {
+                max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
+                sockets = !sockets ? max_cpus / (cores * threads) : sockets;
             }
         } else if (cores == 0) {
             threads = threads > 0 ? threads : 1;
-- 
2.7.4

Re: [Qemu-devel] [PATCH v5 1/2] vl.c deprecate incorrect CPUs topology

[Andrew Jones]

On Tue, Sep 04, 2018 at 03:22:35PM +0200, Igor Mammedov wrote:
> -smp [cpus],sockets/cores/threads[,maxcpus] should describe topology
> so that total number of logical CPUs [sockets * cores * threads]
> would be equal to [maxcpus], however historically we didn't have
> such check in QEMU and it is possible to start VM with an invalid
> topology.
> Deprecate invalid options combination so we can make sure that
> the topology VM started with is always correct in the future.
> Users with an invalid sockets/cores/threads/maxcpus values should
> fix their CLI to make sure that
>    [sockets * cores * threads] == [maxcpus]
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v5:
>   - extend deprecation doc, adding that maxcpus should be multiple of
>     present on CLI [sockets/cores/threads] options
>     (Eduardo Habkost <ehabkost@redhat.com>)
> v4:
>   - missed dot comment, fix it with s/./,/ (Andrew Jones <drjones@redhat.com>)
> v3:
>   - more spelling fixes (Andrew Jones <drjones@redhat.com>)
>   - place deprecation check after (sockets * cores * threads > max_cpus) check
>     (Eduardo Habkost <ehabkost@redhat.com>)
> v2:
>   - spelling&&co fixes (Andrew Jones <drjones@redhat.com>)
> ---
>  qemu-deprecated.texi | 19 +++++++++++++++++++
>  vl.c                 |  7 +++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 87212b6..827c3ce 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -159,6 +159,25 @@ The 'file' driver for drives is no longer appropriate for character or host
>  devices and will only accept regular files (S_IFREG). The correct driver
>  for these file types is 'host_cdrom' or 'host_device' as appropriate.
>  
> +@subsection -smp X,[socket=a,core=b,thread=c],maxcpus=Y (since 3.1)

sockets,cores,threads -- they should all be plural

> +
> +CPU topology properties should describe whole machine topology including
> +possible CPUs, but historically it was possible to start QEMU with
> +an incorrect topology where
> +  sockets * cores * threads >= X && X < maxcpus
> +which could lead to an incorrect topology enumeration by the guest.
> +Support for invalid topologies will be removed, the user must ensure
> +topologies described with -smp include all possible cpus, i.e.
> +  sockets * cores * threads == maxcpus
> +Note: it's assumed that maxcpus value must be multiple of the topology
> +options present on command line to avoid creating an invalid topology.
> +If maxcpus isn't be multiple of present topology options then the condition
> +(sockets * cores * threads == maxcpus) can't be satisfied and it will
> +trigger deprecation warning which later will be converted to a error.
> +If you get deprecation warning it's recommended to explicitly specify
> +a correct topology to make warning go away and ensure that it will
> +continue working in the future.
> +
>  @section QEMU Machine Protocol (QMP) commands
>  
>  @subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
> diff --git a/vl.c b/vl.c
> index 5ba06ad..7fd700e 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1246,6 +1246,13 @@ static void smp_parse(QemuOpts *opts)
>              exit(1);
>          }
>  
> +        if (sockets * cores * threads != max_cpus) {
> +            warn_report("Invalid CPU topology deprecated: "
> +                        "sockets (%u) * cores (%u) * threads (%u) "
> +                        "!= maxcpus (%u)",
> +                        sockets, cores, threads, max_cpus);
> +        }
> +
>          smp_cpus = cpus;
>          smp_cores = cores;
>          smp_threads = threads;
> -- 
> 2.7.4
> 
>

Otherwise
 
Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [Qemu-devel] [PATCH v5 2/2] vl:c: make sure that sockets are calculated correctly in '-smp X' case

[Andrew Jones]

On Tue, Sep 04, 2018 at 03:22:36PM +0200, Igor Mammedov wrote:
> commit
>   (5cdc9b76e3 vl.c: Remove dead assignment)
> removed sockets calculation when 'sockets' weren't provided on CLI
> since there wasn't any users for it back then. Exiting checks
> are neither reachable
>    } else if (sockets * cores * threads < cpus) {
> or nor triggable
>    if (sockets * cores * threads > max_cpus)
> so we weren't noticing wrong topology since then, since users
> recalculate sockets adhoc on their own.
> 
> However with deprecation check it becomes noticable, for example
>   -smp 2
> will start printing warning:
>   "warning: Invalid CPU topology deprecated: sockets (1) * cores (1) * threads (1) != maxcpus (2)"
> calculating sockets if they weren't specified.
> 
> Fix it by returning back sockets calculation if it's omited on CLI.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  vl.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/vl.c b/vl.c
> index 7fd700e..333d638 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1210,11 +1210,14 @@ static void smp_parse(QemuOpts *opts)
>  
>          /* compute missing values, prefer sockets over cores over threads */
>          if (cpus == 0 || sockets == 0) {
> -            sockets = sockets > 0 ? sockets : 1;
>              cores = cores > 0 ? cores : 1;
>              threads = threads > 0 ? threads : 1;
>              if (cpus == 0) {
> +                sockets = sockets > 0 ? sockets : 1;
>                  cpus = cores * threads * sockets;
> +            } else {
> +                max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
> +                sockets = !sockets ? max_cpus / (cores * threads) : sockets;
>              }
>          } else if (cores == 0) {
>              threads = threads > 0 ? threads : 1;
> -- 
> 2.7.4
> 
>
 
Reviewed-by: Andrew Jones <drjones@redhat.com>

[Qemu-devel] [PATCH v6 1/2] vl.c deprecate incorrect CPUs topology

[Igor Mammedov]

-smp [cpus],sockets/cores/threads[,maxcpus] should describe topology
so that total number of logical CPUs [sockets * cores * threads]
would be equal to [maxcpus], however historically we didn't have
such check in QEMU and it is possible to start VM with an invalid
topology.
Deprecate invalid options combination so we can make sure that
the topology VM started with is always correct in the future.
Users with an invalid sockets/cores/threads/maxcpus values should
fix their CLI to make sure that
   [sockets * cores * threads] == [maxcpus]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
---
v6:
  - s/socket/sockets/, the same for core, thread in subsection
    (Andrew Jones <drjones@redhat.com>)
v5:
  - extend deprecation doc, adding that maxcpus should be multiple of
    present on CLI [sockets/cores/threads] options
    (Eduardo Habkost <ehabkost@redhat.com>)
v4:
  - missed dot comment, fix it with s/./,/ (Andrew Jones <drjones@redhat.com>)
v3:
  - more spelling fixes (Andrew Jones <drjones@redhat.com>)
  - place deprecation check after (sockets * cores * threads > max_cpus) check
    (Eduardo Habkost <ehabkost@redhat.com>)
v2:
  - spelling&&co fixes (Andrew Jones <drjones@redhat.com>)
---
 qemu-deprecated.texi | 19 +++++++++++++++++++
 vl.c                 |  7 +++++++
 2 files changed, 26 insertions(+)

diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 87212b6..1fac52f 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -159,6 +159,25 @@ The 'file' driver for drives is no longer appropriate for character or host
 devices and will only accept regular files (S_IFREG). The correct driver
 for these file types is 'host_cdrom' or 'host_device' as appropriate.
 
+@subsection -smp X,[sockets=a,cores=b,threads=c],maxcpus=Y (since 3.1)
+
+CPU topology properties should describe whole machine topology including
+possible CPUs, but historically it was possible to start QEMU with
+an incorrect topology where
+  sockets * cores * threads >= X && X < maxcpus
+which could lead to an incorrect topology enumeration by the guest.
+Support for invalid topologies will be removed, the user must ensure
+topologies described with -smp include all possible cpus, i.e.
+  sockets * cores * threads == maxcpus
+Note: it's assumed that maxcpus value must be multiple of the topology
+options present on command line to avoid creating an invalid topology.
+If maxcpus isn't be multiple of present topology options then the condition
+(sockets * cores * threads == maxcpus) can't be satisfied and it will
+trigger deprecation warning which later will be converted to a error.
+If you get deprecation warning it's recommended to explicitly specify
+a correct topology to make warning go away and ensure that it will
+continue working in the future.
+
 @section QEMU Machine Protocol (QMP) commands
 
 @subsection block-dirty-bitmap-add "autoload" parameter (since 2.12.0)
diff --git a/vl.c b/vl.c
index 5ba06ad..7fd700e 100644
--- a/vl.c
+++ b/vl.c
@@ -1246,6 +1246,13 @@ static void smp_parse(QemuOpts *opts)
             exit(1);
         }
 
+        if (sockets * cores * threads != max_cpus) {
+            warn_report("Invalid CPU topology deprecated: "
+                        "sockets (%u) * cores (%u) * threads (%u) "
+                        "!= maxcpus (%u)",
+                        sockets, cores, threads, max_cpus);
+        }
+
         smp_cpus = cpus;
         smp_cores = cores;
         smp_threads = threads;
-- 
2.7.4

Re: [Qemu-devel] [PATCH v5 2/2] vl:c: make sure that sockets are calculated correctly in '-smp X' case

[Eduardo Habkost]

On Tue, Sep 04, 2018 at 05:18:48PM +0200, Andrew Jones wrote:
> On Tue, Sep 04, 2018 at 03:22:36PM +0200, Igor Mammedov wrote:
> > commit
> >   (5cdc9b76e3 vl.c: Remove dead assignment)
> > removed sockets calculation when 'sockets' weren't provided on CLI
> > since there wasn't any users for it back then. Exiting checks
> > are neither reachable
> >    } else if (sockets * cores * threads < cpus) {
> > or nor triggable
> >    if (sockets * cores * threads > max_cpus)
> > so we weren't noticing wrong topology since then, since users
> > recalculate sockets adhoc on their own.
> > 
> > However with deprecation check it becomes noticable, for example
> >   -smp 2
> > will start printing warning:
> >   "warning: Invalid CPU topology deprecated: sockets (1) * cores (1) * threads (1) != maxcpus (2)"
> > calculating sockets if they weren't specified.
> > 
> > Fix it by returning back sockets calculation if it's omited on CLI.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  vl.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/vl.c b/vl.c
> > index 7fd700e..333d638 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -1210,11 +1210,14 @@ static void smp_parse(QemuOpts *opts)
> >  
> >          /* compute missing values, prefer sockets over cores over threads */
> >          if (cpus == 0 || sockets == 0) {
> > -            sockets = sockets > 0 ? sockets : 1;
> >              cores = cores > 0 ? cores : 1;
> >              threads = threads > 0 ? threads : 1;
> >              if (cpus == 0) {
> > +                sockets = sockets > 0 ? sockets : 1;
> >                  cpus = cores * threads * sockets;
> > +            } else {
> > +                max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
> > +                sockets = !sockets ? max_cpus / (cores * threads) : sockets;

Why the !sockets check here?  If we have reached this statement
we know that (cpus == 0 || sockets == 0) is true and
(cpus == 0) is false, so sockets is guaranteed to be 0.

Sorry for not spotting this earlier.

> >              }
> >          } else if (cores == 0) {
> >              threads = threads > 0 ? threads : 1;
> > -- 
> > 2.7.4
> > 
> >
>  
> Reviewed-by: Andrew Jones <drjones@redhat.com>

-- 
Eduardo

Re: [Qemu-devel] [PATCH v5 1/2] vl.c deprecate incorrect CPUs topology

[Eduardo Habkost]

On Tue, Sep 04, 2018 at 03:22:35PM +0200, Igor Mammedov wrote:
> -smp [cpus],sockets/cores/threads[,maxcpus] should describe topology
> so that total number of logical CPUs [sockets * cores * threads]
> would be equal to [maxcpus], however historically we didn't have
> such check in QEMU and it is possible to start VM with an invalid
> topology.
> Deprecate invalid options combination so we can make sure that
> the topology VM started with is always correct in the future.
> Users with an invalid sockets/cores/threads/maxcpus values should
> fix their CLI to make sure that
>    [sockets * cores * threads] == [maxcpus]
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v5:
>   - extend deprecation doc, adding that maxcpus should be multiple of
>     present on CLI [sockets/cores/threads] options
>     (Eduardo Habkost <ehabkost@redhat.com>)
> v4:
>   - missed dot comment, fix it with s/./,/ (Andrew Jones <drjones@redhat.com>)
> v3:
>   - more spelling fixes (Andrew Jones <drjones@redhat.com>)
>   - place deprecation check after (sockets * cores * threads > max_cpus) check
>     (Eduardo Habkost <ehabkost@redhat.com>)
> v2:
>   - spelling&&co fixes (Andrew Jones <drjones@redhat.com>)
> ---
>  qemu-deprecated.texi | 19 +++++++++++++++++++
>  vl.c                 |  7 +++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 87212b6..827c3ce 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -159,6 +159,25 @@ The 'file' driver for drives is no longer appropriate for character or host
>  devices and will only accept regular files (S_IFREG). The correct driver
>  for these file types is 'host_cdrom' or 'host_device' as appropriate.
>  
> +@subsection -smp X,[socket=a,core=b,thread=c],maxcpus=Y (since 3.1)

Minor: I suggest using @var markup, or maybe just use
"-smp (invalid topologies) (since 3.1)" as subsection title for simplicity?

> +
> +CPU topology properties should describe whole machine topology including
> +possible CPUs, but historically it was possible to start QEMU with
> +an incorrect topology where
> +  sockets * cores * threads >= X && X < maxcpus

Minor: this line formatting is lost on the HTML output.  I
suggest using @var and/or @math.

Minor: I suggest not using C syntax.

i.e. use something like:

  @math{@var{n} <= @var{sockets} * @var{cores} * @var{threads} < @var{maxcpus}},

> +which could lead to an incorrect topology enumeration by the guest.
> +Support for invalid topologies will be removed, the user must ensure
> +topologies described with -smp include all possible cpus, i.e.
> +  sockets * cores * threads == maxcpus

Minor: same as above.  I suggest:

  @math{@var{sockets} * @var{cores} * @var{threads} = @var{maxcpus}}.


> +Note: it's assumed that maxcpus value must be multiple of the topology
> +options present on command line to avoid creating an invalid topology.
> +If maxcpus isn't be multiple of present topology options then the condition
> +(sockets * cores * threads == maxcpus) can't be satisfied and it will
> +trigger deprecation warning which later will be converted to a error.
> +If you get deprecation warning it's recommended to explicitly specify
> +a correct topology to make warning go away and ensure that it will
> +continue working in the future.

I don't understand the purpose of the "Note:" section.  It seems to duplicate
what was already said in the lines above it.  Can we just remove it?

These are just suggestions in case you are going to respin the series, not
blockers.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v5 1/2] vl.c deprecate incorrect CPUs topology

[Igor Mammedov]

On Tue, 4 Sep 2018 23:12:55 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Sep 04, 2018 at 03:22:35PM +0200, Igor Mammedov wrote:
> > -smp [cpus],sockets/cores/threads[,maxcpus] should describe topology
> > so that total number of logical CPUs [sockets * cores * threads]
> > would be equal to [maxcpus], however historically we didn't have
> > such check in QEMU and it is possible to start VM with an invalid
> > topology.
> > Deprecate invalid options combination so we can make sure that
> > the topology VM started with is always correct in the future.
> > Users with an invalid sockets/cores/threads/maxcpus values should
> > fix their CLI to make sure that
> >    [sockets * cores * threads] == [maxcpus]
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v5:
> >   - extend deprecation doc, adding that maxcpus should be multiple of
> >     present on CLI [sockets/cores/threads] options
> >     (Eduardo Habkost <ehabkost@redhat.com>)
> > v4:
> >   - missed dot comment, fix it with s/./,/ (Andrew Jones <drjones@redhat.com>)
> > v3:
> >   - more spelling fixes (Andrew Jones <drjones@redhat.com>)
> >   - place deprecation check after (sockets * cores * threads > max_cpus) check
> >     (Eduardo Habkost <ehabkost@redhat.com>)
> > v2:
> >   - spelling&&co fixes (Andrew Jones <drjones@redhat.com>)
> > ---
> >  qemu-deprecated.texi | 19 +++++++++++++++++++
> >  vl.c                 |  7 +++++++
> >  2 files changed, 26 insertions(+)
> > 
> > diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> > index 87212b6..827c3ce 100644
> > --- a/qemu-deprecated.texi
> > +++ b/qemu-deprecated.texi
> > @@ -159,6 +159,25 @@ The 'file' driver for drives is no longer appropriate for character or host
> >  devices and will only accept regular files (S_IFREG). The correct driver
> >  for these file types is 'host_cdrom' or 'host_device' as appropriate.
> >  
> > +@subsection -smp X,[socket=a,core=b,thread=c],maxcpus=Y (since 3.1)
> 
> Minor: I suggest using @var markup, or maybe just use
> "-smp (invalid topologies) (since 3.1)" as subsection title for simplicity?
> 
> > +
> > +CPU topology properties should describe whole machine topology including
> > +possible CPUs, but historically it was possible to start QEMU with
> > +an incorrect topology where
> > +  sockets * cores * threads >= X && X < maxcpus
> 
> Minor: this line formatting is lost on the HTML output.  I
> suggest using @var and/or @math.
> 
> Minor: I suggest not using C syntax.
> 
> i.e. use something like:
> 
>   @math{@var{n} <= @var{sockets} * @var{cores} * @var{threads} < @var{maxcpus}},
> 
> > +which could lead to an incorrect topology enumeration by the guest.
> > +Support for invalid topologies will be removed, the user must ensure
> > +topologies described with -smp include all possible cpus, i.e.
> > +  sockets * cores * threads == maxcpus
> 
> Minor: same as above.  I suggest:
> 
>   @math{@var{sockets} * @var{cores} * @var{threads} = @var{maxcpus}}.
> 
> 
> > +Note: it's assumed that maxcpus value must be multiple of the topology
> > +options present on command line to avoid creating an invalid topology.
> > +If maxcpus isn't be multiple of present topology options then the condition
> > +(sockets * cores * threads == maxcpus) can't be satisfied and it will
> > +trigger deprecation warning which later will be converted to a error.
> > +If you get deprecation warning it's recommended to explicitly specify
> > +a correct topology to make warning go away and ensure that it will
> > +continue working in the future.
> 
> I don't understand the purpose of the "Note:" section.  It seems to duplicate
> what was already said in the lines above it.  Can we just remove it?
Well, didn't I say that no additional explanation needed in v4?
Note section was added per your suggestion to explicitly say that maxcpus
should be multiple of options present on CLI.

> 
> These are just suggestions in case you are going to respin the series, not
> blockers.
> 

Re: [Qemu-devel] [PATCH v5 2/2] vl:c: make sure that sockets are calculated correctly in '-smp X' case

[Igor Mammedov]

On Tue, 4 Sep 2018 22:40:49 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Sep 04, 2018 at 05:18:48PM +0200, Andrew Jones wrote:
> > On Tue, Sep 04, 2018 at 03:22:36PM +0200, Igor Mammedov wrote:
> > > commit
> > >   (5cdc9b76e3 vl.c: Remove dead assignment)
> > > removed sockets calculation when 'sockets' weren't provided on CLI
> > > since there wasn't any users for it back then. Exiting checks
> > > are neither reachable
> > >    } else if (sockets * cores * threads < cpus) {
> > > or nor triggable
> > >    if (sockets * cores * threads > max_cpus)
> > > so we weren't noticing wrong topology since then, since users
> > > recalculate sockets adhoc on their own.
> > > 
> > > However with deprecation check it becomes noticable, for example
> > >   -smp 2
> > > will start printing warning:
> > >   "warning: Invalid CPU topology deprecated: sockets (1) * cores (1) * threads (1) != maxcpus (2)"
> > > calculating sockets if they weren't specified.
> > > 
> > > Fix it by returning back sockets calculation if it's omited on CLI.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  vl.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/vl.c b/vl.c
> > > index 7fd700e..333d638 100644
> > > --- a/vl.c
> > > +++ b/vl.c
> > > @@ -1210,11 +1210,14 @@ static void smp_parse(QemuOpts *opts)
> > >  
> > >          /* compute missing values, prefer sockets over cores over threads */
> > >          if (cpus == 0 || sockets == 0) {
> > > -            sockets = sockets > 0 ? sockets : 1;
> > >              cores = cores > 0 ? cores : 1;
> > >              threads = threads > 0 ? threads : 1;
> > >              if (cpus == 0) {
> > > +                sockets = sockets > 0 ? sockets : 1;
> > >                  cpus = cores * threads * sockets;
> > > +            } else {
> > > +                max_cpus = qemu_opt_get_number(opts, "maxcpus", cpus);
> > > +                sockets = !sockets ? max_cpus / (cores * threads) : sockets;
> 
> Why the !sockets check here?  If we have reached this statement
> we know that (cpus == 0 || sockets == 0) is true and
> (cpus == 0) is false, so sockets is guaranteed to be 0.
Indeed, thanks for spotting it
I'll fix it up and respin

> 
> Sorry for not spotting this earlier.
> 
> > >              }
> > >          } else if (cores == 0) {
> > >              threads = threads > 0 ? threads : 1;
> > > -- 
> > > 2.7.4
> > > 
> > >
> >  
> > Reviewed-by: Andrew Jones <drjones@redhat.com>
> 

Re: [Qemu-devel] [PATCH v5 1/2] vl.c deprecate incorrect CPUs topology

[Eduardo Habkost]

On Wed, Sep 05, 2018 at 11:25:11AM +0200, Igor Mammedov wrote:
> On Tue, 4 Sep 2018 23:12:55 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Tue, Sep 04, 2018 at 03:22:35PM +0200, Igor Mammedov wrote:
> > > -smp [cpus],sockets/cores/threads[,maxcpus] should describe topology
> > > so that total number of logical CPUs [sockets * cores * threads]
> > > would be equal to [maxcpus], however historically we didn't have
> > > such check in QEMU and it is possible to start VM with an invalid
> > > topology.
> > > Deprecate invalid options combination so we can make sure that
> > > the topology VM started with is always correct in the future.
> > > Users with an invalid sockets/cores/threads/maxcpus values should
> > > fix their CLI to make sure that
> > >    [sockets * cores * threads] == [maxcpus]
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > > v5:
> > >   - extend deprecation doc, adding that maxcpus should be multiple of
> > >     present on CLI [sockets/cores/threads] options
> > >     (Eduardo Habkost <ehabkost@redhat.com>)
> > > v4:
> > >   - missed dot comment, fix it with s/./,/ (Andrew Jones <drjones@redhat.com>)
> > > v3:
> > >   - more spelling fixes (Andrew Jones <drjones@redhat.com>)
> > >   - place deprecation check after (sockets * cores * threads > max_cpus) check
> > >     (Eduardo Habkost <ehabkost@redhat.com>)
> > > v2:
> > >   - spelling&&co fixes (Andrew Jones <drjones@redhat.com>)
> > > ---
> > >  qemu-deprecated.texi | 19 +++++++++++++++++++
> > >  vl.c                 |  7 +++++++
> > >  2 files changed, 26 insertions(+)
> > > 
> > > diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> > > index 87212b6..827c3ce 100644
> > > --- a/qemu-deprecated.texi
> > > +++ b/qemu-deprecated.texi
> > > @@ -159,6 +159,25 @@ The 'file' driver for drives is no longer appropriate for character or host
> > >  devices and will only accept regular files (S_IFREG). The correct driver
> > >  for these file types is 'host_cdrom' or 'host_device' as appropriate.
> > >  
> > > +@subsection -smp X,[socket=a,core=b,thread=c],maxcpus=Y (since 3.1)
> > 
> > Minor: I suggest using @var markup, or maybe just use
> > "-smp (invalid topologies) (since 3.1)" as subsection title for simplicity?
> > 
> > > +
> > > +CPU topology properties should describe whole machine topology including
> > > +possible CPUs, but historically it was possible to start QEMU with
> > > +an incorrect topology where
> > > +  sockets * cores * threads >= X && X < maxcpus
> > 
> > Minor: this line formatting is lost on the HTML output.  I
> > suggest using @var and/or @math.
> > 
> > Minor: I suggest not using C syntax.
> > 
> > i.e. use something like:
> > 
> >   @math{@var{n} <= @var{sockets} * @var{cores} * @var{threads} < @var{maxcpus}},
> > 
> > > +which could lead to an incorrect topology enumeration by the guest.
> > > +Support for invalid topologies will be removed, the user must ensure
> > > +topologies described with -smp include all possible cpus, i.e.
> > > +  sockets * cores * threads == maxcpus
> > 
> > Minor: same as above.  I suggest:
> > 
> >   @math{@var{sockets} * @var{cores} * @var{threads} = @var{maxcpus}}.
> > 
> > 
> > > +Note: it's assumed that maxcpus value must be multiple of the topology
> > > +options present on command line to avoid creating an invalid topology.
> > > +If maxcpus isn't be multiple of present topology options then the condition
> > > +(sockets * cores * threads == maxcpus) can't be satisfied and it will
> > > +trigger deprecation warning which later will be converted to a error.
> > > +If you get deprecation warning it's recommended to explicitly specify
> > > +a correct topology to make warning go away and ensure that it will
> > > +continue working in the future.
> > 
> > I don't understand the purpose of the "Note:" section.  It seems to duplicate
> > what was already said in the lines above it.  Can we just remove it?
> Well, didn't I say that no additional explanation needed in v4?
> Note section was added per your suggestion to explicitly say that maxcpus
> should be multiple of options present on CLI.

You are right: I did ask for additional clarification on the
documentation, because it was not clear that the warning can
still appear even if some of the sockets/cores/threads options
were omitted.  The note didn't make that clearer to me, though.

In either case, this can be done on a follow-up patch if
necessary.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [Qemu-devel] [PATCH v5 1/2] vl.c deprecate incorrect CPUs topology

[Igor Mammedov]

On Wed, 5 Sep 2018 10:45:12 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Sep 05, 2018 at 11:25:11AM +0200, Igor Mammedov wrote:
> > On Tue, 4 Sep 2018 23:12:55 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Tue, Sep 04, 2018 at 03:22:35PM +0200, Igor Mammedov wrote:  
> > > > -smp [cpus],sockets/cores/threads[,maxcpus] should describe topology
> > > > so that total number of logical CPUs [sockets * cores * threads]
> > > > would be equal to [maxcpus], however historically we didn't have
> > > > such check in QEMU and it is possible to start VM with an invalid
> > > > topology.
> > > > Deprecate invalid options combination so we can make sure that
> > > > the topology VM started with is always correct in the future.
> > > > Users with an invalid sockets/cores/threads/maxcpus values should
> > > > fix their CLI to make sure that
> > > >    [sockets * cores * threads] == [maxcpus]
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > > v5:
> > > >   - extend deprecation doc, adding that maxcpus should be multiple of
> > > >     present on CLI [sockets/cores/threads] options
> > > >     (Eduardo Habkost <ehabkost@redhat.com>)
> > > > v4:
> > > >   - missed dot comment, fix it with s/./,/ (Andrew Jones <drjones@redhat.com>)
> > > > v3:
> > > >   - more spelling fixes (Andrew Jones <drjones@redhat.com>)
> > > >   - place deprecation check after (sockets * cores * threads > max_cpus) check
> > > >     (Eduardo Habkost <ehabkost@redhat.com>)
> > > > v2:
> > > >   - spelling&&co fixes (Andrew Jones <drjones@redhat.com>)
> > > > ---
> > > >  qemu-deprecated.texi | 19 +++++++++++++++++++
> > > >  vl.c                 |  7 +++++++
> > > >  2 files changed, 26 insertions(+)
> > > > 
> > > > diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> > > > index 87212b6..827c3ce 100644
> > > > --- a/qemu-deprecated.texi
> > > > +++ b/qemu-deprecated.texi
> > > > @@ -159,6 +159,25 @@ The 'file' driver for drives is no longer appropriate for character or host
> > > >  devices and will only accept regular files (S_IFREG). The correct driver
> > > >  for these file types is 'host_cdrom' or 'host_device' as appropriate.
> > > >  
> > > > +@subsection -smp X,[socket=a,core=b,thread=c],maxcpus=Y (since 3.1)  
> > > 
> > > Minor: I suggest using @var markup, or maybe just use
> > > "-smp (invalid topologies) (since 3.1)" as subsection title for simplicity?
> > >   
> > > > +
> > > > +CPU topology properties should describe whole machine topology including
> > > > +possible CPUs, but historically it was possible to start QEMU with
> > > > +an incorrect topology where
> > > > +  sockets * cores * threads >= X && X < maxcpus  
> > > 
> > > Minor: this line formatting is lost on the HTML output.  I
> > > suggest using @var and/or @math.
> > > 
> > > Minor: I suggest not using C syntax.
> > > 
> > > i.e. use something like:
> > > 
> > >   @math{@var{n} <= @var{sockets} * @var{cores} * @var{threads} < @var{maxcpus}},
> > >   
> > > > +which could lead to an incorrect topology enumeration by the guest.
> > > > +Support for invalid topologies will be removed, the user must ensure
> > > > +topologies described with -smp include all possible cpus, i.e.
> > > > +  sockets * cores * threads == maxcpus  
> > > 
> > > Minor: same as above.  I suggest:
> > > 
> > >   @math{@var{sockets} * @var{cores} * @var{threads} = @var{maxcpus}}.
> > > 
> > >   
> > > > +Note: it's assumed that maxcpus value must be multiple of the topology
> > > > +options present on command line to avoid creating an invalid topology.
> > > > +If maxcpus isn't be multiple of present topology options then the condition
> > > > +(sockets * cores * threads == maxcpus) can't be satisfied and it will
> > > > +trigger deprecation warning which later will be converted to a error.
> > > > +If you get deprecation warning it's recommended to explicitly specify
> > > > +a correct topology to make warning go away and ensure that it will
> > > > +continue working in the future.  
> > > 
> > > I don't understand the purpose of the "Note:" section.  It seems to duplicate
> > > what was already said in the lines above it.  Can we just remove it?  
> > Well, didn't I say that no additional explanation needed in v4?
> > Note section was added per your suggestion to explicitly say that maxcpus
> > should be multiple of options present on CLI.  
> 
> You are right: I did ask for additional clarification on the
> documentation, because it was not clear that the warning can
> still appear even if some of the sockets/cores/threads options
> were omitted.  The note didn't make that clearer to me, though.
I can respin if necessary (we are not in hurry so no need to merge
something that would be removed immediately).
Could you suggest a text for clarification for me to include on respin?

> 
> In either case, this can be done on a follow-up patch if
> necessary.
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 

Re: [Qemu-devel] [PATCH v5 1/2] vl.c deprecate incorrect CPUs topology

[Eduardo Habkost]

On Thu, Sep 06, 2018 at 10:02:13AM +0200, Igor Mammedov wrote:
> On Wed, 5 Sep 2018 10:45:12 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Wed, Sep 05, 2018 at 11:25:11AM +0200, Igor Mammedov wrote:
> > > On Tue, 4 Sep 2018 23:12:55 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >   
> > > > On Tue, Sep 04, 2018 at 03:22:35PM +0200, Igor Mammedov wrote:  
> > > > > -smp [cpus],sockets/cores/threads[,maxcpus] should describe topology
> > > > > so that total number of logical CPUs [sockets * cores * threads]
> > > > > would be equal to [maxcpus], however historically we didn't have
> > > > > such check in QEMU and it is possible to start VM with an invalid
> > > > > topology.
> > > > > Deprecate invalid options combination so we can make sure that
> > > > > the topology VM started with is always correct in the future.
> > > > > Users with an invalid sockets/cores/threads/maxcpus values should
> > > > > fix their CLI to make sure that
> > > > >    [sockets * cores * threads] == [maxcpus]
> > > > > 
> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > ---
> > > > > v5:
> > > > >   - extend deprecation doc, adding that maxcpus should be multiple of
> > > > >     present on CLI [sockets/cores/threads] options
> > > > >     (Eduardo Habkost <ehabkost@redhat.com>)
> > > > > v4:
> > > > >   - missed dot comment, fix it with s/./,/ (Andrew Jones <drjones@redhat.com>)
> > > > > v3:
> > > > >   - more spelling fixes (Andrew Jones <drjones@redhat.com>)
> > > > >   - place deprecation check after (sockets * cores * threads > max_cpus) check
> > > > >     (Eduardo Habkost <ehabkost@redhat.com>)
> > > > > v2:
> > > > >   - spelling&&co fixes (Andrew Jones <drjones@redhat.com>)
> > > > > ---
> > > > >  qemu-deprecated.texi | 19 +++++++++++++++++++
> > > > >  vl.c                 |  7 +++++++
> > > > >  2 files changed, 26 insertions(+)
> > > > > 
> > > > > diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> > > > > index 87212b6..827c3ce 100644
> > > > > --- a/qemu-deprecated.texi
> > > > > +++ b/qemu-deprecated.texi
> > > > > @@ -159,6 +159,25 @@ The 'file' driver for drives is no longer appropriate for character or host
> > > > >  devices and will only accept regular files (S_IFREG). The correct driver
> > > > >  for these file types is 'host_cdrom' or 'host_device' as appropriate.
> > > > >  
> > > > > +@subsection -smp X,[socket=a,core=b,thread=c],maxcpus=Y (since 3.1)  
> > > > 
> > > > Minor: I suggest using @var markup, or maybe just use
> > > > "-smp (invalid topologies) (since 3.1)" as subsection title for simplicity?
> > > >   
> > > > > +
> > > > > +CPU topology properties should describe whole machine topology including
> > > > > +possible CPUs, but historically it was possible to start QEMU with
> > > > > +an incorrect topology where
> > > > > +  sockets * cores * threads >= X && X < maxcpus  
> > > > 
> > > > Minor: this line formatting is lost on the HTML output.  I
> > > > suggest using @var and/or @math.
> > > > 
> > > > Minor: I suggest not using C syntax.
> > > > 
> > > > i.e. use something like:
> > > > 
> > > >   @math{@var{n} <= @var{sockets} * @var{cores} * @var{threads} < @var{maxcpus}},
> > > >   
> > > > > +which could lead to an incorrect topology enumeration by the guest.
> > > > > +Support for invalid topologies will be removed, the user must ensure
> > > > > +topologies described with -smp include all possible cpus, i.e.
> > > > > +  sockets * cores * threads == maxcpus  
> > > > 
> > > > Minor: same as above.  I suggest:
> > > > 
> > > >   @math{@var{sockets} * @var{cores} * @var{threads} = @var{maxcpus}}.
> > > > 
> > > >   
> > > > > +Note: it's assumed that maxcpus value must be multiple of the topology
> > > > > +options present on command line to avoid creating an invalid topology.
> > > > > +If maxcpus isn't be multiple of present topology options then the condition
> > > > > +(sockets * cores * threads == maxcpus) can't be satisfied and it will
> > > > > +trigger deprecation warning which later will be converted to a error.
> > > > > +If you get deprecation warning it's recommended to explicitly specify
> > > > > +a correct topology to make warning go away and ensure that it will
> > > > > +continue working in the future.  
> > > > 
> > > > I don't understand the purpose of the "Note:" section.  It seems to duplicate
> > > > what was already said in the lines above it.  Can we just remove it?  
> > > Well, didn't I say that no additional explanation needed in v4?
> > > Note section was added per your suggestion to explicitly say that maxcpus
> > > should be multiple of options present on CLI.  
> > 
> > You are right: I did ask for additional clarification on the
> > documentation, because it was not clear that the warning can
> > still appear even if some of the sockets/cores/threads options
> > were omitted.  The note didn't make that clearer to me, though.
> I can respin if necessary (we are not in hurry so no need to merge
> something that would be removed immediately).
> Could you suggest a text for clarification for me to include on respin?

I was planning to write a suggestion later, but I didn't want
this series to be delayed because of me.  Just removing the
existing "Notes:" section would be good enough to me.

Probably the right place to document the subtle details of the
s/c/t/maxcpus requirements is the "-smp" section on
qemu-options.hx, not qemu-deprecated.hx.

I was considering something like this:

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 060e015be6..74f6a64b8b 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -139,16 +139,20 @@ The 'file' driver for drives is no longer appropriate for character or host
 devices and will only accept regular files (S_IFREG). The correct driver
 for these file types is 'host_cdrom' or 'host_device' as appropriate.
 
-@subsection -smp X,[socket=a,core=b,thread=c],maxcpus=Y (since 3.1)
+@subsection -smp (invalid topologies) (since 3.1)
 
 CPU topology properties should describe whole machine topology including
-possible CPUs, but historically it was possible to start QEMU with
+possible CPUs.  In other words: @var{maxcpus} should be equal to
+@math{@var{sockets} * @var{cores} * @var{threads}}.
+
+However, historically it was possible to start QEMU with
 an incorrect topology where
-  sockets * cores * threads >= X && X < maxcpus
+@math{@var{n} <= @var{sockets} * @var{cores} * @var{threads} < @var{maxcpus}},
 which could lead to an incorrect topology enumeration by the guest.
 Support for invalid topologies will be removed, the user must ensure
 topologies described with -smp include all possible cpus, i.e.
-  sockets * cores * threads == maxcpus
+@math{@var{sockets} * @var{cores} * @var{threads} = @var{maxcpus}}.
+
 
 @section QEMU Machine Protocol (QMP) commands
 
diff --git a/qemu-options.hx b/qemu-options.hx
index 654ef484d9..2afa271570 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -155,8 +155,13 @@ to 4.
 For the PC target, the number of @var{cores} per socket, the number
 of @var{threads} per cores and the total number of @var{sockets} can be
 specified. Missing values will be computed. If any on the three values is
-given, the total number of CPUs @var{n} can be omitted. @var{maxcpus}
-specifies the maximum number of hotpluggable CPUs.
+given, the total number of CPUs @var{n} can be omitted.
+
+@var{maxcpus} specifies the maximum number of hotpluggable CPUs
+and should be equal to @math{@var{sockets} * @var{cores} * @var{threads}}
+(even if @var{sockets}, @var{cores}, or @var{threads} is omitted
+and computed automatically).
+
 ETEXI
 
 DEF("numa", HAS_ARG, QEMU_OPTION_numa,

-- 
Eduardo

Re: [Qemu-devel] [libvirt] [PATCH v5 1/2] vl.c deprecate incorrect CPUs topology

[Eric Blake]

On 9/4/18 8:22 AM, Igor Mammedov wrote:
> -smp [cpus],sockets/cores/threads[,maxcpus] should describe topology
> so that total number of logical CPUs [sockets * cores * threads]
> would be equal to [maxcpus], however historically we didn't have
> such check in QEMU and it is possible to start VM with an invalid
> topology.
> Deprecate invalid options combination so we can make sure that
> the topology VM started with is always correct in the future.
> Users with an invalid sockets/cores/threads/maxcpus values should
> fix their CLI to make sure that
>     [sockets * cores * threads] == [maxcpus]
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---

>   
> +@subsection -smp X,[socket=a,core=b,thread=c],maxcpus=Y (since 3.1)
> +
> +CPU topology properties should describe whole machine topology including
> +possible CPUs, but historically it was possible to start QEMU with
> +an incorrect topology where
> +  sockets * cores * threads >= X && X < maxcpus
> +which could lead to an incorrect topology enumeration by the guest.
> +Support for invalid topologies will be removed, the user must ensure
> +topologies described with -smp include all possible cpus, i.e.
> +  sockets * cores * threads == maxcpus
> +Note: it's assumed that maxcpus value must be multiple of the topology
> +options present on command line to avoid creating an invalid topology.
> +If maxcpus isn't be multiple of present topology options then the condition

s/be/a/

> +(sockets * cores * threads == maxcpus) can't be satisfied and it will
> +trigger deprecation warning which later will be converted to a error.

a/a error/an error/

> +If you get deprecation warning it's recommended to explicitly specify

s/get/get a/

> +a correct topology to make warning go away and ensure that it will
> +continue working in the future.
> +
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [libvirt] [PATCH v5 1/2] vl.c deprecate incorrect CPUs topology

[Eric Blake]

On 9/10/18 12:49 PM, Eduardo Habkost wrote:

> 
> I was considering something like this:
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 060e015be6..74f6a64b8b 100644
> --- a/qemu-deprecated.texi

> +++ b/qemu-options.hx
> @@ -155,8 +155,13 @@ to 4.
>   For the PC target, the number of @var{cores} per socket, the number
>   of @var{threads} per cores and the total number of @var{sockets} can be
>   specified. Missing values will be computed. If any on the three values is

while here, s/on/of/

> -given, the total number of CPUs @var{n} can be omitted. @var{maxcpus}
> -specifies the maximum number of hotpluggable CPUs.
> +given, the total number of CPUs @var{n} can be omitted.
> +
> +@var{maxcpus} specifies the maximum number of hotpluggable CPUs
> +and should be equal to @math{@var{sockets} * @var{cores} * @var{threads}}
> +(even if @var{sockets}, @var{cores}, or @var{threads} is omitted
> +and computed automatically).
> +
>   ETEXI
>   
>   DEF("numa", HAS_ARG, QEMU_OPTION_numa,
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v5 1/2] vl.c deprecate incorrect CPUs topology

[Igor Mammedov]

On Mon, 10 Sep 2018 14:49:23 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Sep 06, 2018 at 10:02:13AM +0200, Igor Mammedov wrote:
> > On Wed, 5 Sep 2018 10:45:12 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Wed, Sep 05, 2018 at 11:25:11AM +0200, Igor Mammedov wrote:  
> > > > On Tue, 4 Sep 2018 23:12:55 -0300
> > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > >     
> > > > > On Tue, Sep 04, 2018 at 03:22:35PM +0200, Igor Mammedov wrote:    
> > > > > > -smp [cpus],sockets/cores/threads[,maxcpus] should describe topology
> > > > > > so that total number of logical CPUs [sockets * cores * threads]
> > > > > > would be equal to [maxcpus], however historically we didn't have
> > > > > > such check in QEMU and it is possible to start VM with an invalid
> > > > > > topology.
> > > > > > Deprecate invalid options combination so we can make sure that
> > > > > > the topology VM started with is always correct in the future.
> > > > > > Users with an invalid sockets/cores/threads/maxcpus values should
> > > > > > fix their CLI to make sure that
> > > > > >    [sockets * cores * threads] == [maxcpus]
> > > > > > 
> > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > > ---
> > > > > > v5:
> > > > > >   - extend deprecation doc, adding that maxcpus should be multiple of
> > > > > >     present on CLI [sockets/cores/threads] options
> > > > > >     (Eduardo Habkost <ehabkost@redhat.com>)
> > > > > > v4:
> > > > > >   - missed dot comment, fix it with s/./,/ (Andrew Jones <drjones@redhat.com>)
> > > > > > v3:
> > > > > >   - more spelling fixes (Andrew Jones <drjones@redhat.com>)
> > > > > >   - place deprecation check after (sockets * cores * threads > max_cpus) check
> > > > > >     (Eduardo Habkost <ehabkost@redhat.com>)
> > > > > > v2:
> > > > > >   - spelling&&co fixes (Andrew Jones <drjones@redhat.com>)
> > > > > > ---
> > > > > >  qemu-deprecated.texi | 19 +++++++++++++++++++
> > > > > >  vl.c                 |  7 +++++++
> > > > > >  2 files changed, 26 insertions(+)
> > > > > > 
> > > > > > diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> > > > > > index 87212b6..827c3ce 100644
> > > > > > --- a/qemu-deprecated.texi
> > > > > > +++ b/qemu-deprecated.texi
> > > > > > @@ -159,6 +159,25 @@ The 'file' driver for drives is no longer appropriate for character or host
> > > > > >  devices and will only accept regular files (S_IFREG). The correct driver
> > > > > >  for these file types is 'host_cdrom' or 'host_device' as appropriate.
> > > > > >  
> > > > > > +@subsection -smp X,[socket=a,core=b,thread=c],maxcpus=Y (since 3.1)    
> > > > > 
> > > > > Minor: I suggest using @var markup, or maybe just use
> > > > > "-smp (invalid topologies) (since 3.1)" as subsection title for simplicity?
> > > > >     
> > > > > > +
> > > > > > +CPU topology properties should describe whole machine topology including
> > > > > > +possible CPUs, but historically it was possible to start QEMU with
> > > > > > +an incorrect topology where
> > > > > > +  sockets * cores * threads >= X && X < maxcpus    
> > > > > 
> > > > > Minor: this line formatting is lost on the HTML output.  I
> > > > > suggest using @var and/or @math.
> > > > > 
> > > > > Minor: I suggest not using C syntax.
> > > > > 
> > > > > i.e. use something like:
> > > > > 
> > > > >   @math{@var{n} <= @var{sockets} * @var{cores} * @var{threads} < @var{maxcpus}},
> > > > >     
> > > > > > +which could lead to an incorrect topology enumeration by the guest.
> > > > > > +Support for invalid topologies will be removed, the user must ensure
> > > > > > +topologies described with -smp include all possible cpus, i.e.
> > > > > > +  sockets * cores * threads == maxcpus    
> > > > > 
> > > > > Minor: same as above.  I suggest:
> > > > > 
> > > > >   @math{@var{sockets} * @var{cores} * @var{threads} = @var{maxcpus}}.
> > > > > 
> > > > >     
> > > > > > +Note: it's assumed that maxcpus value must be multiple of the topology
> > > > > > +options present on command line to avoid creating an invalid topology.
> > > > > > +If maxcpus isn't be multiple of present topology options then the condition
> > > > > > +(sockets * cores * threads == maxcpus) can't be satisfied and it will
> > > > > > +trigger deprecation warning which later will be converted to a error.
> > > > > > +If you get deprecation warning it's recommended to explicitly specify
> > > > > > +a correct topology to make warning go away and ensure that it will
> > > > > > +continue working in the future.    
> > > > > 
> > > > > I don't understand the purpose of the "Note:" section.  It seems to duplicate
> > > > > what was already said in the lines above it.  Can we just remove it?    
> > > > Well, didn't I say that no additional explanation needed in v4?
> > > > Note section was added per your suggestion to explicitly say that maxcpus
> > > > should be multiple of options present on CLI.    
> > > 
> > > You are right: I did ask for additional clarification on the
> > > documentation, because it was not clear that the warning can
> > > still appear even if some of the sockets/cores/threads options
> > > were omitted.  The note didn't make that clearer to me, though.  
> > I can respin if necessary (we are not in hurry so no need to merge
> > something that would be removed immediately).
> > Could you suggest a text for clarification for me to include on respin?  
> 
> I was planning to write a suggestion later, but I didn't want
> this series to be delayed because of me.  Just removing the
> existing "Notes:" section would be good enough to me.
> 
> Probably the right place to document the subtle details of the
> s/c/t/maxcpus requirements is the "-smp" section on
> qemu-options.hx, not qemu-deprecated.hx.
> 
> I was considering something like this:
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 060e015be6..74f6a64b8b 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -139,16 +139,20 @@ The 'file' driver for drives is no longer appropriate for character or host
>  devices and will only accept regular files (S_IFREG). The correct driver
>  for these file types is 'host_cdrom' or 'host_device' as appropriate.
>  
> -@subsection -smp X,[socket=a,core=b,thread=c],maxcpus=Y (since 3.1)
> +@subsection -smp (invalid topologies) (since 3.1)
>  
>  CPU topology properties should describe whole machine topology including
> -possible CPUs, but historically it was possible to start QEMU with
> +possible CPUs.  In other words: @var{maxcpus} should be equal to
> +@math{@var{sockets} * @var{cores} * @var{threads}}.
> +
> +However, historically it was possible to start QEMU with
>  an incorrect topology where
> -  sockets * cores * threads >= X && X < maxcpus
> +@math{@var{n} <= @var{sockets} * @var{cores} * @var{threads} < @var{maxcpus}},
>  which could lead to an incorrect topology enumeration by the guest.
>  Support for invalid topologies will be removed, the user must ensure
>  topologies described with -smp include all possible cpus, i.e.
> -  sockets * cores * threads == maxcpus
> +@math{@var{sockets} * @var{cores} * @var{threads} = @var{maxcpus}}.
> +
Thanks,
suggestions taken in account and I'll drop Note section

>  @section QEMU Machine Protocol (QMP) commands
>  
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 654ef484d9..2afa271570 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -155,8 +155,13 @@ to 4.
>  For the PC target, the number of @var{cores} per socket, the number
>  of @var{threads} per cores and the total number of @var{sockets} can be
>  specified. Missing values will be computed. If any on the three values is
> -given, the total number of CPUs @var{n} can be omitted. @var{maxcpus}
> -specifies the maximum number of hotpluggable CPUs.
> +given, the total number of CPUs @var{n} can be omitted.
> +
> +@var{maxcpus} specifies the maximum number of hotpluggable CPUs
> +and should be equal to @math{@var{sockets} * @var{cores} * @var{threads}}
> +(even if @var{sockets}, @var{cores}, or @var{threads} is omitted
> +and computed automatically).
Options doc I've left out, it probably should be separate patch.

>  ETEXI
>  
>  DEF("numa", HAS_ARG, QEMU_OPTION_numa,
>