Message-ID: <d3681058-224d-07c7-283f-5f81ab523844@amd.com>
Date:   Wed, 8 Feb 2023 15:21:59 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH v2 07/11] KVM: x86: add a delayed hardware NMI injection
 interface
Content-Language: en-US
To:     Sean Christopherson <seanjc@google.com>,
        Maxim Levitsky <mlevitsk@redhat.com>
Cc:     kvm@vger.kernel.org, Sandipan Das <sandipan.das@amd.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Jim Mattson <jmattson@google.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Borislav Petkov <bp@alien8.de>,
        Pawan Gupta <pawan.kumar.gupta@linux.intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>,
        Josh Poimboeuf <jpoimboe@kernel.org>,
        Daniel Sneddon <daniel.sneddon@linux.intel.com>,
        Jiaxi Chen <jiaxi.chen@linux.intel.com>,
        Babu Moger <babu.moger@amd.com>, linux-kernel@vger.kernel.org,
        Jing Liu <jing2.liu@intel.com>,
        Wyes Karny <wyes.karny@amd.com>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>
References: <20221129193717.513824-1-mlevitsk@redhat.com>
 <20221129193717.513824-8-mlevitsk@redhat.com> <Y9mWFlGdzoa8ZDW7@google.com>
 <Y9mtGV+q0P2U9+M1@google.com>
From:   Santosh Shukla <santosh.shukla@amd.com>
In-Reply-To: <Y9mtGV+q0P2U9+M1@google.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bld6RzJBbjVEQWlLNnBYWGxxTkJCak5LSUVTcmw3VkV2OFhWeDkybEVWZDh6?=
 =?utf-8?B?NHZrNDY3a0paTHptZFJ1TUQrVml0QjNjVjcyQmlIVUw5L0VQRys0ZXBaNFBL?=
 =?utf-8?B?dkdWTEpjaXVrQUtMbkxUY3BuRGdTSkJ6bk9jMzcxSW5lRGxnR3RWT0Q0NlZx?=
 =?utf-8?B?cjROY21sN3hzUVg5TTV0RGtVRHp4WnNUNVlmTktaRGpPUFdEeXFrczVkQ1dE?=
 =?utf-8?B?TURmZTVxTnVDTEN2b0pwTTN3d0JDeTFtbGFMUGI2KzUwMzFCT1JlOFhzQzJk?=
 =?utf-8?B?TjJFZ01xOHFHYVF2VE1wWDljUzlMR0pYSGlJNG0zT3lBdUZyQVBZck5DUmJE?=
 =?utf-8?B?QnI1SmJ5R3JxTWdoa0J0TUNCSEIyWEl3UFB1dXBjcTR0Z1EyL2FyWVg5ckU3?=
 =?utf-8?B?dTgrM2NXOVM1YW42bkZJdVp2dThTV2IyQlNvTnRDaVAwMFNSNE5oMFpHdDgy?=
 =?utf-8?B?SkxuMkgrR3p3L2ZySlpOc0J3d2FmNVFCWFo1VGRIUUxiV2xuZ21mN1pmSTB6?=
 =?utf-8?B?b0ZsSmlndkFGTEJBRHpzc05YclB1Z1BrYTNUREVIb3RNbDkrcWoxT1p4ZmVX?=
 =?utf-8?B?Zmx5SE5oTXNlSnZVMThVelFUUUlBV1BCOUhLTTlUNS9PMjMzWm1hVmhsRG9P?=
 =?utf-8?B?aFJQWFJkR3l6NGM5cFdITzVCUG1CVDJNZGRNbGloTFBkaVh2c0lXbnpLVGZC?=
 =?utf-8?B?RDUyKzFhZjdWanVUVFBTbkJ5bkVMUDFUbDJlKy85OE1HWWRLRlRaeFNITVU0?=
 =?utf-8?B?K1BPM0JVTkwyYlIybGVWSFdoYXp0MVZkVktLU0k5cnhaV1dQQktxVm9kQW5P?=
 =?utf-8?B?aVAzN0IrSFN2ajl5L1NWT3hkUXI0ME0vR0F1UW8xYUdoZ08rL0dWS3pDbSsy?=
 =?utf-8?B?SjhHY0ZIS1lXaTMrSEVoZk9xL2pIdk5TeUFuTGhxVVN6ZytWOXhMUzh6aWR1?=
 =?utf-8?B?VXVZNFNWcUNpYUk1MlRtTFNhbFIzTEsyc3hPS3BpN3UySzlUT2QrZlhyWndX?=
 =?utf-8?B?VjBaaW9RNFM2WFZJV3E2bnFDcEowYUFpclF1RmdsZ0R3aVZGKzZ1VTVKNVQ3?=
 =?utf-8?B?ZEhuRCtnZ05OUzlLRVB5SFc2Z3paZkdnamJ6ZmN3SysrS0Zmd1M3Ni9OSGpt?=
 =?utf-8?B?UW5RK3UvOWZFM3lkdGk0eFR3VzU2azZJL3JmUGxsVVNqSUtYbzBaRCtlOFE2?=
 =?utf-8?B?MEpDUWtVSzlTeTgvd3lpMjFHOU44SEYrUkFpektkSmxyektkenFsdVNtdXFl?=
 =?utf-8?B?bEJNTkJRR3pCaUxUSDU4Q0h0UCtYYzkwQldpTktIY3B3eDZpUTU5RXZPaXcy?=
 =?utf-8?B?WkRjWWo1RnlBOHVMMlZwMVh2NXV3VnZHOEFHQkZOMjlteGQxYm1mMndRRUI5?=
 =?utf-8?B?TTlRWS9OWFlFVk5sMUh2MTB1QzdhVWhZbklpWTlmWE5vWEtwbnAzUk5wdVB6?=
 =?utf-8?B?aHhuMWJRT2cvLzZMancxNXZTeGRQS3pzc3ZsV1BqZERCMlhhZkMyazd2L0wx?=
 =?utf-8?B?eWsySkM2SWJZU1pid2ZmWmJQQmViRmtlQXZFaVlObXYxOW1IdjhZVEVvMU8w?=
 =?utf-8?B?THVkaUwyVENNRHBHY01qMXc2b2tDU0xkK2NXRWswYVBMRVIvZDVjYjM2ZXBI?=
 =?utf-8?B?L0xuVU9vS2VxMUZVTC9VaUlBd2VIVUh4SFRiZWFmV1crRklzbFp2R21hWW5m?=
 =?utf-8?B?YnhVQjh6ZVk2UFk5bDNDSE9BRFc3S2NVdkdQaWZRR0dVSjdHTlVmN3ZNUEdi?=
 =?utf-8?B?TjVYSWw2UFpjOFJ6cVZTWWpUdyswOFNFU0NCN1VvNEhmb3EzVzRKaS9jemdK?=
 =?utf-8?B?U1ZQSGp2NWZpYTJISkhXZzlnOTNxZFZ2WEZDNFRmeTZCcW8wcjhzRzJHTnQ5?=
 =?utf-8?B?K0g1RUtJNVZtclREeHZORVFvbUx6M0hiS3Q3UC9TMDFlRU5DUTRnU3RIa2JF?=
 =?utf-8?B?aWpubExpRFh5b2Rlbm9FS09QZTRvdWlTNHNYV3ZuYXFGQjdvLzJTWGx0dUdi?=
 =?utf-8?B?QVkxbjBjUGliUERraEJCWVJWc1Y3MVFPSUZQYzFvZWVDMk5FTEZLZkRQQVRy?=
 =?utf-8?B?eWs5alB6Z2xmTjRrYUhLcFFEeXpUN1pBUHptd2dwazFXckR4KzVLWklwWTNW?=
 =?utf-8?Q?d7+wgi8YDF0LeJ/VotCGU4CJg?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e4195b6f-5059-4460-a9c5-08db09ba28eb
X-MS-Exchange-CrossTenant-AuthSource: CY5PR12MB6323.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Feb 2023 09:52:16.0031
 (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: 2zIHrKzkk6eUyDo4OXBRoMZKd6xJG/5Grop4uWMTdCR9/a1UFj+CfVFPlj+xXr0cBSHvSS7aGuf4LPJiyqv1DA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR12MB4486
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2/1/2023 5:36 AM, Sean Christopherson wrote:
> On Tue, Jan 31, 2023, Sean Christopherson wrote:
>> On Tue, Nov 29, 2022, Maxim Levitsky wrote:
>>> @@ -10015,13 +10022,34 @@ static void process_nmi(struct kvm_vcpu *vcpu)
>>>  	 * Otherwise, allow two (and we'll inject the first one immediately).
>>>  	 */
>>>  	if (static_call(kvm_x86_get_nmi_mask)(vcpu) || vcpu->arch.nmi_injected)
>>> -		limit = 1;
>>> +		limit--;
>>> +
>>> +	/* Also if there is already a NMI hardware queued to be injected,
>>> +	 * decrease the limit again
>>> +	 */
>>> +	if (static_call(kvm_x86_get_hw_nmi_pending)(vcpu))
>>> +		limit--;
>>
>> I don't think this is correct.  If a vNMI is pending and NMIs are blocked, then
>> limit will end up '0' and KVM will fail to pend the additional NMI in software.
> 
> Scratch that, dropping the second NMI in this case is correct.  The "running" part
> of the existing "x86 is limited to one NMI running, and one NMI pending after it"
> confused me.  The "running" thing is really just a variant on NMIs being blocked.
> 
> I'd also like to avoid the double decrement logic.  Accounting the virtual NMI is
> a very different thing than dealing with concurrent NMIs, I'd prefer to reflect
> that in the code.
> 
> Any objection to folding in the below to end up with:
> 
> 	unsigned limit;
> 
> 	/*
> 	 * x86 is limited to one NMI pending, but because KVM can't react to
> 	 * incoming NMIs as quickly as bare metal, e.g. if the vCPU is
> 	 * scheduled out, KVM needs to play nice with two queued NMIs showing
> 	 * up at the same time.  To handle this scenario, allow two NMIs to be
> 	 * (temporarily) pending so long as NMIs are not blocked and KVM is not
> 	 * waiting for a previous NMI injection to complete (which effectively
> 	 * blocks NMIs).  KVM will immediately inject one of the two NMIs, and
> 	 * will request an NMI window to handle the second NMI.
> 	 */
> 	if (static_call(kvm_x86_get_nmi_mask)(vcpu) || vcpu->arch.nmi_injected)
> 		limit = 1;
> 	else
> 		limit = 2;
> 
> 	/*
> 	 * Adjust the limit to account for pending virtual NMIs, which aren't
> 	 * tracked in in vcpu->arch.nmi_pending.
> 	 */
> 	if (static_call(kvm_x86_is_vnmi_pending)(vcpu))
> 		limit--;
> 
> 	vcpu->arch.nmi_pending += atomic_xchg(&vcpu->arch.nmi_queued, 0);
> 	vcpu->arch.nmi_pending = min(vcpu->arch.nmi_pending, limit);
> 

I believe, you missed the function below hunk -

	if (vcpu->arch.nmi_pending &&
	    static_call(kvm_x86_set_vnmi_pending(vcpu)))
		vcpu->arch.nmi_pending--;

Or am I missing something.. please suggest.

> 	if (vcpu->arch.nmi_pending)
> 		kvm_make_request(KVM_REQ_EVENT, vcpu);
> 
> --
> From: Sean Christopherson <seanjc@google.com>
> Date: Tue, 31 Jan 2023 16:02:21 -0800
> Subject: [PATCH] KVM: x86: Tweak the code and comment related to handling
>  concurrent NMIs
> 
> Tweak the code and comment that deals with concurrent NMIs to explicitly
> call out that x86 allows exactly one pending NMI, but that KVM needs to
> temporarily allow two pending NMIs in order to workaround the fact that
> the target vCPU cannot immediately recognize an incoming NMI, unlike bare
> metal.
> 
> No functional change intended.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/x86.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 030136b6ebbd..fda09ba48b6b 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10122,15 +10122,22 @@ static int kvm_check_and_inject_events(struct kvm_vcpu *vcpu,
>  
>  static void process_nmi(struct kvm_vcpu *vcpu)
>  {
> -	unsigned limit = 2;
> +	unsigned limit;
>  
>  	/*
> -	 * x86 is limited to one NMI running, and one NMI pending after it.
> -	 * If an NMI is already in progress, limit further NMIs to just one.
> -	 * Otherwise, allow two (and we'll inject the first one immediately).
> +	 * x86 is limited to one NMI pending, but because KVM can't react to
> +	 * incoming NMIs as quickly as bare metal, e.g. if the vCPU is
> +	 * scheduled out, KVM needs to play nice with two queued NMIs showing
> +	 * up at the same time.  To handle this scenario, allow two NMIs to be
> +	 * (temporarily) pending so long as NMIs are not blocked and KVM is not
> +	 * waiting for a previous NMI injection to complete (which effectively
> +	 * blocks NMIs).  KVM will immediately inject one of the two NMIs, and
> +	 * will request an NMI window to handle the second NMI.
>  	 */
>  	if (static_call(kvm_x86_get_nmi_mask)(vcpu) || vcpu->arch.nmi_injected)
>  		limit = 1;
> +	else
> +		limit = 2;
>  
>  	vcpu->arch.nmi_pending += atomic_xchg(&vcpu->arch.nmi_queued, 0);
>  	vcpu->arch.nmi_pending = min(vcpu->arch.nmi_pending, limit);
> 

Looks good to me, will include in v3.

Thanks,
Santosh