Message ID | 58AEB956020000780013D199@prv-mh.provo.novell.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 02/23/17 02:28 -0700, Jan Beulich wrote: > Storing -1 into both fields was misleading consumers: We really should > have a manifest constant for "invalid vCPU" here, and the already > existing DOMID_INVALID should be used. > > Also correct a bogus (dead code) check in mca_init_global(), at once > introducing a manifest constant for the early boot "invalid vCPU" > pointer (avoiding proliferation of the open coding). Make that pointer > a non-canonical address at once. > > Finally, don't leave mc_domid uninitialized in mca_init_bank(). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > TBD: Do we need to change XEN_MCA_INTERFACE_VERSION due to the 0xffff > -> DOMID_INVALID change for what mc_domid defaults to? > > --- a/xen/arch/x86/cpu/mcheck/mcaction.c > +++ b/xen/arch/x86/cpu/mcheck/mcaction.c > @@ -100,7 +100,8 @@ mc_memerr_dhandler(struct mca_binfo *bin > goto vmce_failed; > } > > - if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ) > + if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL || > + global->mc_vcpuid == XEN_MC_VCPUID_INVALID) > vmce_vcpuid = VMCE_INJECT_BROADCAST; > else > vmce_vcpuid = global->mc_vcpuid; If an invalid vcpuid is got on AMD machine, should we report error or inject to a default vcpu (vcpu0?) ? Haozhong
>>> On 23.02.17 at 11:01, <haozhong.zhang@intel.com> wrote: > On 02/23/17 02:28 -0700, Jan Beulich wrote: >> --- a/xen/arch/x86/cpu/mcheck/mcaction.c >> +++ b/xen/arch/x86/cpu/mcheck/mcaction.c >> @@ -100,7 +100,8 @@ mc_memerr_dhandler(struct mca_binfo *bin >> goto vmce_failed; >> } >> >> - if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ) >> + if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL || >> + global->mc_vcpuid == XEN_MC_VCPUID_INVALID) >> vmce_vcpuid = VMCE_INJECT_BROADCAST; >> else >> vmce_vcpuid = global->mc_vcpuid; > > If an invalid vcpuid is got on AMD machine, should we report error > or inject to a default vcpu (vcpu0?) ? Well, broadcasting in that case seems the best option to me, but let's add AMD maintainers to Cc. Jan
On 23/02/17 09:28, Jan Beulich wrote: > Storing -1 into both fields was misleading consumers: We really should > have a manifest constant for "invalid vCPU" here, and the already > existing DOMID_INVALID should be used. > > Also correct a bogus (dead code) check in mca_init_global(), at once > introducing a manifest constant for the early boot "invalid vCPU" > pointer (avoiding proliferation of the open coding). Make that pointer > a non-canonical address at once. > > Finally, don't leave mc_domid uninitialized in mca_init_bank(). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, however... > --- > TBD: Do we need to change XEN_MCA_INTERFACE_VERSION due to the 0xffff > -> DOMID_INVALID change for what mc_domid defaults to? > > --- a/xen/arch/x86/cpu/mcheck/mcaction.c > +++ b/xen/arch/x86/cpu/mcheck/mcaction.c > @@ -100,7 +100,8 @@ mc_memerr_dhandler(struct mca_binfo *bin > goto vmce_failed; > } > > - if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ) > + if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL || > + global->mc_vcpuid == XEN_MC_VCPUID_INVALID) Isn't this a backwards step, style-wise? This file, using 4 spaces, is Xen style rather than Linux style. ~Andrew
>>> On 23.02.17 at 13:02, <andrew.cooper3@citrix.com> wrote: > On 23/02/17 09:28, Jan Beulich wrote: >> --- a/xen/arch/x86/cpu/mcheck/mcaction.c >> +++ b/xen/arch/x86/cpu/mcheck/mcaction.c >> @@ -100,7 +100,8 @@ mc_memerr_dhandler(struct mca_binfo *bin >> goto vmce_failed; >> } >> >> - if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ) >> + if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL || >> + global->mc_vcpuid == XEN_MC_VCPUID_INVALID) > > Isn't this a backwards step, style-wise? This file, using 4 spaces, is > Xen style rather than Linux style. The majority of the file is using Linux style _except_ for indentation. There are, oddly enough, a few exceptions in the big if() statement towards the end of the file (where this change also applies to). Jan
On 23/02/17 12:21, Jan Beulich wrote: >>>> On 23.02.17 at 13:02, <andrew.cooper3@citrix.com> wrote: >> On 23/02/17 09:28, Jan Beulich wrote: >>> --- a/xen/arch/x86/cpu/mcheck/mcaction.c >>> +++ b/xen/arch/x86/cpu/mcheck/mcaction.c >>> @@ -100,7 +100,8 @@ mc_memerr_dhandler(struct mca_binfo *bin >>> goto vmce_failed; >>> } >>> >>> - if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ) >>> + if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL || >>> + global->mc_vcpuid == XEN_MC_VCPUID_INVALID) >> Isn't this a backwards step, style-wise? This file, using 4 spaces, is >> Xen style rather than Linux style. > The majority of the file is using Linux style _except_ for > indentation. There are, oddly enough, a few exceptions in the > big if() statement towards the end of the file (where this > change also applies to). We desperately need to reduce the style inconsistencies, because it is a major problem for contributors. IMO, that means we altering files like this to match Xen style. ~Andrew
On 02/23/2017 05:05 AM, Jan Beulich wrote: >>>> On 23.02.17 at 11:01, <haozhong.zhang@intel.com> wrote: >> On 02/23/17 02:28 -0700, Jan Beulich wrote: >>> --- a/xen/arch/x86/cpu/mcheck/mcaction.c >>> +++ b/xen/arch/x86/cpu/mcheck/mcaction.c >>> @@ -100,7 +100,8 @@ mc_memerr_dhandler(struct mca_binfo *bin >>> goto vmce_failed; >>> } >>> >>> - if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ) >>> + if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL || >>> + global->mc_vcpuid == XEN_MC_VCPUID_INVALID) >>> vmce_vcpuid = VMCE_INJECT_BROADCAST; >>> else >>> vmce_vcpuid = global->mc_vcpuid; >> If an invalid vcpuid is got on AMD machine, should we report error >> or inject to a default vcpu (vcpu0?) ? > Well, broadcasting in that case seems the best option to me, > but let's add AMD maintainers to Cc. Yes, I think we should broadcast if we don't know VCPU identity. OTOH, is mc_memerr_dhandler() even called on AMD? The only caller I see is intel_memerr_dhandler(). -boris
>>> On 23.02.17 at 10:28, <JBeulich@suse.com> wrote: > Storing -1 into both fields was misleading consumers: We really should > have a manifest constant for "invalid vCPU" here, and the already > existing DOMID_INVALID should be used. > > Also correct a bogus (dead code) check in mca_init_global(), at once > introducing a manifest constant for the early boot "invalid vCPU" > pointer (avoiding proliferation of the open coding). Make that pointer > a non-canonical address at once. > > Finally, don't leave mc_domid uninitialized in mca_init_bank(). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Christoph, you're at present the only MCE maintainer. Could you please give your feedback on this patch as well as Haozhong's series? Thanks, Jan
--- a/xen/arch/x86/cpu/mcheck/mcaction.c +++ b/xen/arch/x86/cpu/mcheck/mcaction.c @@ -100,7 +100,8 @@ mc_memerr_dhandler(struct mca_binfo *bin goto vmce_failed; } - if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ) + if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL || + global->mc_vcpuid == XEN_MC_VCPUID_INVALID) vmce_vcpuid = VMCE_INJECT_BROADCAST; else vmce_vcpuid = global->mc_vcpuid; --- a/xen/arch/x86/cpu/mcheck/mce.c +++ b/xen/arch/x86/cpu/mcheck/mce.c @@ -18,6 +18,7 @@ #include <xen/cpu.h> #include <asm/processor.h> +#include <asm/setup.h> #include <asm/system.h> #include <asm/apic.h> #include <asm/msr.h> @@ -215,6 +216,7 @@ static void mca_init_bank(enum mca_sourc mib->common.type = MC_TYPE_BANK; mib->common.size = sizeof (struct mcinfo_bank); mib->mc_bank = bank; + mib->mc_domid = DOMID_INVALID; if (mib->mc_status & MCi_STATUS_MISCV) mib->mc_misc = mca_rdmsr(MSR_IA32_MCx_MISC(bank)); @@ -245,15 +247,15 @@ static int mca_init_global(uint32_t flag { uint64_t status; int cpu_nr; - struct vcpu *v = current; - struct domain *d; + const struct vcpu *curr = current; /* Set global information */ mig->common.type = MC_TYPE_GLOBAL; mig->common.size = sizeof (struct mcinfo_global); status = mca_rdmsr(MSR_IA32_MCG_STATUS); mig->mc_gstatus = status; - mig->mc_domid = mig->mc_vcpuid = -1; + mig->mc_domid = DOMID_INVALID; + mig->mc_vcpuid = XEN_MC_VCPUID_INVALID; mig->mc_flags = flags; cpu_nr = smp_processor_id(); /* Retrieve detector information */ @@ -261,13 +263,9 @@ static int mca_init_global(uint32_t flag &mig->mc_coreid, &mig->mc_core_threadid, &mig->mc_apicid, NULL, NULL, NULL); - /* This is really meaningless */ - if (v != NULL && ((d = v->domain) != NULL)) { - mig->mc_domid = d->domain_id; - mig->mc_vcpuid = v->vcpu_id; - } else { - mig->mc_domid = -1; - mig->mc_vcpuid = -1; + if (curr != INVALID_VCPU) { + mig->mc_domid = curr->domain->domain_id; + mig->mc_vcpuid = curr->vcpu_id; } return 0; --- a/xen/arch/x86/cpu/mcheck/vmce.c +++ b/xen/arch/x86/cpu/mcheck/vmce.c @@ -391,7 +391,7 @@ int fill_vmsr_data(struct mcinfo_bank *m { struct vcpu *v = d->vcpu[0]; - if ( mc_bank->mc_domid != (uint16_t)~0 ) + if ( mc_bank->mc_domid != DOMID_INVALID ) { if ( v->arch.vmce.mcg_status & MCG_STATUS_MCIP ) { --- a/xen/arch/x86/domain_page.c +++ b/xen/arch/x86/domain_page.c @@ -16,6 +16,7 @@ #include <asm/current.h> #include <asm/flushtlb.h> #include <asm/hardirq.h> +#include <asm/setup.h> static struct vcpu *__read_mostly override; @@ -28,7 +29,7 @@ static inline struct vcpu *mapcache_curr * When current isn't properly set up yet, this is equivalent to * running in an idle vCPU (callers must check for NULL). */ - if ( v == (struct vcpu *)0xfffff000 ) + if ( v == INVALID_VCPU ) return NULL; /* --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -657,7 +657,7 @@ void __init noreturn __start_xen(unsigne /* Critical region without IDT or TSS. Any fault is deadly! */ set_processor_id(0); - set_current((struct vcpu *)0xfffff000); /* debug sanity. */ + set_current(INVALID_VCPU); /* debug sanity. */ idle_vcpu[0] = current; percpu_init_areas(); --- a/xen/include/asm-x86/setup.h +++ b/xen/include/asm-x86/setup.h @@ -4,6 +4,9 @@ #include <xen/multiboot.h> #include <asm/numa.h> +/* vCPU pointer used prior to there being a valid one around */ +#define INVALID_VCPU ((struct vcpu *)0xccccccccccccc000UL) + extern const char __2M_text_start[], __2M_text_end[]; extern const char __2M_rodata_start[], __2M_rodata_end[]; extern char __2M_init_start[], __2M_init_end[]; --- a/xen/include/public/arch-x86/xen-mca.h +++ b/xen/include/public/arch-x86/xen-mca.h @@ -88,6 +88,8 @@ #define XEN_MC_NOTDELIVERED 0x10 /* Note, XEN_MC_CANNOTHANDLE and XEN_MC_NOTDELIVERED are mutually exclusive. */ +/* Applicable to all mc_vcpuid fields below. */ +#define XEN_MC_VCPUID_INVALID 0xffff #ifndef __ASSEMBLY__