| Message ID | 20170217063936.13208-19-haozhong.zhang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 17.02.17 at 07:39, <haozhong.zhang@intel.com> wrote: > --- a/xen/arch/x86/cpu/mcheck/mce.c > +++ b/xen/arch/x86/cpu/mcheck/mce.c > @@ -1510,6 +1510,7 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_mc_t) u_xen_mc) > { > const cpumask_t *cpumap; > cpumask_var_t cmv; > + int cpu_nr; unsigned int (and likely no need for the _nr suffix) Or perhaps the local variable isn't needed at all. > @@ -1552,6 +1553,21 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_mc_t) u_xen_mc) > send_IPI_mask(cpumap, cmci_apic_vector); > } > break; > + case XEN_MC_INJECT_TYPE_LMCE: > + if ( !lmce_support ) > + { > + ret = x86_mcerr("No LMCE support in platform", -EINVAL); > + break; > + } > + /* ensure at most one CPU is specified */ And what use is none at all? Also - comment style (should start with a capital). > + cpu_nr = cpumask_next(cpumask_first(cpumap), cpumap); > + if ( cpu_nr < nr_cpu_ids ) > + { > + ret = x86_mcerr("More than one CPU specified", -EINVAL); > + break; > + } > + on_selected_cpus(cpumap, x86_mc_mceinject, NULL, 1); > + break; > default: See earlier patch comments regarding blank lines missing here. Jan
On 02/22/17 08:59 -0700, Jan Beulich wrote: > >>> On 17.02.17 at 07:39, <haozhong.zhang@intel.com> wrote: > > --- a/xen/arch/x86/cpu/mcheck/mce.c > > +++ b/xen/arch/x86/cpu/mcheck/mce.c > > @@ -1510,6 +1510,7 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_mc_t) u_xen_mc) > > { > > const cpumask_t *cpumap; > > cpumask_var_t cmv; > > + int cpu_nr; > > unsigned int (and likely no need for the _nr suffix) > > Or perhaps the local variable isn't needed at all. > I'll inline this variable. > > @@ -1552,6 +1553,21 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_mc_t) u_xen_mc) > > send_IPI_mask(cpumap, cmci_apic_vector); > > } > > break; > > + case XEN_MC_INJECT_TYPE_LMCE: > > + if ( !lmce_support ) > > + { > > + ret = x86_mcerr("No LMCE support in platform", -EINVAL); > > + break; > > + } > > + /* ensure at most one CPU is specified */ > > And what use is none at all? Also - comment style (should start with > a capital). > Do you mean the check of empty cpumap? It's checked at the beginning of case XEN_MC_inject_v2. > > + cpu_nr = cpumask_next(cpumask_first(cpumap), cpumap); > > + if ( cpu_nr < nr_cpu_ids ) > > + { > > + ret = x86_mcerr("More than one CPU specified", -EINVAL); > > + break; > > + } > > + on_selected_cpus(cpumap, x86_mc_mceinject, NULL, 1); > > + break; > > default: > > See earlier patch comments regarding blank lines missing here. > I'll add a blank line before the default case. Thanks, Haozhong
>>> On 23.02.17 at 06:14, <haozhong.zhang@intel.com> wrote: > On 02/22/17 08:59 -0700, Jan Beulich wrote: >> >>> On 17.02.17 at 07:39, <haozhong.zhang@intel.com> wrote: >> > @@ -1552,6 +1553,21 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_mc_t) u_xen_mc) >> > send_IPI_mask(cpumap, cmci_apic_vector); >> > } >> > break; >> > + case XEN_MC_INJECT_TYPE_LMCE: >> > + if ( !lmce_support ) >> > + { >> > + ret = x86_mcerr("No LMCE support in platform", -EINVAL); >> > + break; >> > + } >> > + /* ensure at most one CPU is specified */ >> >> And what use is none at all? Also - comment style (should start with >> a capital). >> > > Do you mean the check of empty cpumap? It's checked at the beginning of case > XEN_MC_inject_v2. To be honest, I don't see any such check. But looking at that code makes me notice you should also forbid the combination of XEN_MC_INJECT_CPU_BROADCAST and XEN_MC_INJECT_TYPE_LMCE. >> > + cpu_nr = cpumask_next(cpumask_first(cpumap), cpumap); >> > + if ( cpu_nr < nr_cpu_ids ) >> > + { >> > + ret = x86_mcerr("More than one CPU specified", -EINVAL); >> > + break; >> > + } >> > + on_selected_cpus(cpumap, x86_mc_mceinject, NULL, 1); >> > + break; >> > default: >> >> See earlier patch comments regarding blank lines missing here. > > I'll add a blank line before the default case. And another one before the new case label you add. Jan
On 02/23/17 01:26 -0700, Jan Beulich wrote: > >>> On 23.02.17 at 06:14, <haozhong.zhang@intel.com> wrote: > > On 02/22/17 08:59 -0700, Jan Beulich wrote: > >> >>> On 17.02.17 at 07:39, <haozhong.zhang@intel.com> wrote: > >> > @@ -1552,6 +1553,21 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_mc_t) u_xen_mc) > >> > send_IPI_mask(cpumap, cmci_apic_vector); > >> > } > >> > break; > >> > + case XEN_MC_INJECT_TYPE_LMCE: > >> > + if ( !lmce_support ) > >> > + { > >> > + ret = x86_mcerr("No LMCE support in platform", -EINVAL); > >> > + break; > >> > + } > >> > + /* ensure at most one CPU is specified */ > >> > >> And what use is none at all? Also - comment style (should start with > >> a capital). > >> > > > > Do you mean the check of empty cpumap? It's checked at the beginning of case > > XEN_MC_inject_v2. > > To be honest, I don't see any such check. But looking at that code > makes me notice you should also forbid the combination of > XEN_MC_INJECT_CPU_BROADCAST and XEN_MC_INJECT_TYPE_LMCE. > I mean the following check. Of course, the additional check you suggested must go before it. if ( op->u.mc_inject_v2.flags & XEN_MC_INJECT_CPU_BROADCAST ) cpumap = &cpu_online_map; else { ret = xenctl_bitmap_to_cpumask(&cmv, &op->u.mc_inject_v2.cpumap); if ( ret ) break; cpumap = cmv; if ( !cpumask_intersects(cpumap, &cpu_online_map) ) <=== I mean this one exactly { free_cpumask_var(cmv); ret = x86_mcerr("No online CPU passed\n", -EINVAL); break; } if ( !cpumask_subset(cpumap, &cpu_online_map) ) dprintk(XENLOG_INFO, "Not all required CPUs are online\n"); } Haozhong
>>> On 23.02.17 at 10:14, <haozhong.zhang@intel.com> wrote: > On 02/23/17 01:26 -0700, Jan Beulich wrote: >> >>> On 23.02.17 at 06:14, <haozhong.zhang@intel.com> wrote: >> > On 02/22/17 08:59 -0700, Jan Beulich wrote: >> >> >>> On 17.02.17 at 07:39, <haozhong.zhang@intel.com> wrote: >> >> > @@ -1552,6 +1553,21 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_mc_t) > u_xen_mc) >> >> > send_IPI_mask(cpumap, cmci_apic_vector); >> >> > } >> >> > break; >> >> > + case XEN_MC_INJECT_TYPE_LMCE: >> >> > + if ( !lmce_support ) >> >> > + { >> >> > + ret = x86_mcerr("No LMCE support in platform", -EINVAL); >> >> > + break; >> >> > + } >> >> > + /* ensure at most one CPU is specified */ >> >> >> >> And what use is none at all? Also - comment style (should start with >> >> a capital). >> >> >> > >> > Do you mean the check of empty cpumap? It's checked at the beginning of case >> > XEN_MC_inject_v2. >> >> To be honest, I don't see any such check. But looking at that code >> makes me notice you should also forbid the combination of >> XEN_MC_INJECT_CPU_BROADCAST and XEN_MC_INJECT_TYPE_LMCE. >> > > I mean the following check. Of course, the additional check you > suggested must go before it. > if ( op->u.mc_inject_v2.flags & XEN_MC_INJECT_CPU_BROADCAST ) > cpumap = &cpu_online_map; > else > { > ret = xenctl_bitmap_to_cpumask(&cmv, &op->u.mc_inject_v2.cpumap); > if ( ret ) > break; > cpumap = cmv; > if ( !cpumask_intersects(cpumap, &cpu_online_map) ) <=== I mean this one exactly I've been blind, so I'm sorry for the noise. Jan
diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c index 2d69222..56c1f5e 100644 --- a/xen/arch/x86/cpu/mcheck/mce.c +++ b/xen/arch/x86/cpu/mcheck/mce.c @@ -1510,6 +1510,7 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_mc_t) u_xen_mc) { const cpumask_t *cpumap; cpumask_var_t cmv; + int cpu_nr; if (nr_mce_banks == 0) return x86_mcerr("do_mca #MC", -ENODEV); @@ -1552,6 +1553,21 @@ long do_mca(XEN_GUEST_HANDLE_PARAM(xen_mc_t) u_xen_mc) send_IPI_mask(cpumap, cmci_apic_vector); } break; + case XEN_MC_INJECT_TYPE_LMCE: + if ( !lmce_support ) + { + ret = x86_mcerr("No LMCE support in platform", -EINVAL); + break; + } + /* ensure at most one CPU is specified */ + cpu_nr = cpumask_next(cpumask_first(cpumap), cpumap); + if ( cpu_nr < nr_cpu_ids ) + { + ret = x86_mcerr("More than one CPU specified", -EINVAL); + break; + } + on_selected_cpus(cpumap, x86_mc_mceinject, NULL, 1); + break; default: ret = x86_mcerr("Wrong mca type\n", -EINVAL); break; diff --git a/xen/include/public/arch-x86/xen-mca.h b/xen/include/public/arch-x86/xen-mca.h index 9566a33..037a174 100644 --- a/xen/include/public/arch-x86/xen-mca.h +++ b/xen/include/public/arch-x86/xen-mca.h @@ -412,6 +412,7 @@ struct xen_mc_mceinject { #define XEN_MC_INJECT_TYPE_MASK 0x7 #define XEN_MC_INJECT_TYPE_MCE 0x0 #define XEN_MC_INJECT_TYPE_CMCI 0x1 +#define XEN_MC_INJECT_TYPE_LMCE 0x2 #define XEN_MC_INJECT_CPU_BROADCAST 0x8
Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com> --- Cc: Christoph Egger <chegger@amazon.de> Cc: Liu Jinsong <jinsong.liu@alibaba-inc.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> --- xen/arch/x86/cpu/mcheck/mce.c | 16 ++++++++++++++++ xen/include/public/arch-x86/xen-mca.h | 1 + 2 files changed, 17 insertions(+)