Re: [Xen-devel] [PATCH V2] x86, amd_ucode: Verify max allowed patch size before apply

[Aravind Gopalakrishnan <aravind.gopalakrishnan@xxxxxxx>]

On 4/29/2014 4:33 PM, Aravind Gopalakrishnan wrote:
> On 4/29/2014 3:02 AM, Jan Beulich wrote:
>
> > > @@ -123,8 +151,17 @@ static bool_t microcode_fits(const struct 
> > > microcode_amd *mc_amd, int cpu)
> > >       if ( (mc_header->processor_rev_id) != equiv_cpu_id )
> > >           return 0;
> > >   +    if ( !verify_patch_size(mc_amd->mpb_size) )
> > > +    {
> > > +        printk(XENLOG_DEBUG "microcode: patch size mismatch\n");
> > > +        return -E2BIG;
> > > +    }
> > > +
> > >       if ( mc_header->patch_id <= uci->cpu_sig.rev )
> > > -        return 0;
> > > +    {
> > > +        printk(XENLOG_DEBUG "microcode: patch is already at 
> > > required level or greater.\n");
> > > +        return -EEXIST;
> > > +    }
> > >         printk(KERN_DEBUG "microcode: CPU%d found a matching 
> > > microcode "
> > >              "update with version %#x (current=%#x)\n",
> > Honestly I'm rather hesitant to accept further generally useless
> > messages, no matter that they get printed at KERN_DEBUG only. I'd
> > much rather see these as well as the existing ones to be converted
> > to pr_debug(), thus easily enabled if someone really needs to do
> > debugging here. That's mainly because I (and I suppose other
> > developers do so to) try to run with loglvl=all wherever possible,
> > yet already on the 2x4-core box (not to speak of the newer 2x12-
> > core one) I find these rather annoying.
> Hmm, Okay. I'll work on this and send an updated version..
couple of ideas about implementing this:

1. something similar to mwait-idle.c:
+#ifdef DEBUG
+# define pr_debug(fmt...) printk(KERN_DEBUG fmt)
+#else
+# define pr_debug(fmt...)
+#endif

2. a custom_param: something like mce_verbosity:
(reference code)
+//arav
+int ucode_verbosity;
+static void __init ucode_set_verbosity(char *str)
+{
+    if (strcmp("verbose", str) == 0)
+        ucode_verbosity = 1;
+    else
+        printk(KERN_DEBUG "Microcode verbosity level %s not recognised"
+               "use ucode_verbosity=verbose", str);
+}
+custom_param("ucode_verbosity", ucode_set_verbosity);
+
+#define pr_debug(v, fmt...) do {       \
+        if ((v) <= ucode_verbosity) \
+            printk(fmt);       \
+        } while (0)
+

and using: (example)
+        pr_debug(1,"microcode: patch size mismatch\n");

We can probably put it in a 'microcode.h' (?) file and extend functionality
to microcode_intel as well.. (microcode_intel will only need some very 
minor edits)
I have tried both options and they work fine.
(well, second option i know works for sure on AMD :) )

Thoughts?

-Aravind.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel