On Fri, Jul 13, 2012 at 6:51 PM, Eduardo Habkost <ehabk...@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 <ehabk...@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 _______________________________________________ SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios