Re: [Qemu-devel] [PATCH for-1.4 qom-cpu 8/9 v6] target-i386: Topology & APIC ID utility functions

[Blue Swirl]

On Thu, Jan 24, 2013 at 12:07 PM, Eduardo Habkost <address@hidden> wrote:
> On Wed, Jan 23, 2013 at 08:49:58PM +0100, Andreas Färber wrote:
>> Am 23.01.2013 18:58, schrieb Eduardo Habkost:
>> > This introduces utility functions for the APIC ID calculation, based on:
>> >   Intel® 64 Architecture Processor Topology Enumeration
>> >   
>> > http://software.intel.com/en-us/articles/intel-64-architecture-processor-topology-enumeration/
>> >
>> > The code should be also 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().
>> >
>> > Unit tests included. The code is still not being used anywhere. It will be 
>> > used
>> > by the the next patch.
>>
>> (I would drop this reference to "next patch" when applying.)
>> >
>> > Signed-off-by: Eduardo Habkost <address@hidden>
>> [...]
>> > diff --git a/tests/Makefile b/tests/Makefile
>> > index d86e95a..4b98d4f 100644
>> > --- a/tests/Makefile
>> > +++ b/tests/Makefile
>> > @@ -45,6 +45,10 @@ gcov-files-test-aio-$(CONFIG_WIN32) = aio-win32.c
>> >  gcov-files-test-aio-$(CONFIG_POSIX) = aio-posix.c
>> >  check-unit-y += tests/test-thread-pool$(EXESUF)
>> >  gcov-files-test-thread-pool-y = thread-pool.c
>> > +check-unit-y += tests/test-x86-cpuid$(EXESUF)
>> > +# all code tested by test-x86-cpuid is inside topology.h,
>> > +# so add the test file itself to the gcov list
>> > +gcov-files-test-x86-cpuid-y = tests/test-x86-cpuid.c
>> >
>> >  check-block-$(CONFIG_POSIX) += tests/qemu-iotests-quick.sh
>> >
>>
>> With patch 7/9 dropped I am more comfortable with the test integration.
>>
>> I wonder however whether the gcov line is correct - won't this screw up
>> the statistics so that it's better to drop that line and to add
>> hw/pc_piix.c or target-i386/cpu.c in 9/9 instead? Blue?
>
> I want to make gcov check for coverage only of topology.h (that's where the
> tested code lives). Including test-x86-cpuid.c is the closest I could get to
> that[1]. Including pc_piix.c or cpu.c would surely screw up the numbers, as 
> the
> tests don't cover any of the pc_piix.c or target-i386/cpu.c code.

In my first version of the gcov patch, the statistics were gathered
for all tests, but that did not work well since there were double
counts due to coverage from an emulator run and standalone test case
runs.

>
> [1] If I set gcov-files-test-x86-cpuid-y = target-i386/topology.h, I get:
>
>   GTESTER tests/test-x86-cpuid
>   Gcov report for target-i386/topology.h:
>   target-i386/topology.gcno:cannot open graph file
>
> It looks like the .gcno file generation is per-object-file, not 
> per-source-file
> (gcov-files-*-y being a list of .c files confused me). If that's the case, 
> then
> the only valid value for gcov-files-test-x86-cpuid-y is really
> tests/test-x86-cpuid.c, because all the tested code is being compiled inside
> tests/test-x86-cpuid.o.

But we are not interested in the coverage of the test suite code.

Perhaps the design of gcov is not so well thought out and it is not
very suitable for this kind of files.

>
> --
> Eduardo