diff mbox series

[v3,2/5] x86: Refactor microcode_update() hypercall with flags

Message ID 20240430124709.865183-3-fouad.hilly@cloud.com (mailing list archive)
State New
Headers show
Series x86/xen-ucode: Introduce --force option | expand

Commit Message

Fouad Hilly April 30, 2024, 12:47 p.m. UTC 
Refactor microcode_update() by adding a flags field.
struct xenpf_microcode_update2 added with uint32_t flags field.
Introduce XENPF_microcode_update2 hypercall to handle flags field.
Introduce ucode_force_flag to microcode_update_helper.

Signed-off-by: Fouad Hilly <fouad.hilly@cloud.com>
---
[v3]
1- Updated Commit message description.
2- Revereted changes to a stable ABI and introduced a new struct.
3- ucode_force_flag updated from static to a local variable.
4- microcode_update() updated to reject unsupported flags yet.

[v2]
1- Update message description to highlight interface change.
2- Removed extra empty lines.
3- removed unnecessary define.
4- Corrected long lines.
5- Removed ternary operator.
6- Introduced static ucode_update_flags, which will be used later to determine local ucode_force_flag.
 xen/arch/x86/cpu/microcode/core.c    | 15 ++++++++++++---
 xen/arch/x86/include/asm/microcode.h |  3 ++-
 xen/arch/x86/platform_hypercall.c    | 12 +++++++++++-
 xen/include/public/platform.h        | 14 ++++++++++++++
 4 files changed, 39 insertions(+), 5 deletions(-)

Comments

Jan Beulich May 6, 2024, 9:14 a.m. UTC | #1 
On 30.04.2024 14:47, Fouad Hilly wrote:
> @@ -633,12 +637,12 @@ static long cf_check microcode_update_helper(void *data)
>                                    microcode_cache);
>  
>          if ( result != NEW_UCODE &&
> -             !(opt_ucode_allow_same && result == SAME_UCODE) )
> +             !((opt_ucode_allow_same || ucode_force_flag) && result == SAME_UCODE) )

Why would "force" not also allow a downgrade?

> @@ -708,11 +712,15 @@ static long cf_check microcode_update_helper(void *data)
>      return ret;
>  }
>  
> -int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len)
> +int microcode_update(XEN_GUEST_HANDLE(const_void) buf,
> +                     unsigned long len, unsigned int flags)
>  {
>      int ret;
>      struct ucode_buf *buffer;
>  
> +    if ( flags > 1 )

How is one to connect this literal 1 with what is really meant here? Also
would be nice if this check fit with other similar checks we do, i.e.

    if ( flags & ~XENPF_UCODE_FLAG_FORCE_SET )

> +        return -EINVAL;
> +
>      if ( len != (uint32_t)len )
>          return -E2BIG;

As an aside: Isn't this dead code, with the respective hypercall interface
struct fields (now) both being uint32_t?

> --- a/xen/arch/x86/platform_hypercall.c
> +++ b/xen/arch/x86/platform_hypercall.c
> @@ -311,7 +311,17 @@ ret_t do_platform_op(
>  
>          guest_from_compat_handle(data, op->u.microcode.data);
>  
> -        ret = microcode_update(data, op->u.microcode.length);
> +        ret = microcode_update(data, op->u.microcode.length, 0);
> +        break;
> +    }
> +
> +    case XENPF_microcode_update2:
> +    {
> +        XEN_GUEST_HANDLE(const_void) data;
> +
> +        guest_from_compat_handle(data, op->u.microcode2.data);
> +
> +        ret = microcode_update(data, op->u.microcode2.length, op->u.microcode2.flags);

Nit (style): Overlong line.

> --- a/xen/include/public/platform.h
> +++ b/xen/include/public/platform.h
> @@ -624,6 +624,19 @@ struct xenpf_ucode_revision {
>  typedef struct xenpf_ucode_revision xenpf_ucode_revision_t;
>  DEFINE_XEN_GUEST_HANDLE(xenpf_ucode_revision_t);
>  
> +/* Hypercall to microcode_update with flags */
> +#define XENPF_microcode_update2    66
> +struct xenpf_microcode_update2 {
> +    /* IN variables. */
> +    uint32_t flags;                   /* Flags to be passed with ucode. */
> +/* Force to skip microcode version check when set */
> +#define XENPF_UCODE_FLAG_FORCE_SET     1

Nit: What is "SET" in the identifier intended to mean?

Jan
Fouad Hilly May 9, 2024, 2:15 p.m. UTC | #2 
On Mon, May 6, 2024 at 10:14 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 30.04.2024 14:47, Fouad Hilly wrote:
> > @@ -633,12 +637,12 @@ static long cf_check microcode_update_helper(void *data)
> >                                    microcode_cache);
> >
> >          if ( result != NEW_UCODE &&
> > -             !(opt_ucode_allow_same && result == SAME_UCODE) )
> > +             !((opt_ucode_allow_same || ucode_force_flag) && result == SAME_UCODE) )
>
> Why would "force" not also allow a downgrade?

It should be allowed. Will be fixed in v4

>
> > @@ -708,11 +712,15 @@ static long cf_check microcode_update_helper(void *data)
> >      return ret;
> >  }
> >
> > -int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len)
> > +int microcode_update(XEN_GUEST_HANDLE(const_void) buf,
> > +                     unsigned long len, unsigned int flags)
> >  {
> >      int ret;
> >      struct ucode_buf *buffer;
> >
> > +    if ( flags > 1 )
>
> How is one to connect this literal 1 with what is really meant here? Also
> would be nice if this check fit with other similar checks we do, i.e.
>
>     if ( flags & ~XENPF_UCODE_FLAG_FORCE_SET )
Will be done in v4

>
> > +        return -EINVAL;
> > +
> >      if ( len != (uint32_t)len )
> >          return -E2BIG;
>
> As an aside: Isn't this dead code, with the respective hypercall interface
> struct fields (now) both being uint32_t?

Will be cleaned up in v4.
>
> > --- a/xen/arch/x86/platform_hypercall.c
> > +++ b/xen/arch/x86/platform_hypercall.c
> > @@ -311,7 +311,17 @@ ret_t do_platform_op(
> >
> >          guest_from_compat_handle(data, op->u.microcode.data);
> >
> > -        ret = microcode_update(data, op->u.microcode.length);
> > +        ret = microcode_update(data, op->u.microcode.length, 0);
> > +        break;
> > +    }
> > +
> > +    case XENPF_microcode_update2:
> > +    {
> > +        XEN_GUEST_HANDLE(const_void) data;
> > +
> > +        guest_from_compat_handle(data, op->u.microcode2.data);
> > +
> > +        ret = microcode_update(data, op->u.microcode2.length, op->u.microcode2.flags);
>
> Nit (style): Overlong line.
>
> > --- a/xen/include/public/platform.h
> > +++ b/xen/include/public/platform.h
> > @@ -624,6 +624,19 @@ struct xenpf_ucode_revision {
> >  typedef struct xenpf_ucode_revision xenpf_ucode_revision_t;
> >  DEFINE_XEN_GUEST_HANDLE(xenpf_ucode_revision_t);
> >
> > +/* Hypercall to microcode_update with flags */
> > +#define XENPF_microcode_update2    66
> > +struct xenpf_microcode_update2 {
> > +    /* IN variables. */
> > +    uint32_t flags;                   /* Flags to be passed with ucode. */
> > +/* Force to skip microcode version check when set */
> > +#define XENPF_UCODE_FLAG_FORCE_SET     1
>
> Nit: What is "SET" in the identifier intended to mean?
"SET" to be removed in v4.
>
> Jan
diff mbox series

Patch

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index e738a88f5cd1..4a18ddd2683b 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -40,6 +40,8 @@ 
 #include <asm/processor.h>
 #include <asm/setup.h>
 
+#include <public/platform.h>
+
 #include "private.h"
 
 /*
@@ -570,6 +572,7 @@  static int cf_check do_microcode_update(void *patch)
 }
 
 struct ucode_buf {
+    unsigned int flags;
     unsigned int len;
     char buffer[];
 };
@@ -580,6 +583,7 @@  static long cf_check microcode_update_helper(void *data)
     struct ucode_buf *buffer = data;
     unsigned int cpu, updated;
     struct microcode_patch *patch;
+    bool ucode_force_flag = buffer->flags == XENPF_UCODE_FLAG_FORCE_SET;
 
     /* cpu_online_map must not change during update */
     if ( !get_cpu_maps() )
@@ -633,12 +637,12 @@  static long cf_check microcode_update_helper(void *data)
                                   microcode_cache);
 
         if ( result != NEW_UCODE &&
-             !(opt_ucode_allow_same && result == SAME_UCODE) )
+             !((opt_ucode_allow_same || ucode_force_flag) && result == SAME_UCODE) )
         {
             spin_unlock(&microcode_mutex);
             printk(XENLOG_WARNING
                    "microcode: couldn't find any newer%s revision in the provided blob!\n",
-                   opt_ucode_allow_same ? " (or the same)" : "");
+                   (opt_ucode_allow_same || ucode_force_flag) ? " (or the same)" : "");
             microcode_free_patch(patch);
             ret = -ENOENT;
 
@@ -708,11 +712,15 @@  static long cf_check microcode_update_helper(void *data)
     return ret;
 }
 
-int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len)
+int microcode_update(XEN_GUEST_HANDLE(const_void) buf,
+                     unsigned long len, unsigned int flags)
 {
     int ret;
     struct ucode_buf *buffer;
 
+    if ( flags > 1 )
+        return -EINVAL;
+
     if ( len != (uint32_t)len )
         return -E2BIG;
 
@@ -730,6 +738,7 @@  int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len)
         return -EFAULT;
     }
     buffer->len = len;
+    buffer->flags = flags;
 
     /*
      * Always queue microcode_update_helper() on CPU0.  Most of the logic
diff --git a/xen/arch/x86/include/asm/microcode.h b/xen/arch/x86/include/asm/microcode.h
index 8f59b20b0289..57c08205d475 100644
--- a/xen/arch/x86/include/asm/microcode.h
+++ b/xen/arch/x86/include/asm/microcode.h
@@ -22,7 +22,8 @@  struct cpu_signature {
 DECLARE_PER_CPU(struct cpu_signature, cpu_sig);
 
 void microcode_set_module(unsigned int idx);
-int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len);
+int microcode_update(XEN_GUEST_HANDLE(const_void) buf,
+                     unsigned long len, unsigned int flags);
 int early_microcode_init(unsigned long *module_map,
                          const struct multiboot_info *mbi);
 int microcode_init_cache(unsigned long *module_map,
diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
index 95467b88ab64..a22f2929896e 100644
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -311,7 +311,17 @@  ret_t do_platform_op(
 
         guest_from_compat_handle(data, op->u.microcode.data);
 
-        ret = microcode_update(data, op->u.microcode.length);
+        ret = microcode_update(data, op->u.microcode.length, 0);
+        break;
+    }
+
+    case XENPF_microcode_update2:
+    {
+        XEN_GUEST_HANDLE(const_void) data;
+
+        guest_from_compat_handle(data, op->u.microcode2.data);
+
+        ret = microcode_update(data, op->u.microcode2.length, op->u.microcode2.flags);
         break;
     }
 
diff --git a/xen/include/public/platform.h b/xen/include/public/platform.h
index 15777b541690..a30de7153779 100644
--- a/xen/include/public/platform.h
+++ b/xen/include/public/platform.h
@@ -624,6 +624,19 @@  struct xenpf_ucode_revision {
 typedef struct xenpf_ucode_revision xenpf_ucode_revision_t;
 DEFINE_XEN_GUEST_HANDLE(xenpf_ucode_revision_t);
 
+/* Hypercall to microcode_update with flags */
+#define XENPF_microcode_update2    66
+struct xenpf_microcode_update2 {
+    /* IN variables. */
+    uint32_t flags;                   /* Flags to be passed with ucode. */
+/* Force to skip microcode version check when set */
+#define XENPF_UCODE_FLAG_FORCE_SET     1
+    uint32_t length;                  /* Length of microcode data. */
+    XEN_GUEST_HANDLE(const_void) data;/* Pointer to microcode data */
+};
+typedef struct xenpf_microcode_update2 xenpf_microcode_update2_t;
+DEFINE_XEN_GUEST_HANDLE(xenpf_microcode_update2_t);
+
 /*
  * ` enum neg_errnoval
  * ` HYPERVISOR_platform_op(const struct xen_platform_op*);
@@ -656,6 +669,7 @@  struct xen_platform_op {
         xenpf_symdata_t               symdata;
         xenpf_dom0_console_t          dom0_console;
         xenpf_ucode_revision_t        ucode_revision;
+        xenpf_microcode_update2_t     microcode2;
         uint8_t                       pad[128];
     } u;
 };