On Mon, Sep 08, 2014 at 02:37:50PM -0300, Henrique de Moraes Holschuh wrote:
> Enhance the logging in the Intel early microcode update driver to
> be able to report errors.
>
> Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
> ---
> arch/x86/kernel/cpu/microcode/intel_early.c | 94 +++++++++++++++------------
> 1 file changed, 54 insertions(+), 40 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
> index f73fc0a..8ad50d6 100644
> --- a/arch/x86/kernel/cpu/microcode/intel_early.c
> +++ b/arch/x86/kernel/cpu/microcode/intel_early.c
> @@ -31,6 +31,12 @@
> #include <asm/tlbflush.h>
> #include <asm/setup.h>
>
> +enum {
> + INTEL_EARLYMCU_NONE = 0, /* did nothing */
> + INTEL_EARLYMCU_UPDATEOK, /* microcode updated */
> + INTEL_EARLYMCU_REJECTED, /* cpu rejected it */
> +};
> +
> static unsigned long mc_saved_in_initrd[MAX_UCODE_COUNT];
> static struct mc_saved_data {
> unsigned int mc_saved_count;
> @@ -576,37 +582,50 @@ scan_microcode(unsigned long start, unsigned long end,
>
> /*
> * Print ucode update info.
> + * for status == INTEL_EARLYMCU_UPDATEOK, data should be the mcu date
> + * for status == INTEL_EARLYMCU_REJECTED, data should be mcu revision
> */
> -static void
> -print_ucode_info(struct ucode_cpu_info *uci, unsigned int date)
> +static void print_ucode_info(const unsigned int status,
> + const struct ucode_cpu_info *uci,
> + const unsigned int data)
> {
> int cpu = smp_processor_id();
> -
> - pr_info("CPU%d: entire core updated early to revision 0x%x, date %04x-%02x-%02x\n",
> - cpu,
> - uci->cpu_sig.rev,
> - date & 0xffff,
> - date >> 24,
> - (date >> 16) & 0xff);
> + struct ucode_cpu_info ucil;
> +
> + switch (status) {
> + case INTEL_EARLYMCU_NONE:
> + break;
> + case INTEL_EARLYMCU_UPDATEOK:
> + if (!uci) {
> + collect_cpu_info_early(&ucil);
> + uci = &ucil;
> + }
> + pr_info("CPU%d: entire core updated early to revision 0x%x, date %04x-%02x-%02x\n",
> + cpu,
> + uci->cpu_sig.rev,
> + data & 0xffff,
> + data >> 24,
> + (data >> 16) & 0xff);
> + break;
> + case INTEL_EARLYMCU_REJECTED:
> + pr_err("CPU%d: update to revision 0x%x rejected by the processor\n", cpu, data);
> + break;
> + }
> }
>
> #ifdef CONFIG_X86_32
>
> -static int delay_ucode_info;
> -static int current_mc_date;
> +static unsigned int delay_ucode_info;
> +static unsigned int delay_ucode_info_data;
First of all, this really is date and not data and prefixing it with
"delay" really doesn't make it cleaner.
Then, this whole scheme can be simplified a bit by dropping
delay_ucode_info and using current_mc_date to test whether to print the
message or not. After printing, you set it back to 0.
And then you can drop the _REJECTED case as it is not needed.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Mon, 20 Oct 2014, Borislav Petkov wrote:
> On Mon, Sep 08, 2014 at 02:37:50PM -0300, Henrique de Moraes Holschuh wrote:
> > Enhance the logging in the Intel early microcode update driver to
> > be able to report errors.
> >
> > Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
> > ---
> > arch/x86/kernel/cpu/microcode/intel_early.c | 94 +++++++++++++++------------
> > 1 file changed, 54 insertions(+), 40 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
> > index f73fc0a..8ad50d6 100644
> > --- a/arch/x86/kernel/cpu/microcode/intel_early.c
> > +++ b/arch/x86/kernel/cpu/microcode/intel_early.c
> > @@ -31,6 +31,12 @@
> > #include <asm/tlbflush.h>
> > #include <asm/setup.h>
> >
> > +enum {
> > + INTEL_EARLYMCU_NONE = 0, /* did nothing */
> > + INTEL_EARLYMCU_UPDATEOK, /* microcode updated */
> > + INTEL_EARLYMCU_REJECTED, /* cpu rejected it */
> > +};
> > +
> > static unsigned long mc_saved_in_initrd[MAX_UCODE_COUNT];
> > static struct mc_saved_data {
> > unsigned int mc_saved_count;
> > @@ -576,37 +582,50 @@ scan_microcode(unsigned long start, unsigned long end,
> >
> > /*
> > * Print ucode update info.
> > + * for status == INTEL_EARLYMCU_UPDATEOK, data should be the mcu date
> > + * for status == INTEL_EARLYMCU_REJECTED, data should be mcu revision
> > */
> > -static void
> > -print_ucode_info(struct ucode_cpu_info *uci, unsigned int date)
> > +static void print_ucode_info(const unsigned int status,
> > + const struct ucode_cpu_info *uci,
> > + const unsigned int data)
> > {
> > int cpu = smp_processor_id();
> > -
> > - pr_info("CPU%d: entire core updated early to revision 0x%x, date %04x-%02x-%02x\n",
> > - cpu,
> > - uci->cpu_sig.rev,
> > - date & 0xffff,
> > - date >> 24,
> > - (date >> 16) & 0xff);
> > + struct ucode_cpu_info ucil;
> > +
> > + switch (status) {
> > + case INTEL_EARLYMCU_NONE:
> > + break;
> > + case INTEL_EARLYMCU_UPDATEOK:
> > + if (!uci) {
> > + collect_cpu_info_early(&ucil);
> > + uci = &ucil;
> > + }
> > + pr_info("CPU%d: entire core updated early to revision 0x%x, date %04x-%02x-%02x\n",
> > + cpu,
> > + uci->cpu_sig.rev,
> > + data & 0xffff,
> > + data >> 24,
> > + (data >> 16) & 0xff);
> > + break;
> > + case INTEL_EARLYMCU_REJECTED:
> > + pr_err("CPU%d: update to revision 0x%x rejected by the processor\n", cpu, data);
> > + break;
> > + }
> > }
> >
> > #ifdef CONFIG_X86_32
> >
> > -static int delay_ucode_info;
> > -static int current_mc_date;
> > +static unsigned int delay_ucode_info;
> > +static unsigned int delay_ucode_info_data;
>
> First of all, this really is date and not data and prefixing it with
> "delay" really doesn't make it cleaner.
For INTEL_EARLYMCU_UPDATEOK, it is the date.
For INTEL_EARLYMCU_REJECTED, it is the revision of the microcode that was
rejected (as opposed to the revision of the microcode currently in the
processor), because that's far more important to know than the date.
However, if you prefer, I could have _date, _oldrev, and _newrev, and
enhance the error messages a bit (updated from rev X to rev Y, date Z), etc.
> Then, this whole scheme can be simplified a bit by dropping
> delay_ucode_info and using current_mc_date to test whether to print the
> message or not. After printing, you set it back to 0.
In the INTEL_EARLYMCU_REJECTED case, the data we need to print might be
zero, and it has 32 bits.
Besides, this way one can trivially add new messages when required. And we
will need to do that eventually.
In fact, I have a patch somewhere that needs to add a new failure message:
we have several failure cases which we want to differentiate, at the very
least "processor didn't like it" and "it looks corrupt, so we didn't even
try to install it".
If you want, I will add it when I resubmit this stack, instead of waiting
for the next one.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Tue, Oct 21, 2014 at 12:10:15PM -0200, Henrique de Moraes Holschuh wrote:
> In fact, I have a patch somewhere that needs to add a new failure message:
> we have several failure cases which we want to differentiate, at the very
> least "processor didn't like it" and "it looks corrupt, so we didn't even
> try to install it".
Actually, I don't want to be too chatty with the loader: if the
microcode blob passes checks but it is not for the current processor
we're running, not saying anything is what I want to do.
Why? Because I don't want to disturb people unnecessarily - if the
microcode doesn't apply and everything else checks out, you simply don't
need it.
If one really wants to know, one can always check /proc/cpuinfo and read
out the microcode revision from the blob. But that is for the 1% of the
curious ones - everyone else should simply install microcode blob and
boot machine. Fire and forget.
Only the abnormal error cases should be vocal in saying what's wrong so
that we can actually address those.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Thu, 2014-10-30 at 18:41 +0100, Borislav Petkov wrote:
> On Tue, Oct 21, 2014 at 12:10:15PM -0200, Henrique de Moraes Holschuh wrote:
> > In fact, I have a patch somewhere that needs to add a new failure message:
> > we have several failure cases which we want to differentiate, at the very
> > least "processor didn't like it" and "it looks corrupt, so we didn't even
> > try to install it".
>
> Actually, I don't want to be too chatty with the loader: if the
> microcode blob passes checks but it is not for the current processor
> we're running, not saying anything is what I want to do.
>
> Why? Because I don't want to disturb people unnecessarily - if the
> microcode doesn't apply and everything else checks out, you simply don't
> need it.
>
> If one really wants to know, one can always check /proc/cpuinfo and read
> out the microcode revision from the blob. But that is for the 1% of the
> curious ones - everyone else should simply install microcode blob and
> boot machine. Fire and forget.
>
> Only the abnormal error cases should be vocal in saying what's wrong so
> that we can actually address those.
Maybe make it emit at KERN_DEBUG
On Thu, 30 Oct 2014, Borislav Petkov wrote:
> On Tue, Oct 21, 2014 at 12:10:15PM -0200, Henrique de Moraes Holschuh wrote:
> > In fact, I have a patch somewhere that needs to add a new failure message:
> > we have several failure cases which we want to differentiate, at the very
> > least "processor didn't like it" and "it looks corrupt, so we didn't even
> > try to install it".
>
> Actually, I don't want to be too chatty with the loader: if the
> microcode blob passes checks but it is not for the current processor
> we're running, not saying anything is what I want to do.
>
> Why? Because I don't want to disturb people unnecessarily - if the
> microcode doesn't apply and everything else checks out, you simply don't
> need it.
>
> If one really wants to know, one can always check /proc/cpuinfo and read
> out the microcode revision from the blob. But that is for the 1% of the
> curious ones - everyone else should simply install microcode blob and
> boot machine. Fire and forget.
>
> Only the abnormal error cases should be vocal in saying what's wrong so
> that we can actually address those.
I've answered these points in one of the replies I just sent.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh