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

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 3 Apr 2013 16:27:11 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Apr 03, 2013 at 08:59:07PM +0200, Igor Mammedov wrote:
> > On Wed, 3 Apr 2013 15:10:05 -0300
> > Eduardo Habkost <ehabkost@redhat.com> 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)?
using any kind of thread id is problematic since 2 treads from the same core
could end-up on different nodes.
Maybe placement interface could be better described as node[n]=sockets_list?
 
> - 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.
it's amount of CPUs available to guest, pretty clear from user's POV.
 
> 
> 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.
Once we have CPU unplug, we will have non-contiguous cpu_index. So it will be
part of CPU unplug series to fix cpu_index allocation/usage where necessary.

> 
> > 
> > > 
> > > 
> > > > 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.
What about adding CPU to a specific node then, it would require interface for
communicating to CPU to which node it should be plugged (part of APIC ID, I
guess).

> 
> -- 
> Eduardo
> 


-- 
Regards,
  Igor