Received-SPF: pass (google.com: domain of linux-kernel+bounces-143572-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <e0fe15f6-993c-45ec-aea2-531a055fb0cd@linux.intel.com>
Date: Sat, 13 Apr 2024 12:12:59 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH 37/41] KVM: x86/pmu: Allow writing to fixed counter
 selector if counter is exposed
To: Sean Christopherson <seanjc@google.com>,
 Xiong Zhang <xiong.y.zhang@linux.intel.com>
Cc: pbonzini@redhat.com, peterz@infradead.org, mizhang@google.com,
 kan.liang@intel.com, zhenyuw@linux.intel.com, jmattson@google.com,
 kvm@vger.kernel.org, linux-perf-users@vger.kernel.org,
 linux-kernel@vger.kernel.org, zhiyuan.lv@intel.com, eranian@google.com,
 irogers@google.com, samantha.alt@intel.com, like.xu.linux@gmail.com,
 chao.gao@intel.com
References: <20240126085444.324918-1-xiong.y.zhang@linux.intel.com>
 <20240126085444.324918-38-xiong.y.zhang@linux.intel.com>
 <ZhheJUWRhCmmYa_F@google.com>
Content-Language: en-US
From: "Mi, Dapeng" <dapeng1.mi@linux.intel.com>
In-Reply-To: <ZhheJUWRhCmmYa_F@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 4/12/2024 6:03 AM, Sean Christopherson wrote:
> On Fri, Jan 26, 2024, Xiong Zhang wrote:
>> From: Mingwei Zhang <mizhang@google.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@google.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)