Re: [Qemu-devel] [PATCH 12/12] target-i386: implement CPU hot-add

[Eduardo Habkost]

On Wed, Apr 03, 2013 at 08:59:07PM +0200, Igor Mammedov wrote:
> On Wed, 3 Apr 2013 15:10:05 -0300
> Eduardo Habkost <address@hidden> wrote:
> 
> > On Wed, Apr 03, 2013 at 07:58:00PM +0200, Igor Mammedov wrote:
> > <snip>
> > > > > +void do_cpu_hot_add(const int64_t id, Error **errp)
> > > > > +{
> > > > > +    pc_new_cpu(saved_cpu_model, id, errp);
> > > > > +}
> > > > > +
> > > > 
> > > > Missing x86_cpu_apic_id_from_index(id)?
> > > There was(is?) opposition to using cpu_index to identify x86 CPU.
> > 
> > Really? Do you have a pointer to the discussion?
> Here is what I could find in my mail box:
> http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg02770.html
> Jan could correct me if I'm wrong.



So, quoting Jan:
> From my POV, cpu_index could become equal to the physical APIC ID.
> As long as we can set it freely (provided it remains unique) and
> non-continuously, we don't need separate indexes."

We can't choose APIC IDs freely, because the APIC ID is calculated based
on the CPU topology (socket + core + thread IDs).

So, the cpu_index could be the same as the APIC ID if the cpu_index
value declared opaque, being just a "CPU identifier" that is chosen by
QEMU arbitrarily (and that happens to match the APIC ID). But we will
probably have some problems with this:

- The CPU index are currently allocated contiguously, and probably
  existing interfaces already assume that (e.g. the "-numa' option,
  "info numa", "info cpus" and maybe other monitor commands)
- QEMU must be responsible for calculating the APIC ID of each CPU,
  because it is based on the CPU topology.
- If QEMU is the one who calculates the APIC ID, what kind of identifier
  we can use for a CPU object in the command-line (e.g. in the "-numa"
  option)?
- We may need to redefine the meaning of the "maxcpus" -smp option, if
  all our interfaces are now based in non-contiguous and freely-set CPU
  identifiers.

In short, getting rid of the contiguous CPU indexes sounds very
difficult. We could introduce other kind of identifiers, but probably we
may need to keep the CPU indexes contiguous to keep existing interfaces
working.

> 
> > 
> > 
> > > So, it is expected from management to provide APIC ID instead of 
> > > cpu_index.
> > > It could be useful to make hotplug to a specific NUMA node/cpu to work in
> > > future.
> > > Though interface of possible APIC IDs discovery is not part of this 
> > > series.
> > 
> > That's exactly the opposite of what I expect. The APIC ID is an internal
> > implementation detail, and external tools must _not_ be required to deal
> > with it and to calculate it.
> > 
> > Communication with the BIOS, on the other hand, is entirely based on the
> > APIC ID, and not CPU indexes. So QEMU needs to translate the CPU indexes
> > (used to communicate with the outside world) to APIC IDs when talking to
> > the BIOS.
> cpu_index won't work nicely with hot-adding CPU to specific numa node though.

Well, the "-numa node" options are already based on CPU indexes, so it
would match it the existing NUMA configuration interface.

> with APIC ID (mgmt might treat it as opaque) we could expose something like
> 
> /machine/icc-bridge/link<CPU[apic_id_n]
> ...
> 
> for all possible CPUs, with empty links for non existing ones.
> 
> and later add on something like this:
> 
> /machine/numa_node[x]/link<CPU[apic_id_n]>
> ...
> 
> Libvirt than could just pickup ready apic id from desired place and add CPU
> either using cpu-add id=xxx or device_add x86-cpu-...,apic_id=xxx
> 
> +1 more cpu_index is QEMU implementation detail and we could not add to x86 
> CPU
> cpu-index property since hardware doesn't have such feature, so it won't be
> available with device_add.

I don't mind hiding cpu_index too. I don't mind if we use a cpu_index,
QOM links, arbitrary IDs set by the user. I just have a problem with
requiring libvirt to set the APIC ID.

If you give libvirt an easy way to convert a CPU "location" (index, numa
node, whatever) to an APIC ID that is pre-calculated by QEMU, then it
could work. But do we really need to require libvirt to deal with APIC
ID directly? If you just set the links properly to reflect the CPU
"location", the CPU could calculate its APIC ID based on its "location"
using the links.

-- 
Eduardo