[SeaBIOS] [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions

[Eduardo Habkost]

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 at 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
> >
> >
> 

-- 
Eduardo