Re: [Qemu-devel] [PATCH 2/9] machine: introduce MachineClass.possible_cpu_arch_ids() hook

[Igor Mammedov]

On Fri, 5 Feb 2016 13:50:05 -0200
Eduardo Habkost <address@hidden> wrote:

> On Fri, Feb 05, 2016 at 04:39:46PM +0100, Igor Mammedov wrote:
> > On Fri, 5 Feb 2016 13:04:26 -0200
> > Eduardo Habkost <address@hidden> wrote:
> >   
> > > On Thu, Feb 04, 2016 at 12:47:28PM +0100, Igor Mammedov wrote:  
> > > > on x86 currently range 0..max_cpus is used to generate
> > > > architecture-dependent CPU ID (APIC Id) for each present
> > > > and possible CPUs. However architecture-dependent CPU IDs
> > > > list could be sparse and code that needs to enumerate
> > > > all IDs (ACPI) ended up doing guess work enumerating all
> > > > possible and impossible IDs up to
> > > >   apic_id_limit = x86_cpu_apic_id_from_index(max_cpus).
> > > > 
> > > > That leads to creation of MADT entries and Processor
> > > > objects in ACPI tables for not possible CPUs.
> > > > Fix it by allowing board specify a concrete list of
> > > > CPU IDs accourding its own rules (which for x86 depends
> > > > on topology). So that code that needs this list could
> > > > request it from board instead of trying to figure out
> > > > what IDs are correct on its own.
> > > > 
> > > > Signed-off-by: Igor Mammedov <address@hidden>
> > > > ---
> > > >  hw/i386/pc.c        | 16 ++++++++++++++++
> > > >  include/hw/boards.h | 18 ++++++++++++++++++
> > > >  2 files changed, 34 insertions(+)
> > > > 
> > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > > index d72246d..2fd8fc8 100644
> > > > --- a/hw/i386/pc.c
> > > > +++ b/hw/i386/pc.c
> > > > @@ -1946,6 +1946,21 @@ static unsigned 
> > > > pc_cpu_index_to_socket_id(unsigned cpu_index)
> > > >      return topo.pkg_id;
> > > >  }
> > > >  
> > > > +static GArray *pc_possible_cpu_arch_ids(void)
> > > > +{
> > > > +    int i;
> > > > +    GArray *list = g_array_new (FALSE, FALSE, sizeof (CPUArchId));
> > > > +
> > > > +    for (i = 0; i < max_cpus; i++) {
> > > > +        CPUArchId val;
> > > > +
> > > > +        val.arch_id = x86_cpu_apic_id_from_index(i);
> > > > +        val.cpu = qemu_get_cpu_by_arch_id(val.arch_id);
> > > > +        g_array_append_val(list, val);    
> > > 
> > > What about letting callers call qemu_get_cpu_by_arch_id() only if
> > > they really need it?
> > > 
> > > If you do that, you just need to return an uint64_t array, and
> > > there's no need for struct CPUArchId.  
> > So far all callers that would use it would need to call
> > qemu_get_cpu_by_arch_id() so doing it in one place (here)
> > seems better than to duplicating that call over the code.  
> 
> I see only one place using CPUArchId.cpu. All other callers don't
> use the field.
> 
> Simply replacing "id.cpu" with "qemu_get_cpu_by_arch_id(id)" in
> one line seems worth it, if it's going to save us the trouble of
> defining another struct and avoid lots of unnecessary calls to
> qemu_get_cpu_by_arch_id() (that loops through all CPUs every time
> it's called).
id.cpu is going to be used at other places
when I add xlapic entries and cpu Devices in new CPU hotplug interface
later it will be used for similar purposes for virt-arm machine.

Another reason for struct is to discourage usage of direct access
to elements of array, while with uint64_t it's very tempting to do so
and easy to get wrong. (In the first attempt I did uint64_t array
and then were looking for bugs due to wrong type casting)