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

[Fouad Hilly <fouad.hilly@xxxxxxxxx>]

On Mon, May 6, 2024 at 10:14 AM Jan Beulich <jbeulich@xxxxxxxx> 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