Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <61705187-a0a5-f76c-a006-93494def255d@redhat.com>
Date:   Thu, 10 Mar 2022 17:22:17 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.4.0
Subject: Re: [PATCH 2/4] x86/platform/uv: Update NMI Handler for UV5
Content-Language: en-US
To:     Mike Travis <mike.travis@hpe.com>, Borislav Petkov <bp@alien8.de>,
        Ingo Molnar <mingo@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Steve Wahl <steve.wahl@hpe.com>, x86@kernel.org
Cc:     Dimitri Sivanich <dimitri.sivanich@hpe.com>,
        Andy Shevchenko <andy@infradead.org>,
        Darren Hart <dvhart@infradead.org>,
        "H. Peter Anvin" <hpa@zytor.com>,
        Russ Anderson <russ.anderson@hpe.com>,
        linux-kernel@vger.kernel.org, platform-driver-x86@vger.kernel.org
References: <20220308010537.70150-1-mike.travis@hpe.com>
 <20220308010537.70150-3-mike.travis@hpe.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20220308010537.70150-3-mike.travis@hpe.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

On 3/8/22 02:05, Mike Travis wrote:
> Update NMI handler to interface with UV5 hardware. This involves
> changing the EVENT_OCCURRED MMR used by the hardware and removes
> the check for the newer NMI function supported by UV BIOS.
> 
> Signed-off-by: Mike Travis <mike.travis@hpe.com>
> Reviewed-by: Dimitri Sivanich <dimitri.sivanich@hpe.com>
> Reviewed-by: Steve Wahl <steve.wahl@hpe.com>
> ---
>  arch/x86/platform/uv/uv_nmi.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/platform/uv/uv_nmi.c b/arch/x86/platform/uv/uv_nmi.c
> index 926a4e006e5a..38f4beae9fab 100644
> --- a/arch/x86/platform/uv/uv_nmi.c
> +++ b/arch/x86/platform/uv/uv_nmi.c
> @@ -245,7 +245,7 @@ static inline bool uv_nmi_action_is(const char *action)
>  static void uv_nmi_setup_mmrs(void)
>  {
>  	/* First determine arch specific MMRs to handshake with BIOS */
> -	if (UVH_EVENT_OCCURRED0_EXTIO_INT0_MASK) {
> +	if (UVH_EVENT_OCCURRED0_EXTIO_INT0_MASK) {	/* UV2,3,4 setup */
>  		uvh_nmi_mmrx = UVH_EVENT_OCCURRED0;
>  		uvh_nmi_mmrx_clear = UVH_EVENT_OCCURRED0_ALIAS;
>  		uvh_nmi_mmrx_shift = UVH_EVENT_OCCURRED0_EXTIO_INT0_SHFT;
> @@ -255,26 +255,26 @@ static void uv_nmi_setup_mmrs(void)
>  		uvh_nmi_mmrx_req = UVH_BIOS_KERNEL_MMR_ALIAS_2;
>  		uvh_nmi_mmrx_req_shift = 62;
>  
> -	} else if (UVH_EVENT_OCCURRED1_EXTIO_INT0_MASK) {
> +	} else if (UVH_EVENT_OCCURRED1_EXTIO_INT0_MASK) { /* UV5 setup */
>  		uvh_nmi_mmrx = UVH_EVENT_OCCURRED1;
>  		uvh_nmi_mmrx_clear = UVH_EVENT_OCCURRED1_ALIAS;
>  		uvh_nmi_mmrx_shift = UVH_EVENT_OCCURRED1_EXTIO_INT0_SHFT;
>  		uvh_nmi_mmrx_type = "OCRD1-EXTIO_INT0";
>  
> -		uvh_nmi_mmrx_supported = UVH_EXTIO_INT0_BROADCAST;
> -		uvh_nmi_mmrx_req = UVH_BIOS_KERNEL_MMR_ALIAS_2;
> -		uvh_nmi_mmrx_req_shift = 62;

The dropping of setting uvh_nmi_mmrx_req and uvh_nmi_mmrx_req_shift here
looks weird, this seems like it might break things.

A closer look shows that that is not the case because these 2 *global*
variables are only used inside this 1 function and the added if for
uvh_nmi_mmrx_req being set causes them to now no longer be used in
this code path.

And before this change these 2 global variables where always set to:
UVH_BIOS_KERNEL_MMR_ALIAS_2 resp 62. This is even mentioned in
a comment where they are declared.

IMHO this really should be replaced with a preparation patch
which just adds a #define for the 62 and uses UVH_BIOS_KERNEL_MMR_ALIAS_2
directly, which it seems they really should have been from the start...

> +		uvh_nmi_mmrx_supported = 1;

So now your abusing a value used to store a register address/offset
as bool as well, using a special value for it being a bool rather
then a register offset and just hoping that the offset never is 1.

This is not the way to write maintainable code if you need both
a variable to store a register address (*) and a bool, please
use 2 separate variables for this.

*) Note you don't seem to need a variable for the register
address, this seems to be another case where you should just be
better of directly using the UVH_EXTIO_INT0_BROADCAST #define,
you could even change things to a #define in the same preparation
patch.


> +		uvh_nmi_mmrx_req = 0;>  
>  	} else {
> -		pr_err("UV:%s:cannot find EVENT_OCCURRED*_EXTIO_INT0\n",
> -			__func__);
> +		pr_err("UV:%s:NMI support not available on this system\n", __func__);
>  		return;
>  	}
>  
> -	/* Then find out if new NMI is supported */
> -	if (likely(uv_read_local_mmr(uvh_nmi_mmrx_supported))) {
> -		uv_write_local_mmr(uvh_nmi_mmrx_req,
> -					1UL << uvh_nmi_mmrx_req_shift);
> +	/* Then find out if new NMI is supported (assumed on UV5) */
> +	if (likely(uvh_nmi_mmrx_supported == 1) ||
> +	   (uv_read_local_mmr(uvh_nmi_mmrx_supported) & 1UL << uvh_nmi_mmrx_req_shift)) {
> +		if (uvh_nmi_mmrx_req)
> +			uv_write_local_mmr(uvh_nmi_mmrx_req,
> +						1UL << uvh_nmi_mmrx_req_shift);

The whole switch of just assuming "new NMI" support in one case and
of no longer writing the mmr seems like something which should be
much more clearly described in the commit message.

>  		nmi_mmr = uvh_nmi_mmrx;
>  		nmi_mmr_clear = uvh_nmi_mmrx_clear;
>  		nmi_mmr_pending = 1UL << uvh_nmi_mmrx_shift;


Regards,

Hans