linux-kernel - Re: [PATCH v2 1/8] KVM: selftests: KVM: selftests: Add macros for fixed counters in processor.h

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZJyOCpueM0viGDfX@google.com>
Date:   Wed, 28 Jun 2023 12:46:18 -0700
From:   Sean Christopherson <seanjc@...gle.com>
To:     Jinrong Liang <ljr.kernel@...il.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>, Like Xu <likexu@...cent.com>,
        David Matlack <dmatlack@...gle.com>,
        Aaron Lewis <aaronlewis@...gle.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jinrong Liang <cloudliang@...cent.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/8] KVM: selftests: KVM: selftests: Add macros for
 fixed counters in processor.h

Heh, duplicate "KVM: selftests:" in the shortlog.

On Tue, May 30, 2023, Jinrong Liang wrote:
> From: Jinrong Liang <cloudliang@...cent.com>
> 
> Add macro in processor.h, providing a efficient way to obtain

Try not to describe what the patch literally does in terms of code, the purpose
of the shortlog+changelog is to complement the diff, e.g. it's super obvious from
the diff that this patch adds macros in processor.h.

> the number of fixed counters and fixed counters bit mask. The

Wrap closer to 75 chars, 60 is too aggressive.

> addition of these macro will simplify the handling of fixed
> performance counters, while keeping the code maintainable and
> clean.

Instead of making assertions, justify the patch by stating the effects on code.
Statements like "will simplify the handling" and "keeping the code maintainable
and clean" are subjective.  In cases like these, it's extremely unlikely anyone
will disagree, but getting into the habit of providing concrete justification
even for simple cases makes it easier to write changelogs for more complex changes.

E.g.

  Add x86 properties for the number of PMU fixed counters and the bitmask
  that allows for "discontiguous" fixed counters so that tests don't have
  to manually retrieve the correct CPUID leaf+register, and so that the
  resulting code is self-documenting.

> Signed-off-by: Jinrong Liang <cloudliang@...cent.com>
> ---
>  tools/testing/selftests/kvm/include/x86_64/processor.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
> index aa434c8f19c5..94751bddf1d9 100644
> --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
> +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
> @@ -240,6 +240,8 @@ struct kvm_x86_cpu_property {
>  #define X86_PROPERTY_PMU_VERSION		KVM_X86_CPU_PROPERTY(0xa, 0, EAX, 0, 7)
>  #define X86_PROPERTY_PMU_NR_GP_COUNTERS		KVM_X86_CPU_PROPERTY(0xa, 0, EAX, 8, 15)
>  #define X86_PROPERTY_PMU_EBX_BIT_VECTOR_LENGTH	KVM_X86_CPU_PROPERTY(0xa, 0, EAX, 24, 31)
> +#define X86_PROPERTY_PMU_FIXED_CTRS_BITMASK	KVM_X86_CPU_PROPERTY(0xa, 0, ECX, 0, 31)

Please spell out COUNTERS so that all the properties are consistent.

> +#define X86_PROPERTY_PMU_NR_FIXED_COUNTERS	KVM_X86_CPU_PROPERTY(0xa, 0, EDX, 0, 4)
>  
>  #define X86_PROPERTY_SUPPORTED_XCR0_LO		KVM_X86_CPU_PROPERTY(0xd,  0, EAX,  0, 31)
>  #define X86_PROPERTY_XSTATE_MAX_SIZE_XCR0	KVM_X86_CPU_PROPERTY(0xd,  0, EBX,  0, 31)
> -- 
> 2.31.1
>