| Message ID | 1341951743-2285-7-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Jul 10, 2012 at 8:22 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > target-i386/topology.h | 138 ++++++++++++++++++++++++++++++++++++++++++++++++ > tests/.gitignore | 1 + > tests/Makefile | 7 ++- > tests/test-x86-cpuid.c | 108 +++++++++++++++++++++++++++++++++++++ > 4 files changed, 253 insertions(+), 1 deletion(-) > 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..de0f17a > --- /dev/null > +++ b/target-i386/topology.h > @@ -0,0 +1,138 @@ > +/* > + * 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 __QEMU_X86_TOPOLOGY_H__ > +#define __QEMU_X86_TOPOLOGY_H__ Please remove the leading and trailing underscores. The name should match the path, so it should be TARGET_I386_TOPOLOGY_H. > + > +#include <stdint.h> > +#include <string.h> > + > +#include "bitops.h" > + > +/* Return the bit width needed for 'count' IDs > + */ > +static unsigned bits_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 bits_for_count(nr_threads); > +} > + > +/* Bit width of the Core_ID field > + */ > +static inline unsigned apicid_core_width(unsigned nr_cores, unsigned nr_threads) > +{ > + return bits_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); The indentation seems to be off, please use checkpatch.pl to avoid these issues. > +} > + > +/* 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) + \ > + 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 uint8_t __make_apicid(unsigned nr_cores, unsigned nr_threads, Again, remove leading underscores. > + 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)) | \ > + smt_id; > +} > + > +/* Calculate thread/core/package IDs for a specific topology, > + * based on (continguous) 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; > +} > + > +/* Get package ID from an APIC ID > + */ > +static inline unsigned apicid_pkg_id(unsigned nr_cores, unsigned nr_threads, > + uint8_t apic_id) > +{ > + return apic_id >> apicid_pkg_offset(nr_cores, nr_threads); > +} > + > +/* Get core ID from an APIC ID > + */ > +static inline unsigned apicid_core_id(unsigned nr_cores, unsigned nr_threads, > + uint8_t apic_id) > +{ > + return (apic_id >> apicid_core_offset(nr_cores, nr_threads)) & \ > + ((1 << apicid_core_width(nr_cores, nr_threads)) - 1); > +} > + > +/* Get SMT (thread) ID from an APIC ID > + */ > +static inline unsigned apicid_smt_id(unsigned nr_cores, unsigned nr_threads, > + uint8_t apic_id) > +{ > + return apic_id & ((1 << apicid_smt_width(nr_cores, nr_threads)) - 1); > +} > + > +/* Make APIC ID for the CPU 'cpu_index' > + * > + * 'cpu_index' is a sequential, contiguous ID for the CPU. > + */ > +static inline uint8_t topo_make_apicid(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 __make_apicid(nr_cores, nr_threads, pkg_id, core_id, smt_id); > +} > + > +#endif /* __QMU_X86_TOPOLOGY_H__ */ > diff --git a/tests/.gitignore b/tests/.gitignore > index f9041f3..38c94ef 100644 > --- a/tests/.gitignore > +++ b/tests/.gitignore > @@ -10,4 +10,5 @@ test-qmp-commands.h > test-qmp-commands > test-qmp-input-strict > test-qmp-marshal.c > +test-x86-cpuid > *-test > diff --git a/tests/Makefile b/tests/Makefile > index b605e14..89bd890 100644 > --- a/tests/Makefile > +++ b/tests/Makefile > @@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF) > check-unit-y += tests/test-coroutine$(EXESUF) > check-unit-y += tests/test-visitor-serialization$(EXESUF) > check-unit-y += tests/test-iov$(EXESUF) > +check-unit-y += tests/test-x86-cpuid$(EXESUF) This probably tries to build the cpuid test also for non-x86 targets and break them all. > > check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh > > @@ -33,7 +34,8 @@ test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \ > tests/test-coroutine.o tests/test-string-output-visitor.o \ > tests/test-string-input-visitor.o tests/test-qmp-output-visitor.o \ > tests/test-qmp-input-visitor.o tests/test-qmp-input-strict.o \ > - tests/test-qmp-commands.o tests/test-visitor-serialization.o > + tests/test-qmp-commands.o tests/test-visitor-serialization.o \ > + tests/test-x86-cpuid.o > > test-qapi-obj-y = $(qobject-obj-y) $(qapi-obj-y) $(tools-obj-y) > test-qapi-obj-y += tests/test-qapi-visit.o tests/test-qapi-types.o > @@ -41,6 +43,8 @@ test-qapi-obj-y += module.o > > $(test-obj-y): QEMU_INCLUDES += -Itests > > +tests/test-x86-cpuid.o: QEMU_INCLUDES += -Itarget-i386 > + > tests/check-qint$(EXESUF): tests/check-qint.o qint.o $(tools-obj-y) > tests/check-qstring$(EXESUF): tests/check-qstring.o qstring.o $(tools-obj-y) > tests/check-qdict$(EXESUF): tests/check-qdict.o qdict.o qfloat.o qint.o qstring.o qbool.o qlist.o $(tools-obj-y) > @@ -49,6 +53,7 @@ tests/check-qfloat$(EXESUF): tests/check-qfloat.o qfloat.o $(tools-obj-y) > tests/check-qjson$(EXESUF): tests/check-qjson.o $(qobject-obj-y) $(tools-obj-y) > tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(coroutine-obj-y) $(tools-obj-y) > tests/test-iov$(EXESUF): tests/test-iov.o iov.o > +tests/test-x86-cpuid$(EXESUF): tests/test-x86-cpuid.o > > tests/test-qapi-types.c tests/test-qapi-types.h :\ > $(SRC_PATH)/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-types.py > diff --git a/tests/test-x86-cpuid.c b/tests/test-x86-cpuid.c > new file mode 100644 > index 0000000..b27ed62 > --- /dev/null > +++ b/tests/test-x86-cpuid.c > @@ -0,0 +1,108 @@ > +/* > + * Test code for x86 CPUID and Topology 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. > + */ > + > +#include <glib.h> > + > +/*FIXME: this should be built inside the i386 target directory instead */ > +#include "topology.h" > + > +static void test_topo_bits(void) > +{ > + /* simple tests for 1 thread per core, 1 core per socket */ > + g_assert_cmpuint(apicid_smt_width(1, 1), ==, 0); > + g_assert_cmpuint(apicid_core_width(1, 1), ==, 0); > + > + g_assert_cmpuint(topo_make_apicid(1, 1, 0), ==, 0); > + g_assert_cmpuint(topo_make_apicid(1, 1, 1), ==, 1); > + g_assert_cmpuint(topo_make_apicid(1, 1, 2), ==, 2); > + g_assert_cmpuint(topo_make_apicid(1, 1, 3), ==, 3); > + > + > + /* Test field width calculation for multiple values > + */ > + g_assert_cmpuint(apicid_smt_width(1, 2), ==, 1); > + g_assert_cmpuint(apicid_smt_width(1, 3), ==, 2); > + g_assert_cmpuint(apicid_smt_width(1, 4), ==, 2); > + > + g_assert_cmpuint(apicid_smt_width(1, 14), ==, 4); > + g_assert_cmpuint(apicid_smt_width(1, 15), ==, 4); > + g_assert_cmpuint(apicid_smt_width(1, 16), ==, 4); > + g_assert_cmpuint(apicid_smt_width(1, 17), ==, 5); > + > + > + g_assert_cmpuint(apicid_core_width(30, 2), ==, 5); > + g_assert_cmpuint(apicid_core_width(31, 2), ==, 5); > + g_assert_cmpuint(apicid_core_width(32, 2), ==, 5); > + g_assert_cmpuint(apicid_core_width(33, 2), ==, 6); > + > + > + /* build a weird topology and see if IDs are calculated correctly > + */ > + > + /* This will use 2 bits for thread ID and 3 bits for core ID > + */ > + g_assert_cmpuint(apicid_smt_width(6, 3), ==, 2); > + g_assert_cmpuint(apicid_core_width(6, 3), ==, 3); > + g_assert_cmpuint(apicid_pkg_offset(6, 3), ==, 5); > + > + g_assert_cmpuint(topo_make_apicid(6, 3, 0), ==, 0); > + g_assert_cmpuint(topo_make_apicid(6, 3, 1), ==, 1); > + g_assert_cmpuint(topo_make_apicid(6, 3, 2), ==, 2); > + > + g_assert_cmpuint(topo_make_apicid(6, 3, 1*3 + 0), ==, (1<<2) | 0); Spaces are needed around operators. > + g_assert_cmpuint(topo_make_apicid(6, 3, 1*3 + 1), ==, (1<<2) | 1); > + g_assert_cmpuint(topo_make_apicid(6, 3, 1*3 + 2), ==, (1<<2) | 2); > + > + g_assert_cmpuint(topo_make_apicid(6, 3, 2*3 + 0), ==, (2<<2) | 0); > + g_assert_cmpuint(topo_make_apicid(6, 3, 2*3 + 1), ==, (2<<2) | 1); > + g_assert_cmpuint(topo_make_apicid(6, 3, 2*3 + 2), ==, (2<<2) | 2); > + > + g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 0), ==, (5<<2) | 0); > + g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 1), ==, (5<<2) | 1); > + g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 2), ==, (5<<2) | 2); > + > + g_assert_cmpuint(topo_make_apicid(6, 3, 1*6*3 + 0*3 + 0), ==, (1<<5)); > + g_assert_cmpuint(topo_make_apicid(6, 3, 1*6*3 + 1*3 + 1), ==, > + (1<<5) | (1<<2) | 1); > + > + g_assert_cmpuint(topo_make_apicid(6, 3, 3*6*3 + 5*3 + 2), ==, > + (3<<5) | (5<<2) | 2); > + > + > + /* Check the APIC ID -> {pkg,core,thread} ID functions */ > + g_assert_cmpuint(apicid_pkg_id(6, 3, (3<<5) | (5<<2) | 2), ==, 3); > + g_assert_cmpuint(apicid_core_id(6, 3, (3<<5) | (5<<2) | 2), ==, 5); > + g_assert_cmpuint(apicid_smt_id(6, 3, (3<<5) | (5<<2) | 2), ==, 2); > +} > + > +int main(int argc, char **argv) > +{ > + g_test_init(&argc, &argv, NULL); > + > + g_test_add_func("/cpuid/topology/basic", test_topo_bits); > + > + g_test_run(); > + > + return 0; > +} > -- > 1.7.10.4 > >
On Thu, Jul 12, 2012 at 07:37:26PM +0000, Blue Swirl wrote: > On Tue, Jul 10, 2012 at 8:22 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: [...] > > +#ifndef __QEMU_X86_TOPOLOGY_H__ > > +#define __QEMU_X86_TOPOLOGY_H__ > > Please remove the leading and trailing underscores. The name should > match the path, so it should be TARGET_I386_TOPOLOGY_H. Done. Will be fixed in the next version. > > > +/* 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); > > The indentation seems to be off, please use checkpatch.pl to avoid these issues. Fixed for the next version. (BTW, checkpatch.pl didn't detect any issues on this patch) > > > +} > > + > > +/* 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) + \ > > + 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 uint8_t __make_apicid(unsigned nr_cores, unsigned nr_threads, > > Again, remove leading underscores. Fixed for the next version. > [...] > > diff --git a/tests/Makefile b/tests/Makefile > > index b605e14..89bd890 100644 > > --- a/tests/Makefile > > +++ b/tests/Makefile > > @@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF) > > check-unit-y += tests/test-coroutine$(EXESUF) > > check-unit-y += tests/test-visitor-serialization$(EXESUF) > > check-unit-y += tests/test-iov$(EXESUF) > > +check-unit-y += tests/test-x86-cpuid$(EXESUF) > > This probably tries to build the cpuid test also for non-x86 targets > and break them all. I don't think there's any concept of "targets" for the check-unit tests. I had to do the following, to be able to make a test that uses the target-i386 code: > > +tests/test-x86-cpuid.o: QEMU_INCLUDES += -Itarget-i386 Any suggestions to avoid this hack would be welcome. > [...] > > + g_assert_cmpuint(topo_make_apicid(6, 3, 0), ==, 0); > > + g_assert_cmpuint(topo_make_apicid(6, 3, 1), ==, 1); > > + g_assert_cmpuint(topo_make_apicid(6, 3, 2), ==, 2); > > + > > + g_assert_cmpuint(topo_make_apicid(6, 3, 1*3 + 0), ==, (1<<2) | 0); > > Spaces are needed around operators. > Do you honestly believe that this: g_assert_cmpuint(topo_make_apicid(6, 3, 5 * 3 + 0), ==, (5 << 2) | 0); g_assert_cmpuint(topo_make_apicid(6, 3, 5 * 3 + 1), ==, (5 << 2) | 1); g_assert_cmpuint(topo_make_apicid(6, 3, 5 * 3 + 2), ==, (5 << 2) | 2); is more readable than this: g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 0), ==, (5<<2) | 0); g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 1), ==, (5<<2) | 1); g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 2), ==, (5<<2) | 2); ? (I don't). > > + g_assert_cmpuint(topo_make_apicid(6, 3, 1*3 + 1), ==, (1<<2) | 1); > > + g_assert_cmpuint(topo_make_apicid(6, 3, 1*3 + 2), ==, (1<<2) | 2); > > + > > + g_assert_cmpuint(topo_make_apicid(6, 3, 2*3 + 0), ==, (2<<2) | 0); > > + g_assert_cmpuint(topo_make_apicid(6, 3, 2*3 + 1), ==, (2<<2) | 1); > > + g_assert_cmpuint(topo_make_apicid(6, 3, 2*3 + 2), ==, (2<<2) | 2); > > + > > + g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 0), ==, (5<<2) | 0); > > + g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 1), ==, (5<<2) | 1); > > + g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 2), ==, (5<<2) | 2); > > + > > + g_assert_cmpuint(topo_make_apicid(6, 3, 1*6*3 + 0*3 + 0), ==, (1<<5)); > > + g_assert_cmpuint(topo_make_apicid(6, 3, 1*6*3 + 1*3 + 1), ==, > > + (1<<5) | (1<<2) | 1); > > + > > + g_assert_cmpuint(topo_make_apicid(6, 3, 3*6*3 + 5*3 + 2), ==, > > + (3<<5) | (5<<2) | 2); > > + > > + > > + /* Check the APIC ID -> {pkg,core,thread} ID functions */ > > + g_assert_cmpuint(apicid_pkg_id(6, 3, (3<<5) | (5<<2) | 2), ==, 3); > > + g_assert_cmpuint(apicid_core_id(6, 3, (3<<5) | (5<<2) | 2), ==, 5); > > + g_assert_cmpuint(apicid_smt_id(6, 3, (3<<5) | (5<<2) | 2), ==, 2); > > +} > > + > > +int main(int argc, char **argv) > > +{ > > + g_test_init(&argc, &argv, NULL); > > + > > + g_test_add_func("/cpuid/topology/basic", test_topo_bits); > > + > > + g_test_run(); > > + > > + return 0; > > +} > > -- > > 1.7.10.4 > > > > >
On Fri, Jul 13, 2012 at 6:51 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Thu, Jul 12, 2012 at 07:37:26PM +0000, Blue Swirl wrote: >> On Tue, Jul 10, 2012 at 8:22 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > [...] >> > +#ifndef __QEMU_X86_TOPOLOGY_H__ >> > +#define __QEMU_X86_TOPOLOGY_H__ >> >> Please remove the leading and trailing underscores. The name should >> match the path, so it should be TARGET_I386_TOPOLOGY_H. > > Done. Will be fixed in the next version. > >> >> > +/* 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); >> >> The indentation seems to be off, please use checkpatch.pl to avoid these issues. > > Fixed for the next version. > > (BTW, checkpatch.pl didn't detect any issues on this patch) > >> >> > +} >> > + >> > +/* 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) + \ >> > + 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 uint8_t __make_apicid(unsigned nr_cores, unsigned nr_threads, >> >> Again, remove leading underscores. > > Fixed for the next version. > >> > [...] >> > diff --git a/tests/Makefile b/tests/Makefile >> > index b605e14..89bd890 100644 >> > --- a/tests/Makefile >> > +++ b/tests/Makefile >> > @@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF) >> > check-unit-y += tests/test-coroutine$(EXESUF) >> > check-unit-y += tests/test-visitor-serialization$(EXESUF) >> > check-unit-y += tests/test-iov$(EXESUF) >> > +check-unit-y += tests/test-x86-cpuid$(EXESUF) >> >> This probably tries to build the cpuid test also for non-x86 targets >> and break them all. > > I don't think there's any concept of "targets" for the check-unit tests. How about: check-qtest-i386-y = tests/test-x86-cpuid$(EXESUF) > I had to do the following, to be able to make a test that uses the > target-i386 code: > >> > +tests/test-x86-cpuid.o: QEMU_INCLUDES += -Itarget-i386 > > Any suggestions to avoid this hack would be welcome. Maybe it would be simpler to adjust #include path in the file. > > >> > [...] >> > + g_assert_cmpuint(topo_make_apicid(6, 3, 0), ==, 0); >> > + g_assert_cmpuint(topo_make_apicid(6, 3, 1), ==, 1); >> > + g_assert_cmpuint(topo_make_apicid(6, 3, 2), ==, 2); >> > + >> > + g_assert_cmpuint(topo_make_apicid(6, 3, 1*3 + 0), ==, (1<<2) | 0); >> >> Spaces are needed around operators. >> > > > Do you honestly believe that this: > > g_assert_cmpuint(topo_make_apicid(6, 3, 5 * 3 + 0), ==, (5 << 2) | 0); > g_assert_cmpuint(topo_make_apicid(6, 3, 5 * 3 + 1), ==, (5 << 2) | 1); > g_assert_cmpuint(topo_make_apicid(6, 3, 5 * 3 + 2), ==, (5 << 2) | 2); > > is more readable than this: > > g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 0), ==, (5<<2) | 0); > g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 1), ==, (5<<2) | 1); > g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 2), ==, (5<<2) | 2); > > ? Yes, I do. It's also the common style. > > (I don't). > > >> > + g_assert_cmpuint(topo_make_apicid(6, 3, 1*3 + 1), ==, (1<<2) | 1); >> > + g_assert_cmpuint(topo_make_apicid(6, 3, 1*3 + 2), ==, (1<<2) | 2); >> > + >> > + g_assert_cmpuint(topo_make_apicid(6, 3, 2*3 + 0), ==, (2<<2) | 0); >> > + g_assert_cmpuint(topo_make_apicid(6, 3, 2*3 + 1), ==, (2<<2) | 1); >> > + g_assert_cmpuint(topo_make_apicid(6, 3, 2*3 + 2), ==, (2<<2) | 2); >> > + >> > + g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 0), ==, (5<<2) | 0); >> > + g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 1), ==, (5<<2) | 1); >> > + g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 2), ==, (5<<2) | 2); >> > + >> > + g_assert_cmpuint(topo_make_apicid(6, 3, 1*6*3 + 0*3 + 0), ==, (1<<5)); >> > + g_assert_cmpuint(topo_make_apicid(6, 3, 1*6*3 + 1*3 + 1), ==, >> > + (1<<5) | (1<<2) | 1); >> > + >> > + g_assert_cmpuint(topo_make_apicid(6, 3, 3*6*3 + 5*3 + 2), ==, >> > + (3<<5) | (5<<2) | 2); >> > + >> > + >> > + /* Check the APIC ID -> {pkg,core,thread} ID functions */ >> > + g_assert_cmpuint(apicid_pkg_id(6, 3, (3<<5) | (5<<2) | 2), ==, 3); >> > + g_assert_cmpuint(apicid_core_id(6, 3, (3<<5) | (5<<2) | 2), ==, 5); >> > + g_assert_cmpuint(apicid_smt_id(6, 3, (3<<5) | (5<<2) | 2), ==, 2); >> > +} >> > + >> > +int main(int argc, char **argv) >> > +{ >> > + g_test_init(&argc, &argv, NULL); >> > + >> > + g_test_add_func("/cpuid/topology/basic", test_topo_bits); >> > + >> > + g_test_run(); >> > + >> > + return 0; >> > +} >> > -- >> > 1.7.10.4 >> > >> > >> > > -- > Eduardo
On Sat, Jul 14, 2012 at 09:14:30AM +0000, Blue Swirl wrote: [...] > >> > diff --git a/tests/Makefile b/tests/Makefile > >> > index b605e14..89bd890 100644 > >> > --- a/tests/Makefile > >> > +++ b/tests/Makefile > >> > @@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF) > >> > check-unit-y += tests/test-coroutine$(EXESUF) > >> > check-unit-y += tests/test-visitor-serialization$(EXESUF) > >> > check-unit-y += tests/test-iov$(EXESUF) > >> > +check-unit-y += tests/test-x86-cpuid$(EXESUF) > >> > >> This probably tries to build the cpuid test also for non-x86 targets > >> and break them all. > > > > I don't think there's any concept of "targets" for the check-unit tests. > > How about: > check-qtest-i386-y = tests/test-x86-cpuid$(EXESUF) test-x86-cpuid is not a qtest test case. > > > I had to do the following, to be able to make a test that uses the > > target-i386 code: > > > >> > +tests/test-x86-cpuid.o: QEMU_INCLUDES += -Itarget-i386 > > > > Any suggestions to avoid this hack would be welcome. > > Maybe it would be simpler to adjust #include path in the file. Using the full path on the #include line would break in case target-i386/topology.h include other files from the target-i386 directory.
On Mon, Jul 16, 2012 at 5:42 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Sat, Jul 14, 2012 at 09:14:30AM +0000, Blue Swirl wrote: > [...] >> >> > diff --git a/tests/Makefile b/tests/Makefile >> >> > index b605e14..89bd890 100644 >> >> > --- a/tests/Makefile >> >> > +++ b/tests/Makefile >> >> > @@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF) >> >> > check-unit-y += tests/test-coroutine$(EXESUF) >> >> > check-unit-y += tests/test-visitor-serialization$(EXESUF) >> >> > check-unit-y += tests/test-iov$(EXESUF) >> >> > +check-unit-y += tests/test-x86-cpuid$(EXESUF) >> >> >> >> This probably tries to build the cpuid test also for non-x86 targets >> >> and break them all. >> > >> > I don't think there's any concept of "targets" for the check-unit tests. >> >> How about: >> check-qtest-i386-y = tests/test-x86-cpuid$(EXESUF) > > test-x86-cpuid is not a qtest test case. Why not? I don't think it is a unit test either, judging from what the other unit tests do. > >> >> > I had to do the following, to be able to make a test that uses the >> > target-i386 code: >> > >> >> > +tests/test-x86-cpuid.o: QEMU_INCLUDES += -Itarget-i386 >> > >> > Any suggestions to avoid this hack would be welcome. >> >> Maybe it would be simpler to adjust #include path in the file. > > Using the full path on the #include line would break in case > target-i386/topology.h include other files from the target-i386 > directory. That's fragile. Maybe the target-xyz files should use the full path. > > -- > Eduardo
On Mon, Jul 23, 2012 at 04:49:07PM +0000, Blue Swirl wrote: > On Mon, Jul 16, 2012 at 5:42 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Sat, Jul 14, 2012 at 09:14:30AM +0000, Blue Swirl wrote: > > [...] > >> >> > diff --git a/tests/Makefile b/tests/Makefile > >> >> > index b605e14..89bd890 100644 > >> >> > --- a/tests/Makefile > >> >> > +++ b/tests/Makefile > >> >> > @@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF) > >> >> > check-unit-y += tests/test-coroutine$(EXESUF) > >> >> > check-unit-y += tests/test-visitor-serialization$(EXESUF) > >> >> > check-unit-y += tests/test-iov$(EXESUF) > >> >> > +check-unit-y += tests/test-x86-cpuid$(EXESUF) > >> >> > >> >> This probably tries to build the cpuid test also for non-x86 targets > >> >> and break them all. > >> > > >> > I don't think there's any concept of "targets" for the check-unit tests. > >> > >> How about: > >> check-qtest-i386-y = tests/test-x86-cpuid$(EXESUF) > > > > test-x86-cpuid is not a qtest test case. > > Why not? I don't think it is a unit test either, judging from what the > other unit tests do. It is absolutely a unit test. I don't know why you don't think so. It simply checks the results of the APIC ID calculation functions. > > > > >> > >> > I had to do the following, to be able to make a test that uses the > >> > target-i386 code: > >> > > >> >> > +tests/test-x86-cpuid.o: QEMU_INCLUDES += -Itarget-i386 > >> > > >> > Any suggestions to avoid this hack would be welcome. > >> > >> Maybe it would be simpler to adjust #include path in the file. > > > > Using the full path on the #include line would break in case > > target-i386/topology.h include other files from the target-i386 > > directory. > > That's fragile. Maybe the target-xyz files should use the full path. Yes, it would be fragile. That's why I used -Itarget-i386 (not a perfect solution, but more likely to stay working and not break easily). I don't know if I understand what you propose. You mean to change the 256 target-specific #include lines inside qemu? $ git grep -f <(find target-* -name '*.h' | sed -e 's@target-[^/]*/@#include "@' | sort -u) target-* | wc -l 256
On Mon, Jul 23, 2012 at 6:59 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Mon, Jul 23, 2012 at 04:49:07PM +0000, Blue Swirl wrote: >> On Mon, Jul 16, 2012 at 5:42 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: >> > On Sat, Jul 14, 2012 at 09:14:30AM +0000, Blue Swirl wrote: >> > [...] >> >> >> > diff --git a/tests/Makefile b/tests/Makefile >> >> >> > index b605e14..89bd890 100644 >> >> >> > --- a/tests/Makefile >> >> >> > +++ b/tests/Makefile >> >> >> > @@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF) >> >> >> > check-unit-y += tests/test-coroutine$(EXESUF) >> >> >> > check-unit-y += tests/test-visitor-serialization$(EXESUF) >> >> >> > check-unit-y += tests/test-iov$(EXESUF) >> >> >> > +check-unit-y += tests/test-x86-cpuid$(EXESUF) >> >> >> >> >> >> This probably tries to build the cpuid test also for non-x86 targets >> >> >> and break them all. >> >> > >> >> > I don't think there's any concept of "targets" for the check-unit tests. >> >> >> >> How about: >> >> check-qtest-i386-y = tests/test-x86-cpuid$(EXESUF) >> > >> > test-x86-cpuid is not a qtest test case. >> >> Why not? I don't think it is a unit test either, judging from what the >> other unit tests do. > > It is absolutely a unit test. I don't know why you don't think so. It > simply checks the results of the APIC ID calculation functions. Yes, but the other 'unit tests' (the names used here are very confusing, btw) check supporting functions like coroutines, not hardware. Hardware tests (qtest) need to bind to an architecture, in this case x86. > > >> >> > >> >> >> >> > I had to do the following, to be able to make a test that uses the >> >> > target-i386 code: >> >> > >> >> >> > +tests/test-x86-cpuid.o: QEMU_INCLUDES += -Itarget-i386 >> >> > >> >> > Any suggestions to avoid this hack would be welcome. >> >> >> >> Maybe it would be simpler to adjust #include path in the file. >> > >> > Using the full path on the #include line would break in case >> > target-i386/topology.h include other files from the target-i386 >> > directory. >> >> That's fragile. Maybe the target-xyz files should use the full path. > > Yes, it would be fragile. That's why I used -Itarget-i386 (not a perfect > solution, but more likely to stay working and not break easily). > > I don't know if I understand what you propose. You mean to change the > 256 target-specific #include lines inside qemu? > > $ git grep -f <(find target-* -name '*.h' | sed -e 's@target-[^/]*/@#include "@' | sort -u) target-* | wc -l > 256 That does not look very attractive either. Scripting could help for such mechanical changes. Maybe later. > > -- > Eduardo
On Mon, Jul 23, 2012 at 07:11:11PM +0000, Blue Swirl wrote: > On Mon, Jul 23, 2012 at 6:59 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Mon, Jul 23, 2012 at 04:49:07PM +0000, Blue Swirl wrote: > >> On Mon, Jul 16, 2012 at 5:42 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > >> > On Sat, Jul 14, 2012 at 09:14:30AM +0000, Blue Swirl wrote: > >> > [...] > >> >> >> > diff --git a/tests/Makefile b/tests/Makefile > >> >> >> > index b605e14..89bd890 100644 > >> >> >> > --- a/tests/Makefile > >> >> >> > +++ b/tests/Makefile > >> >> >> > @@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF) > >> >> >> > check-unit-y += tests/test-coroutine$(EXESUF) > >> >> >> > check-unit-y += tests/test-visitor-serialization$(EXESUF) > >> >> >> > check-unit-y += tests/test-iov$(EXESUF) > >> >> >> > +check-unit-y += tests/test-x86-cpuid$(EXESUF) > >> >> >> > >> >> >> This probably tries to build the cpuid test also for non-x86 targets > >> >> >> and break them all. > >> >> > > >> >> > I don't think there's any concept of "targets" for the check-unit tests. > >> >> > >> >> How about: > >> >> check-qtest-i386-y = tests/test-x86-cpuid$(EXESUF) > >> > > >> > test-x86-cpuid is not a qtest test case. > >> > >> Why not? I don't think it is a unit test either, judging from what the > >> other unit tests do. > > > > It is absolutely a unit test. I don't know why you don't think so. It > > simply checks the results of the APIC ID calculation functions. > > Yes, but the other 'unit tests' (the names used here are very > confusing, btw) check supporting functions like coroutines, not > hardware. Hardware tests (qtest) need to bind to an architecture, in > this case x86. test-x86-cpuid doesn't check hardware, either. It just checks the simple functions that calculate APIC IDs (to make sure the math is correct). Those functions aren't even used by any hardware emulation code until later in the patch series (when the CPU initialization code gets changed to use the new function).
On Mon, Jul 23, 2012 at 7:28 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Mon, Jul 23, 2012 at 07:11:11PM +0000, Blue Swirl wrote: >> On Mon, Jul 23, 2012 at 6:59 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: >> > On Mon, Jul 23, 2012 at 04:49:07PM +0000, Blue Swirl wrote: >> >> On Mon, Jul 16, 2012 at 5:42 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: >> >> > On Sat, Jul 14, 2012 at 09:14:30AM +0000, Blue Swirl wrote: >> >> > [...] >> >> >> >> > diff --git a/tests/Makefile b/tests/Makefile >> >> >> >> > index b605e14..89bd890 100644 >> >> >> >> > --- a/tests/Makefile >> >> >> >> > +++ b/tests/Makefile >> >> >> >> > @@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF) >> >> >> >> > check-unit-y += tests/test-coroutine$(EXESUF) >> >> >> >> > check-unit-y += tests/test-visitor-serialization$(EXESUF) >> >> >> >> > check-unit-y += tests/test-iov$(EXESUF) >> >> >> >> > +check-unit-y += tests/test-x86-cpuid$(EXESUF) >> >> >> >> >> >> >> >> This probably tries to build the cpuid test also for non-x86 targets >> >> >> >> and break them all. >> >> >> > >> >> >> > I don't think there's any concept of "targets" for the check-unit tests. >> >> >> >> >> >> How about: >> >> >> check-qtest-i386-y = tests/test-x86-cpuid$(EXESUF) >> >> > >> >> > test-x86-cpuid is not a qtest test case. >> >> >> >> Why not? I don't think it is a unit test either, judging from what the >> >> other unit tests do. >> > >> > It is absolutely a unit test. I don't know why you don't think so. It >> > simply checks the results of the APIC ID calculation functions. >> >> Yes, but the other 'unit tests' (the names used here are very >> confusing, btw) check supporting functions like coroutines, not >> hardware. Hardware tests (qtest) need to bind to an architecture, in >> this case x86. > > test-x86-cpuid doesn't check hardware, either. It just checks the simple > functions that calculate APIC IDs (to make sure the math is correct). > Those functions aren't even used by any hardware emulation code until > later in the patch series (when the CPU initialization code gets changed > to use the new function). By that time the function is clearly x86 HW and the check is an x86 hardware check. QEMU as whole consists of simple functions that calculate something. > > -- > Eduardo
On Mon, Jul 23, 2012 at 07:44:44PM +0000, Blue Swirl wrote: > On Mon, Jul 23, 2012 at 7:28 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Mon, Jul 23, 2012 at 07:11:11PM +0000, Blue Swirl wrote: > >> On Mon, Jul 23, 2012 at 6:59 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > >> > On Mon, Jul 23, 2012 at 04:49:07PM +0000, Blue Swirl wrote: > >> >> On Mon, Jul 16, 2012 at 5:42 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > >> >> > On Sat, Jul 14, 2012 at 09:14:30AM +0000, Blue Swirl wrote: > >> >> > [...] > >> >> >> >> > diff --git a/tests/Makefile b/tests/Makefile > >> >> >> >> > index b605e14..89bd890 100644 > >> >> >> >> > --- a/tests/Makefile > >> >> >> >> > +++ b/tests/Makefile > >> >> >> >> > @@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF) > >> >> >> >> > check-unit-y += tests/test-coroutine$(EXESUF) > >> >> >> >> > check-unit-y += tests/test-visitor-serialization$(EXESUF) > >> >> >> >> > check-unit-y += tests/test-iov$(EXESUF) > >> >> >> >> > +check-unit-y += tests/test-x86-cpuid$(EXESUF) > >> >> >> >> > >> >> >> >> This probably tries to build the cpuid test also for non-x86 targets > >> >> >> >> and break them all. > >> >> >> > > >> >> >> > I don't think there's any concept of "targets" for the check-unit tests. > >> >> >> > >> >> >> How about: > >> >> >> check-qtest-i386-y = tests/test-x86-cpuid$(EXESUF) > >> >> > > >> >> > test-x86-cpuid is not a qtest test case. > >> >> > >> >> Why not? I don't think it is a unit test either, judging from what the > >> >> other unit tests do. > >> > > >> > It is absolutely a unit test. I don't know why you don't think so. It > >> > simply checks the results of the APIC ID calculation functions. > >> > >> Yes, but the other 'unit tests' (the names used here are very > >> confusing, btw) check supporting functions like coroutines, not > >> hardware. Hardware tests (qtest) need to bind to an architecture, in > >> this case x86. > > > > test-x86-cpuid doesn't check hardware, either. It just checks the simple > > functions that calculate APIC IDs (to make sure the math is correct). > > Those functions aren't even used by any hardware emulation code until > > later in the patch series (when the CPU initialization code gets changed > > to use the new function). > > By that time the function is clearly x86 HW and the check is an x86 > hardware check. QEMU as whole consists of simple functions that > calculate something. It's useful onily for a specific architecture, yes, but it surely doesn't make libqtest necessary. That's the difference between the unit tests and qtest test cases: unit tests simply test small parts of the code (that may or may not be hardware-specific), and qtest tests hardware emulation after starting up a whole qemu process. It's a different kind of testing, with different sets of goals. I suppose you are not arguing that no function inside target-* would be ever allowed to have a unit test written for it. That would be a very silly constraint for people writing tests.
On Mon, Jul 23, 2012 at 8:14 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Mon, Jul 23, 2012 at 07:44:44PM +0000, Blue Swirl wrote: >> On Mon, Jul 23, 2012 at 7:28 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: >> > On Mon, Jul 23, 2012 at 07:11:11PM +0000, Blue Swirl wrote: >> >> On Mon, Jul 23, 2012 at 6:59 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: >> >> > On Mon, Jul 23, 2012 at 04:49:07PM +0000, Blue Swirl wrote: >> >> >> On Mon, Jul 16, 2012 at 5:42 PM, Eduardo Habkost <ehabkost@redhat.com> wrote: >> >> >> > On Sat, Jul 14, 2012 at 09:14:30AM +0000, Blue Swirl wrote: >> >> >> > [...] >> >> >> >> >> > diff --git a/tests/Makefile b/tests/Makefile >> >> >> >> >> > index b605e14..89bd890 100644 >> >> >> >> >> > --- a/tests/Makefile >> >> >> >> >> > +++ b/tests/Makefile >> >> >> >> >> > @@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF) >> >> >> >> >> > check-unit-y += tests/test-coroutine$(EXESUF) >> >> >> >> >> > check-unit-y += tests/test-visitor-serialization$(EXESUF) >> >> >> >> >> > check-unit-y += tests/test-iov$(EXESUF) >> >> >> >> >> > +check-unit-y += tests/test-x86-cpuid$(EXESUF) >> >> >> >> >> >> >> >> >> >> This probably tries to build the cpuid test also for non-x86 targets >> >> >> >> >> and break them all. >> >> >> >> > >> >> >> >> > I don't think there's any concept of "targets" for the check-unit tests. >> >> >> >> >> >> >> >> How about: >> >> >> >> check-qtest-i386-y = tests/test-x86-cpuid$(EXESUF) >> >> >> > >> >> >> > test-x86-cpuid is not a qtest test case. >> >> >> >> >> >> Why not? I don't think it is a unit test either, judging from what the >> >> >> other unit tests do. >> >> > >> >> > It is absolutely a unit test. I don't know why you don't think so. It >> >> > simply checks the results of the APIC ID calculation functions. >> >> >> >> Yes, but the other 'unit tests' (the names used here are very >> >> confusing, btw) check supporting functions like coroutines, not >> >> hardware. Hardware tests (qtest) need to bind to an architecture, in >> >> this case x86. >> > >> > test-x86-cpuid doesn't check hardware, either. It just checks the simple >> > functions that calculate APIC IDs (to make sure the math is correct). >> > Those functions aren't even used by any hardware emulation code until >> > later in the patch series (when the CPU initialization code gets changed >> > to use the new function). >> >> By that time the function is clearly x86 HW and the check is an x86 >> hardware check. QEMU as whole consists of simple functions that >> calculate something. > > It's useful onily for a specific architecture, yes, but it surely > doesn't make libqtest necessary. > > That's the difference between the unit tests and qtest test cases: unit > tests simply test small parts of the code (that may or may not be > hardware-specific), and qtest tests hardware emulation after starting up > a whole qemu process. It's a different kind of testing, with different > sets of goals. > Thanks for the clarification. I agree now that this is not a qtest. I think this is a new category of tests compared to those we have now: supporting function unit tests and hardware tests at device I/O boundary level. > I suppose you are not arguing that no function inside target-* would be > ever allowed to have a unit test written for it. That would be a very > silly constraint for people writing tests. Of course. What I really want is that if x86 targets are not built, this test is skipped like qtests. This could be achieved with something like: check-unit-arch-i386-y = tests/test-x86-cpuid$(EXESUF) check-unit-y += $(foreach TARGET,$(TARGETS), $(check-unit-arch-$(TARGET)-y)) > > -- > Eduardo
diff --git a/target-i386/topology.h b/target-i386/topology.h new file mode 100644 index 0000000..de0f17a --- /dev/null +++ b/target-i386/topology.h @@ -0,0 +1,138 @@ +/* + * 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 __QEMU_X86_TOPOLOGY_H__ +#define __QEMU_X86_TOPOLOGY_H__ + +#include <stdint.h> +#include <string.h> + +#include "bitops.h" + +/* Return the bit width needed for 'count' IDs + */ +static unsigned bits_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 bits_for_count(nr_threads); +} + +/* Bit width of the Core_ID field + */ +static inline unsigned apicid_core_width(unsigned nr_cores, unsigned nr_threads) +{ + return bits_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) + \ + 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 uint8_t __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)) | \ + smt_id; +} + +/* Calculate thread/core/package IDs for a specific topology, + * based on (continguous) 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; +} + +/* Get package ID from an APIC ID + */ +static inline unsigned apicid_pkg_id(unsigned nr_cores, unsigned nr_threads, + uint8_t apic_id) +{ + return apic_id >> apicid_pkg_offset(nr_cores, nr_threads); +} + +/* Get core ID from an APIC ID + */ +static inline unsigned apicid_core_id(unsigned nr_cores, unsigned nr_threads, + uint8_t apic_id) +{ + return (apic_id >> apicid_core_offset(nr_cores, nr_threads)) & \ + ((1 << apicid_core_width(nr_cores, nr_threads)) - 1); +} + +/* Get SMT (thread) ID from an APIC ID + */ +static inline unsigned apicid_smt_id(unsigned nr_cores, unsigned nr_threads, + uint8_t apic_id) +{ + return apic_id & ((1 << apicid_smt_width(nr_cores, nr_threads)) - 1); +} + +/* Make APIC ID for the CPU 'cpu_index' + * + * 'cpu_index' is a sequential, contiguous ID for the CPU. + */ +static inline uint8_t topo_make_apicid(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 __make_apicid(nr_cores, nr_threads, pkg_id, core_id, smt_id); +} + +#endif /* __QMU_X86_TOPOLOGY_H__ */ diff --git a/tests/.gitignore b/tests/.gitignore index f9041f3..38c94ef 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -10,4 +10,5 @@ test-qmp-commands.h test-qmp-commands test-qmp-input-strict test-qmp-marshal.c +test-x86-cpuid *-test diff --git a/tests/Makefile b/tests/Makefile index b605e14..89bd890 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -15,6 +15,7 @@ check-unit-y += tests/test-string-output-visitor$(EXESUF) check-unit-y += tests/test-coroutine$(EXESUF) check-unit-y += tests/test-visitor-serialization$(EXESUF) check-unit-y += tests/test-iov$(EXESUF) +check-unit-y += tests/test-x86-cpuid$(EXESUF) check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh @@ -33,7 +34,8 @@ test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \ tests/test-coroutine.o tests/test-string-output-visitor.o \ tests/test-string-input-visitor.o tests/test-qmp-output-visitor.o \ tests/test-qmp-input-visitor.o tests/test-qmp-input-strict.o \ - tests/test-qmp-commands.o tests/test-visitor-serialization.o + tests/test-qmp-commands.o tests/test-visitor-serialization.o \ + tests/test-x86-cpuid.o test-qapi-obj-y = $(qobject-obj-y) $(qapi-obj-y) $(tools-obj-y) test-qapi-obj-y += tests/test-qapi-visit.o tests/test-qapi-types.o @@ -41,6 +43,8 @@ test-qapi-obj-y += module.o $(test-obj-y): QEMU_INCLUDES += -Itests +tests/test-x86-cpuid.o: QEMU_INCLUDES += -Itarget-i386 + tests/check-qint$(EXESUF): tests/check-qint.o qint.o $(tools-obj-y) tests/check-qstring$(EXESUF): tests/check-qstring.o qstring.o $(tools-obj-y) tests/check-qdict$(EXESUF): tests/check-qdict.o qdict.o qfloat.o qint.o qstring.o qbool.o qlist.o $(tools-obj-y) @@ -49,6 +53,7 @@ tests/check-qfloat$(EXESUF): tests/check-qfloat.o qfloat.o $(tools-obj-y) tests/check-qjson$(EXESUF): tests/check-qjson.o $(qobject-obj-y) $(tools-obj-y) tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(coroutine-obj-y) $(tools-obj-y) tests/test-iov$(EXESUF): tests/test-iov.o iov.o +tests/test-x86-cpuid$(EXESUF): tests/test-x86-cpuid.o tests/test-qapi-types.c tests/test-qapi-types.h :\ $(SRC_PATH)/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-types.py diff --git a/tests/test-x86-cpuid.c b/tests/test-x86-cpuid.c new file mode 100644 index 0000000..b27ed62 --- /dev/null +++ b/tests/test-x86-cpuid.c @@ -0,0 +1,108 @@ +/* + * Test code for x86 CPUID and Topology 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. + */ + +#include <glib.h> + +/*FIXME: this should be built inside the i386 target directory instead */ +#include "topology.h" + +static void test_topo_bits(void) +{ + /* simple tests for 1 thread per core, 1 core per socket */ + g_assert_cmpuint(apicid_smt_width(1, 1), ==, 0); + g_assert_cmpuint(apicid_core_width(1, 1), ==, 0); + + g_assert_cmpuint(topo_make_apicid(1, 1, 0), ==, 0); + g_assert_cmpuint(topo_make_apicid(1, 1, 1), ==, 1); + g_assert_cmpuint(topo_make_apicid(1, 1, 2), ==, 2); + g_assert_cmpuint(topo_make_apicid(1, 1, 3), ==, 3); + + + /* Test field width calculation for multiple values + */ + g_assert_cmpuint(apicid_smt_width(1, 2), ==, 1); + g_assert_cmpuint(apicid_smt_width(1, 3), ==, 2); + g_assert_cmpuint(apicid_smt_width(1, 4), ==, 2); + + g_assert_cmpuint(apicid_smt_width(1, 14), ==, 4); + g_assert_cmpuint(apicid_smt_width(1, 15), ==, 4); + g_assert_cmpuint(apicid_smt_width(1, 16), ==, 4); + g_assert_cmpuint(apicid_smt_width(1, 17), ==, 5); + + + g_assert_cmpuint(apicid_core_width(30, 2), ==, 5); + g_assert_cmpuint(apicid_core_width(31, 2), ==, 5); + g_assert_cmpuint(apicid_core_width(32, 2), ==, 5); + g_assert_cmpuint(apicid_core_width(33, 2), ==, 6); + + + /* build a weird topology and see if IDs are calculated correctly + */ + + /* This will use 2 bits for thread ID and 3 bits for core ID + */ + g_assert_cmpuint(apicid_smt_width(6, 3), ==, 2); + g_assert_cmpuint(apicid_core_width(6, 3), ==, 3); + g_assert_cmpuint(apicid_pkg_offset(6, 3), ==, 5); + + g_assert_cmpuint(topo_make_apicid(6, 3, 0), ==, 0); + g_assert_cmpuint(topo_make_apicid(6, 3, 1), ==, 1); + g_assert_cmpuint(topo_make_apicid(6, 3, 2), ==, 2); + + g_assert_cmpuint(topo_make_apicid(6, 3, 1*3 + 0), ==, (1<<2) | 0); + g_assert_cmpuint(topo_make_apicid(6, 3, 1*3 + 1), ==, (1<<2) | 1); + g_assert_cmpuint(topo_make_apicid(6, 3, 1*3 + 2), ==, (1<<2) | 2); + + g_assert_cmpuint(topo_make_apicid(6, 3, 2*3 + 0), ==, (2<<2) | 0); + g_assert_cmpuint(topo_make_apicid(6, 3, 2*3 + 1), ==, (2<<2) | 1); + g_assert_cmpuint(topo_make_apicid(6, 3, 2*3 + 2), ==, (2<<2) | 2); + + g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 0), ==, (5<<2) | 0); + g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 1), ==, (5<<2) | 1); + g_assert_cmpuint(topo_make_apicid(6, 3, 5*3 + 2), ==, (5<<2) | 2); + + g_assert_cmpuint(topo_make_apicid(6, 3, 1*6*3 + 0*3 + 0), ==, (1<<5)); + g_assert_cmpuint(topo_make_apicid(6, 3, 1*6*3 + 1*3 + 1), ==, + (1<<5) | (1<<2) | 1); + + g_assert_cmpuint(topo_make_apicid(6, 3, 3*6*3 + 5*3 + 2), ==, + (3<<5) | (5<<2) | 2); + + + /* Check the APIC ID -> {pkg,core,thread} ID functions */ + g_assert_cmpuint(apicid_pkg_id(6, 3, (3<<5) | (5<<2) | 2), ==, 3); + g_assert_cmpuint(apicid_core_id(6, 3, (3<<5) | (5<<2) | 2), ==, 5); + g_assert_cmpuint(apicid_smt_id(6, 3, (3<<5) | (5<<2) | 2), ==, 2); +} + +int main(int argc, char **argv) +{ + g_test_init(&argc, &argv, NULL); + + g_test_add_func("/cpuid/topology/basic", test_topo_bits); + + g_test_run(); + + return 0; +}
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- target-i386/topology.h | 138 ++++++++++++++++++++++++++++++++++++++++++++++++ tests/.gitignore | 1 + tests/Makefile | 7 ++- tests/test-x86-cpuid.c | 108 +++++++++++++++++++++++++++++++++++++ 4 files changed, 253 insertions(+), 1 deletion(-) create mode 100644 target-i386/topology.h create mode 100644 tests/test-x86-cpuid.c