[Xen-devel] [PATCH 0/7] x86/ucode: Cleanup and fixes - Part 3/n (Intel)

[Xen-devel] [PATCH 0/7] x86/ucode: Cleanup and fixes - Part 3/n (Intel)

[Andrew Cooper]

This focuses on the Intel ucode driver, removing the gratuitous memory
allocations and indirection, as well as minor fixes in other areas of the
logic.

It depends on both the Part 1 and 2 series, and hopefully better demonstrates
why making struct microcode_patch opaque is a sensible move forward.

Andrew Cooper (7):
  x86/ucode: Document the behaviour of the microcode_ops hooks
  x86/ucode/intel: Adjust microcode_sanity_check() to not take void *
  x86/ucode/intel: Remove gratuitous memory allocations from cpu_request_microcode()
  x86/ucode/intel: Reimplement get_{data,total}size() helpers
  x86/ucode/intel: Clean up microcode_update_match()
  x86/ucode/intel: Clean up microcode_sanity_check()
  x86/ucode/intel: Fold structures together

 xen/arch/x86/cpu/microcode/intel.c   | 371 +++++++++++++++--------------------
 xen/arch/x86/cpu/microcode/private.h |  46 +++++
 xen/include/asm-x86/microcode.h      |   5 +
 3 files changed, 214 insertions(+), 208 deletions(-)

-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 1/7] x86/ucode: Document the behaviour of the microcode_ops hooks

[Andrew Cooper]

... and struct cpu_signature for good measure.

No comment is passed on the suitability of the behaviour...

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpu/microcode/private.h | 46 ++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/microcode.h      |  5 ++++
 2 files changed, 51 insertions(+)

diff --git a/xen/arch/x86/cpu/microcode/private.h b/xen/arch/x86/cpu/microcode/private.h
index e64168a502..a2aec53047 100644
--- a/xen/arch/x86/cpu/microcode/private.h
+++ b/xen/arch/x86/cpu/microcode/private.h
@@ -14,14 +14,60 @@ enum microcode_match_result {
 struct microcode_patch; /* Opaque */
 
 struct microcode_ops {
+    /*
+     * Parse a microcode container.  Format is vendor-specific.
+     *
+     * Search within the container for the patch, suitable for the current
+     * CPU, which has the highest revision.  (Note: May be a patch which is
+     * older that what is running in the CPU.  This is a feature, to better
+     * cope with corner cases from buggy firmware.)
+     *
+     * If one is found, allocate and return a struct microcode_patch
+     * encapsulating the appropriate microcode patch.  Does not alias the
+     * original buffer.
+     *
+     * If one is not found, (nothing matches the current CPU), return NULL.
+     * Also may return ERR_PTR(-err), e.g. bad container, out of memory.
+     */
     struct microcode_patch *(*cpu_request_microcode)(const void *buf,
                                                      size_t size);
+
+    /* Obtain microcode-relevant details for the current CPU. */
     int (*collect_cpu_info)(struct cpu_signature *csig);
+
+    /*
+     * Attempt to load the provided patch into the CPU.  Returns -EIO if
+     * anything didn't go as expected.
+     */
     int (*apply_microcode)(const struct microcode_patch *patch);
+
+    /*
+     * Optional.  If provided and applicable to the specific update attempt,
+     * is run once by the initiating CPU.  Returning an error will abort the
+     * load attempt.
+     */
     int (*start_update)(void);
+
+    /*
+     * Optional.  If provided, called on every CPU which completes a microcode
+     * load.  May be called in the case of some errors, and not others.  May
+     * be called even if start_update() wasn't.
+     */
     void (*end_update_percpu)(void);
+
+    /* Free a patch previously allocated by cpu_request_microcode(). */
     void (*free_patch)(struct microcode_patch *patch);
+
+    /*
+     * Is the microcode patch applicable for the current CPU, and newer than
+     * the currently running patch?
+     */
     bool (*match_cpu)(const struct microcode_patch *patch);
+
+    /*
+     * Given two patches, are they both applicable to the current CPU, and is
+     * new a higher revision than old?
+     */
     enum microcode_match_result (*compare_patch)(
         const struct microcode_patch *new, const struct microcode_patch *old);
 };
diff --git a/xen/include/asm-x86/microcode.h b/xen/include/asm-x86/microcode.h
index 89b9aaa02d..41e85a24d2 100644
--- a/xen/include/asm-x86/microcode.h
+++ b/xen/include/asm-x86/microcode.h
@@ -7,8 +7,13 @@
 #include <public/xen.h>
 
 struct cpu_signature {
+    /* CPU signature (CPUID.1.EAX).  Only written on Intel. */
     unsigned int sig;
+
+    /* Platform Flags (only actually 1 bit).  Only applicable to Intel. */
     unsigned int pf;
+
+    /* Microcode Revision. */
     unsigned int rev;
 };
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/7] x86/ucode: Document the behaviour of the microcode_ops hooks

[Jan Beulich]

On 23.03.2020 11:17, Andrew Cooper wrote:
> ... and struct cpu_signature for good measure.
> 
> No comment is passed on the suitability of the behaviour...
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/cpu/microcode/private.h | 46 ++++++++++++++++++++++++++++++++++++
>  xen/include/asm-x86/microcode.h      |  5 ++++
>  2 files changed, 51 insertions(+)
> 
> diff --git a/xen/arch/x86/cpu/microcode/private.h b/xen/arch/x86/cpu/microcode/private.h
> index e64168a502..a2aec53047 100644
> --- a/xen/arch/x86/cpu/microcode/private.h
> +++ b/xen/arch/x86/cpu/microcode/private.h
> @@ -14,14 +14,60 @@ enum microcode_match_result {
>  struct microcode_patch; /* Opaque */
>  
>  struct microcode_ops {
> +    /*
> +     * Parse a microcode container.  Format is vendor-specific.
> +     *
> +     * Search within the container for the patch, suitable for the current
> +     * CPU, which has the highest revision.  (Note: May be a patch which is
> +     * older that what is running in the CPU.  This is a feature, to better
> +     * cope with corner cases from buggy firmware.)
> +     *
> +     * If one is found, allocate and return a struct microcode_patch
> +     * encapsulating the appropriate microcode patch.  Does not alias the
> +     * original buffer.
> +     *
> +     * If one is not found, (nothing matches the current CPU), return NULL.
> +     * Also may return ERR_PTR(-err), e.g. bad container, out of memory.
> +     */
>      struct microcode_patch *(*cpu_request_microcode)(const void *buf,
>                                                       size_t size);
> +
> +    /* Obtain microcode-relevant details for the current CPU. */
>      int (*collect_cpu_info)(struct cpu_signature *csig);
> +
> +    /*
> +     * Attempt to load the provided patch into the CPU.  Returns -EIO if
> +     * anything didn't go as expected.
> +     */
>      int (*apply_microcode)(const struct microcode_patch *patch);

While at present -EIO may be the only error that may come back here, do
we want to risk the comment going stale when another error return gets
added? IOW - perhaps add "e.g." or some such?

> --- a/xen/include/asm-x86/microcode.h
> +++ b/xen/include/asm-x86/microcode.h
> @@ -7,8 +7,13 @@
>  #include <public/xen.h>
>  
>  struct cpu_signature {
> +    /* CPU signature (CPUID.1.EAX).  Only written on Intel. */
>      unsigned int sig;
> +
> +    /* Platform Flags (only actually 1 bit).  Only applicable to Intel. */
>      unsigned int pf;

To me "only actually 1 bit" makes it an implication that this is the
lowest bit (like in a bool represented in a 32-bit memory location).
I didn't think this was the case though, so unless I'm wrong, could
you clarify this a little?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/7] x86/ucode: Document the behaviour of the microcode_ops hooks

[Andrew Cooper]

On 23/03/2020 12:33, Jan Beulich wrote:
> On 23.03.2020 11:17, Andrew Cooper wrote:
>> ... and struct cpu_signature for good measure.
>>
>> No comment is passed on the suitability of the behaviour...
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wl@xen.org>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>>  xen/arch/x86/cpu/microcode/private.h | 46 ++++++++++++++++++++++++++++++++++++
>>  xen/include/asm-x86/microcode.h      |  5 ++++
>>  2 files changed, 51 insertions(+)
>>
>> diff --git a/xen/arch/x86/cpu/microcode/private.h b/xen/arch/x86/cpu/microcode/private.h
>> index e64168a502..a2aec53047 100644
>> --- a/xen/arch/x86/cpu/microcode/private.h
>> +++ b/xen/arch/x86/cpu/microcode/private.h
>> @@ -14,14 +14,60 @@ enum microcode_match_result {
>>  struct microcode_patch; /* Opaque */
>>  
>>  struct microcode_ops {
>> +    /*
>> +     * Parse a microcode container.  Format is vendor-specific.
>> +     *
>> +     * Search within the container for the patch, suitable for the current
>> +     * CPU, which has the highest revision.  (Note: May be a patch which is
>> +     * older that what is running in the CPU.  This is a feature, to better
>> +     * cope with corner cases from buggy firmware.)
>> +     *
>> +     * If one is found, allocate and return a struct microcode_patch
>> +     * encapsulating the appropriate microcode patch.  Does not alias the
>> +     * original buffer.
>> +     *
>> +     * If one is not found, (nothing matches the current CPU), return NULL.
>> +     * Also may return ERR_PTR(-err), e.g. bad container, out of memory.
>> +     */
>>      struct microcode_patch *(*cpu_request_microcode)(const void *buf,
>>                                                       size_t size);
>> +
>> +    /* Obtain microcode-relevant details for the current CPU. */
>>      int (*collect_cpu_info)(struct cpu_signature *csig);
>> +
>> +    /*
>> +     * Attempt to load the provided patch into the CPU.  Returns -EIO if
>> +     * anything didn't go as expected.
>> +     */
>>      int (*apply_microcode)(const struct microcode_patch *patch);
> While at present -EIO may be the only error that may come back here, do
> we want to risk the comment going stale when another error return gets
> added? IOW - perhaps add "e.g." or some such?

Can do.

>
>> --- a/xen/include/asm-x86/microcode.h
>> +++ b/xen/include/asm-x86/microcode.h
>> @@ -7,8 +7,13 @@
>>  #include <public/xen.h>
>>  
>>  struct cpu_signature {
>> +    /* CPU signature (CPUID.1.EAX).  Only written on Intel. */
>>      unsigned int sig;
>> +
>> +    /* Platform Flags (only actually 1 bit).  Only applicable to Intel. */
>>      unsigned int pf;
> To me "only actually 1 bit" makes it an implication that this is the
> lowest bit (like in a bool represented in a 32-bit memory location).
> I didn't think this was the case though, so unless I'm wrong, could
> you clarify this a little?

There will be a single bit within the bottom 8 set (the 1 <<
MSR_PLATFORM_ID[52:50]), despite this field being called "Platform Flags".

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/7] x86/ucode: Document the behaviour of the microcode_ops hooks

[Jan Beulich]

On 23.03.2020 14:26, Andrew Cooper wrote:
> On 23/03/2020 12:33, Jan Beulich wrote:
>> On 23.03.2020 11:17, Andrew Cooper wrote:
>>> --- a/xen/include/asm-x86/microcode.h
>>> +++ b/xen/include/asm-x86/microcode.h
>>> @@ -7,8 +7,13 @@
>>>  #include <public/xen.h>
>>>  
>>>  struct cpu_signature {
>>> +    /* CPU signature (CPUID.1.EAX).  Only written on Intel. */
>>>      unsigned int sig;
>>> +
>>> +    /* Platform Flags (only actually 1 bit).  Only applicable to Intel. */
>>>      unsigned int pf;
>> To me "only actually 1 bit" makes it an implication that this is the
>> lowest bit (like in a bool represented in a 32-bit memory location).
>> I didn't think this was the case though, so unless I'm wrong, could
>> you clarify this a little?
> 
> There will be a single bit within the bottom 8 set (the 1 <<
> MSR_PLATFORM_ID[52:50]), despite this field being called "Platform Flags".

That's what I recalled. Could I talk you into
s/only actually 1 bit/only actually a single set bit/ or some such?
If so,
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 2/7] x86/ucode/intel: Adjust microcode_sanity_check() to not take void *

[Andrew Cooper]

microcode_sanity_check()'s callers actually call it with a mixture of
microcode_intel and microcode_header_intel pointers, which is fragile.

Rework it to take struct microcode_intel *, which in turn requires
microcode_update_match()'s type to be altered.

No functional change - compiled binary is identical.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpu/microcode/intel.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index f26511da98..f0beefe1bb 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -119,9 +119,9 @@ static int collect_cpu_info(struct cpu_signature *csig)
     return 0;
 }
 
-static int microcode_sanity_check(const void *mc)
+static int microcode_sanity_check(const struct microcode_intel *mc)
 {
-    const struct microcode_header_intel *mc_header = mc;
+    const struct microcode_header_intel *mc_header = &mc->hdr;
     const struct extended_sigtable *ext_header = NULL;
     const struct extended_signature *ext_sig;
     unsigned long total_size, data_size, ext_table_size;
@@ -153,7 +153,7 @@ static int microcode_sanity_check(const void *mc)
                    "Small exttable size in microcode data file\n");
             return -EINVAL;
         }
-        ext_header = mc + MC_HEADER_SIZE + data_size;
+        ext_header = (void *)mc + MC_HEADER_SIZE + data_size;
         if ( ext_table_size != exttable_size(ext_header) )
         {
             printk(KERN_ERR "microcode: error! "
@@ -211,8 +211,9 @@ static int microcode_sanity_check(const void *mc)
 
 /* Check an update against the CPU signature and current update revision */
 static enum microcode_match_result microcode_update_match(
-    const struct microcode_header_intel *mc_header)
+    const struct microcode_intel *mc)
 {
+    const struct microcode_header_intel *mc_header = &mc->hdr;
     const struct extended_sigtable *ext_header;
     const struct extended_signature *ext_sig;
     unsigned int i;
@@ -223,7 +224,7 @@ static enum microcode_match_result microcode_update_match(
     unsigned long data_size = get_datasize(mc_header);
     const void *end = (const void *)mc_header + get_totalsize(mc_header);
 
-    ASSERT(!microcode_sanity_check(mc_header));
+    ASSERT(!microcode_sanity_check(mc));
     if ( sigmatch(sig, mc_header->sig, pf, mc_header->pf) )
         return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
 
@@ -249,7 +250,7 @@ static bool match_cpu(const struct microcode_patch *patch)
     if ( !patch )
         return false;
 
-    return microcode_update_match(&patch->mc_intel->hdr) == NEW_UCODE;
+    return microcode_update_match(patch->mc_intel) == NEW_UCODE;
 }
 
 static void free_patch(struct microcode_patch *patch)
@@ -268,8 +269,8 @@ static enum microcode_match_result compare_patch(
      * Both patches to compare are supposed to be applicable to local CPU.
      * Just compare the revision number.
      */
-    ASSERT(microcode_update_match(&old->mc_intel->hdr) != MIS_UCODE);
-    ASSERT(microcode_update_match(&new->mc_intel->hdr) != MIS_UCODE);
+    ASSERT(microcode_update_match(old->mc_intel) != MIS_UCODE);
+    ASSERT(microcode_update_match(new->mc_intel) != MIS_UCODE);
 
     return (new->mc_intel->hdr.rev > old->mc_intel->hdr.rev) ? NEW_UCODE
                                                              : OLD_UCODE;
@@ -367,7 +368,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
          * If the new update covers current CPU, compare updates and store the
          * one with higher revision.
          */
-        if ( (microcode_update_match(&mc->hdr) != MIS_UCODE) &&
+        if ( (microcode_update_match(mc) != MIS_UCODE) &&
              (!saved || (mc->hdr.rev > saved->hdr.rev)) )
         {
             xfree(saved);
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/7] x86/ucode/intel: Adjust microcode_sanity_check() to not take void *

[Jan Beulich]

On 23.03.2020 11:17, Andrew Cooper wrote:
> microcode_sanity_check()'s callers actually call it with a mixture of
> microcode_intel and microcode_header_intel pointers, which is fragile.
> 
> Rework it to take struct microcode_intel *, which in turn requires
> microcode_update_match()'s type to be altered.
> 
> No functional change - compiled binary is identical.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

[Xen-devel] [PATCH 3/7] x86/ucode/intel: Remove gratuitous memory allocations from cpu_request_microcode()

[Andrew Cooper]

cpu_request_microcode() needs to scan its container and duplicate one blob,
but the get_next_ucode_from_buffer() helper duplicates every blob in turn.
Furthermore, the length checking is only safe from overflow in 64bit builds.

Delete get_next_ucode_from_buffer() and alter the purpose of the saved
variable to simply point somewhere in buf until we're ready to return.

This is only a modest reduction in absolute code size (-144), but avoids
making memory allocations for every blob in the container.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpu/microcode/intel.c | 69 ++++++++++++++------------------------
 1 file changed, 26 insertions(+), 43 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index f0beefe1bb..0cceac6255 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -321,75 +321,58 @@ static int apply_microcode(const struct microcode_patch *patch)
     return 0;
 }
 
-static long get_next_ucode_from_buffer(struct microcode_intel **mc,
-                                       const uint8_t *buf, unsigned long size,
-                                       unsigned long offset)
-{
-    struct microcode_header_intel *mc_header;
-    unsigned long total_size;
-
-    /* No more data */
-    if ( offset >= size )
-        return 0;
-    mc_header = (struct microcode_header_intel *)(buf + offset);
-    total_size = get_totalsize(mc_header);
-
-    if ( (offset + total_size) > size )
-    {
-        printk(KERN_ERR "microcode: error! Bad data in microcode data file\n");
-        return -EINVAL;
-    }
-
-    *mc = xmemdup_bytes(mc_header, total_size);
-    if ( *mc == NULL )
-        return -ENOMEM;
-
-    return offset + total_size;
-}
-
 static struct microcode_patch *cpu_request_microcode(const void *buf,
                                                      size_t size)
 {
-    long offset = 0;
     int error = 0;
-    struct microcode_intel *mc, *saved = NULL;
+    const struct microcode_intel *saved = NULL;
     struct microcode_patch *patch = NULL;
 
-    while ( (offset = get_next_ucode_from_buffer(&mc, buf, size, offset)) > 0 )
+    while ( size )
     {
-        error = microcode_sanity_check(mc);
-        if ( error )
+        const struct microcode_intel *mc;
+        unsigned int blob_size;
+
+        if ( size < MC_HEADER_SIZE ||       /* Insufficient space for header? */
+             (mc = buf)->hdr.hdrver != 1 || /* Unrecognised header version?   */
+             mc->hdr.ldrver != 1 ||         /* Unrecognised loader version?   */
+             size < (blob_size =            /* Insufficient space for patch?  */
+                     get_totalsize(&mc->hdr)) )
         {
-            xfree(mc);
+            error = -EINVAL;
             break;
         }
 
+        error = microcode_sanity_check(mc);
+        if ( error )
+            break;
+
         /*
          * If the new update covers current CPU, compare updates and store the
          * one with higher revision.
          */
         if ( (microcode_update_match(mc) != MIS_UCODE) &&
              (!saved || (mc->hdr.rev > saved->hdr.rev)) )
-        {
-            xfree(saved);
             saved = mc;
-        }
-        else
-            xfree(mc);
+
+        buf  += blob_size;
+        size -= blob_size;
     }
-    if ( offset < 0 )
-        error = offset;
 
     if ( saved )
     {
         patch = xmalloc(struct microcode_patch);
         if ( patch )
-            patch->mc_intel = saved;
-        else
         {
-            xfree(saved);
-            error = -ENOMEM;
+            patch->mc_intel = xmemdup_bytes(saved, get_totalsize(&saved->hdr));
+            if ( !patch->mc_intel )
+            {
+                XFREE(patch);
+                error = -ENOMEM;
+            }
         }
+        else
+            error = -ENOMEM;
     }
 
     if ( error && !patch )
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/7] x86/ucode/intel: Remove gratuitous memory allocations from cpu_request_microcode()

[Jan Beulich]

On 23.03.2020 11:17, Andrew Cooper wrote:
>  static struct microcode_patch *cpu_request_microcode(const void *buf,
>                                                       size_t size)
>  {
> -    long offset = 0;
>      int error = 0;
> -    struct microcode_intel *mc, *saved = NULL;
> +    const struct microcode_intel *saved = NULL;
>      struct microcode_patch *patch = NULL;
>  
> -    while ( (offset = get_next_ucode_from_buffer(&mc, buf, size, offset)) > 0 )
> +    while ( size )
>      {
> -        error = microcode_sanity_check(mc);
> -        if ( error )
> +        const struct microcode_intel *mc;
> +        unsigned int blob_size;
> +
> +        if ( size < MC_HEADER_SIZE ||       /* Insufficient space for header? */
> +             (mc = buf)->hdr.hdrver != 1 || /* Unrecognised header version?   */
> +             mc->hdr.ldrver != 1 ||         /* Unrecognised loader version?   */

These two duplicate what microcode_sanity_check() does,
plus the function issues a log message when either check
fails. Since I think ...

> +             size < (blob_size =            /* Insufficient space for patch?  */
> +                     get_totalsize(&mc->hdr)) )

... this wants to come after the two version checks, how
about lifting the printk() here? The other user of
microcode_sanity_check() is an ASSERT(), where it's rather
less relevant to have such a printk().

Jan

[Xen-devel] [PATCH 4/7] x86/ucode/intel: Reimplement get_{data, total}size() helpers

[Andrew Cooper]

Every caller actually passes a struct microcode_header_intel.  Implement the
helpers with proper types, and leave a comment explaining the Pentium Pro/II
behaviour with empty {data,total}size fields.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpu/microcode/intel.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 0cceac6255..dfe44679be 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -46,9 +46,16 @@ struct microcode_header_intel {
     unsigned int sig;
     unsigned int cksum;
     unsigned int ldrver;
+
+    /*
+     * Microcode for the Pentium Pro and II had all further fields in the
+     * header reserved, had a fixed datasize of 2000 and totalsize of 2048,
+     * and didn't use platform flags despite the availability of the MSR.
+     */
+
     unsigned int pf;
-    unsigned int datasize;
-    unsigned int totalsize;
+    unsigned int _datasize;
+    unsigned int _totalsize;
     unsigned int reserved[3];
 };
 
@@ -75,20 +82,21 @@ struct microcode_patch {
     struct microcode_intel *mc_intel;
 };
 
-#define DEFAULT_UCODE_DATASIZE  (2000)
+#define PPRO_UCODE_DATASIZE     2000
 #define MC_HEADER_SIZE          (sizeof(struct microcode_header_intel))
-#define DEFAULT_UCODE_TOTALSIZE (DEFAULT_UCODE_DATASIZE + MC_HEADER_SIZE)
 #define EXT_HEADER_SIZE         (sizeof(struct extended_sigtable))
 #define EXT_SIGNATURE_SIZE      (sizeof(struct extended_signature))
 #define DWSIZE                  (sizeof(u32))
-#define get_totalsize(mc) \
-        (((struct microcode_intel *)mc)->hdr.totalsize ? \
-         ((struct microcode_intel *)mc)->hdr.totalsize : \
-         DEFAULT_UCODE_TOTALSIZE)
-
-#define get_datasize(mc) \
-        (((struct microcode_intel *)mc)->hdr.datasize ? \
-         ((struct microcode_intel *)mc)->hdr.datasize : DEFAULT_UCODE_DATASIZE)
+
+static uint32_t get_datasize(const struct microcode_header_intel *hdr)
+{
+    return hdr->_datasize ?: PPRO_UCODE_DATASIZE;
+}
+
+static uint32_t get_totalsize(const struct microcode_header_intel *hdr)
+{
+    return hdr->_totalsize ?: PPRO_UCODE_DATASIZE + MC_HEADER_SIZE;
+}
 
 #define sigmatch(s1, s2, p1, p2) \
         (((s1) == (s2)) && (((p1) & (p2)) || (((p1) == 0) && ((p2) == 0))))
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/7] x86/ucode/intel: Reimplement get_{data, total}size() helpers

[Jan Beulich]

On 23.03.2020 11:17, Andrew Cooper wrote:
> Every caller actually passes a struct microcode_header_intel.  Implement the
> helpers with proper types, and leave a comment explaining the Pentium Pro/II
> behaviour with empty {data,total}size fields.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with...

> --- a/xen/arch/x86/cpu/microcode/intel.c
> +++ b/xen/arch/x86/cpu/microcode/intel.c
> @@ -46,9 +46,16 @@ struct microcode_header_intel {
>      unsigned int sig;
>      unsigned int cksum;
>      unsigned int ldrver;
> +
> +    /*
> +     * Microcode for the Pentium Pro and II had all further fields in the
> +     * header reserved, had a fixed datasize of 2000 and totalsize of 2048,
> +     * and didn't use platform flags despite the availability of the MSR.
> +     */
> +
>      unsigned int pf;
> -    unsigned int datasize;
> -    unsigned int totalsize;
> +    unsigned int _datasize;
> +    unsigned int _totalsize;

... the underscores here dropped again. Or else - why did you add
them? This (to me at least) doesn't e.g. make any more clear that
the fields may be zero on old hardware.

Furthermore - do we really need this PPro/PentiumII logic seeing
that these aren't 64-bit capable CPUs?

Jan

Re: [Xen-devel] [PATCH 4/7] x86/ucode/intel: Reimplement get_{data, total}size() helpers

[Andrew Cooper]

On 25/03/2020 13:41, Jan Beulich wrote:
> On 23.03.2020 11:17, Andrew Cooper wrote:
>> Every caller actually passes a struct microcode_header_intel.  Implement the
>> helpers with proper types, and leave a comment explaining the Pentium Pro/II
>> behaviour with empty {data,total}size fields.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with...
>
>> --- a/xen/arch/x86/cpu/microcode/intel.c
>> +++ b/xen/arch/x86/cpu/microcode/intel.c
>> @@ -46,9 +46,16 @@ struct microcode_header_intel {
>>      unsigned int sig;
>>      unsigned int cksum;
>>      unsigned int ldrver;
>> +
>> +    /*
>> +     * Microcode for the Pentium Pro and II had all further fields in the
>> +     * header reserved, had a fixed datasize of 2000 and totalsize of 2048,
>> +     * and didn't use platform flags despite the availability of the MSR.
>> +     */
>> +
>>      unsigned int pf;
>> -    unsigned int datasize;
>> -    unsigned int totalsize;
>> +    unsigned int _datasize;
>> +    unsigned int _totalsize;
> ... the underscores here dropped again. Or else - why did you add
> them? This (to me at least) doesn't e.g. make any more clear that
> the fields may be zero on old hardware.

No, but it is our normal hint that you shouldn't be using the field
directly, and should be using the accessors instead.

> Furthermore - do we really need this PPro/PentiumII logic seeing
> that these aren't 64-bit capable CPUs?

I did actually drop support in one version of my series, but put it back in.

These old microcode blobs are still around, including in some versions
of microcode.dat.  By dropping the ability to recognise them as
legitimate, we'd break the logic to search through a container of
multiple blobs to find the one which matches.

~Andrew

Re: [Xen-devel] [PATCH 4/7] x86/ucode/intel: Reimplement get_{data, total}size() helpers

[Jan Beulich]

On 26.03.2020 15:35, Andrew Cooper wrote:
> On 25/03/2020 13:41, Jan Beulich wrote:
>> On 23.03.2020 11:17, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/cpu/microcode/intel.c
>>> +++ b/xen/arch/x86/cpu/microcode/intel.c
>>> @@ -46,9 +46,16 @@ struct microcode_header_intel {
>>>      unsigned int sig;
>>>      unsigned int cksum;
>>>      unsigned int ldrver;
>>> +
>>> +    /*
>>> +     * Microcode for the Pentium Pro and II had all further fields in the
>>> +     * header reserved, had a fixed datasize of 2000 and totalsize of 2048,
>>> +     * and didn't use platform flags despite the availability of the MSR.
>>> +     */
>>> +
>>>      unsigned int pf;
>>> -    unsigned int datasize;
>>> -    unsigned int totalsize;
>>> +    unsigned int _datasize;
>>> +    unsigned int _totalsize;
>> ... the underscores here dropped again. Or else - why did you add
>> them? This (to me at least) doesn't e.g. make any more clear that
>> the fields may be zero on old hardware.
> 
> No, but it is our normal hint that you shouldn't be using the field
> directly, and should be using the accessors instead.

Yet in patch 5 you do. Perhaps for an understandable reason, but
that way you at least partly invalidate what you say above.

>> Furthermore - do we really need this PPro/PentiumII logic seeing
>> that these aren't 64-bit capable CPUs?
> 
> I did actually drop support in one version of my series, but put it back in.
> 
> These old microcode blobs are still around, including in some versions
> of microcode.dat.  By dropping the ability to recognise them as
> legitimate, we'd break the logic to search through a container of
> multiple blobs to find the one which matches.

Oh, good point.

Jan

Re: [Xen-devel] [PATCH 4/7] x86/ucode/intel: Reimplement get_{data, total}size() helpers

[Andrew Cooper]

On 26/03/2020 14:56, Jan Beulich wrote:
> On 26.03.2020 15:35, Andrew Cooper wrote:
>> On 25/03/2020 13:41, Jan Beulich wrote:
>>> On 23.03.2020 11:17, Andrew Cooper wrote:
>>>> --- a/xen/arch/x86/cpu/microcode/intel.c
>>>> +++ b/xen/arch/x86/cpu/microcode/intel.c
>>>> @@ -46,9 +46,16 @@ struct microcode_header_intel {
>>>>      unsigned int sig;
>>>>      unsigned int cksum;
>>>>      unsigned int ldrver;
>>>> +
>>>> +    /*
>>>> +     * Microcode for the Pentium Pro and II had all further fields in the
>>>> +     * header reserved, had a fixed datasize of 2000 and totalsize of 2048,
>>>> +     * and didn't use platform flags despite the availability of the MSR.
>>>> +     */
>>>> +
>>>>      unsigned int pf;
>>>> -    unsigned int datasize;
>>>> -    unsigned int totalsize;
>>>> +    unsigned int _datasize;
>>>> +    unsigned int _totalsize;
>>> ... the underscores here dropped again. Or else - why did you add
>>> them? This (to me at least) doesn't e.g. make any more clear that
>>> the fields may be zero on old hardware.
>> No, but it is our normal hint that you shouldn't be using the field
>> directly, and should be using the accessors instead.
> Yet in patch 5 you do. Perhaps for an understandable reason, but
> that way you at least partly invalidate what you say above.

The net result of of patch 5 is three adjacent helpers, which are the
only code which use the fields themselves.

I can drop if you really insist.  We're only talking about 400 lines or
code, or thereabouts.

>>> Furthermore - do we really need this PPro/PentiumII logic seeing
>>> that these aren't 64-bit capable CPUs?
>> I did actually drop support in one version of my series, but put it back in.
>>
>> These old microcode blobs are still around, including in some versions
>> of microcode.dat.  By dropping the ability to recognise them as
>> legitimate, we'd break the logic to search through a container of
>> multiple blobs to find the one which matches.
> Oh, good point.

Shame I only came to that realisation after having reworked the series
to take it out...

I'm constructing companion document to
https://xenbits.xen.org/docs/sphinx-unstable/admin-guide/microcode-loading.html
which will live in hypervisor-guide section, and cover a whole range of
topics like this.

~Andrew

Re: [Xen-devel] [PATCH 4/7] x86/ucode/intel: Reimplement get_{data, total}size() helpers

[Jan Beulich]

On 26.03.2020 16:09, Andrew Cooper wrote:
> On 26/03/2020 14:56, Jan Beulich wrote:
>> On 26.03.2020 15:35, Andrew Cooper wrote:
>>> On 25/03/2020 13:41, Jan Beulich wrote:
>>>> On 23.03.2020 11:17, Andrew Cooper wrote:
>>>>> --- a/xen/arch/x86/cpu/microcode/intel.c
>>>>> +++ b/xen/arch/x86/cpu/microcode/intel.c
>>>>> @@ -46,9 +46,16 @@ struct microcode_header_intel {
>>>>>      unsigned int sig;
>>>>>      unsigned int cksum;
>>>>>      unsigned int ldrver;
>>>>> +
>>>>> +    /*
>>>>> +     * Microcode for the Pentium Pro and II had all further fields in the
>>>>> +     * header reserved, had a fixed datasize of 2000 and totalsize of 2048,
>>>>> +     * and didn't use platform flags despite the availability of the MSR.
>>>>> +     */
>>>>> +
>>>>>      unsigned int pf;
>>>>> -    unsigned int datasize;
>>>>> -    unsigned int totalsize;
>>>>> +    unsigned int _datasize;
>>>>> +    unsigned int _totalsize;
>>>> ... the underscores here dropped again. Or else - why did you add
>>>> them? This (to me at least) doesn't e.g. make any more clear that
>>>> the fields may be zero on old hardware.
>>> No, but it is our normal hint that you shouldn't be using the field
>>> directly, and should be using the accessors instead.
>> Yet in patch 5 you do. Perhaps for an understandable reason, but
>> that way you at least partly invalidate what you say above.
> 
> The net result of of patch 5 is three adjacent helpers, which are the
> only code which use the fields themselves.
> 
> I can drop if you really insist.  We're only talking about 400 lines or
> code, or thereabouts.

Well, I find it odd this way, but no, if you're convinced it's better,
I'm not going to insist.

Jan

[Xen-devel] [PATCH 5/7] x86/ucode/intel: Clean up microcode_update_match()

[Andrew Cooper]

Implement a new get_ext_sigtable() helper to abstract the logic for
identifying whether an extended signature table exists.  As part of this,
rename microcode_intel.bits to data and change its type so it can be usefully
used in combination with the datasize header field.

Also, replace the sigmatch() macro with a static inline with a more useful
API, and an explanation of why it is safe to drop one of the previous
conditionals.

No practical change in behaviour.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpu/microcode/intel.c | 75 +++++++++++++++++++++++++-------------
 1 file changed, 49 insertions(+), 26 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index dfe44679be..bc3bbf139e 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -61,7 +61,7 @@ struct microcode_header_intel {
 
 struct microcode_intel {
     struct microcode_header_intel hdr;
-    unsigned int bits[0];
+    uint8_t data[];
 };
 
 /* microcode format is extended from prescott processors */
@@ -98,8 +98,41 @@ static uint32_t get_totalsize(const struct microcode_header_intel *hdr)
     return hdr->_totalsize ?: PPRO_UCODE_DATASIZE + MC_HEADER_SIZE;
 }
 
-#define sigmatch(s1, s2, p1, p2) \
-        (((s1) == (s2)) && (((p1) & (p2)) || (((p1) == 0) && ((p2) == 0))))
+/*
+ * A piece of microcode has an extended signature table if there is space
+ * between the end of data[] and the total size.  (This logic also works
+ * appropriately for Pentium Pro/II microcode, which has 0 for both size
+ * fields, and no extended signature table.)
+ */
+static const struct extended_sigtable *get_ext_sigtable(
+    const struct microcode_intel *mc)
+{
+    if ( mc->hdr._totalsize > (MC_HEADER_SIZE + mc->hdr._datasize) )
+        return (void *)&mc->data[mc->hdr._datasize];
+
+    return NULL;
+}
+
+/*
+ * A piece of microcode is applicable for a CPU if:
+ *  1) the signatures (CPUID.1.EAX - Family/Model/Stepping) match, and
+ *  2) The Platform Flags bitmap intersect.
+ *
+ * A CPU will have a single Platform Flag bit, while the microcode may be
+ * common to multiple platforms and have multiple bits set.
+ *
+ * Note: The Pentium Pro/II microcode didn't use platform flags, and should
+ * treat 0 as a match.  However, Xen being 64bit means that the cpu signature
+ * won't match, allowing us to simplify the logic.
+ */
+static bool signature_maches(const struct cpu_signature *cpu_sig,
+                             unsigned int ucode_sig, unsigned int ucode_pf)
+{
+    if ( cpu_sig->sig != ucode_sig )
+        return false;
+
+    return cpu_sig->pf & ucode_pf;
+}
 
 #define exttable_size(et) ((et)->count * EXT_SIGNATURE_SIZE + EXT_HEADER_SIZE)
 
@@ -221,36 +254,26 @@ static int microcode_sanity_check(const struct microcode_intel *mc)
 static enum microcode_match_result microcode_update_match(
     const struct microcode_intel *mc)
 {
-    const struct microcode_header_intel *mc_header = &mc->hdr;
-    const struct extended_sigtable *ext_header;
-    const struct extended_signature *ext_sig;
+    const struct extended_sigtable *ext;
     unsigned int i;
     struct cpu_signature *cpu_sig = &this_cpu(cpu_sig);
-    unsigned int sig = cpu_sig->sig;
-    unsigned int pf = cpu_sig->pf;
-    unsigned int rev = cpu_sig->rev;
-    unsigned long data_size = get_datasize(mc_header);
-    const void *end = (const void *)mc_header + get_totalsize(mc_header);
 
     ASSERT(!microcode_sanity_check(mc));
-    if ( sigmatch(sig, mc_header->sig, pf, mc_header->pf) )
-        return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
 
-    ext_header = (const void *)(mc_header + 1) + data_size;
-    ext_sig = (const void *)(ext_header + 1);
+    /* Check the main microcode signature. */
+    if ( signature_maches(cpu_sig, mc->hdr.sig, mc->hdr.pf) )
+        goto found;
 
-    /*
-     * Make sure there is enough space to hold an extended header and enough
-     * array elements.
-     */
-    if ( end <= (const void *)ext_sig )
-        return MIS_UCODE;
-
-    for ( i = 0; i < ext_header->count; i++ )
-        if ( sigmatch(sig, ext_sig[i].sig, pf, ext_sig[i].pf) )
-            return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
+    /* If there is an extended signature table, check each of them. */
+    if ( (ext = get_ext_sigtable(mc)) != NULL )
+        for ( i = 0; i < ext->count; ++i )
+            if ( signature_maches(cpu_sig, ext->sigs[i].sig, ext->sigs[i].pf) )
+                goto found;
 
     return MIS_UCODE;
+
+ found:
+    return mc->hdr.rev > cpu_sig->rev ? NEW_UCODE : OLD_UCODE;
 }
 
 static bool match_cpu(const struct microcode_patch *patch)
@@ -303,7 +326,7 @@ static int apply_microcode(const struct microcode_patch *patch)
     BUG_ON(local_irq_is_enabled());
 
     /* write microcode via MSR 0x79 */
-    wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc_intel->bits);
+    wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc_intel->data);
     wrmsrl(MSR_IA32_UCODE_REV, 0x0ULL);
 
     /* As documented in the SDM: Do a CPUID 1 here */
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 5/7] x86/ucode/intel: Clean up microcode_update_match()

[Jan Beulich]

On 23.03.2020 11:17, Andrew Cooper wrote:
> Implement a new get_ext_sigtable() helper to abstract the logic for
> identifying whether an extended signature table exists.  As part of this,
> rename microcode_intel.bits to data and change its type so it can be usefully
> used in combination with the datasize header field.
> 
> Also, replace the sigmatch() macro with a static inline with a more useful
> API, and an explanation of why it is safe to drop one of the previous
> conditionals.
> 
> No practical change in behaviour.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/cpu/microcode/intel.c | 75 +++++++++++++++++++++++++-------------
>  1 file changed, 49 insertions(+), 26 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
> index dfe44679be..bc3bbf139e 100644
> --- a/xen/arch/x86/cpu/microcode/intel.c
> +++ b/xen/arch/x86/cpu/microcode/intel.c
> @@ -61,7 +61,7 @@ struct microcode_header_intel {
>  
>  struct microcode_intel {
>      struct microcode_header_intel hdr;
> -    unsigned int bits[0];
> +    uint8_t data[];
>  };
>  
>  /* microcode format is extended from prescott processors */
> @@ -98,8 +98,41 @@ static uint32_t get_totalsize(const struct microcode_header_intel *hdr)
>      return hdr->_totalsize ?: PPRO_UCODE_DATASIZE + MC_HEADER_SIZE;
>  }
>  
> -#define sigmatch(s1, s2, p1, p2) \
> -        (((s1) == (s2)) && (((p1) & (p2)) || (((p1) == 0) && ((p2) == 0))))
> +/*
> + * A piece of microcode has an extended signature table if there is space
> + * between the end of data[] and the total size.  (This logic also works
> + * appropriately for Pentium Pro/II microcode, which has 0 for both size
> + * fields, and no extended signature table.)
> + */
> +static const struct extended_sigtable *get_ext_sigtable(
> +    const struct microcode_intel *mc)
> +{
> +    if ( mc->hdr._totalsize > (MC_HEADER_SIZE + mc->hdr._datasize) )
> +        return (void *)&mc->data[mc->hdr._datasize];
> +
> +    return NULL;
> +}
> +
> +/*
> + * A piece of microcode is applicable for a CPU if:
> + *  1) the signatures (CPUID.1.EAX - Family/Model/Stepping) match, and
> + *  2) The Platform Flags bitmap intersect.
> + *
> + * A CPU will have a single Platform Flag bit, while the microcode may be
> + * common to multiple platforms and have multiple bits set.
> + *
> + * Note: The Pentium Pro/II microcode didn't use platform flags, and should
> + * treat 0 as a match.  However, Xen being 64bit means that the cpu signature
> + * won't match, allowing us to simplify the logic.
> + */
> +static bool signature_maches(const struct cpu_signature *cpu_sig,
> +                             unsigned int ucode_sig, unsigned int ucode_pf)

I guess you've lost a 't' here and really mean signature_matches()?
If so with this taken care of
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [Xen-devel] [PATCH 5/7] x86/ucode/intel: Clean up microcode_update_match()

[Andrew Cooper]

On 25/03/2020 13:51, Jan Beulich wrote:
> On 23.03.2020 11:17, Andrew Cooper wrote:
>> Implement a new get_ext_sigtable() helper to abstract the logic for
>> identifying whether an extended signature table exists.  As part of this,
>> rename microcode_intel.bits to data and change its type so it can be usefully
>> used in combination with the datasize header field.
>>
>> Also, replace the sigmatch() macro with a static inline with a more useful
>> API, and an explanation of why it is safe to drop one of the previous
>> conditionals.
>>
>> No practical change in behaviour.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wl@xen.org>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>>  xen/arch/x86/cpu/microcode/intel.c | 75 +++++++++++++++++++++++++-------------
>>  1 file changed, 49 insertions(+), 26 deletions(-)
>>
>> diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
>> index dfe44679be..bc3bbf139e 100644
>> --- a/xen/arch/x86/cpu/microcode/intel.c
>> +++ b/xen/arch/x86/cpu/microcode/intel.c
>> @@ -61,7 +61,7 @@ struct microcode_header_intel {
>>  
>>  struct microcode_intel {
>>      struct microcode_header_intel hdr;
>> -    unsigned int bits[0];
>> +    uint8_t data[];
>>  };
>>  
>>  /* microcode format is extended from prescott processors */
>> @@ -98,8 +98,41 @@ static uint32_t get_totalsize(const struct microcode_header_intel *hdr)
>>      return hdr->_totalsize ?: PPRO_UCODE_DATASIZE + MC_HEADER_SIZE;
>>  }
>>  
>> -#define sigmatch(s1, s2, p1, p2) \
>> -        (((s1) == (s2)) && (((p1) & (p2)) || (((p1) == 0) && ((p2) == 0))))
>> +/*
>> + * A piece of microcode has an extended signature table if there is space
>> + * between the end of data[] and the total size.  (This logic also works
>> + * appropriately for Pentium Pro/II microcode, which has 0 for both size
>> + * fields, and no extended signature table.)
>> + */
>> +static const struct extended_sigtable *get_ext_sigtable(
>> +    const struct microcode_intel *mc)
>> +{
>> +    if ( mc->hdr._totalsize > (MC_HEADER_SIZE + mc->hdr._datasize) )
>> +        return (void *)&mc->data[mc->hdr._datasize];
>> +
>> +    return NULL;
>> +}
>> +
>> +/*
>> + * A piece of microcode is applicable for a CPU if:
>> + *  1) the signatures (CPUID.1.EAX - Family/Model/Stepping) match, and
>> + *  2) The Platform Flags bitmap intersect.
>> + *
>> + * A CPU will have a single Platform Flag bit, while the microcode may be
>> + * common to multiple platforms and have multiple bits set.
>> + *
>> + * Note: The Pentium Pro/II microcode didn't use platform flags, and should
>> + * treat 0 as a match.  However, Xen being 64bit means that the cpu signature
>> + * won't match, allowing us to simplify the logic.
>> + */
>> +static bool signature_maches(const struct cpu_signature *cpu_sig,
>> +                             unsigned int ucode_sig, unsigned int ucode_pf)
> I guess you've lost a 't' here and really mean signature_matches()?

Oops - completely missed that.  Will fix.

> If so with this taken care of
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks,

~Andrew

[Xen-devel] [PATCH 6/7] x86/ucode/intel: Clean up microcode_sanity_check()

[Andrew Cooper]

Rewrite the size checks in a way which which doesn't depend on Xen being
compiled as 64bit.

Introduce a check missing from the old code, that total_size is a multiple of
1024 bytes, and drop unnecessarily defines/macros/structures.

No practical change in behaviour.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpu/microcode/intel.c | 147 +++++++++++++++----------------------
 1 file changed, 58 insertions(+), 89 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index bc3bbf139e..2cccf9c26d 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -65,17 +65,15 @@ struct microcode_intel {
 };
 
 /* microcode format is extended from prescott processors */
-struct extended_signature {
-    unsigned int sig;
-    unsigned int pf;
-    unsigned int cksum;
-};
-
 struct extended_sigtable {
     unsigned int count;
     unsigned int cksum;
     unsigned int reserved[3];
-    struct extended_signature sigs[0];
+    struct {
+        unsigned int sig;
+        unsigned int pf;
+        unsigned int cksum;
+    } sigs[];
 };
 
 struct microcode_patch {
@@ -84,9 +82,6 @@ struct microcode_patch {
 
 #define PPRO_UCODE_DATASIZE     2000
 #define MC_HEADER_SIZE          (sizeof(struct microcode_header_intel))
-#define EXT_HEADER_SIZE         (sizeof(struct extended_sigtable))
-#define EXT_SIGNATURE_SIZE      (sizeof(struct extended_signature))
-#define DWSIZE                  (sizeof(u32))
 
 static uint32_t get_datasize(const struct microcode_header_intel *hdr)
 {
@@ -134,8 +129,6 @@ static bool signature_maches(const struct cpu_signature *cpu_sig,
     return cpu_sig->pf & ucode_pf;
 }
 
-#define exttable_size(et) ((et)->count * EXT_SIGNATURE_SIZE + EXT_HEADER_SIZE)
-
 static int collect_cpu_info(struct cpu_signature *csig)
 {
     uint64_t msr_content;
@@ -160,93 +153,69 @@ static int collect_cpu_info(struct cpu_signature *csig)
     return 0;
 }
 
+/*
+ * Sanity check a blob which is expected to be a microcode patch.  The 48 byte
+ * header is of a known format, and together with totalsize are within the
+ * bounds of the container.  Everything else is unchecked.
+ */
 static int microcode_sanity_check(const struct microcode_intel *mc)
 {
-    const struct microcode_header_intel *mc_header = &mc->hdr;
-    const struct extended_sigtable *ext_header = NULL;
-    const struct extended_signature *ext_sig;
-    unsigned long total_size, data_size, ext_table_size;
-    unsigned int ext_sigcount = 0, i;
-    uint32_t sum, orig_sum;
-
-    total_size = get_totalsize(mc_header);
-    data_size = get_datasize(mc_header);
-    if ( (data_size + MC_HEADER_SIZE) > total_size )
-    {
-        printk(KERN_ERR "microcode: error! "
-               "Bad data size in microcode data file\n");
+    const struct extended_sigtable *ext;
+    unsigned int total_size = get_totalsize(&mc->hdr);
+    unsigned int data_size = get_datasize(&mc->hdr);
+    unsigned int i, ext_size;
+    uint32_t sum, *ptr;
+
+    /*
+     * Total size must be a multiple of 1024 bytes.  Data size and the header
+     * must fit within it.
+     */
+    if ( (total_size & 1023) ||
+         data_size > (total_size - MC_HEADER_SIZE) )
         return -EINVAL;
-    }
 
-    if ( (mc_header->ldrver != 1) || (mc_header->hdrver != 1) )
-    {
-        printk(KERN_ERR "microcode: error! "
-               "Unknown microcode update format\n");
+    /* Checksum the main header and data. */
+    for ( sum = 0, ptr = (uint32_t *)mc;
+          ptr < (uint32_t *)&mc->data[data_size]; ++ptr )
+        sum += *ptr;
+
+    if ( sum != 0 )
         return -EINVAL;
-    }
-    ext_table_size = total_size - (MC_HEADER_SIZE + data_size);
-    if ( ext_table_size )
-    {
-        if ( (ext_table_size < EXT_HEADER_SIZE) ||
-             ((ext_table_size - EXT_HEADER_SIZE) % EXT_SIGNATURE_SIZE) )
-        {
-            printk(KERN_ERR "microcode: error! "
-                   "Small exttable size in microcode data file\n");
-            return -EINVAL;
-        }
-        ext_header = (void *)mc + MC_HEADER_SIZE + data_size;
-        if ( ext_table_size != exttable_size(ext_header) )
-        {
-            printk(KERN_ERR "microcode: error! "
-                   "Bad exttable size in microcode data file\n");
-            return -EFAULT;
-        }
-        ext_sigcount = ext_header->count;
-    }
 
-    /* check extended table checksum */
-    if ( ext_table_size )
-    {
-        uint32_t ext_table_sum = 0;
-        uint32_t *ext_tablep = (uint32_t *)ext_header;
+    /* Look to see if there is an extended signature table. */
+    ext_size = total_size - data_size - MC_HEADER_SIZE;
 
-        i = ext_table_size / DWSIZE;
-        while ( i-- )
-            ext_table_sum += ext_tablep[i];
-        if ( ext_table_sum )
-        {
-            printk(KERN_WARNING "microcode: aborting, "
-                   "bad extended signature table checksum\n");
-            return -EINVAL;
-        }
-    }
+    /* No extended signature table?  All done. */
+    if ( ext_size == 0 )
+        return 0;
 
-    /* calculate the checksum */
-    orig_sum = 0;
-    i = (MC_HEADER_SIZE + data_size) / DWSIZE;
-    while ( i-- )
-        orig_sum += ((uint32_t *)mc)[i];
-    if ( orig_sum )
-    {
-        printk(KERN_ERR "microcode: aborting, bad checksum\n");
+    /*
+     * Check the structure of the extended signature table, ensuring that it
+     * fits exactly in the remaining space.
+     */
+    ext = (void *)&mc->data[data_size];
+    if ( ext_size < sizeof(*ext) ||
+         (ext_size - sizeof(*ext)) % sizeof(ext->sigs[0]) ||
+         (ext_size - sizeof(*ext)) / sizeof(ext->sigs[0]) != ext->count )
         return -EINVAL;
-    }
-    if ( !ext_table_size )
-        return 0;
-    /* check extended signature checksum */
-    for ( i = 0; i < ext_sigcount; i++ )
-    {
-        ext_sig = (void *)ext_header + EXT_HEADER_SIZE +
-            EXT_SIGNATURE_SIZE * i;
-        sum = orig_sum
-            - (mc_header->sig + mc_header->pf + mc_header->cksum)
-            + (ext_sig->sig + ext_sig->pf + ext_sig->cksum);
-        if ( sum )
-        {
-            printk(KERN_ERR "microcode: aborting, bad checksum\n");
+
+    /* Checksum the whole extended signature table. */
+    for ( sum = 0, ptr = (uint32_t *)ext;
+          ptr < (uint32_t *)&ext->sigs[ext->count]; ++ptr )
+        sum += *ptr;
+
+    if ( sum != 0 )
+        return -EINVAL;
+
+    /*
+     * Checksum each indiviudal extended signature as if it had been in the
+     * main header.
+     */
+    sum = mc->hdr.sig + mc->hdr.pf + mc->hdr.cksum;
+    for ( i = 0; i < ext->count; ++i )
+        if ( sum != (ext->sigs[i].sig + ext->sigs[i].pf + ext->sigs[i].cksum) )
             return -EINVAL;
-        }
-    }
+
     return 0;
 }
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 6/7] x86/ucode/intel: Clean up microcode_sanity_check()

[Jan Beulich]

On 23.03.2020 11:17, Andrew Cooper wrote:
> Rewrite the size checks in a way which which doesn't depend on Xen being
> compiled as 64bit.

One too many "which"?

> Introduce a check missing from the old code, that total_size is a multiple of
> 1024 bytes,

Where is this documented? The rather brief section in SDM vol 3 doesn't
mention anything like this.

> and drop unnecessarily defines/macros/structures.

unnecessary?

> @@ -160,93 +153,69 @@ static int collect_cpu_info(struct cpu_signature *csig)
>      return 0;
>  }
>  
> +/*
> + * Sanity check a blob which is expected to be a microcode patch.  The 48 byte
> + * header is of a known format, and together with totalsize are within the
> + * bounds of the container.  Everything else is unchecked.
> + */
>  static int microcode_sanity_check(const struct microcode_intel *mc)
>  {
> -    const struct microcode_header_intel *mc_header = &mc->hdr;
> -    const struct extended_sigtable *ext_header = NULL;
> -    const struct extended_signature *ext_sig;
> -    unsigned long total_size, data_size, ext_table_size;
> -    unsigned int ext_sigcount = 0, i;
> -    uint32_t sum, orig_sum;
> -
> -    total_size = get_totalsize(mc_header);
> -    data_size = get_datasize(mc_header);
> -    if ( (data_size + MC_HEADER_SIZE) > total_size )
> -    {
> -        printk(KERN_ERR "microcode: error! "
> -               "Bad data size in microcode data file\n");
> +    const struct extended_sigtable *ext;
> +    unsigned int total_size = get_totalsize(&mc->hdr);
> +    unsigned int data_size = get_datasize(&mc->hdr);
> +    unsigned int i, ext_size;
> +    uint32_t sum, *ptr;
> +
> +    /*
> +     * Total size must be a multiple of 1024 bytes.  Data size and the header
> +     * must fit within it.
> +     */
> +    if ( (total_size & 1023) ||

Personally I'd fine a hex number easier to recognize in cases like
this.

> +         data_size > (total_size - MC_HEADER_SIZE) )
>          return -EINVAL;
> -    }
>  
> -    if ( (mc_header->ldrver != 1) || (mc_header->hdrver != 1) )
> -    {

Ah - you're dropping this check here altogether. As said on the
earlier patch, I think this may more logically go there.

> -        printk(KERN_ERR "microcode: error! "
> -               "Unknown microcode update format\n");

While this printk() was already suggested to be moved, I'm not
convinced dropping others further down is helpful in case of
issues. We'd see just -EINVAL with no further indication of
what was (deemed) wrong.

> +    /* Checksum the main header and data. */
> +    for ( sum = 0, ptr = (uint32_t *)mc;
> +          ptr < (uint32_t *)&mc->data[data_size]; ++ptr )

You're casting away constness here which future compilers may
(legitimately) warn about. (Similarly again further down.)

Jan

Re: [Xen-devel] [PATCH 6/7] x86/ucode/intel: Clean up microcode_sanity_check()

[Andrew Cooper]

On 25/03/2020 14:07, Jan Beulich wrote:
>> Introduce a check missing from the old code, that total_size is a multiple of
>> 1024 bytes,
> Where is this documented? The rather brief section in SDM vol 3 doesn't
> mention anything like this.

It is in the middle of the final paragraph of 9.11.1 Microcode Update,
immediately preceding Table 9-7

"The total size field of the microcode update header specifies the
encrypted data size plus the header size; its value must be in multiples
of 1024 bytes (1 KBytes)."

~Andrew

(I've lost count of how many times I've read this chapter over the
course of developing this series...)

Re: [Xen-devel] [PATCH 6/7] x86/ucode/intel: Clean up microcode_sanity_check()

[Jan Beulich]

On 26.03.2020 15:41, Andrew Cooper wrote:
> On 25/03/2020 14:07, Jan Beulich wrote:
>>> Introduce a check missing from the old code, that total_size is a multiple of
>>> 1024 bytes,
>> Where is this documented? The rather brief section in SDM vol 3 doesn't
>> mention anything like this.
> 
> It is in the middle of the final paragraph of 9.11.1 Microcode Update,
> immediately preceding Table 9-7
> 
> "The total size field of the microcode update header specifies the
> encrypted data size plus the header size; its value must be in multiples
> of 1024 bytes (1 KBytes)."

Oh, I had looked at 8.8.5 Microcode Update Resources instead,
being surprised it was much less information than I recalled
was available.

Jan

[Xen-devel] [PATCH 7/7] x86/ucode/intel: Fold structures together

[Andrew Cooper]

Currently, we allocate an 8 byte struct microcode_patch to point at a
separately allocated struct microcode_intel.  This is wasteful.

Fold struct microcode_header_intel and microcode_intel into microcode_patch to
simplify the code and remove a level of indirection.

The two semantic changes are in free_patch() and cpu_request_microcode() which
deal with the memory allocation aspects.  Everything else is no functional
change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpu/microcode/intel.c | 103 +++++++++++++------------------------
 1 file changed, 37 insertions(+), 66 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 2cccf9c26d..8f4ebbd759 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -32,17 +32,12 @@
 
 #define pr_debug(x...) ((void)0)
 
-struct microcode_header_intel {
+struct microcode_patch {
     unsigned int hdrver;
     unsigned int rev;
-    union {
-        struct {
-            uint16_t year;
-            uint8_t day;
-            uint8_t month;
-        };
-        unsigned int date;
-    };
+    uint16_t year;
+    uint8_t  day;
+    uint8_t  month;
     unsigned int sig;
     unsigned int cksum;
     unsigned int ldrver;
@@ -57,10 +52,7 @@ struct microcode_header_intel {
     unsigned int _datasize;
     unsigned int _totalsize;
     unsigned int reserved[3];
-};
 
-struct microcode_intel {
-    struct microcode_header_intel hdr;
     uint8_t data[];
 };
 
@@ -76,21 +68,17 @@ struct extended_sigtable {
     } sigs[];
 };
 
-struct microcode_patch {
-    struct microcode_intel *mc_intel;
-};
-
 #define PPRO_UCODE_DATASIZE     2000
-#define MC_HEADER_SIZE          (sizeof(struct microcode_header_intel))
+#define MC_HEADER_SIZE          offsetof(struct microcode_patch, data)
 
-static uint32_t get_datasize(const struct microcode_header_intel *hdr)
+static uint32_t get_datasize(const struct microcode_patch *mc)
 {
-    return hdr->_datasize ?: PPRO_UCODE_DATASIZE;
+    return mc->_datasize ?: PPRO_UCODE_DATASIZE;
 }
 
-static uint32_t get_totalsize(const struct microcode_header_intel *hdr)
+static uint32_t get_totalsize(const struct microcode_patch *mc)
 {
-    return hdr->_totalsize ?: PPRO_UCODE_DATASIZE + MC_HEADER_SIZE;
+    return mc->_totalsize ?: PPRO_UCODE_DATASIZE + MC_HEADER_SIZE;
 }
 
 /*
@@ -100,10 +88,10 @@ static uint32_t get_totalsize(const struct microcode_header_intel *hdr)
  * fields, and no extended signature table.)
  */
 static const struct extended_sigtable *get_ext_sigtable(
-    const struct microcode_intel *mc)
+    const struct microcode_patch *mc)
 {
-    if ( mc->hdr._totalsize > (MC_HEADER_SIZE + mc->hdr._datasize) )
-        return (void *)&mc->data[mc->hdr._datasize];
+    if ( mc->_totalsize > (MC_HEADER_SIZE + mc->_datasize) )
+        return (void *)&mc->data[mc->_datasize];
 
     return NULL;
 }
@@ -158,11 +146,11 @@ static int collect_cpu_info(struct cpu_signature *csig)
  * header is of a known format, and together with totalsize are within the
  * bounds of the container.  Everything else is unchecked.
  */
-static int microcode_sanity_check(const struct microcode_intel *mc)
+static int microcode_sanity_check(const struct microcode_patch *mc)
 {
     const struct extended_sigtable *ext;
-    unsigned int total_size = get_totalsize(&mc->hdr);
-    unsigned int data_size = get_datasize(&mc->hdr);
+    unsigned int total_size = get_totalsize(mc);
+    unsigned int data_size = get_datasize(mc);
     unsigned int i, ext_size;
     uint32_t sum, *ptr;
 
@@ -211,7 +199,7 @@ static int microcode_sanity_check(const struct microcode_intel *mc)
      * Checksum each indiviudal extended signature as if it had been in the
      * main header.
      */
-    sum = mc->hdr.sig + mc->hdr.pf + mc->hdr.cksum;
+    sum = mc->sig + mc->pf + mc->cksum;
     for ( i = 0; i < ext->count; ++i )
         if ( sum != (ext->sigs[i].sig + ext->sigs[i].pf + ext->sigs[i].cksum) )
             return -EINVAL;
@@ -221,7 +209,7 @@ static int microcode_sanity_check(const struct microcode_intel *mc)
 
 /* Check an update against the CPU signature and current update revision */
 static enum microcode_match_result microcode_update_match(
-    const struct microcode_intel *mc)
+    const struct microcode_patch *mc)
 {
     const struct extended_sigtable *ext;
     unsigned int i;
@@ -230,7 +218,7 @@ static enum microcode_match_result microcode_update_match(
     ASSERT(!microcode_sanity_check(mc));
 
     /* Check the main microcode signature. */
-    if ( signature_maches(cpu_sig, mc->hdr.sig, mc->hdr.pf) )
+    if ( signature_maches(cpu_sig, mc->sig, mc->pf) )
         goto found;
 
     /* If there is an extended signature table, check each of them. */
@@ -242,7 +230,7 @@ static enum microcode_match_result microcode_update_match(
     return MIS_UCODE;
 
  found:
-    return mc->hdr.rev > cpu_sig->rev ? NEW_UCODE : OLD_UCODE;
+    return mc->rev > cpu_sig->rev ? NEW_UCODE : OLD_UCODE;
 }
 
 static bool match_cpu(const struct microcode_patch *patch)
@@ -250,16 +238,12 @@ static bool match_cpu(const struct microcode_patch *patch)
     if ( !patch )
         return false;
 
-    return microcode_update_match(patch->mc_intel) == NEW_UCODE;
+    return microcode_update_match(patch) == NEW_UCODE;
 }
 
 static void free_patch(struct microcode_patch *patch)
 {
-    if ( patch )
-    {
-        xfree(patch->mc_intel);
-        xfree(patch);
-    }
+    xfree(patch);
 }
 
 static enum microcode_match_result compare_patch(
@@ -269,11 +253,10 @@ static enum microcode_match_result compare_patch(
      * Both patches to compare are supposed to be applicable to local CPU.
      * Just compare the revision number.
      */
-    ASSERT(microcode_update_match(old->mc_intel) != MIS_UCODE);
-    ASSERT(microcode_update_match(new->mc_intel) != MIS_UCODE);
+    ASSERT(microcode_update_match(old) != MIS_UCODE);
+    ASSERT(microcode_update_match(new) != MIS_UCODE);
 
-    return (new->mc_intel->hdr.rev > old->mc_intel->hdr.rev) ? NEW_UCODE
-                                                             : OLD_UCODE;
+    return new->rev > old->rev ? NEW_UCODE : OLD_UCODE;
 }
 
 static int apply_microcode(const struct microcode_patch *patch)
@@ -281,7 +264,6 @@ static int apply_microcode(const struct microcode_patch *patch)
     uint64_t msr_content;
     unsigned int cpu = smp_processor_id();
     struct cpu_signature *sig = &this_cpu(cpu_sig);
-    const struct microcode_intel *mc_intel;
     uint32_t rev, old_rev = sig->rev;
 
     if ( !patch )
@@ -290,12 +272,10 @@ static int apply_microcode(const struct microcode_patch *patch)
     if ( !match_cpu(patch) )
         return -EINVAL;
 
-    mc_intel = patch->mc_intel;
-
     BUG_ON(local_irq_is_enabled());
 
     /* write microcode via MSR 0x79 */
-    wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc_intel->data);
+    wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)patch->data);
     wrmsrl(MSR_IA32_UCODE_REV, 0x0ULL);
 
     /* As documented in the SDM: Do a CPUID 1 here */
@@ -305,18 +285,17 @@ static int apply_microcode(const struct microcode_patch *patch)
     rdmsrl(MSR_IA32_UCODE_REV, msr_content);
     sig->rev = rev = msr_content >> 32;
 
-    if ( rev != mc_intel->hdr.rev )
+    if ( rev != patch->rev )
     {
         printk(XENLOG_ERR
                "microcode: CPU%u update rev %#x to %#x failed, result %#x\n",
-               cpu, old_rev, mc_intel->hdr.rev, rev);
+               cpu, old_rev, patch->rev, rev);
         return -EIO;
     }
 
     printk(XENLOG_WARNING
            "microcode: CPU%u updated from revision %#x to %#x, date = %04x-%02x-%02x\n",
-           cpu, old_rev, rev, mc_intel->hdr.year,
-           mc_intel->hdr.month, mc_intel->hdr.day);
+           cpu, old_rev, rev, patch->year, patch->month, patch->day);
 
     return 0;
 }
@@ -325,19 +304,19 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
                                                      size_t size)
 {
     int error = 0;
-    const struct microcode_intel *saved = NULL;
+    const struct microcode_patch *saved = NULL;
     struct microcode_patch *patch = NULL;
 
     while ( size )
     {
-        const struct microcode_intel *mc;
+        const struct microcode_patch *mc;
         unsigned int blob_size;
 
         if ( size < MC_HEADER_SIZE ||       /* Insufficient space for header? */
-             (mc = buf)->hdr.hdrver != 1 || /* Unrecognised header version?   */
-             mc->hdr.ldrver != 1 ||         /* Unrecognised loader version?   */
+             (mc = buf)->hdrver != 1 ||     /* Unrecognised header version?   */
+             mc->ldrver != 1 ||             /* Unrecognised loader version?   */
              size < (blob_size =            /* Insufficient space for patch?  */
-                     get_totalsize(&mc->hdr)) )
+                     get_totalsize(mc)) )
         {
             error = -EINVAL;
             break;
@@ -352,7 +331,7 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
          * one with higher revision.
          */
         if ( (microcode_update_match(mc) != MIS_UCODE) &&
-             (!saved || (mc->hdr.rev > saved->hdr.rev)) )
+             (!saved || (mc->rev > saved->rev)) )
             saved = mc;
 
         buf  += blob_size;
@@ -361,17 +340,9 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
 
     if ( saved )
     {
-        patch = xmalloc(struct microcode_patch);
-        if ( patch )
-        {
-            patch->mc_intel = xmemdup_bytes(saved, get_totalsize(&saved->hdr));
-            if ( !patch->mc_intel )
-            {
-                XFREE(patch);
-                error = -ENOMEM;
-            }
-        }
-        else
+        patch = xmemdup_bytes(saved, get_totalsize(saved));
+
+        if ( !patch )
             error = -ENOMEM;
     }
 
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 7/7] x86/ucode/intel: Fold structures together

[Jan Beulich]

On 23.03.2020 11:17, Andrew Cooper wrote:
> Currently, we allocate an 8 byte struct microcode_patch to point at a
> separately allocated struct microcode_intel.  This is wasteful.

As indicated elsewhere I'm very much in favor of this, but I think it
wants doing in one of the earlier series, and then for AMD at the same
time. Possibly, to limit code churn there, ...

> --- a/xen/arch/x86/cpu/microcode/intel.c
> +++ b/xen/arch/x86/cpu/microcode/intel.c
> @@ -32,17 +32,12 @@
>  
>  #define pr_debug(x...) ((void)0)
>  
> -struct microcode_header_intel {
> +struct microcode_patch {

... accompanying this with

#define microcode_header_intel microcode_patch

or even ...

> -    union {
> -        struct {
> -            uint16_t year;
> -            uint8_t day;
> -            uint8_t month;
> -        };
> -        unsigned int date;
> -    };
> +    uint16_t year;
> +    uint8_t  day;
> +    uint8_t  month;
>      unsigned int sig;
>      unsigned int cksum;
>      unsigned int ldrver;
> @@ -57,10 +52,7 @@ struct microcode_header_intel {
>      unsigned int _datasize;
>      unsigned int _totalsize;
>      unsigned int reserved[3];
> -};
>  
> -struct microcode_intel {
> -    struct microcode_header_intel hdr;
>      uint8_t data[];
>  };

... keeping the two structures separate until here, which would
make this one what would initially become struct microcode_patch.
This is in particular because ...

>  static void free_patch(struct microcode_patch *patch)
>  {
> -    if ( patch )
> -    {
> -        xfree(patch->mc_intel);
> -        xfree(patch);
> -    }
> +    xfree(patch);
>  }

... in that earlier series you've moved the 2nd xfree() here just
to now delete it again.

Jan

Re: [Xen-devel] [PATCH 7/7] x86/ucode/intel: Fold structures together

[Andrew Cooper]

On 25/03/2020 14:16, Jan Beulich wrote:
> On 23.03.2020 11:17, Andrew Cooper wrote:
>> Currently, we allocate an 8 byte struct microcode_patch to point at a
>> separately allocated struct microcode_intel.  This is wasteful.
> As indicated elsewhere I'm very much in favor of this, but I think it
> wants doing in one of the earlier series, and then for AMD at the same
> time.

I've got some ideas for an AMD series given the replies I got, and will
be able to do an equivalent microcode_amd => microcode_patch folding on
that side.  There is also further work to do, including unbreaking the
OSVW logic (which has been totally clobbered by the start/end_update
debacle).

However, given that it taken this whole series to make the transition
look safe on the Intel side, I really don't see a way of doing this
"earlier".

In particular, no amount of ifdefary suggested below can AFAICT make it
safe to do this transform without having microcode_patch opaque to being
with.

Yes - there is a bit of churn, but I can't see a safe alternative.

~Andrew

>  Possibly, to limit code churn there, ...
>
>> --- a/xen/arch/x86/cpu/microcode/intel.c
>> +++ b/xen/arch/x86/cpu/microcode/intel.c
>> @@ -32,17 +32,12 @@
>>  
>>  #define pr_debug(x...) ((void)0)
>>  
>> -struct microcode_header_intel {
>> +struct microcode_patch {
> ... accompanying this with
>
> #define microcode_header_intel microcode_patch
>
> or even ...
>
>> -    union {
>> -        struct {
>> -            uint16_t year;
>> -            uint8_t day;
>> -            uint8_t month;
>> -        };
>> -        unsigned int date;
>> -    };
>> +    uint16_t year;
>> +    uint8_t  day;
>> +    uint8_t  month;
>>      unsigned int sig;
>>      unsigned int cksum;
>>      unsigned int ldrver;
>> @@ -57,10 +52,7 @@ struct microcode_header_intel {
>>      unsigned int _datasize;
>>      unsigned int _totalsize;
>>      unsigned int reserved[3];
>> -};
>>  
>> -struct microcode_intel {
>> -    struct microcode_header_intel hdr;
>>      uint8_t data[];
>>  };
> ... keeping the two structures separate until here, which would
> make this one what would initially become struct microcode_patch.
> This is in particular because ...
>
>>  static void free_patch(struct microcode_patch *patch)
>>  {
>> -    if ( patch )
>> -    {
>> -        xfree(patch->mc_intel);
>> -        xfree(patch);
>> -    }
>> +    xfree(patch);
>>  }
> ... in that earlier series you've moved the 2nd xfree() here just
> to now delete it again.
>
> Jan

Re: [Xen-devel] [PATCH 7/7] x86/ucode/intel: Fold structures together

[Jan Beulich]

On 25.03.2020 15:32, Andrew Cooper wrote:
> On 25/03/2020 14:16, Jan Beulich wrote:
>> On 23.03.2020 11:17, Andrew Cooper wrote:
>>> Currently, we allocate an 8 byte struct microcode_patch to point at a
>>> separately allocated struct microcode_intel.  This is wasteful.
>> As indicated elsewhere I'm very much in favor of this, but I think it
>> wants doing in one of the earlier series, and then for AMD at the same
>> time.
> 
> I've got some ideas for an AMD series given the replies I got, and will
> be able to do an equivalent microcode_amd => microcode_patch folding on
> that side.  There is also further work to do, including unbreaking the
> OSVW logic (which has been totally clobbered by the start/end_update
> debacle).
> 
> However, given that it taken this whole series to make the transition
> look safe on the Intel side, I really don't see a way of doing this
> "earlier".
> 
> In particular, no amount of ifdefary suggested below can AFAICT make it
> safe to do this transform without having microcode_patch opaque to being
> with.
> 
> Yes - there is a bit of churn, but I can't see a safe alternative.

Something like the one below (compile tested only, and not really
cleaned up in any way)?

Jan

--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -60,13 +60,15 @@ struct __packed microcode_header_amd {
 #define UCODE_EQUIV_CPU_TABLE_TYPE 0x00000000
 #define UCODE_UCODE_TYPE           0x00000001
 
-struct microcode_amd {
+struct microcode_patch {
     void *mpb;
     size_t mpb_size;
     struct equiv_cpu_entry *equiv_cpu_table;
     size_t equiv_cpu_table_size;
 };
 
+#define microcode_amd microcode_patch
+
 struct mpbhdr {
     uint32_t type;
     uint32_t len;
@@ -177,13 +179,11 @@ static enum microcode_match_result micro
 
 static bool match_cpu(const struct microcode_patch *patch)
 {
-    return patch && (microcode_fits(patch->mc_amd) == NEW_UCODE);
+    return patch && (microcode_fits(patch) == NEW_UCODE);
 }
 
-static void free_patch(void *mc)
+static void free_patch(struct microcode_patch *mc_amd)
 {
-    struct microcode_amd *mc_amd = mc;
-
     if ( mc_amd )
     {
         xfree(mc_amd->equiv_cpu_table);
@@ -206,12 +206,12 @@ static enum microcode_match_result compa
 static enum microcode_match_result compare_patch(
     const struct microcode_patch *new, const struct microcode_patch *old)
 {
-    const struct microcode_header_amd *new_header = new->mc_amd->mpb;
-    const struct microcode_header_amd *old_header = old->mc_amd->mpb;
+    const struct microcode_header_amd *new_header = new->mpb;
+    const struct microcode_header_amd *old_header = old->mpb;
 
     /* Both patches to compare are supposed to be applicable to local CPU. */
-    ASSERT(microcode_fits(new->mc_amd) != MIS_UCODE);
-    ASSERT(microcode_fits(new->mc_amd) != MIS_UCODE);
+    ASSERT(microcode_fits(new) != MIS_UCODE);
+    ASSERT(microcode_fits(new) != MIS_UCODE);
 
     return compare_header(new_header, old_header);
 }
@@ -230,7 +230,7 @@ static int apply_microcode(const struct
     if ( !match_cpu(patch) )
         return -EINVAL;
 
-    hdr = patch->mc_amd->mpb;
+    hdr = patch->mpb;
 
     BUG_ON(local_irq_is_enabled());
 
@@ -412,7 +412,6 @@ static struct microcode_patch *cpu_reque
 {
     struct microcode_amd *mc_amd;
     struct microcode_header_amd *saved = NULL;
-    struct microcode_patch *patch = NULL;
     size_t offset = 0, saved_size = 0;
     int error = 0;
     unsigned int current_cpu_id;
@@ -559,23 +558,18 @@ static struct microcode_patch *cpu_reque
     {
         mc_amd->mpb = saved;
         mc_amd->mpb_size = saved_size;
-        patch = xmalloc(struct microcode_patch);
-        if ( patch )
-            patch->mc_amd = mc_amd;
-        else
-        {
-            free_patch(mc_amd);
-            error = -ENOMEM;
-        }
     }
     else
+    {
         free_patch(mc_amd);
+        mc_amd = NULL;
+    }
 
   out:
-    if ( error && !patch )
-        patch = ERR_PTR(error);
+    if ( error && !mc_amd )
+        mc_amd = ERR_PTR(error);
 
-    return patch;
+    return mc_amd;
 }
 
 #ifdef CONFIG_HVM
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -245,8 +245,7 @@ static struct microcode_patch *parse_blo
 
 static void microcode_free_patch(struct microcode_patch *microcode_patch)
 {
-    microcode_ops->free_patch(microcode_patch->mc);
-    xfree(microcode_patch);
+    microcode_ops->free_patch(microcode_patch);
 }
 
 /* Return true if cache gets updated. Otherwise, return false */
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -52,11 +52,13 @@ struct microcode_header_intel {
     unsigned int reserved[3];
 };
 
-struct microcode_intel {
+struct microcode_patch {
     struct microcode_header_intel hdr;
     unsigned int bits[0];
 };
 
+#define microcode_intel microcode_patch
+
 /* microcode format is extended from prescott processors */
 struct extended_signature {
     unsigned int sig;
@@ -245,12 +247,12 @@ static bool match_cpu(const struct micro
     if ( !patch )
         return false;
 
-    return microcode_update_match(&patch->mc_intel->hdr) == NEW_UCODE;
+    return microcode_update_match(&patch->hdr) == NEW_UCODE;
 }
 
-static void free_patch(void *mc)
+static void free_patch(struct microcode_patch *patch)
 {
-    xfree(mc);
+    xfree(patch);
 }
 
 static enum microcode_match_result compare_patch(
@@ -260,10 +262,10 @@ static enum microcode_match_result compa
      * Both patches to compare are supposed to be applicable to local CPU.
      * Just compare the revision number.
      */
-    ASSERT(microcode_update_match(&old->mc_intel->hdr) != MIS_UCODE);
-    ASSERT(microcode_update_match(&new->mc_intel->hdr) != MIS_UCODE);
+    ASSERT(microcode_update_match(&old->hdr) != MIS_UCODE);
+    ASSERT(microcode_update_match(&new->hdr) != MIS_UCODE);
 
-    return (new->mc_intel->hdr.rev > old->mc_intel->hdr.rev) ? NEW_UCODE
+    return (new->hdr.rev > old->hdr.rev) ? NEW_UCODE
                                                              : OLD_UCODE;
 }
 
@@ -281,7 +283,7 @@ static int apply_microcode(const struct
     if ( !match_cpu(patch) )
         return -EINVAL;
 
-    mc_intel = patch->mc_intel;
+    mc_intel = patch;
 
     BUG_ON(local_irq_is_enabled());
 
@@ -347,7 +349,6 @@ static struct microcode_patch *cpu_reque
     long offset = 0;
     int error = 0;
     struct microcode_intel *mc, *saved = NULL;
-    struct microcode_patch *patch = NULL;
 
     while ( (offset = get_next_ucode_from_buffer(&mc, buf, size, offset)) > 0 )
     {
@@ -374,22 +375,10 @@ static struct microcode_patch *cpu_reque
     if ( offset < 0 )
         error = offset;
 
-    if ( saved )
-    {
-        patch = xmalloc(struct microcode_patch);
-        if ( patch )
-            patch->mc_intel = saved;
-        else
-        {
-            xfree(saved);
-            error = -ENOMEM;
-        }
-    }
-
-    if ( error && !patch )
-        patch = ERR_PTR(error);
+    if ( error && !saved )
+        saved = ERR_PTR(error);
 
-    return patch;
+    return saved;
 }
 
 const struct microcode_ops intel_ucode_ops = {
--- a/xen/arch/x86/cpu/microcode/private.h
+++ b/xen/arch/x86/cpu/microcode/private.h
@@ -11,13 +11,7 @@ enum microcode_match_result {
     MIS_UCODE, /* signature mismatched */
 };
 
-struct microcode_patch {
-    union {
-        struct microcode_intel *mc_intel;
-        struct microcode_amd *mc_amd;
-        void *mc;
-    };
-};
+struct microcode_patch;
 
 struct microcode_ops {
     struct microcode_patch *(*cpu_request_microcode)(const void *buf,
@@ -26,7 +20,7 @@ struct microcode_ops {
     int (*apply_microcode)(const struct microcode_patch *patch);
     int (*start_update)(void);
     void (*end_update_percpu)(void);
-    void (*free_patch)(void *mc);
+    void (*free_patch)(struct microcode_patch *patch);
     bool (*match_cpu)(const struct microcode_patch *patch);
     enum microcode_match_result (*compare_patch)(
         const struct microcode_patch *new, const struct microcode_patch *old);

Re: [Xen-devel] [PATCH 7/7] x86/ucode/intel: Fold structures together

[Andrew Cooper]

On 26/03/2020 12:24, Jan Beulich wrote:
> On 25.03.2020 15:32, Andrew Cooper wrote:
>> On 25/03/2020 14:16, Jan Beulich wrote:
>>> On 23.03.2020 11:17, Andrew Cooper wrote:
>>>> Currently, we allocate an 8 byte struct microcode_patch to point at a
>>>> separately allocated struct microcode_intel.  This is wasteful.
>>> As indicated elsewhere I'm very much in favor of this, but I think it
>>> wants doing in one of the earlier series, and then for AMD at the same
>>> time.
>> I've got some ideas for an AMD series given the replies I got, and will
>> be able to do an equivalent microcode_amd => microcode_patch folding on
>> that side.  There is also further work to do, including unbreaking the
>> OSVW logic (which has been totally clobbered by the start/end_update
>> debacle).
>>
>> However, given that it taken this whole series to make the transition
>> look safe on the Intel side, I really don't see a way of doing this
>> "earlier".
>>
>> In particular, no amount of ifdefary suggested below can AFAICT make it
>> safe to do this transform without having microcode_patch opaque to being
>> with.
>>
>> Yes - there is a bit of churn, but I can't see a safe alternative.
> Something like the one below (compile tested only, and not really
> cleaned up in any way)?
>
> Jan

Thanks.  I'll experiment with this approach.

On a perhaps tangential note, what (if anything) are you plans regarding
backport here?

These defines are ok for a transitional period across a series (and
probably means I'll need to get the AMD side ready to be committed at
the same time), but I don't think we'd want them in the code for the
longterm.

I personally wasn't overly concerned about backports, but if you are, we
should probably take this into consideration for the fixes.

~Andrew

Re: [Xen-devel] [PATCH 7/7] x86/ucode/intel: Fold structures together

[Jan Beulich]

On 26.03.2020 15:50, Andrew Cooper wrote:
> On a perhaps tangential note, what (if anything) are you plans regarding
> backport here?
> 
> These defines are ok for a transitional period across a series (and
> probably means I'll need to get the AMD side ready to be committed at
> the same time), but I don't think we'd want them in the code for the
> longterm.
> 
> I personally wasn't overly concerned about backports, but if you are, we
> should probably take this into consideration for the fixes.

Till now I didn't see a strong reason why backporting might be
needed (or even just wanted). If you think there is one,
arranging for backport material to come first would of course
be nice. And indeed, the #define-s you mention are meant to be
there just to limit the churn of this immediate patch; I'd be
happy to see them go away in another patch immediately after.

Jan

Re: [Xen-devel] [PATCH 7/7] x86/ucode/intel: Fold structures together

[Andrew Cooper]

On 26/03/2020 15:05, Jan Beulich wrote:
> On 26.03.2020 15:50, Andrew Cooper wrote:
>> On a perhaps tangential note, what (if anything) are you plans regarding
>> backport here?
>>
>> These defines are ok for a transitional period across a series (and
>> probably means I'll need to get the AMD side ready to be committed at
>> the same time), but I don't think we'd want them in the code for the
>> longterm.
>>
>> I personally wasn't overly concerned about backports, but if you are, we
>> should probably take this into consideration for the fixes.
> Till now I didn't see a strong reason why backporting might be
> needed (or even just wanted).

The gratuitous memory allocation fixes would be the most compelling (and
even then, not massively so), but they're sufficiently interlaced with
the rest of the cleanup that I wasn't expecting backports to be a
pleasant idea.

~Andrew