Received-SPF: pass (google.com: domain of linux-kernel+bounces-94984-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <23a1af5f-e08d-4cf6-b4bd-8cfb6266f441@amd.com>
Date: Thu, 7 Mar 2024 11:09:16 +0530
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] KVM: x86: Do not mask LVTPC when handling a PMI on AMD
 platforms
To: Sean Christopherson <seanjc@google.com>, Jim Mattson <jmattson@google.com>
Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, pbonzini@redhat.com,
 tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
 dave.hansen@linux.intel.com, x86@kernel.org, hpa@zytor.com,
 mlevitsk@redhat.com, vkuznets@redhat.com, mizhang@google.com,
 tao1.su@linux.intel.com, andriy.shevchenko@linux.intel.com,
 ravi.bangoria@amd.com, ananth.narayan@amd.com, nikunj.dadhania@amd.com,
 santosh.shukla@amd.com, manali.shukla@amd.com
References: <20240301074423.643779-1-sandipan.das@amd.com>
 <CALMp9eRbOQpVsKkZ9N1VYTyOrKPWCmvi0B5WZCFXAPsSkShmEA@mail.gmail.com>
 <ZeYz7zPGcIQSH_NI@google.com>
Content-Language: en-US
From: Sandipan Das <sandipan.das@amd.com>
In-Reply-To: <ZeYz7zPGcIQSH_NI@google.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?VFM5SE0ybUxxekxPdi9GMGZ0aGY0elRIbllZeVNmRzcwSDFyOE1yVnBnV3pU?=
 =?utf-8?B?bmd5Q29RL2VIWlk3VjJ4cERhVjluaW9TcGxDUHlybEZITXp3dkVPVWUwSmZp?=
 =?utf-8?B?VW1lSGtjVktiRzVITkliTHhZVGtHUmFmYVFPS2JESkUvVGEwYXc1V0cvbVhP?=
 =?utf-8?B?eitzWVFRMGtnMHBtamFpT2pnSGVJR2hKK3pnaFJrejZCSll3ek8rbWV2Qkln?=
 =?utf-8?B?amhoZE4rV2Y5MGQyUVgxdzBqaVNvdFRWYlh6QVZjUEpPSW5tQlh3dnpnVzIv?=
 =?utf-8?B?U05vYm9PT1BRSmM5cWZXbEZCT2lFRVh3SVg3dit5eUp6a3JONE9hdmZybW1U?=
 =?utf-8?B?MDNSU2MzTnVPbTN6S250dFE0TEhENmZKUER6WFR6RW5GZ2NmVjV3bWVlWmtq?=
 =?utf-8?B?S1Z4U3MwU2QrOGdjUjdyalI3RTg4TnZneklKSHZVREM4NUdyeVJSbDB3RXE1?=
 =?utf-8?B?cWVDSi9ZRWlLdGhjRHpHZFpLRlNTVVo3TWFRT2xrMWovS1JNYlljK280L2x1?=
 =?utf-8?B?ZGd6L0R0SjNIb05hd2xVVzJBRnJzT1BMNkpocVVyYTB0OEt6UmsxNU95RGha?=
 =?utf-8?B?bGE0T0FtSHJ6SVNhQXlZbXpId2FiZmszTkJLTXEraWZ3ek1WbXF0Sk4rN2JT?=
 =?utf-8?B?eVpzOHBRd0tzVUlOK0RUWlc3UWtFRjBZTUcvOGx4cHR4UVE3OEZnV2xGS2Jw?=
 =?utf-8?B?eUhKWk1Gb3pMVm5rbVZESzVjUmpqQjlseUY5WUw3R1FHTUI3dEJlWHlGdWcy?=
 =?utf-8?B?dHh4QXhQclpjV2IvV1pSRnhOYmx4MjdlMG5PU2EvZ283c29INmFEZW1SWGRG?=
 =?utf-8?B?VXJ2ay83TkNmOGphOEVNaTRjZ0pVOVNIV09rL2hiTkc5YkNBZmRJOGtudmlX?=
 =?utf-8?B?czRoZSt0Tm9HUVVtbWVocjdOOXN4N0htVHVwQlFhdzhJa1hrY2pHM254MUNI?=
 =?utf-8?B?OGpya0dGaFJSZ1RlT01rSjI3ZHBKeW1IVGdZZlEzR2ZIWC9EUEpER05oRWdH?=
 =?utf-8?B?cVQreDFZbW5kak56b280dDBmRFdwNFVpWEF1aG1mejNublliMW5yOE1DcmV0?=
 =?utf-8?B?RmlBak9qT3Qxd0gwdUJmQXI2bENSR2RrcmxVOVhFaTZmdzd6OFhCd0RuL0lq?=
 =?utf-8?B?ZFJDWURaOFlZRFJGYTZCMzNzNFZVeFZaeHJTK2o2WkNOWm5nWWxrYWpjbW5P?=
 =?utf-8?B?aHVaMVdmVTdSVlEwWWFKVjh1K1EzZXJZTTFEWldvbFhJcEpZVWJHbTJ3RXJu?=
 =?utf-8?B?Wjh0cWVDK3FzRkdadS9HR1VObGRURitsb1pqajVSNXVCMjFkV1E4VXh2N1FU?=
 =?utf-8?B?c3FxMzdxdndSL1h2aDUrUDlMdmw4clpzZlUrdGQ4WE5uSXJiWGVGL1oxRWFQ?=
 =?utf-8?B?V25LMW9vZU1WV0tldDFJczNMWnk3bHJINFdWTHdmd3NrYVd1WlBBQmtock9l?=
 =?utf-8?B?bWMyK2tJcjN5SkYwNjFhNzlaY0pKb1VqYmdObG8ydVBDSXBWVzRpUjlvaDg5?=
 =?utf-8?B?NGNGbGRoTFkrR09iUy9nMHJjekVWeXBPdTFDRWF4T21HM29aOUJuSGJBbk5R?=
 =?utf-8?B?UGphMjlVZHJJdEJHRndGdXNyZi9ZN2xRREZHSHhiTTBQWk15VzFWTEZCdGky?=
 =?utf-8?B?aHhFd0RDdC9LUUkrYlVxNnl1dzl2UFkrZnRDRnErM1didHJYUXE4RXZHYzZ1?=
 =?utf-8?B?L1I2YmFGdU9IS3lHRlo0SnNudDQrMitING1VZTZldFFSclg4Wm9hdE5Mb3RP?=
 =?utf-8?B?Tms0ZDlFNEFrZnNDT1RBRkZSbTlISlZaZExNaGxlckJ6WDZnbExqdmZNY1RL?=
 =?utf-8?B?VzlmU1hic283aUwwY2xyN0k3OTBJOWlJeXh4anVZUlNrODd5WkQzektsUWZv?=
 =?utf-8?B?bnRTZzFrZ2ZCZW5zdmtSSDgxbURFNmxDZDI1cUR3YkFBTGV3UDYrU0JVK2Nl?=
 =?utf-8?B?cURhUGpSdlhmblY3amRZYmFUM3FLTWlOVE4wZTRtblZJMmFzSGJvd2REaUJy?=
 =?utf-8?B?bFk4TS9VNW85dElsZnZHTWZSR0lFNHBBT3JEeFNHNG9ZZnRpT0M4bHBKandS?=
 =?utf-8?B?aEQ4aGo1Y29YNThmQks0aHc4TVhCT285NzMvY3ozM284RmJMRjFrU0R6VDFo?=
 =?utf-8?Q?7aZKEmUtVH9iqFKNs6+QpMOp0?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e76fac34-0eb5-4b0a-9ad8-08dc3e68f716
X-MS-Exchange-CrossTenant-AuthSource: PH7PR12MB5712.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Mar 2024 05:39:32.4109
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 06C4tXg0DKE7nnpVys7qWa4egjozlz1KAvOVzFvum1jucU5ZbDdlg/SwZjE28IGs5+dv/kv3Okn8ESSPITLCjA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR12MB8876

On 3/5/2024 2:19 AM, Sean Christopherson wrote:
> On Fri, Mar 01, 2024, Jim Mattson wrote:
>> On Thu, Feb 29, 2024 at 11:44 PM Sandipan Das <sandipan.das@amd.com> wrote:
>>>
>>> On AMD and Hygon platforms, the local APIC does not automatically set
>>> the mask bit of the LVTPC register when handling a PMI and there is
>>> no need to clear it in the kernel's PMI handler.
>>
>> I don't know why it didn't occur to me that different x86 vendors
>> wouldn't agree on this specification. :)
> 
> Because you're sane?  :-D
> 
>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>>> index 3242f3da2457..0959a887c306 100644
>>> --- a/arch/x86/kvm/lapic.c
>>> +++ b/arch/x86/kvm/lapic.c
>>> @@ -2768,7 +2768,7 @@ int kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type)
>>>                 trig_mode = reg & APIC_LVT_LEVEL_TRIGGER;
>>>
>>>                 r = __apic_accept_irq(apic, mode, vector, 1, trig_mode, NULL);
>>> -               if (r && lvt_type == APIC_LVTPC)
>>> +               if (r && lvt_type == APIC_LVTPC && !guest_cpuid_is_amd_or_hygon(apic->vcpu))
>>
>> Perhaps we could use a positive predicate instead:
>> guest_cpuid_is_intel(apic->vcpu)?
> 
> AFAICT, Zhaoxin follows intel behavior, so we'd theoretically have to allow for
> that too.  The many checks of guest_cpuid_is_intel() in KVM suggest that no one
> actually cares about about correctly virtualizing Zhaoxin CPUs, but it's an easy
> enough problem to solve.
> 
> I'm also very tempted to say KVM should cache the Intel vs. AMD vCPU model.  E.g.
> if userspace does something weird with guest CPUID and puts CPUID.0x0 somewhere
> other than the zeroth entry, KVM's linear walk to find a CPUID entry will make
> this a pretty slow lookup.
> 
> Then we could also encapsulate the gory details for Intel vs. Zhaoxin vs. Centaur,
> and AMD vs. Hygon, e.g.
> 
> 		if (r && lvt_type == APIC_LVTPC &&
> 		    apic->vcpu->arch.is_model_intel_compatible)

The following are excerpts from some changes that I have been working on. Instead
of a boolean flag, this saves the compatible vendor in kvm_vcpu_arch and tries
to match it against X86_VENDOR_* values. The goal is to replace
guest_cpuid_is_{intel,amd_or_hygon}() with
guest_vendor_is_compatible(vcpu, X86_VENDOR_{INTEL,AMD}). Is this viable?

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index d271ba20a0b2..c4ada5b12fc3 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1042,6 +1042,7 @@ struct kvm_vcpu_arch {
 #if IS_ENABLED(CONFIG_HYPERV)
        hpa_t hv_root_tdp;
 #endif
+       u8 compat_vendor;
 };

 struct kvm_lpage_info {
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index adba49afb5fe..00170762e72a 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -376,6 +376,13 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
        kvm_hv_set_cpuid(vcpu, kvm_cpuid_has_hyperv(vcpu->arch.cpuid_entries,
                                                    vcpu->arch.cpuid_nent));

+       if (guest_cpuid_is_intel_compatible(vcpu))
+               vcpu->arch.compat_vendor = X86_VENDOR_INTEL;
+       else if (guest_cpuid_is_amd_or_hygon(vcpu))
+               vcpu->arch.compat_vendor = X86_VENDOR_AMD;
+       else
+               vcpu->arch.compat_vendor = X86_VENDOR_UNKNOWN;
+
        /* Invoke the vendor callback only after the above state is updated. */
        static_call(kvm_x86_vcpu_after_set_cpuid)(vcpu);

diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index 856e3037e74f..8c73db586231 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -120,6 +120,17 @@ static inline bool guest_cpuid_is_intel(struct kvm_vcpu *vcpu)
        return best && is_guest_vendor_intel(best->ebx, best->ecx, best->edx);
 }

+static inline bool guest_cpuid_is_intel_compatible(struct kvm_vcpu *vcpu)
+{
+       struct kvm_cpuid_entry2 *best;
+
+       best = kvm_find_cpuid_entry(vcpu, 0);
+       return best &&
+              (is_guest_vendor_intel(best->ebx, best->ecx, best->edx) ||
+               is_guest_vendor_centaur(best->ebx, best->ecx, best->edx) ||
+               is_guest_vendor_zhaoxin(best->ebx, best->ecx, best->edx));
+}
+
 static inline int guest_cpuid_family(struct kvm_vcpu *vcpu)
 {
        struct kvm_cpuid_entry2 *best;
@@ -142,6 +153,11 @@ static inline int guest_cpuid_model(struct kvm_vcpu *vcpu)
        return x86_model(best->eax);
 }

+static inline bool guest_vendor_is_compatible(struct kvm_vcpu *vcpu, u8 vendor)
+{
+       return vcpu->arch.compat_vendor == vendor;
+}
+
 static inline bool cpuid_model_is_consistent(struct kvm_vcpu *vcpu)
 {
        return boot_cpu_data.x86_model == guest_cpuid_model(vcpu);