Re: [Qemu-devel] [PATCH RFC 0/4] target-i386: PC socket/core/thread modeling, part 1

[Igor Mammedov]

On Mon, 23 Mar 2015 18:31:59 +0100
Andreas Färber <address@hidden> wrote:

> Hello,
> 
> This long-postponed series proposes a hierarchical QOM model of socket
> and core objects for the x86 PC machines.
> 
> Background is that due to qdev limitations we had to introduce an ICC bus
> to be able to hot-add CPUs and their APICs. By now this limitation could be
> resolved via a QOM hotplug handler interface.
> 
> However, the QOM hotplug model is associated with having link<> properties.
> Given that physically we cannot hot-add hyperthreads but full CPU sockets,
> this series prepares the underbelly for having those link properties be of
> the new type X86CPUSocket rather than X86CPU.
Having hotplug associated with link<> properties, was Anthony's suggestion
to overcome limitation of non hotpluggable Sysbus back then, hence ICC bus
has been introduced as an alternative solution to CPU hotplug in target-i386
code. With current hotplug handler interface we in process of removing
ICC bus and switching x86 CPUs to bus-less hotplug.
That by itself doesn't mandate/need link<> properties in hotplug or modeling
sockets at all. Also I don't see much usefulness in limiting CPU hotplug to
socket based just because physical hardware can't it.

I'm thinking about using CPU socket modeling as a way to provide
a sane topology on CLI and other management interfaces, which is currently
represented (or not) by -numa/-smp/-cpu/cpu-add mess.
It's not clear what conversion in this series would give us.
It would be better for us first figure out what is the end goals
of this conversion and how QEMU would benefit from it.
(i.e. lets make a plan what we intend to do first)

> 
> As final step in this series, the X86CPU allocation model is changed to be
> init'ed in-place, as part of an "atomic" socket object. A follow-up will be
> to attempt the same in-place allocation model for the APIC; one difficulty
> there is that several places do a NULL check for the APIC pointer as quick
> way of telling whether an APIC is present or not.
Maybe it's obvious but what does in-place initialization give us?
(other than replacing a bunch of small malloc-s with a big one)

> NOT IN THIS SERIES is converting cpu-add to the same socket/core/thread model
> and initializing them in-place. The current PoC implementation assumes that
> CPUs get added sequentially and that the preceding CPU can be used to obtain
> the core via unclean QOM parent accesses.
> IIUC that must be changed so that an arbitrary thread added via cpu-add
I'd prefer not to use cpu-add for socket based, i.e. deprecate it and leave
it for compatibility for old machine types and for socket based CPUs use
-device instead.

> creates the full socket and core(s). That would work best if indexed link<>
> properties could be used.
> That's an undecided design question of whether
> we want to have /machine/cpu-socket[n] or whether it makes sense to integrate
> NUMA modeling while at it and rather have /machine/node[n]/socket[m].

It might have sense to have both i.e. make "node[n]" optional for numa-less
case.
but lets start with problems we have with current impl.
I'd speak from x86 cpu hotplug/-device X86CPU point of view:

when we plug CPU we have to specify which CPU we want to plug,
currently QEMU uses
  cpu-add cpu_index
as a way to specify which CPU thread to plug or if CPU were created
via -device then cpu_index would be used implicitly.
Problems with it are:
 1. user doesn't really know where given CPU will be plugged in
    /socket, core, thread/ since it's might depend on arch
    specific routine which translates cpu_index into arch specific
    topology info [in case of x86 it's APIC ID]
 2. in NUMA case we can specify which CPU threads are belong to which
    node using the same cpu_index. Problem is the same as with #1,
    user has no idea if CPU thread can be located on that node.
    It's possible to check using target specific hook like has been done
    for default case 
https://lists.gnu.org/archive/html/qemu-devel/2015-03/msg04252.html.
 3. topology info using -smp allows to create only machines with
    homogeneous topology CPUs, the same applies to -cpu.

So my grudge is mainly against using cpu_index in user interface
and that is where I see sockets could be useful.
 * To address NUMA concern remodel CLI to:
   -numa node,cpu_sockets=...

 * Convert -cpu to global properties so that they could be applied
   as default properties to CPUs

 * allow create CPUs with -device_add 
cpupkg,socket=x[,cores=y[,threads=z]],[type=foo]
   in this case cpupkg could be comoposit object with several cores/threads
   or even microthread like CPUs are now, it could work in both cases.

 * Convert -smp to perform set of device_add cpu,... operations
   to keep CLI compatible with old versions and simple

As result CLI would be pretty arch agnostic without using some vague 
cpu_index-es
and with generic cpupkg we wouldn't have to create ARCH-FOO-cpu-socket objects
in for every target.

> 
> Note that this socket modeling is not only PC-specific in the softmmu sense
> but also in that not every X86CPU must be on a socket (e.g., Quark X1000).
> Therefore it was a concious decision to not label some things target-i386
> and to place code in pc.c rather than cpu.c.
Strictly speaking Quark X1000 is FCBGA393 socket packaged,
it just happens that package has a little bit more than a CPU core.
I don't like open codding of composing CPU socket in board code like
it's done here, there should be socket object that encapsulates all
that code, while board would just use it.

So maybe socket is wrong term to use in this model, but we need some name
to tell where CPU is plugged in, generic 'cpu' would be a good choice
for composit CPU object if it were not claimed already by cpu threads
and sockets could be links<> in /machine/[node[x]/]cpu_socket<>[y]
topology view.

> 
> Further note that it is ignored here that -smp enforces that AMD CPUs don't
> have hyperthreads, i.e. AMD X86CPUs will have only one thread[n] child<>.
> 
> Context:
> 
>    qemu.git master
>    "pc: Ensure non-zero CPU ref count after attaching to ICC bus"
> -> this series adding socket/core objects
>    cpu-add conversion
>    APIC cleanups
> 
> Available for testing here:
> git://github.com/afaerber/qemu-cpu.git qom-cpu-x86-sockets-1.v1
> https://github.com/afaerber/qemu-cpu/commits/qom-cpu-x86-sockets-1.v1
> 
> Regards,
> Andreas
> 
> Cc: Eduardo Habkost <address@hidden>
> Cc: Igor Mammedov <address@hidden>
> Cc: Paolo Bonzini <address@hidden>
> Cc: Peter Maydell <address@hidden>
> Cc: Bharata B Rao <address@hidden>
> Cc: Christian Borntraeger <address@hidden>
> 
> Andreas Färber (4):
>   cpu: Prepare Socket container type
>   target-i386: Prepare CPU socket/core abstraction
>   pc: Create sockets and cores for CPUs
>   pc: Create initial CPUs in-place
> 
>  hw/cpu/Makefile.objs         |  2 +-
>  hw/cpu/socket.c              | 21 ++++++++++
>  hw/i386/Makefile.objs        |  1 +
>  hw/i386/cpu-core.c           | 45 +++++++++++++++++++++
>  hw/i386/cpu-socket.c         | 45 +++++++++++++++++++++
>  hw/i386/pc.c                 | 95 
> ++++++++++++++++++++++++++++++++++++++++----
>  include/hw/cpu/socket.h      | 14 +++++++
>  include/hw/i386/cpu-core.h   | 29 ++++++++++++++
>  include/hw/i386/cpu-socket.h | 29 ++++++++++++++
>  9 files changed, 272 insertions(+), 9 deletions(-)
>  create mode 100644 hw/cpu/socket.c
>  create mode 100644 hw/i386/cpu-core.c
>  create mode 100644 hw/i386/cpu-socket.c
>  create mode 100644 include/hw/cpu/socket.h
>  create mode 100644 include/hw/i386/cpu-core.h
>  create mode 100644 include/hw/i386/cpu-socket.h
>