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

[Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>]

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.