[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility f
From: |
Blue Swirl |
Subject: |
Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions |
Date: |
Sat, 14 Jul 2012 09:14:30 +0000 |
On Fri, Jul 13, 2012 at 6:51 PM, Eduardo Habkost <address@hidden> 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 <address@hidden> 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
- Re: [Qemu-devel] [QEMU PATCH 2/7] hw/apic.c: rename bit functions to not conflict with bitops.h, (continued)
[Qemu-devel] [Seabios RFC PATCH 1/1] get lapic IDs from fw_cfg, Eduardo Habkost, 2012/07/10
[Qemu-devel] [QEMU RFC PATCH 4/7] i386: create apic_id_for_cpu() function, Eduardo Habkost, 2012/07/10
[Qemu-devel] [QEMU RFC PATCH 7/7] generate APIC IDs according to CPU topology, Eduardo Habkost, 2012/07/10
[Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions, Eduardo Habkost, 2012/07/10
- Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions, Blue Swirl, 2012/07/12
- Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions, Eduardo Habkost, 2012/07/13
- Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions,
Blue Swirl <=
- Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions, Eduardo Habkost, 2012/07/16
- Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions, Blue Swirl, 2012/07/23
- Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions, Eduardo Habkost, 2012/07/23
- Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions, Blue Swirl, 2012/07/23
- Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions, Eduardo Habkost, 2012/07/23
- Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions, Blue Swirl, 2012/07/23
- Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions, Eduardo Habkost, 2012/07/23
- Re: [Qemu-devel] [QEMU RFC PATCH 6/7] i386: topology & APIC ID utility functions, Blue Swirl, 2012/07/24
[Qemu-devel] [QEMU PATCH 3/7] kvm: set vcpu_id to APIC ID instead of CPU index, Eduardo Habkost, 2012/07/10
[Qemu-devel] [QEMU RFC PATCH 5/7] pc: write lapic info (apic IDs) to fw_cfg so seabios can use it, Eduardo Habkost, 2012/07/10