Re: [PATCH] x86/ucode: Refresh raw CPU policy after microcode load

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, May 04, 2023 at 09:08:02AM +0100, Andrew Cooper wrote:
> On 04/05/2023 8:08 am, Jan Beulich wrote:
> > On 03.05.2023 20:58, Andrew Cooper wrote:
> >> Loading microcode can cause new features to appear.
> > Or disappear (LWP)? While I don't think we want to panic() in this
> > case (we do on the S3 resume path when recheck_cpu_features() fails
> > on the BSP), it would seem to me that we want to go a step further
> > than you do and at least warn when a feature went amiss. Or is that
> > too much of a scope-creeping request right here?
> 
> You're correct that I ought to discuss the disappear case.  But like
> livepatching, it's firmly in the realm of "the end user takes
> responsibility for trying this in their test system before running it in
> production".
> 
> For LWP specifically, we ought to explicitly permit its disappearance in
> recheck_cpu_features(), because this specific example is known to exist,
> and known safe as Xen never used or virtualised LWP functionality. 
> Crashing on S3

Printing disappearing features might be a nice bonus, but could be
done from the tool that loads the microcode itself, no need to do such
work in the hypervisor IMO.

> >
> >> @@ -677,6 +678,9 @@ static long cf_check microcode_update_helper(void 
> >> *data)
> >>          spin_lock(&microcode_mutex);
> >>          microcode_update_cache(patch);
> >>          spin_unlock(&microcode_mutex);
> >> +
> >> +        /* Refresh the raw CPU policy, in case the features have changed. 
> >> */
> >> +        calculate_raw_cpu_policy();
> > I understand this is in line with what we do during boot, but there
> > and here I wonder whether this wouldn't better deal with possible
> > asymmetries (e.g. in case ucode loading failed on one of the CPUs),
> > along the lines of what we do near the end of identify_cpu() for
> > APs. (Unlike the question higher up, this is definitely only a
> > remark here, not something I'd consider dealing with right in this
> > change.)
> 
> Asymmetry is an increasingly theoretical problem.  Yeah, it exists in
> principle, but Xen has no way of letting you explicitly get into that
> situation.
> 
> This too falls firmly into the "end user takes responsibility for
> testing it properly first" category.
> 
> We have explicit symmetric assumptions/requirements elsewhere (e.g. for
> a single system, there's 1 correct ucode blob).
> 
> We can acknowledge that asymmetry exists, but there is basically nothing
> Xen can do about it other than highlight that something is very wrong on
> the system.  Odds are that a system which gets into such a state won't
> survive much longer.

Would it make sense to only update the CPU policy if updated ==
nr_cores?

Thanks, Roger.