Re: [XEN PATCH v1 7/7] x86/MCE: optional build of AMD/Intel MCE code

[Sergiy Kibrik <sergiy_kibrik@xxxxxxxx>]

29.04.24 18:54, Jan Beulich:
> On 27.04.2024 01:16, Stefano Stabellini wrote:
> > On Tue, 23 Apr 2024, Sergiy Kibrik wrote:
> > > --- a/xen/arch/x86/cpu/mcheck/Makefile
> > > +++ b/xen/arch/x86/cpu/mcheck/Makefile
> > > @@ -1,12 +1,10 @@
> > > -obj-y += amd_nonfatal.o
> > > -obj-y += mce_amd.o
> > >   obj-y += mcaction.o
> > >   obj-y += barrier.o
> > > -obj-y += intel-nonfatal.o
> > >   obj-y += mctelem.o
> > >   obj-y += mce.o
> > >   obj-y += mce-apei.o
> > > -obj-y += mce_intel.o
> > > +obj-$(CONFIG_AMD) += mce_amd.o amd_nonfatal.o
> > > +obj-$(CONFIG_INTEL) += mce_intel.o intel-nonfatal.o
> > >   obj-y += non-fatal.o
> > >   obj-y += util.o
> > >   obj-y += vmce.o
> > Awesome!
> Almost. I'd appreciate if the ordering of files would be retained. It's
> not quite alphabetic, but still. Moving mce_amd.o and mcaction.o to their
> designated slots may or may not be done right here.
sure, I'll leave ordering as before

> > > --- a/xen/arch/x86/cpu/mcheck/non-fatal.c
> > > +++ b/xen/arch/x86/cpu/mcheck/non-fatal.c
> > > @@ -24,14 +24,20 @@ static int __init cf_check init_nonfatal_mce_checker(void)
> > >          * Check for non-fatal errors every MCE_RATE s
> > >          */
> > >         switch (c->x86_vendor) {
> > > +#ifdef CONFIG_AMD
> > >         case X86_VENDOR_AMD:
> > >         case X86_VENDOR_HYGON:
> > >                 /* Assume we are on K8 or newer AMD or Hygon CPU here */
> > >                 amd_nonfatal_mcheck_init(c);
> > >                 break;
> > > +#endif
> > > +#ifdef CONFIG_INTEL
> > >         case X86_VENDOR_INTEL:
> > >                 intel_nonfatal_mcheck_init(c);
> > >                 break;
> > > +#endif
> > > +       default:
> > > +               return -ENODEV;
> This, while perhaps desirable, doesn't fit ...
>
> > >         }
> > >         printk(KERN_INFO "mcheck_poll: Machine check polling timer started.\n");
> > >         return 0;
> ... earlier behavior, and hence is somewhat unexpected in a change which, by
> its description, looks like a "no functional change" one.
I see, will try to describe it a bit better then.

  -Sergiy