Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: MGul3XlqCh8eF0ltbz9vf5HWc0rcYwRQSlqg0RhPdzZnjLEFhB54FW0dpYiAm86IYt/HO5HEld
 BWoGxOCpPKtg==
IronPort-SDR: 0Y5NNDMqa45GgezOLyZGo9r8aF1T93aO2XijT0PR13FtODXOlI0A+3m/eiyJiE90gfi0q2nXh+
 VjuEKKEMRVeA==
Subject: Re: [PATCH v2 1/4] KVM: vmx/pmu: Add MSR_ARCH_LBR_DEPTH emulation for
 Arch LBR
To:     Sean Christopherson <seanjc@google.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20210203135714.318356-1-like.xu@linux.intel.com>
 <20210203135714.318356-2-like.xu@linux.intel.com>
 <YD1r2G1UQjVXkUk5@google.com>
From:   Like Xu <like.xu@linux.intel.com>
Organization: Intel OTC
Message-ID: <d194f89a-cd56-6f32-4797-ae11956bf5ba@linux.intel.com>
Date:   Tue, 2 Mar 2021 10:52:51 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.0
MIME-Version: 1.0
In-Reply-To: <YD1r2G1UQjVXkUk5@google.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2021/3/2 6:34, Sean Christopherson wrote:
> On Wed, Feb 03, 2021, Like Xu wrote:
>> @@ -348,10 +352,26 @@ static bool intel_pmu_handle_lbr_msrs_access(struct kvm_vcpu *vcpu,
>>   	return true;
>>   }
>>   
>> +/*
>> + * Check if the requested depth values is supported
>> + * based on the bits [0:7] of the guest cpuid.1c.eax.
>> + */
>> +static bool arch_lbr_depth_is_valid(struct kvm_vcpu *vcpu, u64 depth)
>> +{
>> +	struct kvm_cpuid_entry2 *best;
>> +
>> +	best = kvm_find_cpuid_entry(vcpu, 0x1c, 0);
>> +	if (depth && best)
> 
>> +		return (best->eax & 0xff) & (1ULL << (depth / 8 - 1));
> 
> I believe this will genereate undefined behavior if depth > 64.  Or if depth < 8.
> And I believe this check also needs to enforce that depth is a multiple of 8.
> 
>     For each bit n set in this field, the IA32_LBR_DEPTH.DEPTH value 8*(n+1) is
>     supported.
> 
> Thus it's impossible for 0-7, 9-15, etc... to be legal depths.

Thank you! How about:

	best = kvm_find_cpuid_entry(vcpu, 0x1c, 0);
	if (best && depth && !(depth % 8))
		return (best->eax & 0xff) & (1ULL << (depth / 8 - 1));

	return false;

> 
> 
>> +
>> +	return false;
>> +}
>> +
>