From: Fouad Hilly <fouad.hilly@cloud.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: "Andrew Cooper" <andrew.cooper3@citrix.com>,
"Roger Pau Monné" <roger.pau@citrix.com>,
"George Dunlap" <george.dunlap@citrix.com>,
"Julien Grall" <julien@xen.org>,
"Stefano Stabellini" <sstabellini@kernel.org>,
Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v3 2/5] x86: Refactor microcode_update() hypercall with flags
Date: Thu, 9 May 2024 15:15:17 +0100 [thread overview]
Message-ID: <CAJKAvHajhOy2SpYZk1G6MtiK1weL6cuD+bp8puY=o9XGkt0TAw@mail.gmail.com> (raw)
In-Reply-To: <ec48e32a-30e5-45ab-8f11-7d3d6ce6122e@suse.com>
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
next prev parent reply other threads:[~2024-05-09 14:15 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-30 12:47 [PATCH v3 0/5] x86/xen-ucode: Introduce --force option Fouad Hilly
2024-04-30 12:47 ` [PATCH v3 1/5] x86: Update x86 low level version check of microcode Fouad Hilly
2024-05-06 8:45 ` Jan Beulich
2024-05-09 14:33 ` Fouad Hilly
2024-04-30 12:47 ` [PATCH v3 2/5] x86: Refactor microcode_update() hypercall with flags Fouad Hilly
2024-05-06 9:14 ` Jan Beulich
2024-05-09 14:15 ` Fouad Hilly [this message]
2024-04-30 12:47 ` [PATCH v3 3/5] x86: Add usage() to print out usage message Fouad Hilly
2024-05-06 8:20 ` Jan Beulich
2024-05-09 13:59 ` Fouad Hilly
2024-04-30 12:47 ` [PATCH v3 4/5] x86: Use getopt to handle command line args Fouad Hilly
2024-05-06 8:21 ` Jan Beulich
2024-05-09 13:59 ` Fouad Hilly
2024-04-30 12:47 ` [PATCH v3 5/5] Add --force option to xen-ucode to override microcode version check Fouad Hilly
2024-05-06 9:39 ` Jan Beulich
2024-05-09 14:31 ` Fouad Hilly
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAJKAvHajhOy2SpYZk1G6MtiK1weL6cuD+bp8puY=o9XGkt0TAw@mail.gmail.com' \
--to=fouad.hilly@cloud.com \
--cc=andrew.cooper3@citrix.com \
--cc=george.dunlap@citrix.com \
--cc=jbeulich@suse.com \
--cc=julien@xen.org \
--cc=roger.pau@citrix.com \
--cc=sstabellini@kernel.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).