Received-SPF: pass (google.com: domain of linux-kernel+bounces-194700-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <0bac819f-fdbe-4de2-8a5f-30ded87bb036@zytor.com>
Date: Wed, 29 May 2024 13:47:09 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 4/6] x86/irq: Process nmi sources in NMI handler
To: Jacob Pan <jacob.jun.pan@linux.intel.com>, X86 Kernel <x86@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Dave Hansen <dave.hansen@intel.com>, Ingo Molnar <mingo@redhat.com>,
        Borislav Petkov <bp@alien8.de>, linux-perf-users@vger.kernel.org,
        Peter Zijlstra <peterz@infradead.org>
Cc: Andi Kleen <andi.kleen@intel.com>, Xin Li <xin3.li@intel.com>
References: <20240529203325.3039243-1-jacob.jun.pan@linux.intel.com>
 <20240529203325.3039243-5-jacob.jun.pan@linux.intel.com>
Content-Language: en-US
From: "H. Peter Anvin" <hpa@zytor.com>
In-Reply-To: <20240529203325.3039243-5-jacob.jun.pan@linux.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 5/29/24 13:33, Jacob Pan wrote:
> +
> +	/*
> +	 * Per NMI source specification, there is no guarantee that a valid
> +	 * NMI vector is always delivered, even when the source specified
> +	 * one. It is software's responsibility to check all available NMI
> +	 * sources when bit 0 is set in the NMI source bitmap. i.e. we have
> +	 * to call every handler as if we have no NMI source.
> +	 * On the other hand, if we do get non-zero vectors, we know exactly
> +	 * what the sources are. So we only call the handlers with the bit set.
> +	 */
> +	if (source_bitmask & BIT(NMI_SOURCE_VEC_UNKNOWN)) {
> +		pr_warn_ratelimited("NMI received with unknown source\n");
> +		return 0;
> +	}
> +

Note: if bit 0 is set, you can process any other bits first (on the 
general assumption that if you bother with NMI source then those events 
are performance sensitive), and you could even exclude them from the 
poll. This is an optimization, and what you have here is correct from a 
functional point of view.

> +	source_bitmask = fred_event_data(regs);
> +	if (!source_bitmask) {
> +		pr_warn_ratelimited("NMI received without source information!\n");
> +		return 0;
> +	}

If the event data word is 0, it probably should be treated as a 
*permanent* failure, as it is a Should Not Happen[TM] situation, and 
means there is an implementation (or, perhaps more likely, 
virtualization!) bug, and as such it may not be safe to trust the NMI 
source information in the future.

> +	if (!cpu_feature_enabled(X86_FEATURE_NMI_SOURCE) || type != NMI_LOCAL)
> +		return 0;

I'm not sure I understand why you are requiring type to be NMI_LOCAL here?

	-hpa