From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
Jan Beulich <JBeulich@suse.com>,
Fouad Hilly <fouad.hilly@cloud.com>
Subject: Re: [PATCH] x86/ucode: Further fixes to identify "ucode already up to date"
Date: Thu, 16 May 2024 14:45:25 +0200 [thread overview]
Message-ID: <ZkX_5W-b2J6eR7Py@macbook> (raw)
In-Reply-To: <4bc4f2f5-6914-445a-a9a1-a609c0c3cf1f@citrix.com>
On Thu, May 16, 2024 at 01:30:21PM +0100, Andrew Cooper wrote:
> On 16/05/2024 12:50 pm, Roger Pau Monné wrote:
> > On Thu, May 16, 2024 at 12:31:03PM +0100, Andrew Cooper wrote:
> >> When the revision in hardware is newer than anything Xen has to hand,
> >> 'microcode_cache' isn't set up. Then, `xen-ucode` initiates the update
> >> because it doesn't know whether the revisions across the system are symmetric
> >> or not. This involves the patch getting all the way into the
> >> apply_microcode() hooks before being found to be too old.
> >>
> >> This is all a giant mess and needs an overhaul, but in the short term simply
> >> adjust the apply_microcode() to return -EEXIST.
> >>
> >> Also, unconditionally print the preexisting microcode revision on boot. It's
> >> relevant information which is otherwise unavailable if Xen doesn't find new
> >> microcode to use.
> >>
> >> Fixes: 648db37a155a ("x86/ucode: Distinguish "ucode already up to date"")
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> ---
> >> CC: Jan Beulich <JBeulich@suse.com>
> >> CC: Roger Pau Monné <roger.pau@citrix.com>
> >> CC: Fouad Hilly <fouad.hilly@cloud.com>
> >>
> >> Sorry Fouad, but this collides with your `--force` series once again.
> >> Hopefully it might make things fractionally easier.
> >>
> >> Background: For 06-55-04 (Skylake server, stepping 4 specifically), there's a
> >> recent production firmware update which has a newer microcode revision than
> >> exists in the Intel public microcode repository. It's causing a mess in our
> >> automated testing, although it is finding good bugs...
> >> ---
> >> xen/arch/x86/cpu/microcode/amd.c | 7 +++++--
> >> xen/arch/x86/cpu/microcode/core.c | 2 ++
> >> xen/arch/x86/cpu/microcode/intel.c | 7 +++++--
> >> 3 files changed, 12 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
> >> index 17e68697d5bf..f76a563c8b84 100644
> >> --- a/xen/arch/x86/cpu/microcode/amd.c
> >> +++ b/xen/arch/x86/cpu/microcode/amd.c
> >> @@ -222,12 +222,15 @@ static int cf_check apply_microcode(const struct microcode_patch *patch)
> >> uint32_t rev, old_rev = sig->rev;
> >> enum microcode_match_result result = microcode_fits(patch);
> >>
> >> + if ( result == MIS_UCODE )
> >> + return -EINVAL;
> >> +
> >> /*
> >> * Allow application of the same revision to pick up SMT-specific changes
> >> * even if the revision of the other SMT thread is already up-to-date.
> >> */
> >> - if ( result != NEW_UCODE && result != SAME_UCODE )
> >> - return -EINVAL;
> >> + if ( result == OLD_UCODE )
> >> + return -EEXIST;
> > Won't it be simpler to just add this check ahead of the existing one,
> > so that you can leave the code as-is, iow:
> >
> > if ( result == OLD_UCODE )
> > return -EEXIST;
> >
> > /*
> > * Allow application of the same revision to pick up SMT-specific changes
> > * even if the revision of the other SMT thread is already up-to-date.
> > */
> > if ( result != NEW_UCODE && result != SAME_UCODE )
> > return -EINVAL;
> >
> > Thanks, Roger.
>
> Not really, no. That still leaves this piece of logic which is
> misleading IMO.
>
> MIS_UCODE is the only -EINVAL worthy case.
>
> Every other *_UCODE constant needs to be 0 or -EEXIST, depending on
> allow-same/--force.
OK, my main concern was the previous logic wouldn't allow a newly
introduced state to get past the return -EINVAL, while the new logic
could possibly allow it to pass through.
I don't think adding states is that common, and if you prefer it that
way it's fine.
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
Thanks, Roger.
prev parent reply other threads:[~2024-05-16 12:45 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-16 11:31 [PATCH] x86/ucode: Further fixes to identify "ucode already up to date" Andrew Cooper
2024-05-16 11:44 ` Jan Beulich
2024-05-16 12:31 ` Andrew Cooper
2024-05-16 11:50 ` Roger Pau Monné
2024-05-16 12:30 ` Andrew Cooper
2024-05-16 12:45 ` Roger Pau Monné [this message]
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=ZkX_5W-b2J6eR7Py@macbook \
--to=roger.pau@citrix.com \
--cc=JBeulich@suse.com \
--cc=andrew.cooper3@citrix.com \
--cc=fouad.hilly@cloud.com \
--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).