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 <ehabk...@redhat.com>
> > ---
> > 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
