* [PATCH 1/3] xen/mcelog: drop __MC_MSR_MCGCAP
2019-11-11 14:43 [PATCH 0/3] xen/mcelog: assorted adjustments Jan Beulich
@ 2019-11-11 14:45 ` Jan Beulich
2019-11-12 23:55 ` Boris Ostrovsky
2019-11-11 14:46 ` [PATCH 2/3] xen/mcelog: add PPIN to record when available Jan Beulich
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2019-11-11 14:45 UTC (permalink / raw)
To: Boris Ostrovsky, Juergen Gross; +Cc: xen-devel, lkml
It has never been part of Xen's public interface, and there's therefore
no guarantee for MCG_CAP's value to always be present in array entry 0.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/drivers/xen/mcelog.c
+++ b/drivers/xen/mcelog.c
@@ -222,7 +222,7 @@ static int convert_log(struct mc_info *m
struct mcinfo_global *mc_global;
struct mcinfo_bank *mc_bank;
struct xen_mce m;
- uint32_t i;
+ unsigned int i, j;
mic = NULL;
x86_mcinfo_lookup(&mic, mi, MC_TYPE_GLOBAL);
@@ -248,7 +248,12 @@ static int convert_log(struct mc_info *m
m.socketid = g_physinfo[i].mc_chipid;
m.cpu = m.extcpu = g_physinfo[i].mc_cpunr;
m.cpuvendor = (__u8)g_physinfo[i].mc_vendor;
- m.mcgcap = g_physinfo[i].mc_msrvalues[__MC_MSR_MCGCAP].value;
+ for (j = 0; j < g_physinfo[i].mc_nmsrvals; ++j)
+ switch (g_physinfo[i].mc_msrvalues[j].reg) {
+ case MSR_IA32_MCG_CAP:
+ m.mcgcap = g_physinfo[i].mc_msrvalues[j].value;
+ break;
+ }
mic = NULL;
x86_mcinfo_lookup(&mic, mi, MC_TYPE_BANK);
--- a/include/xen/interface/xen-mca.h
+++ b/include/xen/interface/xen-mca.h
@@ -183,7 +183,6 @@ struct mc_info {
DEFINE_GUEST_HANDLE_STRUCT(mc_info);
#define __MC_MSR_ARRAYSIZE 8
-#define __MC_MSR_MCGCAP 0
#define __MC_NMSRS 1
#define MC_NCAPS 7
struct mcinfo_logical_cpu {
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 2/3] xen/mcelog: add PPIN to record when available
2019-11-11 14:43 [PATCH 0/3] xen/mcelog: assorted adjustments Jan Beulich
2019-11-11 14:45 ` [PATCH 1/3] xen/mcelog: drop __MC_MSR_MCGCAP Jan Beulich
@ 2019-11-11 14:46 ` Jan Beulich
2019-11-13 0:11 ` Boris Ostrovsky
2019-11-11 14:46 ` [PATCH 3/3] xen/mcelog: also allow building for 32-bit kernels Jan Beulich
2019-11-14 13:05 ` [PATCH 0/3] xen/mcelog: assorted adjustments Jürgen Groß
3 siblings, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2019-11-11 14:46 UTC (permalink / raw)
To: Boris Ostrovsky, Juergen Gross; +Cc: xen-devel, lkml, the arch/x86 maintainers
This is to augment commit 3f5a7896a5 ("x86/mce: Include the PPIN in MCE
records when available").
I'm also adding "synd" and "ipid" fields to struct xen_mce, in an
attempt to keep field offsets in sync with struct mce. These two fields
won't get populated for now, though.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -393,6 +393,8 @@
#define MSR_AMD_PSTATE_DEF_BASE 0xc0010064
#define MSR_AMD64_OSVW_ID_LENGTH 0xc0010140
#define MSR_AMD64_OSVW_STATUS 0xc0010141
+#define MSR_AMD_PPIN_CTL 0xc00102f0
+#define MSR_AMD_PPIN 0xc00102f1
#define MSR_AMD64_LS_CFG 0xc0011020
#define MSR_AMD64_DC_CFG 0xc0011022
#define MSR_AMD64_BU_CFG2 0xc001102a
--- a/drivers/xen/mcelog.c
+++ b/drivers/xen/mcelog.c
@@ -253,6 +253,11 @@ static int convert_log(struct mc_info *m
case MSR_IA32_MCG_CAP:
m.mcgcap = g_physinfo[i].mc_msrvalues[j].value;
break;
+
+ case MSR_PPIN:
+ case MSR_AMD_PPIN:
+ m.ppin = g_physinfo[i].mc_msrvalues[j].value;
+ break;
}
mic = NULL;
--- a/include/xen/interface/xen-mca.h
+++ b/include/xen/interface/xen-mca.h
@@ -332,7 +332,11 @@ struct xen_mc {
};
DEFINE_GUEST_HANDLE_STRUCT(xen_mc);
-/* Fields are zero when not available */
+/*
+ * Fields are zero when not available. Also, this struct is shared with
+ * userspace mcelog and thus must keep existing fields at current offsets.
+ * Only add new fields to the end of the structure
+ */
struct xen_mce {
__u64 status;
__u64 misc;
@@ -353,6 +357,9 @@ struct xen_mce {
__u32 socketid; /* CPU socket ID */
__u32 apicid; /* CPU initial apic ID */
__u64 mcgcap; /* MCGCAP MSR: machine check capabilities of CPU */
+ __u64 synd; /* MCA_SYND MSR: only valid on SMCA systems */
+ __u64 ipid; /* MCA_IPID MSR: only valid on SMCA systems */
+ __u64 ppin; /* Protected Processor Inventory Number */
};
/*
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 2/3] xen/mcelog: add PPIN to record when available
2019-11-11 14:46 ` [PATCH 2/3] xen/mcelog: add PPIN to record when available Jan Beulich
@ 2019-11-13 0:11 ` Boris Ostrovsky
2019-11-13 13:44 ` [Xen-devel] " Jan Beulich
0 siblings, 1 reply; 13+ messages in thread
From: Boris Ostrovsky @ 2019-11-13 0:11 UTC (permalink / raw)
To: Jan Beulich, Juergen Gross; +Cc: xen-devel, lkml, the arch/x86 maintainers
On 11/11/19 9:46 AM, Jan Beulich wrote:
> This is to augment commit 3f5a7896a5 ("x86/mce: Include the PPIN in MCE
> records when available").
>
> I'm also adding "synd" and "ipid" fields to struct xen_mce, in an
> attempt to keep field offsets in sync with struct mce. These two fields
> won't get populated for now, though.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -393,6 +393,8 @@
> #define MSR_AMD_PSTATE_DEF_BASE 0xc0010064
> #define MSR_AMD64_OSVW_ID_LENGTH 0xc0010140
> #define MSR_AMD64_OSVW_STATUS 0xc0010141
> +#define MSR_AMD_PPIN_CTL 0xc00102f0
> +#define MSR_AMD_PPIN 0xc00102f1
Which processors are these defined for? I looked at a couple (fam 15h
and 17h) and didn't see those. And I don't see them in Linux.
> #define MSR_AMD64_LS_CFG 0xc0011020
> #define MSR_AMD64_DC_CFG 0xc0011022
> #define MSR_AMD64_BU_CFG2 0xc001102a
> --- a/drivers/xen/mcelog.c
> +++ b/drivers/xen/mcelog.c
> @@ -253,6 +253,11 @@ static int convert_log(struct mc_info *m
> case MSR_IA32_MCG_CAP:
> m.mcgcap = g_physinfo[i].mc_msrvalues[j].value;
> break;
> +
> + case MSR_PPIN:
> + case MSR_AMD_PPIN:
> + m.ppin = g_physinfo[i].mc_msrvalues[j].value;
> + break;
> }
>
> mic = NULL;
> --- a/include/xen/interface/xen-mca.h
> +++ b/include/xen/interface/xen-mca.h
> @@ -332,7 +332,11 @@ struct xen_mc {
> };
> DEFINE_GUEST_HANDLE_STRUCT(xen_mc);
>
> -/* Fields are zero when not available */
> +/*
> + * Fields are zero when not available. Also, this struct is shared with
> + * userspace mcelog and thus must keep existing fields at current offsets.
> + * Only add new fields to the end of the structure
> + */
> struct xen_mce {
Why is this structure is part of the interface?
-boris
> __u64 status;
> __u64 misc;
> @@ -353,6 +357,9 @@ struct xen_mce {
> __u32 socketid; /* CPU socket ID */
> __u32 apicid; /* CPU initial apic ID */
> __u64 mcgcap; /* MCGCAP MSR: machine check capabilities of CPU */
> + __u64 synd; /* MCA_SYND MSR: only valid on SMCA systems */
> + __u64 ipid; /* MCA_IPID MSR: only valid on SMCA systems */
> + __u64 ppin; /* Protected Processor Inventory Number */
> };
>
> /*
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Xen-devel] [PATCH 2/3] xen/mcelog: add PPIN to record when available
2019-11-13 0:11 ` Boris Ostrovsky
@ 2019-11-13 13:44 ` Jan Beulich
2019-11-13 16:34 ` Boris Ostrovsky
0 siblings, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2019-11-13 13:44 UTC (permalink / raw)
To: Boris Ostrovsky; +Cc: Juergen Gross, the arch/x86 maintainers, xen-devel, lkml
On 13.11.2019 01:11, Boris Ostrovsky wrote:
> On 11/11/19 9:46 AM, Jan Beulich wrote:
>> --- a/arch/x86/include/asm/msr-index.h
>> +++ b/arch/x86/include/asm/msr-index.h
>> @@ -393,6 +393,8 @@
>> #define MSR_AMD_PSTATE_DEF_BASE 0xc0010064
>> #define MSR_AMD64_OSVW_ID_LENGTH 0xc0010140
>> #define MSR_AMD64_OSVW_STATUS 0xc0010141
>> +#define MSR_AMD_PPIN_CTL 0xc00102f0
>> +#define MSR_AMD_PPIN 0xc00102f1
>
> Which processors are these defined for? I looked at a couple (fam 15h
> and 17h) and didn't see those. And I don't see them in Linux.
Certain Fam17 ones, Rome in particular (which is where I've
tested this).
>> --- a/include/xen/interface/xen-mca.h
>> +++ b/include/xen/interface/xen-mca.h
>> @@ -332,7 +332,11 @@ struct xen_mc {
>> };
>> DEFINE_GUEST_HANDLE_STRUCT(xen_mc);
>>
>> -/* Fields are zero when not available */
>> +/*
>> + * Fields are zero when not available. Also, this struct is shared with
>> + * userspace mcelog and thus must keep existing fields at current offsets.
>> + * Only add new fields to the end of the structure
>> + */
>> struct xen_mce {
>
> Why is this structure is part of the interface?
That's a question to whoever put it there. There look to have
been decisions (see also patch 1) to have the Linux clones of
Xen's public headers deviate far more from their original
than I would consider reasonable.
Jan
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Xen-devel] [PATCH 2/3] xen/mcelog: add PPIN to record when available
2019-11-13 13:44 ` [Xen-devel] " Jan Beulich
@ 2019-11-13 16:34 ` Boris Ostrovsky
0 siblings, 0 replies; 13+ messages in thread
From: Boris Ostrovsky @ 2019-11-13 16:34 UTC (permalink / raw)
To: Jan Beulich; +Cc: Juergen Gross, the arch/x86 maintainers, xen-devel, lkml
On 11/13/19 8:44 AM, Jan Beulich wrote:
> On 13.11.2019 01:11, Boris Ostrovsky wrote:
>> On 11/11/19 9:46 AM, Jan Beulich wrote:
>>> --- a/arch/x86/include/asm/msr-index.h
>>> +++ b/arch/x86/include/asm/msr-index.h
>>> @@ -393,6 +393,8 @@
>>> #define MSR_AMD_PSTATE_DEF_BASE 0xc0010064
>>> #define MSR_AMD64_OSVW_ID_LENGTH 0xc0010140
>>> #define MSR_AMD64_OSVW_STATUS 0xc0010141
>>> +#define MSR_AMD_PPIN_CTL 0xc00102f0
>>> +#define MSR_AMD_PPIN 0xc00102f1
>> Which processors are these defined for? I looked at a couple (fam 15h
>> and 17h) and didn't see those. And I don't see them in Linux.
> Certain Fam17 ones, Rome in particular (which is where I've
> tested this).
I was looking at Naples, can't find Rome on AMD's page.
>
>>> --- a/include/xen/interface/xen-mca.h
>>> +++ b/include/xen/interface/xen-mca.h
>>> @@ -332,7 +332,11 @@ struct xen_mc {
>>> };
>>> DEFINE_GUEST_HANDLE_STRUCT(xen_mc);
>>>
>>> -/* Fields are zero when not available */
>>> +/*
>>> + * Fields are zero when not available. Also, this struct is shared with
>>> + * userspace mcelog and thus must keep existing fields at current offsets.
>>> + * Only add new fields to the end of the structure
>>> + */
>>> struct xen_mce {
>> Why is this structure is part of the interface?
> That's a question to whoever put it there. There look to have
> been decisions (see also patch 1) to have the Linux clones of
> Xen's public headers deviate far more from their original
> than I would consider reasonable.
Yes, this goes back to when the file was first created.
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
(but if you want to move non-interface parts into, say,
drivers/xen/mcelog.h I won't stand in your way ;-) )
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] xen/mcelog: also allow building for 32-bit kernels
2019-11-11 14:43 [PATCH 0/3] xen/mcelog: assorted adjustments Jan Beulich
2019-11-11 14:45 ` [PATCH 1/3] xen/mcelog: drop __MC_MSR_MCGCAP Jan Beulich
2019-11-11 14:46 ` [PATCH 2/3] xen/mcelog: add PPIN to record when available Jan Beulich
@ 2019-11-11 14:46 ` Jan Beulich
2019-11-13 0:15 ` Boris Ostrovsky
2019-11-14 13:05 ` [PATCH 0/3] xen/mcelog: assorted adjustments Jürgen Groß
3 siblings, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2019-11-11 14:46 UTC (permalink / raw)
To: Boris Ostrovsky, Juergen Gross; +Cc: xen-devel, lkml
There's no apparent reason why it can be used on 64-bit only.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/drivers/xen/Kconfig
+++ b/drivers/xen/Kconfig
@@ -285,7 +285,7 @@ config XEN_ACPI_PROCESSOR
config XEN_MCE_LOG
bool "Xen platform mcelog"
- depends on XEN_DOM0 && X86_64 && X86_MCE
+ depends on XEN_DOM0 && X86 && X86_MCE
help
Allow kernel fetching MCE error from Xen platform and
converting it into Linux mcelog format for mcelog tools
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] xen/mcelog: also allow building for 32-bit kernels
2019-11-11 14:46 ` [PATCH 3/3] xen/mcelog: also allow building for 32-bit kernels Jan Beulich
@ 2019-11-13 0:15 ` Boris Ostrovsky
2019-11-13 13:47 ` [Xen-devel] " Jan Beulich
0 siblings, 1 reply; 13+ messages in thread
From: Boris Ostrovsky @ 2019-11-13 0:15 UTC (permalink / raw)
To: Jan Beulich, Juergen Gross; +Cc: xen-devel, lkml
On 11/11/19 9:46 AM, Jan Beulich wrote:
> There's no apparent reason why it can be used on 64-bit only.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/drivers/xen/Kconfig
> +++ b/drivers/xen/Kconfig
> @@ -285,7 +285,7 @@ config XEN_ACPI_PROCESSOR
>
> config XEN_MCE_LOG
> bool "Xen platform mcelog"
> - depends on XEN_DOM0 && X86_64 && X86_MCE
> + depends on XEN_DOM0 && X86 && X86_MCE
Can we have X86_MCE without X86?
-boris
> help
> Allow kernel fetching MCE error from Xen platform and
> converting it into Linux mcelog format for mcelog tools
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Xen-devel] [PATCH 3/3] xen/mcelog: also allow building for 32-bit kernels
2019-11-13 0:15 ` Boris Ostrovsky
@ 2019-11-13 13:47 ` Jan Beulich
2019-11-13 14:06 ` Jürgen Groß
2019-11-14 7:54 ` Jürgen Groß
0 siblings, 2 replies; 13+ messages in thread
From: Jan Beulich @ 2019-11-13 13:47 UTC (permalink / raw)
To: Boris Ostrovsky; +Cc: Juergen Gross, xen-devel, lkml
On 13.11.2019 01:15, Boris Ostrovsky wrote:
> On 11/11/19 9:46 AM, Jan Beulich wrote:
>> There's no apparent reason why it can be used on 64-bit only.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/drivers/xen/Kconfig
>> +++ b/drivers/xen/Kconfig
>> @@ -285,7 +285,7 @@ config XEN_ACPI_PROCESSOR
>>
>> config XEN_MCE_LOG
>> bool "Xen platform mcelog"
>> - depends on XEN_DOM0 && X86_64 && X86_MCE
>> + depends on XEN_DOM0 && X86 && X86_MCE
>
> Can we have X86_MCE without X86?
I don't think we can. Is this a request to drop the middle
operand altogether?
Jan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Xen-devel] [PATCH 3/3] xen/mcelog: also allow building for 32-bit kernels
2019-11-13 13:47 ` [Xen-devel] " Jan Beulich
@ 2019-11-13 14:06 ` Jürgen Groß
2019-11-14 7:54 ` Jürgen Groß
1 sibling, 0 replies; 13+ messages in thread
From: Jürgen Groß @ 2019-11-13 14:06 UTC (permalink / raw)
To: Jan Beulich, Boris Ostrovsky; +Cc: xen-devel, lkml
On 13.11.19 14:47, Jan Beulich wrote:
> On 13.11.2019 01:15, Boris Ostrovsky wrote:
>> On 11/11/19 9:46 AM, Jan Beulich wrote:
>>> There's no apparent reason why it can be used on 64-bit only.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> --- a/drivers/xen/Kconfig
>>> +++ b/drivers/xen/Kconfig
>>> @@ -285,7 +285,7 @@ config XEN_ACPI_PROCESSOR
>>>
>>> config XEN_MCE_LOG
>>> bool "Xen platform mcelog"
>>> - depends on XEN_DOM0 && X86_64 && X86_MCE
>>> + depends on XEN_DOM0 && X86 && X86_MCE
>>
>> Can we have X86_MCE without X86?
>
> I don't think we can. Is this a request to drop the middle
> operand altogether?
I think this would be reasonable. X86_64 was an additional dependency,
while X86 isn't.
With the X86 removed:
Reviewed-by: Juergen Gross <jgross@suse.com>
Juergen
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Xen-devel] [PATCH 3/3] xen/mcelog: also allow building for 32-bit kernels
2019-11-13 13:47 ` [Xen-devel] " Jan Beulich
2019-11-13 14:06 ` Jürgen Groß
@ 2019-11-14 7:54 ` Jürgen Groß
1 sibling, 0 replies; 13+ messages in thread
From: Jürgen Groß @ 2019-11-14 7:54 UTC (permalink / raw)
To: Jan Beulich, Boris Ostrovsky; +Cc: xen-devel, lkml
On 13.11.19 14:47, Jan Beulich wrote:
> On 13.11.2019 01:15, Boris Ostrovsky wrote:
>> On 11/11/19 9:46 AM, Jan Beulich wrote:
>>> There's no apparent reason why it can be used on 64-bit only.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> --- a/drivers/xen/Kconfig
>>> +++ b/drivers/xen/Kconfig
>>> @@ -285,7 +285,7 @@ config XEN_ACPI_PROCESSOR
>>>
>>> config XEN_MCE_LOG
>>> bool "Xen platform mcelog"
>>> - depends on XEN_DOM0 && X86_64 && X86_MCE
>>> + depends on XEN_DOM0 && X86 && X86_MCE
>>
>> Can we have X86_MCE without X86?
>
> I don't think we can. Is this a request to drop the middle
> operand altogether?
No need to resend the series. I can make this change while committing.
Juergen
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/3] xen/mcelog: assorted adjustments
2019-11-11 14:43 [PATCH 0/3] xen/mcelog: assorted adjustments Jan Beulich
` (2 preceding siblings ...)
2019-11-11 14:46 ` [PATCH 3/3] xen/mcelog: also allow building for 32-bit kernels Jan Beulich
@ 2019-11-14 13:05 ` Jürgen Groß
3 siblings, 0 replies; 13+ messages in thread
From: Jürgen Groß @ 2019-11-14 13:05 UTC (permalink / raw)
To: Jan Beulich, Boris Ostrovsky; +Cc: xen-devel, lkml
On 11.11.19 15:43, Jan Beulich wrote:
> The 1st change is simple cleanup, noticed while preparing for the
> 2nd patch, which presents the consumer of the interface extension
> proposed in
> https://lists.xenproject.org/archives/html/xen-devel/2019-11/msg00377.html.
> The 3rd patch is sort of optional, considering that 32-bit Xen
> support is slated to be phased out of the kernel.
>
> 1: drop __MC_MSR_MCGCAP
> 2: add PPIN to record when available
> 3: also allow building for 32-bit kernels
Pushed the series to xen/tip.git for-linus-5.5a
Juergen
^ permalink raw reply [flat|nested] 13+ messages in thread