Re: [Qemu-devel] [PATCH for-1.4 11/12] target-i386: Topology & APIC ID utility functions

[Eduardo Habkost]

On Mon, Jan 21, 2013 at 12:28:21PM +0100, Andreas Färber wrote:
> Am 17.01.2013 21:59, schrieb Eduardo Habkost:
> > Signed-off-by: Eduardo Habkost <address@hidden>
> > ---
> > Changes v1 -> v2:
> >  - Support 32-bit APIC IDs (in case x2APIC is going to be used)
> >  - Coding style changes
> >  - Use TARGET_I386_TOPOLOGY_H instead of __QEMU_X86_TOPOLOGY_H__
> >  - Rename topo_make_apic_id() to topo_apicid_for_cpu()
> >  - Rename __make_apicid() to topo_make_apicid()
> >  - Spaces around operators on test-x86-cpuid.c, as requested by
> >    Blue Swirl
> >  - Make test-x86-cpuid a target-specific test
> > 
> > Changes v2 -> v3:
> >  - Add documentation pointers to the code
> >  - Rename bits_for_count() to bitwidth_for_count()
> >  - Remove unused apicid_*_id() functions
> > 
> > Changes v3 -> v4:
> >  - Remove now-obsolete FIXME comment from test-x86-cpuid.c
> >  - Change bitops.h include to qemu/bitops.h
> >  - Add gcov file list to test-x86-cpuid
> > ---
> >  target-i386/topology.h | 133 
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/.gitignore       |   1 +
> >  tests/Makefile         |   7 +++
> >  tests/test-x86-cpuid.c | 101 +++++++++++++++++++++++++++++++++++++
> >  4 files changed, 242 insertions(+)
> >  create mode 100644 target-i386/topology.h
> >  create mode 100644 tests/test-x86-cpuid.c
> > 
> > diff --git a/target-i386/topology.h b/target-i386/topology.h
> > new file mode 100644
> > index 0000000..833ab47
> > --- /dev/null
> > +++ b/target-i386/topology.h
> > @@ -0,0 +1,133 @@
> > +/*
> > + *  x86 CPU topology data structures and functions
> > + *
> > + *  Copyright (c) 2012 Red Hat Inc.
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a 
> > copy
> > + * of this software and associated documentation files (the "Software"), 
> > to deal
> > + * in the Software without restriction, including without limitation the 
> > rights
> > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or 
> > sell
> > + * copies of the Software, and to permit persons to whom the Software is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included 
> > in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
> > OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR 
> > OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> > FROM,
> > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS 
> > IN
> > + * THE SOFTWARE.
> > + */
> > +#ifndef TARGET_I386_TOPOLOGY_H
> > +#define TARGET_I386_TOPOLOGY_H
> > +
> > +/* This file implements the APIC-ID-based CPU topology enumeration logic,
> > + * documented at the following document:
> > + *   Intel® 64 Architecture Processor Topology Enumeration
> > + *   
> > http://software.intel.com/en-us/articles/intel-64-architecture-processor-topology-enumeration/
> > + *
> > + * This code should be compatible with AMD's "Extended Method" described 
> > at:
> > + *   AMD CPUID Specification (Publication #25481)
> > + *   Section 3: Multiple Core Calcuation
> > + * as long as:
> > + *  nr_threads is set to 1;
> > + *  OFFSET_IDX is assumed to be 0;
> > + *  CPUID Fn8000_0008_ECX[ApicIdCoreIdSize[3:0]] is set to 
> > apicid_core_width().
> > + */
> > +
> > +#include <stdint.h>
> > +#include <string.h>
> > +
> > +#include "qemu/bitops.h"
> > +
> > +/* APIC IDs can be 32-bit, but beware: APIC IDs > 255 require x2APIC 
> > support
> > + */
> > +typedef uint32_t apic_id_t;
> 
> Is this file imported from somewhere?

It's used by PATCH 12/12, when actually implementing the topology-aware
APIC ID calculation in the CPU code.

> There was a discussion some time
> ago about not using _t since reserved by POSIX...

This is the current QEMU coding style:

"Scalar type names are lower_case_with_underscores_ending_with_a_t, like
the POSIX uint64_t and family.  Note that this last convention
contradicts POSIX and is therefore likely to be changed."



> 
> > +
> > +/* Return the bit width needed for 'count' IDs
> > + */
> > +static unsigned bitwidth_for_count(unsigned count)
> > +{
> > +    g_assert(count >= 1);
> > +    if (count == 1) {
> > +        return 0;
> > +    }
> > +    return bitops_flsl(count - 1) + 1;
> > +}
> > +
> > +/* Bit width of the SMT_ID (thread ID) field on the APIC ID
> > + */
> > +static inline unsigned apicid_smt_width(unsigned nr_cores, unsigned 
> > nr_threads)
> > +{
> > +    return bitwidth_for_count(nr_threads);
> > +}
> > +
> > +/* Bit width of the Core_ID field
> > + */
> > +static inline unsigned apicid_core_width(unsigned nr_cores, unsigned 
> > nr_threads)
> > +{
> > +    return bitwidth_for_count(nr_cores);
> > +}
> > +
> > +/* Bit offset of the Core_ID field
> > + */
> > +static inline unsigned apicid_core_offset(unsigned nr_cores,
> > +                                          unsigned nr_threads)
> > +{
> > +    return apicid_smt_width(nr_cores, nr_threads);
> > +}
> > +
> > +/* Bit offset of the Pkg_ID (socket ID) field
> > + */
> > +static inline unsigned apicid_pkg_offset(unsigned nr_cores, unsigned 
> > nr_threads)
> > +{
> > +    return apicid_core_offset(nr_cores, nr_threads) + \
> 
> Not a macro. :)

Leftover from a time when this was being implemented as a macro. I will
remove.

> 
> > +           apicid_core_width(nr_cores, nr_threads);
> > +}
> > +
> > +/* Make APIC ID for the CPU based on Pkg_ID, Core_ID, SMT_ID
> > + *
> > + * The caller must make sure core_id < nr_cores and smt_id < nr_threads.
> > + */
> > +static inline apic_id_t topo_make_apicid(unsigned nr_cores,
> > +                                         unsigned nr_threads,
> > +                                         unsigned pkg_id, unsigned core_id,
> > +                                         unsigned smt_id)
> > +{
> > +    return (pkg_id  << apicid_pkg_offset(nr_cores, nr_threads)) | \
> > +           (core_id << apicid_core_offset(nr_cores, nr_threads)) | \
> 
> Ditto.

I will remove it.

> 
> > +           smt_id;
> > +}
> > +
> > +/* Calculate thread/core/package IDs for a specific topology,
> > + * based on (contiguous) CPU index
> > + */
> > +static inline void topo_ids_from_idx(unsigned nr_cores, unsigned 
> > nr_threads,
> > +                                     unsigned cpu_index,
> > +                                     unsigned *pkg_id, unsigned *core_id,
> > +                                     unsigned *smt_id)
> > +{
> > +    unsigned core_index = cpu_index / nr_threads;
> > +    *smt_id = cpu_index % nr_threads;
> > +    *core_id = core_index % nr_cores;
> > +    *pkg_id = core_index / nr_cores;
> > +}
> > +
> > +/* Make APIC ID for the CPU 'cpu_index'
> > + *
> > + * 'cpu_index' is a sequential, contiguous ID for the CPU.
> > + */
> > +static inline apic_id_t topo_apicid_for_cpu(unsigned nr_cores,
> > +                                            unsigned nr_threads,
> > +                                            unsigned cpu_index)
> > +{
> > +    unsigned pkg_id, core_id, smt_id;
> > +    topo_ids_from_idx(nr_cores, nr_threads, cpu_index,
> > +                      &pkg_id, &core_id, &smt_id);
> > +    return topo_make_apicid(nr_cores, nr_threads, pkg_id, core_id, smt_id);
> > +}
> > +
> > +#endif /* TARGET_I386_TOPOLOGY_H */
> 
> As a follow-up it would be nice to clean up the documentation gtk-doc
> style, e.g. @cpu_index: bla bla, and first function name, then
> parameters, then description.

I would happily do that if I had a way to test the documentation
strings. Where's the "make docs" Makefile rule? :-)

> 
[...]

-- 
Eduardo