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

[Jan Beulich <jbeulich@xxxxxxxx>]

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