Patchwork [for-1.4,qom-cpu,8/9,v6] target-i386: Topology & APIC ID utility functions

login
register
mail settings
Submitter Eduardo Habkost
Date Jan. 23, 2013, 5:58 p.m.
Message ID <20130123175827.GF31391@otherpad.lan.raisama.net>
Download mbox | patch
Permalink /patch/215011/
State New
Headers show

Comments

Eduardo Habkost - Jan. 23, 2013, 5:58 p.m.
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.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v2:
 - Support 32-bit APIC IDs (in case x2APIC is going to be used)
 - Coding style changes
 - Use TARGET_I386_TOPOLOGY_H instead of __QEMU_X86_TOPOLOGY_H__
 - Rename topo_make_apic_id() to topo_apicid_for_cpu()
 - Rename __make_apicid() to topo_make_apicid()
 - Spaces around operators on test-x86-cpuid.c, as requested by
   Blue Swirl
 - Make test-x86-cpuid a target-specific test

Changes v3:
 - Add documentation pointers to the code
 - Rename bits_for_count() to bitwidth_for_count()
 - Remove unused apicid_*_id() functions

Changes v4:
 - Remove now-obsolete FIXME comment from test-x86-cpuid.c
 - Change bitops.h include to qemu/bitops.h
 - Add gcov file list to test-x86-cpuid

Changes v5:
 - Remove trailing backslashes (leftovers from macro versions of the
   code)
 - Rename functions to use either "apicid_" or "x86_" prefixes

Changes v6:
 - Simply set QEMU_INCLUDES manually for test-x86-cpuid.o,
   instead of using a generic "per-target unit tests" mechanism
   Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-i386/topology.h | 136 +++++++++++++++++++++++++++++++++++++++++++++++++
 tests/.gitignore       |   1 +
 tests/Makefile         |  10 +++-
 tests/test-x86-cpuid.c | 110 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 256 insertions(+), 1 deletion(-)
 create mode 100644 target-i386/topology.h
 create mode 100644 tests/test-x86-cpuid.c
Andreas Färber - Jan. 23, 2013, 7:49 p.m.
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 <ehabkost@redhat.com>
[...]
> 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?

Regards,
Andreas
Eduardo Habkost - Jan. 24, 2013, 12:07 p.m.
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 <ehabkost@redhat.com>
> [...]
> > 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.

[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.
Blue Swirl - Jan. 25, 2013, 5:47 p.m.
On Thu, Jan 24, 2013 at 12:07 PM, Eduardo Habkost <ehabkost@redhat.com> 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 <ehabkost@redhat.com>
>> [...]
>> > 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
Eduardo Habkost - Jan. 25, 2013, 6:01 p.m.
On Fri, Jan 25, 2013 at 05:47:46PM +0000, Blue Swirl wrote:
> On Thu, Jan 24, 2013 at 12:07 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Wed, Jan 23, 2013 at 08:49:58PM +0100, Andreas Färber wrote:
> >> Am 23.01.2013 18:58, schrieb Eduardo Habkost:
[...]
> >> > 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.

I see. Thanks for the explanation.


> >
> > [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.

I agree it is not perfect, but including test-x86-cpuid.c in the list is
the only way to get a report of the topology.h test coverage. Do you see
any reason to not do it?
Blue Swirl - Jan. 25, 2013, 6:27 p.m.
On Fri, Jan 25, 2013 at 6:01 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Fri, Jan 25, 2013 at 05:47:46PM +0000, Blue Swirl wrote:
>> On Thu, Jan 24, 2013 at 12:07 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> > On Wed, Jan 23, 2013 at 08:49:58PM +0100, Andreas Färber wrote:
>> >> Am 23.01.2013 18:58, schrieb Eduardo Habkost:
> [...]
>> >> > 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.
>
> I see. Thanks for the explanation.
>
>
>> >
>> > [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.
>
> I agree it is not perfect, but including test-x86-cpuid.c in the list is
> the only way to get a report of the topology.h test coverage. Do you see
> any reason to not do it?

The statistics for topology.h would be slightly biased by
test-x86-cpuid.c, meaning that even if test-x86-cpuid.c didn't test
anything but still gained some coverage, the some total coverage would
be reported. Also the opposite case could decrease 100% test coverage.

But I think this would only matter if we made a huge effort to reach
100% coverage for whole of QEMU and the opposite case somehow became
the only block keeping us from reaching that 100%. Until that day
comes (in the late 2050s perhaps, or it could be never if new code is
always added faster than coverage is gained) slightly biased
statistics are much better than no statistics at all.

>
> --
> Eduardo
Andreas Färber - Jan. 25, 2013, 6:55 p.m.
Am 23.01.2013 18:58, schrieb Eduardo Habkost:
> 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
>  
> @@ -70,12 +74,15 @@ 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 = tests/test-qapi-visit.o tests/test-qapi-types.o
>  
>  $(test-obj-y): QEMU_INCLUDES += -Itests
>  
> +tests/test-x86-cpuid.o: QEMU_INCLUDES += -Itarget-i386
> +
>  tests/check-qint$(EXESUF): tests/check-qint.o libqemuutil.a
>  tests/check-qstring$(EXESUF): tests/check-qstring.o libqemuutil.a
>  tests/check-qdict$(EXESUF): tests/check-qdict.o libqemuutil.a
> @@ -86,6 +93,7 @@ tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(block-obj-y) libqemuutil
>  tests/test-aio$(EXESUF): tests/test-aio.o $(block-obj-y) libqemuutil.a libqemustub.a
>  tests/test-thread-pool$(EXESUF): tests/test-thread-pool.o $(block-obj-y) libqemuutil.a libqemustub.a
>  tests/test-iov$(EXESUF): tests/test-iov.o libqemuutil.a
> +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
[snip]


checkpatch.pl complained about encoding of (R) in topology.h not being
UTF-8. I fixed that locally, but:

  CC    tests/test-x86-cpuid.o
cc1: error: target-i386: No such file or directory [-Werror]
cc1: all warnings being treated as errors
make: *** [tests/test-x86-cpuid.o] Error 1

Eduardo, can you supply a fixup diff for out-of-tree builds please?

Thanks,
Andreas
Eduardo Habkost - Jan. 25, 2013, 7:51 p.m.
On Fri, Jan 25, 2013 at 06:27:47PM +0000, Blue Swirl wrote:
[...]
> >> > [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.
> >
> > I agree it is not perfect, but including test-x86-cpuid.c in the list is
> > the only way to get a report of the topology.h test coverage. Do you see
> > any reason to not do it?
> 
> The statistics for topology.h would be slightly biased by
> test-x86-cpuid.c, meaning that even if test-x86-cpuid.c didn't test
> anything but still gained some coverage, the some total coverage would
> be reported. Also the opposite case could decrease 100% test coverage.

Good point. I believe it is more than "slightly biased", as all the
functions in topology.h are static inline: any code that wouldn't be
covered by the test generate any machine code at all.

I quickly tested this by adding extra functions to topology.h without
adding any test code, and coverage report was still 100%, as the
functions are inline and didn't generated any machine code in
test-x86-cpuid.o. That makes the topology.h coverage statistics useless
and misleading.

> 
> But I think this would only matter if we made a huge effort to reach
> 100% coverage for whole of QEMU and the opposite case somehow became
> the only block keeping us from reaching that 100%. Until that day
> comes (in the late 2050s perhaps, or it could be never if new code is
> always added faster than coverage is gained) slightly biased
> statistics are much better than no statistics at all.

In the topology.h case I would call it "completely useless" statistics,
as any percentage reported for that file would mean nothing. I prefer to
not have any coverage statistics from that file at all, than having
misleading statistics.

So I am OK with simply removing gcov-files-test-x86-cpuid-y from my
patch.

The only thing that gcov-files-test-x86-cpuid-y does is to make "make
check" print the (useless) percentages. Coverage reports can still be
generated manually by simply running "gcov tests/test-x86-cpuid.gcno" if
somebody wants to see the line-by-line coverage report.
Blue Swirl - Jan. 25, 2013, 8:01 p.m.
On Fri, Jan 25, 2013 at 7:51 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Fri, Jan 25, 2013 at 06:27:47PM +0000, Blue Swirl wrote:
> [...]
>> >> > [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.
>> >
>> > I agree it is not perfect, but including test-x86-cpuid.c in the list is
>> > the only way to get a report of the topology.h test coverage. Do you see
>> > any reason to not do it?
>>
>> The statistics for topology.h would be slightly biased by
>> test-x86-cpuid.c, meaning that even if test-x86-cpuid.c didn't test
>> anything but still gained some coverage, the some total coverage would
>> be reported. Also the opposite case could decrease 100% test coverage.
>
> Good point. I believe it is more than "slightly biased", as all the
> functions in topology.h are static inline: any code that wouldn't be
> covered by the test generate any machine code at all.
>
> I quickly tested this by adding extra functions to topology.h without
> adding any test code, and coverage report was still 100%, as the
> functions are inline and didn't generated any machine code in
> test-x86-cpuid.o. That makes the topology.h coverage statistics useless
> and misleading.

This supports my view that gcov is not useful for all cases.

Maybe this could work:
Change topology.h to not inline always:
#ifdef QTEST
#define TOPOLOGY_H_INLINE
#else
#define TOPOLOGY_H_INLINE inline
#endif
and use TOPOLOGY_H_INLINE instead of 'inline'.

Then introduce topology.c:
#define QTEST
#include "topology.h"

>>
>> But I think this would only matter if we made a huge effort to reach
>> 100% coverage for whole of QEMU and the opposite case somehow became
>> the only block keeping us from reaching that 100%. Until that day
>> comes (in the late 2050s perhaps, or it could be never if new code is
>> always added faster than coverage is gained) slightly biased
>> statistics are much better than no statistics at all.
>
> In the topology.h case I would call it "completely useless" statistics,
> as any percentage reported for that file would mean nothing. I prefer to
> not have any coverage statistics from that file at all, than having
> misleading statistics.
>
> So I am OK with simply removing gcov-files-test-x86-cpuid-y from my
> patch.

OK.

> The only thing that gcov-files-test-x86-cpuid-y does is to make "make
> check" print the (useless) percentages. Coverage reports can still be
> generated manually by simply running "gcov tests/test-x86-cpuid.gcno" if
> somebody wants to see the line-by-line coverage report.
>
> --
> Eduardo

Patch

diff --git a/target-i386/topology.h b/target-i386/topology.h
new file mode 100644
index 0000000..24ed525
--- /dev/null
+++ b/target-i386/topology.h
@@ -0,0 +1,136 @@ 
+/*
+ *  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 TARGET_I386_TOPOLOGY_H
+#define TARGET_I386_TOPOLOGY_H
+
+/* This file implements the APIC-ID-based CPU topology enumeration logic,
+ * documented at the following document:
+ *   Intel® 64 Architecture Processor Topology Enumeration
+ *   http://software.intel.com/en-us/articles/intel-64-architecture-processor-topology-enumeration/
+ *
+ * This code should be 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().
+ */
+
+#include <stdint.h>
+#include <string.h>
+
+#include "qemu/bitops.h"
+
+/* APIC IDs can be 32-bit, but beware: APIC IDs > 255 require x2APIC support
+ */
+typedef uint32_t apic_id_t;
+
+/* Return the bit width needed for 'count' IDs
+ */
+static unsigned apicid_bitwidth_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 apicid_bitwidth_for_count(nr_threads);
+}
+
+/* Bit width of the Core_ID field
+ */
+static inline unsigned apicid_core_width(unsigned nr_cores, unsigned nr_threads)
+{
+    return apicid_bitwidth_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 apic_id_t apicid_from_topo_ids(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 (contiguous) CPU index
+ */
+static inline void x86_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;
+}
+
+/* Make APIC ID for the CPU 'cpu_index'
+ *
+ * 'cpu_index' is a sequential, contiguous ID for the CPU.
+ */
+static inline apic_id_t x86_apicid_from_cpu_idx(unsigned nr_cores,
+                                                unsigned nr_threads,
+                                                unsigned cpu_index)
+{
+    unsigned pkg_id, core_id, smt_id;
+    x86_topo_ids_from_idx(nr_cores, nr_threads, cpu_index,
+                          &pkg_id, &core_id, &smt_id);
+    return apicid_from_topo_ids(nr_cores, nr_threads, pkg_id, core_id, smt_id);
+}
+
+#endif /* TARGET_I386_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 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
 
@@ -70,12 +74,15 @@  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 = tests/test-qapi-visit.o tests/test-qapi-types.o
 
 $(test-obj-y): QEMU_INCLUDES += -Itests
 
+tests/test-x86-cpuid.o: QEMU_INCLUDES += -Itarget-i386
+
 tests/check-qint$(EXESUF): tests/check-qint.o libqemuutil.a
 tests/check-qstring$(EXESUF): tests/check-qstring.o libqemuutil.a
 tests/check-qdict$(EXESUF): tests/check-qdict.o libqemuutil.a
@@ -86,6 +93,7 @@  tests/test-coroutine$(EXESUF): tests/test-coroutine.o $(block-obj-y) libqemuutil
 tests/test-aio$(EXESUF): tests/test-aio.o $(block-obj-y) libqemuutil.a libqemustub.a
 tests/test-thread-pool$(EXESUF): tests/test-thread-pool.o $(block-obj-y) libqemuutil.a libqemustub.a
 tests/test-iov$(EXESUF): tests/test-iov.o libqemuutil.a
+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..8d9f96a
--- /dev/null
+++ b/tests/test-x86-cpuid.c
@@ -0,0 +1,110 @@ 
+/*
+ *  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>
+
+#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(x86_apicid_from_cpu_idx(1, 1, 0), ==, 0);
+    g_assert_cmpuint(x86_apicid_from_cpu_idx(1, 1, 1), ==, 1);
+    g_assert_cmpuint(x86_apicid_from_cpu_idx(1, 1, 2), ==, 2);
+    g_assert_cmpuint(x86_apicid_from_cpu_idx(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(x86_apicid_from_cpu_idx(6, 3, 0), ==, 0);
+    g_assert_cmpuint(x86_apicid_from_cpu_idx(6, 3, 1), ==, 1);
+    g_assert_cmpuint(x86_apicid_from_cpu_idx(6, 3, 2), ==, 2);
+
+    g_assert_cmpuint(x86_apicid_from_cpu_idx(6, 3, 1 * 3 + 0), ==,
+                     (1 << 2) | 0);
+    g_assert_cmpuint(x86_apicid_from_cpu_idx(6, 3, 1 * 3 + 1), ==,
+                     (1 << 2) | 1);
+    g_assert_cmpuint(x86_apicid_from_cpu_idx(6, 3, 1 * 3 + 2), ==,
+                     (1 << 2) | 2);
+
+    g_assert_cmpuint(x86_apicid_from_cpu_idx(6, 3, 2 * 3 + 0), ==,
+                     (2 << 2) | 0);
+    g_assert_cmpuint(x86_apicid_from_cpu_idx(6, 3, 2 * 3 + 1), ==,
+                     (2 << 2) | 1);
+    g_assert_cmpuint(x86_apicid_from_cpu_idx(6, 3, 2 * 3 + 2), ==,
+                     (2 << 2) | 2);
+
+    g_assert_cmpuint(x86_apicid_from_cpu_idx(6, 3, 5 * 3 + 0), ==,
+                     (5 << 2) | 0);
+    g_assert_cmpuint(x86_apicid_from_cpu_idx(6, 3, 5 * 3 + 1), ==,
+                     (5 << 2) | 1);
+    g_assert_cmpuint(x86_apicid_from_cpu_idx(6, 3, 5 * 3 + 2), ==,
+                     (5 << 2) | 2);
+
+    g_assert_cmpuint(x86_apicid_from_cpu_idx(6, 3, 1 * 6 * 3 + 0 * 3 + 0), ==,
+                     (1 << 5));
+    g_assert_cmpuint(x86_apicid_from_cpu_idx(6, 3, 1 * 6 * 3 + 1 * 3 + 1), ==,
+                     (1 << 5) | (1 << 2) | 1);
+    g_assert_cmpuint(x86_apicid_from_cpu_idx(6, 3, 3 * 6 * 3 + 5 * 3 + 2), ==,
+                     (3 << 5) | (5 << 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;
+}