linux-kernel - Re: [RFC PATCH 37/41] KVM: x86/pmu: Allow writing to fixed counter selector if counter is exposed

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]

Date: Sat, 13 Apr 2024 12:12:59 +0800
From: "Mi, Dapeng" <dapeng1.mi@...ux.intel.com>
To: Sean Christopherson <seanjc@...gle.com>,
 Xiong Zhang <xiong.y.zhang@...ux.intel.com>
Cc: pbonzini@...hat.com, peterz@...radead.org, mizhang@...gle.com,
 kan.liang@...el.com, zhenyuw@...ux.intel.com, jmattson@...gle.com,
 kvm@...r.kernel.org, linux-perf-users@...r.kernel.org,
 linux-kernel@...r.kernel.org, zhiyuan.lv@...el.com, eranian@...gle.com,
 irogers@...gle.com, samantha.alt@...el.com, like.xu.linux@...il.com,
 chao.gao@...el.com
Subject: Re: [RFC PATCH 37/41] KVM: x86/pmu: Allow writing to fixed counter
 selector if counter is exposed


On 4/12/2024 6:03 AM, Sean Christopherson wrote:
> On Fri, Jan 26, 2024, Xiong Zhang wrote:
>> From: Mingwei Zhang <mizhang@...gle.com>
>>
>> Allow writing to fixed counter selector if counter is exposed. If this
>> fixed counter is filtered out, this counter won't be enabled on HW.
>>
>> Passthrough PMU implements the context switch at VM Enter/Exit boundary the
>> guest value cannot be directly written to HW since the HW PMU is owned by
>> the host. Introduce a new field fixed_ctr_ctrl_hw in kvm_pmu to cache the
>> guest value.  which will be assigne to HW at PMU context restore.
>>
>> Since passthrough PMU intercept writes to fixed counter selector, there is
>> no need to read the value at pmu context save, but still clear the fix
>> counter ctrl MSR and counters when switching out to host PMU.
>>
>> Signed-off-by: Mingwei Zhang <mizhang@...gle.com>
>> ---
>>   arch/x86/include/asm/kvm_host.h |  1 +
>>   arch/x86/kvm/vmx/pmu_intel.c    | 28 ++++++++++++++++++++++++----
>>   2 files changed, 25 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index fd1c69371dbf..b02688ed74f7 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -527,6 +527,7 @@ struct kvm_pmu {
>>   	unsigned nr_arch_fixed_counters;
>>   	unsigned available_event_types;
>>   	u64 fixed_ctr_ctrl;
>> +	u64 fixed_ctr_ctrl_hw;
>>   	u64 fixed_ctr_ctrl_mask;
> Before introduce more fields, can someone please send a patch/series to rename
> the _mask fields?  AFAIK, they all should be e.g. fixed_ctr_ctrl_rsvd, or something
> to that effect.

Yeah, I remember I ever said to cook a patch to rename all these _mask 
fields. I would do it now.


>
> Because I think we should avoid reinventing the naming wheel, and use "shadow"
> instead of "hw", because KVM developers already know what "shadow" means.  But
> "mask" also has very specific meaning for shadowed fields.  That, and "mask" is
> a freaking awful name in the first place.
>
>>   	u64 global_ctrl;
>>   	u64 global_status;
>> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
>> index 713c2a7c7f07..93cfb86c1292 100644
>> --- a/arch/x86/kvm/vmx/pmu_intel.c
>> +++ b/arch/x86/kvm/vmx/pmu_intel.c
>> @@ -68,6 +68,25 @@ static int fixed_pmc_events[] = {
>>   	[2] = PSEUDO_ARCH_REFERENCE_CYCLES,
>>   };
>>   
>> +static void reprogram_fixed_counters_in_passthrough_pmu(struct kvm_pmu *pmu, u64 data)
> We need to come up with shorter names, this ain't Java.  :-)  Heh, that can be
> another argument for "mediated", it saves three characters.
>
> And somewhat related, kernel style is <scope>_<blah>, i.e.
>
> static void mediated_pmu_reprogram_fixed_counters(struct kvm_pmu *pmu, u64 data)