* [PATCH V4.1] mcheck, vmce: Allow vmce_amd_* functions to handle AMD thresolding MSRs
@ 2014-02-12 23:26 Aravind Gopalakrishnan
2014-02-13 8:38 ` Jan Beulich
2014-02-18 4:25 ` Liu, Jinsong
0 siblings, 2 replies; 14+ messages in thread
From: Aravind Gopalakrishnan @ 2014-02-12 23:26 UTC (permalink / raw)
To: chegger, jinsong.liu, suravee.suthikulpanit, boris.ostrovsky,
xen-devel, JBeulich
Cc: Aravind Gopalakrishnan
vmce_amd_[rd|wr]msr functions can handle accesses to AMD thresholding
registers. But due to this statement here:
switch ( msr & (MSR_IA32_MC0_CTL | 3) )
we are wrongly masking off top two bits which meant the register
accesses never made it to vmce_amd_* functions.
Corrected this problem by modifying the mask in this patch to allow
AMD thresholding registers to fall to 'default' case which in turn
allows vmce_amd_* functions to handle access to the registers.
While at it, remove some clutter in the vmce_amd* functions. Retained
current policy of returning zero for reads and ignoring writes.
Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
---
xen/arch/x86/cpu/mcheck/amd_f10.c | 41 ++++++-------------------------------
xen/arch/x86/cpu/mcheck/vmce.c | 14 +++++++++++--
2 files changed, 18 insertions(+), 37 deletions(-)
diff --git a/xen/arch/x86/cpu/mcheck/amd_f10.c b/xen/arch/x86/cpu/mcheck/amd_f10.c
index 61319dc..03797ab 100644
--- a/xen/arch/x86/cpu/mcheck/amd_f10.c
+++ b/xen/arch/x86/cpu/mcheck/amd_f10.c
@@ -105,43 +105,14 @@ enum mcheck_type amd_f10_mcheck_init(struct cpuinfo_x86 *c)
/* amd specific MCA MSR */
int vmce_amd_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
{
- switch (msr) {
- case MSR_F10_MC4_MISC1: /* DRAM error type */
- v->arch.vmce.bank[1].mci_misc = val;
- mce_printk(MCE_VERBOSE, "MCE: wr msr %#"PRIx64"\n", val);
- break;
- case MSR_F10_MC4_MISC2: /* Link error type */
- case MSR_F10_MC4_MISC3: /* L3 cache error type */
- /* ignore write: we do not emulate link and l3 cache errors
- * to the guest.
- */
- mce_printk(MCE_VERBOSE, "MCE: wr msr %#"PRIx64"\n", val);
- break;
- default:
- return 0;
- }
-
- return 1;
+ /* Do nothing as we don't emulate this MC bank currently */
+ mce_printk(MCE_VERBOSE, "MCE: wr msr %#"PRIx64"\n", val);
+ return 1;
}
int vmce_amd_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
{
- switch (msr) {
- case MSR_F10_MC4_MISC1: /* DRAM error type */
- *val = v->arch.vmce.bank[1].mci_misc;
- mce_printk(MCE_VERBOSE, "MCE: rd msr %#"PRIx64"\n", *val);
- break;
- case MSR_F10_MC4_MISC2: /* Link error type */
- case MSR_F10_MC4_MISC3: /* L3 cache error type */
- /* we do not emulate link and l3 cache
- * errors to the guest.
- */
- *val = 0;
- mce_printk(MCE_VERBOSE, "MCE: rd msr %#"PRIx64"\n", *val);
- break;
- default:
- return 0;
- }
-
- return 1;
+ /* Assign '0' as we don't emulate this MC bank currently */
+ *val = 0;
+ return 1;
}
diff --git a/xen/arch/x86/cpu/mcheck/vmce.c b/xen/arch/x86/cpu/mcheck/vmce.c
index f6c35db..84843fc 100644
--- a/xen/arch/x86/cpu/mcheck/vmce.c
+++ b/xen/arch/x86/cpu/mcheck/vmce.c
@@ -107,7 +107,8 @@ static int bank_mce_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
*val = 0;
- switch ( msr & (MSR_IA32_MC0_CTL | 3) )
+ /* Allow only first 3 MC banks into switch() */
+ switch ( msr & (-MSR_IA32_MC0_CTL | 3) )
{
case MSR_IA32_MC0_CTL:
/* stick all 1's to MCi_CTL */
@@ -148,6 +149,10 @@ static int bank_mce_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
ret = vmce_intel_rdmsr(v, msr, val);
break;
case X86_VENDOR_AMD:
+ /*
+ * Extended block of AMD thresholding registers fall into default.
+ * Handle reads here.
+ */
ret = vmce_amd_rdmsr(v, msr, val);
break;
default:
@@ -210,7 +215,8 @@ static int bank_mce_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
int ret = 1;
unsigned int bank = (msr - MSR_IA32_MC0_CTL) / 4;
- switch ( msr & (MSR_IA32_MC0_CTL | 3) )
+ /* Allow only first 3 MC banks into switch() */
+ switch ( msr & (-MSR_IA32_MC0_CTL | 3) )
{
case MSR_IA32_MC0_CTL:
/*
@@ -246,6 +252,10 @@ static int bank_mce_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
ret = vmce_intel_wrmsr(v, msr, val);
break;
case X86_VENDOR_AMD:
+ /*
+ * Extended block of AMD thresholding registers fall into default.
+ * Handle writes here.
+ */
ret = vmce_amd_wrmsr(v, msr, val);
break;
default:
--
1.7.9.5
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH V4.1] mcheck, vmce: Allow vmce_amd_* functions to handle AMD thresolding MSRs
2014-02-12 23:26 [PATCH V4.1] mcheck, vmce: Allow vmce_amd_* functions to handle AMD thresolding MSRs Aravind Gopalakrishnan
@ 2014-02-13 8:38 ` Jan Beulich
2014-02-13 17:27 ` Aravind Gopalakrishnan
2014-02-18 4:25 ` Liu, Jinsong
1 sibling, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2014-02-13 8:38 UTC (permalink / raw)
To: Aravind Gopalakrishnan
Cc: jinsong.liu, boris.ostrovsky, chegger, suravee.suthikulpanit, xen-devel
>>> On 13.02.14 at 00:26, Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com> wrote:
> --- a/xen/arch/x86/cpu/mcheck/vmce.c
> +++ b/xen/arch/x86/cpu/mcheck/vmce.c
> @@ -107,7 +107,8 @@ static int bank_mce_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>
> *val = 0;
>
> - switch ( msr & (MSR_IA32_MC0_CTL | 3) )
> + /* Allow only first 3 MC banks into switch() */
> + switch ( msr & (-MSR_IA32_MC0_CTL | 3) )
> {
> case MSR_IA32_MC0_CTL:
> /* stick all 1's to MCi_CTL */
I'm confused: You now add a comment as if the mask was including
bit 4, which it doesn't. What am I missing?
Also, please get used to mention (commonly at the bottom of the
commit message, after a --- separator) what changed compared to
the previous iteration.
Jan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V4.1] mcheck, vmce: Allow vmce_amd_* functions to handle AMD thresolding MSRs
2014-02-13 8:38 ` Jan Beulich
@ 2014-02-13 17:27 ` Aravind Gopalakrishnan
2014-02-13 18:24 ` Aravind Gopalakrishnan
0 siblings, 1 reply; 14+ messages in thread
From: Aravind Gopalakrishnan @ 2014-02-13 17:27 UTC (permalink / raw)
To: Jan Beulich
Cc: jinsong.liu, boris.ostrovsky, chegger, suravee.suthikulpanit, xen-devel
On 2/13/2014 2:38 AM, Jan Beulich wrote:
>> *val = 0;
>>
>> - switch ( msr & (MSR_IA32_MC0_CTL | 3) )
>> + /* Allow only first 3 MC banks into switch() */
>> + switch ( msr & (-MSR_IA32_MC0_CTL | 3) )
>> {
>> case MSR_IA32_MC0_CTL:
>> /* stick all 1's to MCi_CTL */
> I'm confused: You now add a comment as if the mask was including
> bit 4, which it doesn't. What am I missing?
Darn. Sorry about that. Will fix..
> Also, please get used to mention (commonly at the bottom of the
> commit message, after a --- separator) what changed compared to
> the previous iteration.
>
>
Okay, will do.
-Aravind.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V4.1] mcheck, vmce: Allow vmce_amd_* functions to handle AMD thresolding MSRs
2014-02-13 17:27 ` Aravind Gopalakrishnan
@ 2014-02-13 18:24 ` Aravind Gopalakrishnan
2014-02-14 8:28 ` Jan Beulich
0 siblings, 1 reply; 14+ messages in thread
From: Aravind Gopalakrishnan @ 2014-02-13 18:24 UTC (permalink / raw)
To: Jan Beulich
Cc: jinsong.liu, boris.ostrovsky, chegger, suravee.suthikulpanit, xen-devel
On 2/13/2014 11:27 AM, Aravind Gopalakrishnan wrote:
> On 2/13/2014 2:38 AM, Jan Beulich wrote:
>>> *val = 0;
>>> - switch ( msr & (MSR_IA32_MC0_CTL | 3) )
>>> + /* Allow only first 3 MC banks into switch() */
>>> + switch ( msr & (-MSR_IA32_MC0_CTL | 3) )
>>> {
>>> case MSR_IA32_MC0_CTL:
>>> /* stick all 1's to MCi_CTL */
>> I'm confused: You now add a comment as if the mask was including
>> bit 4, which it doesn't. What am I missing?
>
> Darn. Sorry about that. Will fix..
Jan,
Do let me know if the following wording is fine:
/*
* Apply mask to allow bits[0:1] (necessary to uniquely identify MC0)
* MC1 is handled by virtue of 'bank' value.
*/
If not, I'm open to suggestions:)
Thanks,
-Aravind.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V4.1] mcheck, vmce: Allow vmce_amd_* functions to handle AMD thresolding MSRs
2014-02-13 18:24 ` Aravind Gopalakrishnan
@ 2014-02-14 8:28 ` Jan Beulich
2014-02-16 14:47 ` Liu, Jinsong
0 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2014-02-14 8:28 UTC (permalink / raw)
To: chegger, Aravind Gopalakrishnan
Cc: jinsong.liu, boris.ostrovsky, suravee.suthikulpanit, xen-devel
>>> On 13.02.14 at 19:24, Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
wrote:
> On 2/13/2014 11:27 AM, Aravind Gopalakrishnan wrote:
>> On 2/13/2014 2:38 AM, Jan Beulich wrote:
>>>> *val = 0;
>>>> - switch ( msr & (MSR_IA32_MC0_CTL | 3) )
>>>> + /* Allow only first 3 MC banks into switch() */
>>>> + switch ( msr & (-MSR_IA32_MC0_CTL | 3) )
>>>> {
>>>> case MSR_IA32_MC0_CTL:
>>>> /* stick all 1's to MCi_CTL */
>>> I'm confused: You now add a comment as if the mask was including
>>> bit 4, which it doesn't. What am I missing?
>>
>> Darn. Sorry about that. Will fix..
>
> Jan,
>
> Do let me know if the following wording is fine:
>
> /*
> * Apply mask to allow bits[0:1] (necessary to uniquely identify MC0)
> * MC1 is handled by virtue of 'bank' value.
> */
>
> If not, I'm open to suggestions:)
I don't particularly like this, but I also don't have a good alternative
suggestion. It was Christoph who asked for a comment in the first
place. Since I don't see a particular need for a comment here, you
two should work out what best suits both of you.
Jan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V4.1] mcheck, vmce: Allow vmce_amd_* functions to handle AMD thresolding MSRs
2014-02-14 8:28 ` Jan Beulich
@ 2014-02-16 14:47 ` Liu, Jinsong
0 siblings, 0 replies; 14+ messages in thread
From: Liu, Jinsong @ 2014-02-16 14:47 UTC (permalink / raw)
To: Jan Beulich, chegger, Aravind Gopalakrishnan
Cc: boris.ostrovsky, suravee.suthikulpanit, xen-devel
Sorry, Jan and Aravind, I just return from long Chinese Spring Festival vacation. I will review the thread ASAP.
Thanks,
Jinsong
Jan Beulich wrote:
>>>> On 13.02.14 at 19:24, Aravind Gopalakrishnan
>>>> <aravind.gopalakrishnan@amd.com>
> wrote:
>> On 2/13/2014 11:27 AM, Aravind Gopalakrishnan wrote:
>>> On 2/13/2014 2:38 AM, Jan Beulich wrote:
>>>>> *val = 0;
>>>>> - switch ( msr & (MSR_IA32_MC0_CTL | 3) )
>>>>> + /* Allow only first 3 MC banks into switch() */
>>>>> + switch ( msr & (-MSR_IA32_MC0_CTL | 3) )
>>>>> {
>>>>> case MSR_IA32_MC0_CTL:
>>>>> /* stick all 1's to MCi_CTL */
>>>> I'm confused: You now add a comment as if the mask was including
>>>> bit 4, which it doesn't. What am I missing?
>>>
>>> Darn. Sorry about that. Will fix..
>>
>> Jan,
>>
>> Do let me know if the following wording is fine:
>>
>> /*
>> * Apply mask to allow bits[0:1] (necessary to uniquely identify
>> MC0)
>> * MC1 is handled by virtue of 'bank' value.
>> */
>>
>> If not, I'm open to suggestions:)
>
> I don't particularly like this, but I also don't have a good
> alternative suggestion. It was Christoph who asked for a comment in
> the first place. Since I don't see a particular need for a comment
> here, you
> two should work out what best suits both of you.
>
> Jan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V4.1] mcheck, vmce: Allow vmce_amd_* functions to handle AMD thresolding MSRs
2014-02-12 23:26 [PATCH V4.1] mcheck, vmce: Allow vmce_amd_* functions to handle AMD thresolding MSRs Aravind Gopalakrishnan
2014-02-13 8:38 ` Jan Beulich
@ 2014-02-18 4:25 ` Liu, Jinsong
2014-02-18 9:41 ` Jan Beulich
1 sibling, 1 reply; 14+ messages in thread
From: Liu, Jinsong @ 2014-02-18 4:25 UTC (permalink / raw)
To: Aravind Gopalakrishnan, chegger, suravee.suthikulpanit,
boris.ostrovsky, xen-devel, JBeulich
For Intel it didn't disturb Intel's vmce logic so it's OK for me.
For AMD, c000_040x is bank4 (MC4_MISCj), while vmce current only support bank0/1. Even considering in the future AMD may add MC0/1_MISCj, it doesn't need emulation (say, read return 0 and write ignore). So how about simply filter out AMD MCx_MISCj at mce_vendor_bank_msr()?
Thanks,
Jinsong
Aravind Gopalakrishnan wrote:
> vmce_amd_[rd|wr]msr functions can handle accesses to AMD thresholding
> registers. But due to this statement here:
> switch ( msr & (MSR_IA32_MC0_CTL | 3) )
> we are wrongly masking off top two bits which meant the register
> accesses never made it to vmce_amd_* functions.
>
> Corrected this problem by modifying the mask in this patch to allow
> AMD thresholding registers to fall to 'default' case which in turn
> allows vmce_amd_* functions to handle access to the registers.
>
> While at it, remove some clutter in the vmce_amd* functions. Retained
> current policy of returning zero for reads and ignoring writes.
>
> Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
> ---
> xen/arch/x86/cpu/mcheck/amd_f10.c | 41
> ++++++------------------------------- xen/arch/x86/cpu/mcheck/vmce.c
> | 14 +++++++++++-- 2 files changed, 18 insertions(+), 37
> deletions(-)
>
> diff --git a/xen/arch/x86/cpu/mcheck/amd_f10.c
> b/xen/arch/x86/cpu/mcheck/amd_f10.c
> index 61319dc..03797ab 100644
> --- a/xen/arch/x86/cpu/mcheck/amd_f10.c
> +++ b/xen/arch/x86/cpu/mcheck/amd_f10.c
> @@ -105,43 +105,14 @@ enum mcheck_type amd_f10_mcheck_init(struct
> cpuinfo_x86 *c) /* amd specific MCA MSR */
> int vmce_amd_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
> {
> - switch (msr) {
> - case MSR_F10_MC4_MISC1: /* DRAM error type */
> - v->arch.vmce.bank[1].mci_misc = val;
> - mce_printk(MCE_VERBOSE, "MCE: wr msr %#"PRIx64"\n", val);
> - break;
> - case MSR_F10_MC4_MISC2: /* Link error type */
> - case MSR_F10_MC4_MISC3: /* L3 cache error type */
> - /* ignore write: we do not emulate link and l3 cache errors
> - * to the guest.
> - */
> - mce_printk(MCE_VERBOSE, "MCE: wr msr %#"PRIx64"\n", val);
> - break;
> - default:
> - return 0;
> - }
> -
> - return 1;
> + /* Do nothing as we don't emulate this MC bank currently */
> + mce_printk(MCE_VERBOSE, "MCE: wr msr %#"PRIx64"\n", val);
> + return 1;
> }
>
> int vmce_amd_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
> {
> - switch (msr) {
> - case MSR_F10_MC4_MISC1: /* DRAM error type */
> - *val = v->arch.vmce.bank[1].mci_misc;
> - mce_printk(MCE_VERBOSE, "MCE: rd msr %#"PRIx64"\n", *val);
> - break;
> - case MSR_F10_MC4_MISC2: /* Link error type */
> - case MSR_F10_MC4_MISC3: /* L3 cache error type */
> - /* we do not emulate link and l3 cache
> - * errors to the guest.
> - */
> - *val = 0;
> - mce_printk(MCE_VERBOSE, "MCE: rd msr %#"PRIx64"\n", *val);
> - break;
> - default:
> - return 0;
> - }
> -
> - return 1;
> + /* Assign '0' as we don't emulate this MC bank currently */
> + *val = 0;
> + return 1;
> }
> diff --git a/xen/arch/x86/cpu/mcheck/vmce.c
> b/xen/arch/x86/cpu/mcheck/vmce.c
> index f6c35db..84843fc 100644
> --- a/xen/arch/x86/cpu/mcheck/vmce.c
> +++ b/xen/arch/x86/cpu/mcheck/vmce.c
> @@ -107,7 +107,8 @@ static int bank_mce_rdmsr(const struct vcpu *v,
> uint32_t msr, uint64_t *val)
>
> *val = 0;
>
> - switch ( msr & (MSR_IA32_MC0_CTL | 3) )
> + /* Allow only first 3 MC banks into switch() */
> + switch ( msr & (-MSR_IA32_MC0_CTL | 3) )
> {
> case MSR_IA32_MC0_CTL:
> /* stick all 1's to MCi_CTL */
> @@ -148,6 +149,10 @@ static int bank_mce_rdmsr(const struct vcpu *v,
> uint32_t msr, uint64_t *val) ret = vmce_intel_rdmsr(v,
> msr, val); break;
> case X86_VENDOR_AMD:
> + /*
> + * Extended block of AMD thresholding registers fall
> into default. + * Handle reads here.
> + */
> ret = vmce_amd_rdmsr(v, msr, val);
> break;
> default:
> @@ -210,7 +215,8 @@ static int bank_mce_wrmsr(struct vcpu *v,
> uint32_t msr, uint64_t val) int ret = 1;
> unsigned int bank = (msr - MSR_IA32_MC0_CTL) / 4;
>
> - switch ( msr & (MSR_IA32_MC0_CTL | 3) )
> + /* Allow only first 3 MC banks into switch() */
> + switch ( msr & (-MSR_IA32_MC0_CTL | 3) )
> {
> case MSR_IA32_MC0_CTL:
> /*
> @@ -246,6 +252,10 @@ static int bank_mce_wrmsr(struct vcpu *v,
> uint32_t msr, uint64_t val) ret = vmce_intel_wrmsr(v,
> msr, val); break;
> case X86_VENDOR_AMD:
> + /*
> + * Extended block of AMD thresholding registers fall
> into default. + * Handle writes here.
> + */
> ret = vmce_amd_wrmsr(v, msr, val);
> break;
> default:
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V4.1] mcheck, vmce: Allow vmce_amd_* functions to handle AMD thresolding MSRs
2014-02-18 4:25 ` Liu, Jinsong
@ 2014-02-18 9:41 ` Jan Beulich
2014-02-18 10:52 ` Liu, Jinsong
0 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2014-02-18 9:41 UTC (permalink / raw)
To: Jinsong Liu
Cc: boris.ostrovsky, chegger, Aravind Gopalakrishnan,
suravee.suthikulpanit, xen-devel
>>> On 18.02.14 at 05:25, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:
> For Intel it didn't disturb Intel's vmce logic so it's OK for me.
>
> For AMD, c000_040x is bank4 (MC4_MISCj), while vmce current only support
> bank0/1. Even considering in the future AMD may add MC0/1_MISCj, it doesn't
> need emulation (say, read return 0 and write ignore). So how about simply
> filter out AMD MCx_MISCj at mce_vendor_bank_msr()?
mce_vendor_bank_msr() already has
case X86_VENDOR_AMD:
switch (msr) {
case MSR_F10_MC4_MISC1:
case MSR_F10_MC4_MISC2:
case MSR_F10_MC4_MISC3:
return 1;
}
Jan
> Aravind Gopalakrishnan wrote:
>> vmce_amd_[rd|wr]msr functions can handle accesses to AMD thresholding
>> registers. But due to this statement here:
>> switch ( msr & (MSR_IA32_MC0_CTL | 3) )
>> we are wrongly masking off top two bits which meant the register
>> accesses never made it to vmce_amd_* functions.
>>
>> Corrected this problem by modifying the mask in this patch to allow
>> AMD thresholding registers to fall to 'default' case which in turn
>> allows vmce_amd_* functions to handle access to the registers.
>>
>> While at it, remove some clutter in the vmce_amd* functions. Retained
>> current policy of returning zero for reads and ignoring writes.
>>
>> Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
>> ---
>> xen/arch/x86/cpu/mcheck/amd_f10.c | 41
>> ++++++------------------------------- xen/arch/x86/cpu/mcheck/vmce.c
>> | 14 +++++++++++-- 2 files changed, 18 insertions(+), 37
>> deletions(-)
>>
>> diff --git a/xen/arch/x86/cpu/mcheck/amd_f10.c
>> b/xen/arch/x86/cpu/mcheck/amd_f10.c
>> index 61319dc..03797ab 100644
>> --- a/xen/arch/x86/cpu/mcheck/amd_f10.c
>> +++ b/xen/arch/x86/cpu/mcheck/amd_f10.c
>> @@ -105,43 +105,14 @@ enum mcheck_type amd_f10_mcheck_init(struct
>> cpuinfo_x86 *c) /* amd specific MCA MSR */
>> int vmce_amd_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
>> {
>> - switch (msr) {
>> - case MSR_F10_MC4_MISC1: /* DRAM error type */
>> - v->arch.vmce.bank[1].mci_misc = val;
>> - mce_printk(MCE_VERBOSE, "MCE: wr msr %#"PRIx64"\n", val);
>> - break;
>> - case MSR_F10_MC4_MISC2: /* Link error type */
>> - case MSR_F10_MC4_MISC3: /* L3 cache error type */
>> - /* ignore write: we do not emulate link and l3 cache errors
>> - * to the guest.
>> - */
>> - mce_printk(MCE_VERBOSE, "MCE: wr msr %#"PRIx64"\n", val);
>> - break;
>> - default:
>> - return 0;
>> - }
>> -
>> - return 1;
>> + /* Do nothing as we don't emulate this MC bank currently */
>> + mce_printk(MCE_VERBOSE, "MCE: wr msr %#"PRIx64"\n", val);
>> + return 1;
>> }
>>
>> int vmce_amd_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
>> {
>> - switch (msr) {
>> - case MSR_F10_MC4_MISC1: /* DRAM error type */
>> - *val = v->arch.vmce.bank[1].mci_misc;
>> - mce_printk(MCE_VERBOSE, "MCE: rd msr %#"PRIx64"\n", *val);
>> - break;
>> - case MSR_F10_MC4_MISC2: /* Link error type */
>> - case MSR_F10_MC4_MISC3: /* L3 cache error type */
>> - /* we do not emulate link and l3 cache
>> - * errors to the guest.
>> - */
>> - *val = 0;
>> - mce_printk(MCE_VERBOSE, "MCE: rd msr %#"PRIx64"\n", *val);
>> - break;
>> - default:
>> - return 0;
>> - }
>> -
>> - return 1;
>> + /* Assign '0' as we don't emulate this MC bank currently */
>> + *val = 0;
>> + return 1;
>> }
>> diff --git a/xen/arch/x86/cpu/mcheck/vmce.c
>> b/xen/arch/x86/cpu/mcheck/vmce.c
>> index f6c35db..84843fc 100644
>> --- a/xen/arch/x86/cpu/mcheck/vmce.c
>> +++ b/xen/arch/x86/cpu/mcheck/vmce.c
>> @@ -107,7 +107,8 @@ static int bank_mce_rdmsr(const struct vcpu *v,
>> uint32_t msr, uint64_t *val)
>>
>> *val = 0;
>>
>> - switch ( msr & (MSR_IA32_MC0_CTL | 3) )
>> + /* Allow only first 3 MC banks into switch() */
>> + switch ( msr & (-MSR_IA32_MC0_CTL | 3) )
>> {
>> case MSR_IA32_MC0_CTL:
>> /* stick all 1's to MCi_CTL */
>> @@ -148,6 +149,10 @@ static int bank_mce_rdmsr(const struct vcpu *v,
>> uint32_t msr, uint64_t *val) ret = vmce_intel_rdmsr(v,
>> msr, val); break;
>> case X86_VENDOR_AMD:
>> + /*
>> + * Extended block of AMD thresholding registers fall
>> into default. + * Handle reads here.
>> + */
>> ret = vmce_amd_rdmsr(v, msr, val);
>> break;
>> default:
>> @@ -210,7 +215,8 @@ static int bank_mce_wrmsr(struct vcpu *v,
>> uint32_t msr, uint64_t val) int ret = 1;
>> unsigned int bank = (msr - MSR_IA32_MC0_CTL) / 4;
>>
>> - switch ( msr & (MSR_IA32_MC0_CTL | 3) )
>> + /* Allow only first 3 MC banks into switch() */
>> + switch ( msr & (-MSR_IA32_MC0_CTL | 3) )
>> {
>> case MSR_IA32_MC0_CTL:
>> /*
>> @@ -246,6 +252,10 @@ static int bank_mce_wrmsr(struct vcpu *v,
>> uint32_t msr, uint64_t val) ret = vmce_intel_wrmsr(v,
>> msr, val); break;
>> case X86_VENDOR_AMD:
>> + /*
>> + * Extended block of AMD thresholding registers fall
>> into default. + * Handle writes here.
>> + */
>> ret = vmce_amd_wrmsr(v, msr, val);
>> break;
>> default:
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V4.1] mcheck, vmce: Allow vmce_amd_* functions to handle AMD thresolding MSRs
2014-02-18 9:41 ` Jan Beulich
@ 2014-02-18 10:52 ` Liu, Jinsong
2014-02-18 11:02 ` Jan Beulich
0 siblings, 1 reply; 14+ messages in thread
From: Liu, Jinsong @ 2014-02-18 10:52 UTC (permalink / raw)
To: Jan Beulich
Cc: boris.ostrovsky, chegger, Aravind Gopalakrishnan,
suravee.suthikulpanit, xen-devel
Jan Beulich wrote:
>>>> On 18.02.14 at 05:25, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:
>> For Intel it didn't disturb Intel's vmce logic so it's OK for me.
>>
>> For AMD, c000_040x is bank4 (MC4_MISCj), while vmce current only
>> support bank0/1. Even considering in the future AMD may add
>> MC0/1_MISCj, it doesn't need emulation (say, read return 0 and write
>> ignore). So how about simply filter out AMD MCx_MISCj at
>> mce_vendor_bank_msr()?
>
> mce_vendor_bank_msr() already has
>
> case X86_VENDOR_AMD:
> switch (msr) {
> case MSR_F10_MC4_MISC1:
> case MSR_F10_MC4_MISC2:
> case MSR_F10_MC4_MISC3:
> return 1;
> }
>
> Jan
OK.
>
>> Aravind Gopalakrishnan wrote:
>>> vmce_amd_[rd|wr]msr functions can handle accesses to AMD
>>> thresholding registers. But due to this statement here:
>>> switch ( msr & (MSR_IA32_MC0_CTL | 3) )
>>> we are wrongly masking off top two bits which meant the register
>>> accesses never made it to vmce_amd_* functions.
>>>
>>> Corrected this problem by modifying the mask in this patch to allow
>>> AMD thresholding registers to fall to 'default' case which in turn
>>> allows vmce_amd_* functions to handle access to the registers.
>>>
>>> While at it, remove some clutter in the vmce_amd* functions.
>>> Retained current policy of returning zero for reads and ignoring
>>> writes.
>>>
>>> Signed-off-by: Aravind Gopalakrishnan
>>> <aravind.gopalakrishnan@amd.com> ---
>>> xen/arch/x86/cpu/mcheck/amd_f10.c | 41
>>> ++++++-------------------------------
>>> xen/arch/x86/cpu/mcheck/vmce.c | 14 +++++++++++-- 2 files
>>> changed, 18 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/cpu/mcheck/amd_f10.c
>>> b/xen/arch/x86/cpu/mcheck/amd_f10.c
>>> index 61319dc..03797ab 100644
>>> --- a/xen/arch/x86/cpu/mcheck/amd_f10.c
>>> +++ b/xen/arch/x86/cpu/mcheck/amd_f10.c
>>> @@ -105,43 +105,14 @@ enum mcheck_type amd_f10_mcheck_init(struct
>>> cpuinfo_x86 *c) /* amd specific MCA MSR */
>>> int vmce_amd_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val) {
>>> - switch (msr) {
>>> - case MSR_F10_MC4_MISC1: /* DRAM error type */
>>> - v->arch.vmce.bank[1].mci_misc = val;
>>> - mce_printk(MCE_VERBOSE, "MCE: wr msr %#"PRIx64"\n", val);
>>> - break;
>>> - case MSR_F10_MC4_MISC2: /* Link error type */
>>> - case MSR_F10_MC4_MISC3: /* L3 cache error type */
>>> - /* ignore write: we do not emulate link and l3 cache errors
>>> - * to the guest.
>>> - */
>>> - mce_printk(MCE_VERBOSE, "MCE: wr msr %#"PRIx64"\n", val);
>>> - break;
>>> - default:
>>> - return 0;
>>> - }
>>> -
>>> - return 1;
>>> + /* Do nothing as we don't emulate this MC bank currently */
>>> + mce_printk(MCE_VERBOSE, "MCE: wr msr %#"PRIx64"\n", val); +
>>> return 1; }
>>>
>>> int vmce_amd_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t
>>> *val) {
>>> - switch (msr) {
>>> - case MSR_F10_MC4_MISC1: /* DRAM error type */
>>> - *val = v->arch.vmce.bank[1].mci_misc;
>>> - mce_printk(MCE_VERBOSE, "MCE: rd msr %#"PRIx64"\n", *val);
>>> - break;
>>> - case MSR_F10_MC4_MISC2: /* Link error type */
>>> - case MSR_F10_MC4_MISC3: /* L3 cache error type */
>>> - /* we do not emulate link and l3 cache
>>> - * errors to the guest.
>>> - */
>>> - *val = 0;
>>> - mce_printk(MCE_VERBOSE, "MCE: rd msr %#"PRIx64"\n", *val);
>>> - break;
>>> - default:
>>> - return 0;
>>> - }
>>> -
>>> - return 1;
>>> + /* Assign '0' as we don't emulate this MC bank currently */ +
>>> *val = 0; + return 1;
>>> }
>>> diff --git a/xen/arch/x86/cpu/mcheck/vmce.c
>>> b/xen/arch/x86/cpu/mcheck/vmce.c
>>> index f6c35db..84843fc 100644
>>> --- a/xen/arch/x86/cpu/mcheck/vmce.c
>>> +++ b/xen/arch/x86/cpu/mcheck/vmce.c
>>> @@ -107,7 +107,8 @@ static int bank_mce_rdmsr(const struct vcpu *v,
>>> uint32_t msr, uint64_t *val)
>>>
>>> *val = 0;
>>>
>>> - switch ( msr & (MSR_IA32_MC0_CTL | 3) )
>>> + /* Allow only first 3 MC banks into switch() */
I don't think this comments is good here. Remove it is better.
Thanks,
Jinsong
>>> + switch ( msr & (-MSR_IA32_MC0_CTL | 3) )
>>> {
>>> case MSR_IA32_MC0_CTL:
>>> /* stick all 1's to MCi_CTL */
>>> @@ -148,6 +149,10 @@ static int bank_mce_rdmsr(const struct vcpu *v,
>>> uint32_t msr, uint64_t *val) ret = vmce_intel_rdmsr(v,
>>> msr, val); break;
>>> case X86_VENDOR_AMD:
>>> + /*
>>> + * Extended block of AMD thresholding registers fall
>>> into default. + * Handle reads here.
>>> + */
>>> ret = vmce_amd_rdmsr(v, msr, val);
>>> break;
>>> default:
>>> @@ -210,7 +215,8 @@ static int bank_mce_wrmsr(struct vcpu *v,
>>> uint32_t msr, uint64_t val) int ret = 1;
>>> unsigned int bank = (msr - MSR_IA32_MC0_CTL) / 4;
>>>
>>> - switch ( msr & (MSR_IA32_MC0_CTL | 3) )
>>> + /* Allow only first 3 MC banks into switch() */
>>> + switch ( msr & (-MSR_IA32_MC0_CTL | 3) )
>>> {
>>> case MSR_IA32_MC0_CTL:
>>> /*
>>> @@ -246,6 +252,10 @@ static int bank_mce_wrmsr(struct vcpu *v,
>>> uint32_t msr, uint64_t val) ret = vmce_intel_wrmsr(v,
>>> msr, val); break;
>>> case X86_VENDOR_AMD:
>>> + /*
>>> + * Extended block of AMD thresholding registers fall
>>> into default. + * Handle writes here.
>>> + */
>>> ret = vmce_amd_wrmsr(v, msr, val);
>>> break;
>>> default:
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V4.1] mcheck, vmce: Allow vmce_amd_* functions to handle AMD thresolding MSRs
2014-02-18 10:52 ` Liu, Jinsong
@ 2014-02-18 11:02 ` Jan Beulich
2014-02-18 11:42 ` Liu, Jinsong
0 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2014-02-18 11:02 UTC (permalink / raw)
To: Jinsong Liu
Cc: boris.ostrovsky, chegger, Aravind Gopalakrishnan,
suravee.suthikulpanit, xen-devel
>>> On 18.02.14 at 11:52, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:
>>>> --- a/xen/arch/x86/cpu/mcheck/vmce.c
>>>> +++ b/xen/arch/x86/cpu/mcheck/vmce.c
>>>> @@ -107,7 +107,8 @@ static int bank_mce_rdmsr(const struct vcpu *v,
>>>> uint32_t msr, uint64_t *val)
>>>>
>>>> *val = 0;
>>>>
>>>> - switch ( msr & (MSR_IA32_MC0_CTL | 3) )
>>>> + /* Allow only first 3 MC banks into switch() */
>
> I don't think this comments is good here. Remove it is better.
I had asked for this to be removed again too. I'm really thinking
that V3 is what we should go with.
Jan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V4.1] mcheck, vmce: Allow vmce_amd_* functions to handle AMD thresolding MSRs
2014-02-18 11:02 ` Jan Beulich
@ 2014-02-18 11:42 ` Liu, Jinsong
2014-02-18 12:36 ` Jan Beulich
0 siblings, 1 reply; 14+ messages in thread
From: Liu, Jinsong @ 2014-02-18 11:42 UTC (permalink / raw)
To: Jan Beulich
Cc: boris.ostrovsky, chegger, Aravind Gopalakrishnan,
suravee.suthikulpanit, xen-devel
Jan Beulich wrote:
>>>> On 18.02.14 at 11:52, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:
>>>>> --- a/xen/arch/x86/cpu/mcheck/vmce.c
>>>>> +++ b/xen/arch/x86/cpu/mcheck/vmce.c
>>>>> @@ -107,7 +107,8 @@ static int bank_mce_rdmsr(const struct vcpu
>>>>> *v, uint32_t msr, uint64_t *val)
>>>>>
>>>>> *val = 0;
>>>>>
>>>>> - switch ( msr & (MSR_IA32_MC0_CTL | 3) )
>>>>> + /* Allow only first 3 MC banks into switch() */
>>
>> I don't think this comments is good here. Remove it is better.
>
> I had asked for this to be removed again too. I'm really thinking
> that V3 is what we should go with.
>
> Jan
V3 is fine, except adding comments for '-MSR_IA32_MC0_CTL' is slightly better.
Thanks,
Jinsong
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V4.1] mcheck, vmce: Allow vmce_amd_* functions to handle AMD thresolding MSRs
2014-02-18 11:42 ` Liu, Jinsong
@ 2014-02-18 12:36 ` Jan Beulich
2014-02-18 12:42 ` Liu, Jinsong
0 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2014-02-18 12:36 UTC (permalink / raw)
To: Jinsong Liu
Cc: boris.ostrovsky, chegger, Aravind Gopalakrishnan,
suravee.suthikulpanit, xen-devel
>>> On 18.02.14 at 12:42, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:
> Jan Beulich wrote:
>>>>> On 18.02.14 at 11:52, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:
>>>>>> --- a/xen/arch/x86/cpu/mcheck/vmce.c
>>>>>> +++ b/xen/arch/x86/cpu/mcheck/vmce.c
>>>>>> @@ -107,7 +107,8 @@ static int bank_mce_rdmsr(const struct vcpu
>>>>>> *v, uint32_t msr, uint64_t *val)
>>>>>>
>>>>>> *val = 0;
>>>>>>
>>>>>> - switch ( msr & (MSR_IA32_MC0_CTL | 3) )
>>>>>> + /* Allow only first 3 MC banks into switch() */
>>>
>>> I don't think this comments is good here. Remove it is better.
>>
>> I had asked for this to be removed again too. I'm really thinking
>> that V3 is what we should go with.
>
> V3 is fine, except adding comments for '-MSR_IA32_MC0_CTL' is slightly
> better.
Can I read this as an ack then (I already explained elsewhere
why I think a comment there is rather pointless)?
Jan
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V4.1] mcheck, vmce: Allow vmce_amd_* functions to handle AMD thresolding MSRs
2014-02-18 12:36 ` Jan Beulich
@ 2014-02-18 12:42 ` Liu, Jinsong
2014-02-18 16:23 ` Aravind Gopalakrishnan
0 siblings, 1 reply; 14+ messages in thread
From: Liu, Jinsong @ 2014-02-18 12:42 UTC (permalink / raw)
To: Jan Beulich
Cc: boris.ostrovsky, chegger, Aravind Gopalakrishnan,
suravee.suthikulpanit, xen-devel
Jan Beulich wrote:
>>>> On 18.02.14 at 12:42, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:
>> Jan Beulich wrote:
>>>>>> On 18.02.14 at 11:52, "Liu, Jinsong" <jinsong.liu@intel.com>
>>>>>> wrote:
>>>>>>> --- a/xen/arch/x86/cpu/mcheck/vmce.c
>>>>>>> +++ b/xen/arch/x86/cpu/mcheck/vmce.c
>>>>>>> @@ -107,7 +107,8 @@ static int bank_mce_rdmsr(const struct vcpu
>>>>>>> *v, uint32_t msr, uint64_t *val)
>>>>>>>
>>>>>>> *val = 0;
>>>>>>>
>>>>>>> - switch ( msr & (MSR_IA32_MC0_CTL | 3) )
>>>>>>> + /* Allow only first 3 MC banks into switch() */
>>>>
>>>> I don't think this comments is good here. Remove it is better.
>>>
>>> I had asked for this to be removed again too. I'm really thinking
>>> that V3 is what we should go with.
>>
>> V3 is fine, except adding comments for '-MSR_IA32_MC0_CTL' is
>> slightly better.
>
> Can I read this as an ack then (I already explained elsewhere
> why I think a comment there is rather pointless)?
>
> Jan
Yes, please.
Reviewed-by: Liu Jinsong <jinsong.liu@intel.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V4.1] mcheck, vmce: Allow vmce_amd_* functions to handle AMD thresolding MSRs
2014-02-18 12:42 ` Liu, Jinsong
@ 2014-02-18 16:23 ` Aravind Gopalakrishnan
0 siblings, 0 replies; 14+ messages in thread
From: Aravind Gopalakrishnan @ 2014-02-18 16:23 UTC (permalink / raw)
To: Liu, Jinsong, Jan Beulich
Cc: boris.ostrovsky, chegger, suravee.suthikulpanit, xen-devel
On 2/18/2014 6:42 AM, Liu, Jinsong wrote:
> Jan Beulich wrote:
>>>>> On 18.02.14 at 12:42, "Liu, Jinsong" <jinsong.liu@intel.com> wrote:
>>> Jan Beulich wrote:
>>>>>>> On 18.02.14 at 11:52, "Liu, Jinsong" <jinsong.liu@intel.com>
>>>>>>> wrote:
>>>>>>>> --- a/xen/arch/x86/cpu/mcheck/vmce.c
>>>>>>>> +++ b/xen/arch/x86/cpu/mcheck/vmce.c
>>>>>>>> @@ -107,7 +107,8 @@ static int bank_mce_rdmsr(const struct vcpu
>>>>>>>> *v, uint32_t msr, uint64_t *val)
>>>>>>>>
>>>>>>>> *val = 0;
>>>>>>>>
>>>>>>>> - switch ( msr & (MSR_IA32_MC0_CTL | 3) )
>>>>>>>> + /* Allow only first 3 MC banks into switch() */
>>>>> I don't think this comments is good here. Remove it is better.
>>>> I had asked for this to be removed again too. I'm really thinking
>>>> that V3 is what we should go with.
>>> V3 is fine, except adding comments for '-MSR_IA32_MC0_CTL' is
>>> slightly better.
>> Can I read this as an ack then (I already explained elsewhere
>> why I think a comment there is rather pointless)?
>>
>> Jan
Ok, guess V3 is good enough..
> Yes, please.
>
> Reviewed-by: Liu Jinsong <jinsong.liu@intel.com>
Thanks,
-Aravind.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2014-02-18 16:23 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-12 23:26 [PATCH V4.1] mcheck, vmce: Allow vmce_amd_* functions to handle AMD thresolding MSRs Aravind Gopalakrishnan
2014-02-13 8:38 ` Jan Beulich
2014-02-13 17:27 ` Aravind Gopalakrishnan
2014-02-13 18:24 ` Aravind Gopalakrishnan
2014-02-14 8:28 ` Jan Beulich
2014-02-16 14:47 ` Liu, Jinsong
2014-02-18 4:25 ` Liu, Jinsong
2014-02-18 9:41 ` Jan Beulich
2014-02-18 10:52 ` Liu, Jinsong
2014-02-18 11:02 ` Jan Beulich
2014-02-18 11:42 ` Liu, Jinsong
2014-02-18 12:36 ` Jan Beulich
2014-02-18 12:42 ` Liu, Jinsong
2014-02-18 16:23 ` Aravind Gopalakrishnan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.