Hi Linus,
please pull the x86/platform queue.
Thx.
---
The following changes since commit a1b8638ba1320e6684aa98233c15255eb803fac7:
Linux 5.9-rc7 (2020-09-27 14:38:10 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git tags/x86_platform_for_v5.10
for you to fetch changes up to 7a6d94f0ed957fb667d4d74c5c6c640a26e87c8f:
x86/platform/uv: Update Copyrights to conform to HPE standards (2020-10-07 09:10:07 +0200)
----------------------------------------------------------------
* Cleanup different aspects of the UV code and start adding support for
the new UV5 class of systems, by Mike Travis.
* Use a flexible array for a dynamically sized struct uv_rtc_timer_head,
by Gustavo A. R. Silva.
----------------------------------------------------------------
Gustavo A. R. Silva (1):
x86/uv/time: Use a flexible array in struct uv_rtc_timer_head
Mike Travis (13):
x86/platform/uv: Remove UV BAU TLB Shootdown Handler
x86/platform/uv: Remove SCIR MMR references for UV systems
drivers/misc/sgi-xp: Adjust references in UV kernel modules
x86/platform/uv: Update UV MMRs for UV5
x86/platform/uv: Add UV5 direct references
x86/platform/uv: Add and decode Arch Type in UVsystab
x86/platform/uv: Update MMIOH references based on new UV5 MMRs
x86/platform/uv: Adjust GAM MMR references affected by UV5 updates
x86/platform/uv: Update UV5 MMR references in UV GRU
x86/platform/uv: Update node present counting
x86/platform/uv: Update UV5 TSC checking
x86/platform/uv: Update for UV5 NMI MMR changes
x86/platform/uv: Update Copyrights to conform to HPE standards
arch/x86/include/asm/idtentry.h | 4 -
arch/x86/include/asm/uv/bios.h | 17 +-
arch/x86/include/asm/uv/uv.h | 4 +-
arch/x86/include/asm/uv/uv_bau.h | 755 ----
arch/x86/include/asm/uv/uv_hub.h | 165 +-
arch/x86/include/asm/uv/uv_mmrs.h | 7646 +++++++++++++++++++----------------
arch/x86/kernel/apic/x2apic_uv_x.c | 822 ++--
arch/x86/kernel/idt.c | 3 -
arch/x86/mm/tlb.c | 24 -
arch/x86/platform/uv/Makefile | 2 +-
arch/x86/platform/uv/bios_uv.c | 28 +-
arch/x86/platform/uv/tlb_uv.c | 2097 ----------
arch/x86/platform/uv/uv_nmi.c | 65 +-
arch/x86/platform/uv/uv_time.c | 18 +-
drivers/misc/sgi-gru/grufile.c | 3 +-
drivers/misc/sgi-xp/xp.h | 8 +-
drivers/misc/sgi-xp/xp_main.c | 5 +-
drivers/misc/sgi-xp/xp_uv.c | 7 +-
drivers/misc/sgi-xp/xpc_main.c | 7 +-
drivers/misc/sgi-xp/xpc_partition.c | 3 +-
drivers/misc/sgi-xp/xpnet.c | 3 +-
21 files changed, 4797 insertions(+), 6889 deletions(-)
delete mode 100644 arch/x86/include/asm/uv/uv_bau.h
delete mode 100644 arch/x86/platform/uv/tlb_uv.c
--
Regards/Gruss,
Boris.
SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg
On Mon, Oct 12, 2020 at 3:10 AM Borislav Petkov <[email protected]> wrote:
>
> please pull the x86/platform queue.
Hmm. I didn't immediately notice this new warning, because it only
happens with the clang build that I don't do in between every pull.
But this pull causes new warnings from clang:
arch/x86/platform/uv/uv_nmi.c:250:23: warning: implicit conversion
from 'unsigned long' to 'int' changes value from 1152921504606846976
to 0 [-Wconstant-conversion]
uvh_nmi_mmrx_mask = UVH_EVENT_OCCURRED0_EXTIO_INT0_MASK;
~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
and I think that warning is correct, and the code is wrong.
In particular, we have
static int uvh_nmi_mmrx_mask;
so it's a signed 32-bit integer, and the code is treating it like it's
a 64-bit mask.
Of course, it also looks like that 'uvh_nmi_mmrx_mask' thing is a
write-only variable so it doesn't matter, but can we _please_ get this
code fixed ASAP?
Linus
On 10/12/2020 2:10 PM, Linus Torvalds wrote:
> On Mon, Oct 12, 2020 at 3:10 AM Borislav Petkov <[email protected]> wrote:
>>
>> please pull the x86/platform queue.
>
> Hmm. I didn't immediately notice this new warning, because it only
> happens with the clang build that I don't do in between every pull.
>
> But this pull causes new warnings from clang:
>
> arch/x86/platform/uv/uv_nmi.c:250:23: warning: implicit conversion
> from 'unsigned long' to 'int' changes value from 1152921504606846976
> to 0 [-Wconstant-conversion]
> uvh_nmi_mmrx_mask = UVH_EVENT_OCCURRED0_EXTIO_INT0_MASK;
> ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> and I think that warning is correct, and the code is wrong.
>
> In particular, we have
>
> static int uvh_nmi_mmrx_mask;
>
> so it's a signed 32-bit integer, and the code is treating it like it's
> a 64-bit mask.
>
> Of course, it also looks like that 'uvh_nmi_mmrx_mask' thing is a
> write-only variable so it doesn't matter, but can we _please_ get this
> code fixed ASAP?
Yes, I'll look at it right now. Thanks.
>
> Linus
>
On Mon, Oct 12, 2020 at 02:15:55PM -0700, Mike Travis wrote:
> > Of course, it also looks like that 'uvh_nmi_mmrx_mask' thing is a
> > write-only variable so it doesn't matter, but can we _please_ get this
> > code fixed ASAP?
>
> Yes, I'll look at it right now. Thanks.
As this variable is write-only you could simply zap it now with a patch
ontop to fix the build and then introduce it properly later, when you
really need it?
--
Regards/Gruss,
Boris.
SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg
On Mon, Oct 12, 2020 at 2:42 PM Mike Travis <[email protected]> wrote:
>
> It should have been an unsigned long instead of an int as Linus
> suggested. I'm not sure it's a write only variable as I think the mask
> is used to check if the interrupt occurred (I'll have to look closer).
At least "git grep" only shows two assignments to it.
Of course, that would miss any cases that play games with preprocessor
token pasting etc, so it's not entirely meaningful, but it's certainly
a hint..
And yes, I expect that the fix is to just make it "unsigned long", but
if it truly isn't actually used, maybe removal is better.
Linus
On 10/12/2020 2:42 PM, Mike Travis wrote:
>
>
> On 10/12/2020 2:27 PM, Borislav Petkov wrote:
>> On Mon, Oct 12, 2020 at 02:15:55PM -0700, Mike Travis wrote:
>>>> Of course, it also looks like that 'uvh_nmi_mmrx_mask' thing is a
>>>> write-only variable so it doesn't matter, but can we _please_ get this
>>>> code fixed ASAP?
>>>
>>> Yes, I'll look at it right now.? Thanks.
>>
>> As this variable is write-only you could simply zap it now with a patch
>> ontop to fix the build and then introduce it properly later, when you
>> really need it?
>>
>
> It should have been an unsigned long instead of an int as Linus
> suggested.? I'm not sure it's a write only variable as I think the mask
> is used to check if the interrupt occurred (I'll have to look closer).
> I'm trying now to send the fixed patch.? It only has this change:
>
> dog 74> diff -u patches/uv5_update_nmi{.v1,}
> --- patches/uv5_update_nmi.v1?? 2020-10-12 16:30:06.083941459 -0500
> +++ patches/uv5_update_nmi????? 2020-10-12 16:30:46.663903731 -0500
> @@ -55,7 +55,7 @@
> ?+static unsigned long uvh_nmi_mmrx;??????????? /*
> UVH_EVENT_OCCURRED0/1 */
> ?+static unsigned long uvh_nmi_mmrx_clear;????? /*
> UVH_EVENT_OCCURRED0/1_ALIAS */
> ?+static int uvh_nmi_mmrx_shift;??????????????????????? /*
> UVH_EVENT_OCCURRED0/1_EXTIO_INT0_SHFT */
> -+static int uvh_nmi_mmrx_mask;???????????????? /*
> UVH_EVENT_OCCURRED0/1_EXTIO_INT0_MASK */
> ++static unsigned long uvh_nmi_mmrx_mask;?????????????? /*
> UVH_EVENT_OCCURRED0/1_EXTIO_INT0_MASK */
> ?+static char *uvh_nmi_mmrx_type;?????????????????????? /* "EXTIO_INT0" */
> ?+
> ?+/* Non-zero indicates newer SMM NMI handler present */
>
> (or quoted)
>
>> dog 74> diff -u patches/uv5_update_nmi{.v1,}
>> --- patches/uv5_update_nmi.v1?? 2020-10-12 16:30:06.083941459 -0500
>> +++ patches/uv5_update_nmi????? 2020-10-12 16:30:46.663903731 -0500
>> @@ -55,7 +55,7 @@
>> ?+static unsigned long uvh_nmi_mmrx;??????????? /*
>> UVH_EVENT_OCCURRED0/1 */
>> ?+static unsigned long uvh_nmi_mmrx_clear;????? /*
>> UVH_EVENT_OCCURRED0/1_ALIAS */
>> ?+static int uvh_nmi_mmrx_shift;??????????????????????? /*
>> UVH_EVENT_OCCURRED0/1_EXTIO_INT0_SHFT */
>> -+static int uvh_nmi_mmrx_mask;???????????????? /*
>> UVH_EVENT_OCCURRED0/1_EXTIO_INT0_MASK */
>> ++static unsigned long uvh_nmi_mmrx_mask;?????????????? /*
>> UVH_EVENT_OCCURRED0/1_EXTIO_INT0_MASK */
>> ?+static char *uvh_nmi_mmrx_type;?????????????????????? /*
>> "EXTIO_INT0" */
>> ?+
>> ?+/* Non-zero indicates newer SMM NMI handler present */
I was in the process of tracing it through and perhaps it does need a
bit more analysis to be correct. What does it mean to send a patch to
fix the compile error, just remove it?
On 10/12/2020 2:56 PM, Borislav Petkov wrote:
> On Mon, Oct 12, 2020 at 02:46:10PM -0700, Linus Torvalds wrote:
>> At least "git grep" only shows two assignments to it.
>>
>> Of course, that would miss any cases that play games with preprocessor
>> token pasting etc, so it's not entirely meaningful, but it's certainly
>> a hint..
>
> From a quick staring at gcc asm, it looks write only. And gcc didn't
> warn because it optimized that assignment away completely AFAICT.
>
>> And yes, I expect that the fix is to just make it "unsigned long", but
>> if it truly isn't actually used, maybe removal is better.
>
> Yeah, below is a proper patch which builds fine with gcc and clang-10.
> You guys have fun - I'm going to bed. :-)
>
> ---
> From: Mike Travis <[email protected]>
> Date: Mon, 12 Oct 2020 23:46:34 +0200
> Subject: [PATCH] x86/platform/uv: Correct uvh_nmi_mmrx_mask's type
>
> Clang rightfully warns:
>
> arch/x86/platform/uv/uv_nmi.c:250:23: warning: implicit conversion
> from 'unsigned long' to 'int' changes value from 1152921504606846976
> to 0 [-Wconstant-conversion]
> uvh_nmi_mmrx_mask = UVH_EVENT_OCCURRED0_EXTIO_INT0_MASK;
> ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Make the variable unsigned long.
>
> [ bp: Productize it. ]
Thanks, I will look at it (and test it on hardware and the UV5 simulator
to make sure it's correct.)
>
> Signed-off-by: Mike Travis <[email protected]>
> Signed-off-by: Borislav Petkov <[email protected]>
> ---
> arch/x86/platform/uv/uv_nmi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/platform/uv/uv_nmi.c b/arch/x86/platform/uv/uv_nmi.c
> index 0f5cbcf0da63..8566730f154d 100644
> --- a/arch/x86/platform/uv/uv_nmi.c
> +++ b/arch/x86/platform/uv/uv_nmi.c
> @@ -59,7 +59,7 @@ DEFINE_PER_CPU(struct uv_cpu_nmi_s, uv_cpu_nmi);
> static unsigned long uvh_nmi_mmrx; /* UVH_EVENT_OCCURRED0/1 */
> static unsigned long uvh_nmi_mmrx_clear; /* UVH_EVENT_OCCURRED0/1_ALIAS */
> static int uvh_nmi_mmrx_shift; /* UVH_EVENT_OCCURRED0/1_EXTIO_INT0_SHFT */
> -static int uvh_nmi_mmrx_mask; /* UVH_EVENT_OCCURRED0/1_EXTIO_INT0_MASK */
> +static unsigned long uvh_nmi_mmrx_mask; /* UVH_EVENT_OCCURRED0/1_EXTIO_INT0_MASK */
> static char *uvh_nmi_mmrx_type; /* "EXTIO_INT0" */
>
> /* Non-zero indicates newer SMM NMI handler present */
>
On Mon, Oct 12, 2020 at 02:58:07PM -0700, Mike Travis wrote:
> I was in the process of tracing it through and perhaps it does need a bit
> more analysis to be correct. What does it mean to send a patch to fix the
> compile error, just remove it?
Yes, to remove it for now as it is unused currently. But making it an
unsigned long is ok too AFAICT. So should I queue it and send it to
Linus later?
--
Regards/Gruss,
Boris.
SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg
On 10/12/2020 2:27 PM, Borislav Petkov wrote:
> On Mon, Oct 12, 2020 at 02:15:55PM -0700, Mike Travis wrote:
>>> Of course, it also looks like that 'uvh_nmi_mmrx_mask' thing is a
>>> write-only variable so it doesn't matter, but can we _please_ get this
>>> code fixed ASAP?
>>
>> Yes, I'll look at it right now. Thanks.
>
> As this variable is write-only you could simply zap it now with a patch
> ontop to fix the build and then introduce it properly later, when you
> really need it?
>
It should have been an unsigned long instead of an int as Linus
suggested. I'm not sure it's a write only variable as I think the mask
is used to check if the interrupt occurred (I'll have to look closer).
I'm trying now to send the fixed patch. It only has this change:
dog 74> diff -u patches/uv5_update_nmi{.v1,}
--- patches/uv5_update_nmi.v1 2020-10-12 16:30:06.083941459 -0500
+++ patches/uv5_update_nmi 2020-10-12 16:30:46.663903731 -0500
@@ -55,7 +55,7 @@
+static unsigned long uvh_nmi_mmrx; /* UVH_EVENT_OCCURRED0/1 */
+static unsigned long uvh_nmi_mmrx_clear; /*
UVH_EVENT_OCCURRED0/1_ALIAS */
+static int uvh_nmi_mmrx_shift; /*
UVH_EVENT_OCCURRED0/1_EXTIO_INT0_SHFT */
-+static int uvh_nmi_mmrx_mask; /*
UVH_EVENT_OCCURRED0/1_EXTIO_INT0_MASK */
++static unsigned long uvh_nmi_mmrx_mask; /*
UVH_EVENT_OCCURRED0/1_EXTIO_INT0_MASK */
+static char *uvh_nmi_mmrx_type; /* "EXTIO_INT0" */
+
+/* Non-zero indicates newer SMM NMI handler present */
(or quoted)
> dog 74> diff -u patches/uv5_update_nmi{.v1,}
> --- patches/uv5_update_nmi.v1 2020-10-12 16:30:06.083941459 -0500
> +++ patches/uv5_update_nmi 2020-10-12 16:30:46.663903731 -0500
> @@ -55,7 +55,7 @@
> +static unsigned long uvh_nmi_mmrx; /* UVH_EVENT_OCCURRED0/1 */
> +static unsigned long uvh_nmi_mmrx_clear; /* UVH_EVENT_OCCURRED0/1_ALIAS */
> +static int uvh_nmi_mmrx_shift; /* UVH_EVENT_OCCURRED0/1_EXTIO_INT0_SHFT */
> -+static int uvh_nmi_mmrx_mask; /* UVH_EVENT_OCCURRED0/1_EXTIO_INT0_MASK */
> ++static unsigned long uvh_nmi_mmrx_mask; /* UVH_EVENT_OCCURRED0/1_EXTIO_INT0_MASK */
> +static char *uvh_nmi_mmrx_type; /* "EXTIO_INT0" */
> +
> +/* Non-zero indicates newer SMM NMI handler present */
On Mon, Oct 12, 2020 at 02:46:10PM -0700, Linus Torvalds wrote:
> At least "git grep" only shows two assignments to it.
>
> Of course, that would miss any cases that play games with preprocessor
> token pasting etc, so it's not entirely meaningful, but it's certainly
> a hint..
From a quick staring at gcc asm, it looks write only. And gcc didn't
warn because it optimized that assignment away completely AFAICT.
> And yes, I expect that the fix is to just make it "unsigned long", but
> if it truly isn't actually used, maybe removal is better.
Yeah, below is a proper patch which builds fine with gcc and clang-10.
You guys have fun - I'm going to bed. :-)
---
From: Mike Travis <[email protected]>
Date: Mon, 12 Oct 2020 23:46:34 +0200
Subject: [PATCH] x86/platform/uv: Correct uvh_nmi_mmrx_mask's type
Clang rightfully warns:
arch/x86/platform/uv/uv_nmi.c:250:23: warning: implicit conversion
from 'unsigned long' to 'int' changes value from 1152921504606846976
to 0 [-Wconstant-conversion]
uvh_nmi_mmrx_mask = UVH_EVENT_OCCURRED0_EXTIO_INT0_MASK;
~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Make the variable unsigned long.
[ bp: Productize it. ]
Signed-off-by: Mike Travis <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
---
arch/x86/platform/uv/uv_nmi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/platform/uv/uv_nmi.c b/arch/x86/platform/uv/uv_nmi.c
index 0f5cbcf0da63..8566730f154d 100644
--- a/arch/x86/platform/uv/uv_nmi.c
+++ b/arch/x86/platform/uv/uv_nmi.c
@@ -59,7 +59,7 @@ DEFINE_PER_CPU(struct uv_cpu_nmi_s, uv_cpu_nmi);
static unsigned long uvh_nmi_mmrx; /* UVH_EVENT_OCCURRED0/1 */
static unsigned long uvh_nmi_mmrx_clear; /* UVH_EVENT_OCCURRED0/1_ALIAS */
static int uvh_nmi_mmrx_shift; /* UVH_EVENT_OCCURRED0/1_EXTIO_INT0_SHFT */
-static int uvh_nmi_mmrx_mask; /* UVH_EVENT_OCCURRED0/1_EXTIO_INT0_MASK */
+static unsigned long uvh_nmi_mmrx_mask; /* UVH_EVENT_OCCURRED0/1_EXTIO_INT0_MASK */
static char *uvh_nmi_mmrx_type; /* "EXTIO_INT0" */
/* Non-zero indicates newer SMM NMI handler present */
--
2.21.0
--
Regards/Gruss,
Boris.
SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg
On 10/13/2020 6:37 AM, Mike Travis wrote:
>
>
> On 10/13/2020 6:29 AM, Borislav Petkov wrote:
>> On Tue, Oct 13, 2020 at 05:33:37AM -0700, Mike Travis wrote:
>>> I'm working on the correct code now, and I have UV4 & UV4A machine time
>>> starting at 7am (PDT) to test it.? The UV5 simulator does not yet
>>> emulate
>>> console initiated NMI from the BMC.
>>
>> Ok, let me put it another way: is this simple fix good enough for now so
>> that it doesn't trigger the build error on Linus' tree or not? You can
>> take your time and do all kinds of fixing later but we need a minimal
>> fix *now*!
>>
>> Pretty please?
>>
>
> Yes, it does fix the compile error.
Turns out I was combining 3 different sources to determine if the NMI
INT occurred and I used the 1ULL << SHIFT to check each one. So the
MASK is indeed extraneous and can be removed. Tested and patch follows.
On 10/13/2020 4:11 AM, Borislav Petkov wrote:
> On Mon, Oct 12, 2020 at 02:58:07PM -0700, Mike Travis wrote:
>> I was in the process of tracing it through and perhaps it does need a bit
>> more analysis to be correct. What does it mean to send a patch to fix the
>> compile error, just remove it?
>
> Yes, to remove it for now as it is unused currently. But making it an
> unsigned long is ok too AFAICT. So should I queue it and send it to
> Linus later?
>
I'm working on the correct code now, and I have UV4 & UV4A machine time
starting at 7am (PDT) to test it. The UV5 simulator does not yet
emulate console initiated NMI from the BMC.
On Tue, Oct 13, 2020 at 05:33:37AM -0700, Mike Travis wrote:
> I'm working on the correct code now, and I have UV4 & UV4A machine time
> starting at 7am (PDT) to test it. The UV5 simulator does not yet emulate
> console initiated NMI from the BMC.
Ok, let me put it another way: is this simple fix good enough for now so
that it doesn't trigger the build error on Linus' tree or not? You can
take your time and do all kinds of fixing later but we need a minimal
fix *now*!
Pretty please?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On 10/13/2020 6:29 AM, Borislav Petkov wrote:
> On Tue, Oct 13, 2020 at 05:33:37AM -0700, Mike Travis wrote:
>> I'm working on the correct code now, and I have UV4 & UV4A machine time
>> starting at 7am (PDT) to test it. The UV5 simulator does not yet emulate
>> console initiated NMI from the BMC.
>
> Ok, let me put it another way: is this simple fix good enough for now so
> that it doesn't trigger the build error on Linus' tree or not? You can
> take your time and do all kinds of fixing later but we need a minimal
> fix *now*!
>
> Pretty please?
>
Yes, it does fix the compile error.