| Message ID | 20241112211915.1473121-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/ucode: Simplify/fix loading paths further | expand |
On 12.11.2024 22:19, Andrew Cooper wrote: > With the tangle of logic starting to come under control, it is now plain to > see that parse_blob()'s side effect of re-gathering the signature/revision is > pointless. > > The cpu_request_microcode() hooks need the signature only. The BSP gathers > this in early_microcode_init(), the APs and S3 in microcode_update_cpu(). That's microcode_update_one() after 502478bc1d9d if I'm not mistaken. In the course of determining that I'm afraid I also found the first sentence of this paragraph rather misleading than helpful: While it is true what is being said, in both cases it is collect_cpu_info() that is being invoked, retrieving both signature and revision. IOW logic needing the signature only doesn't really matter here (and the sentence made me hunt for cases where we would read just the signature, aiming at verifying that leaving the revision field unset would indeed not be a problem). > For > good measure, the apply_microcode() hooks also keep the revision correct as > load attempts are made. > > This finally gets us down to a single call per CPU on boot / S3 resume, and no > calls during late-load hypercalls. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Preferably with the problematic sentence dropped or clarified: Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 14/11/2024 11:11 am, Jan Beulich wrote: > On 12.11.2024 22:19, Andrew Cooper wrote: >> With the tangle of logic starting to come under control, it is now plain to >> see that parse_blob()'s side effect of re-gathering the signature/revision is >> pointless. >> >> The cpu_request_microcode() hooks need the signature only. The BSP gathers >> this in early_microcode_init(), the APs and S3 in microcode_update_cpu(). > That's microcode_update_one() after 502478bc1d9d if I'm not mistaken. I wouldn't necessarily say "after". microcode_update_cpu() has been the way the APs and S3 get this information for ages, whether its in the function directly, or in an immediate callee. > In the > course of determining that I'm afraid I also found the first sentence of this > paragraph rather misleading than helpful: Do you mean "The cpu_request_microcode() hooks need the signature only" ? > While it is true what is being said, > in both cases it is collect_cpu_info() that is being invoked, retrieving both > signature and revision. IOW logic needing the signature only doesn't really > matter here (and the sentence made me hunt for cases where we would read just > the signature, aiming at verifying that leaving the revision field unset > would indeed not be a problem). It probably doesn't come as a surprise that I'm intending to rework collect_cpu_info() entirely. It's a mess. The signature and platform flags are invariants for a CPU. (In fact, Platform Flags had better be the same for an entire system). The revision does change with type, but apply_microcode() keeps it up to date. Yet we had logic which was throwing the details away and re-gathering (which is quite expensive) for basically every microcode operation. What I'm trying to express is "this information is collected once at the start of day, and kept up to date, so collect_cpu_info() should not be called under any other circumstance". Perhaps I should just say that directly? >> For >> good measure, the apply_microcode() hooks also keep the revision correct as >> load attempts are made. >> >> This finally gets us down to a single call per CPU on boot / S3 resume, and no >> calls during late-load hypercalls. >> >> No functional change. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Preferably with the problematic sentence dropped or clarified: > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks.
On 14.11.2024 16:59, Andrew Cooper wrote: > On 14/11/2024 11:11 am, Jan Beulich wrote: >> On 12.11.2024 22:19, Andrew Cooper wrote: >>> With the tangle of logic starting to come under control, it is now plain to >>> see that parse_blob()'s side effect of re-gathering the signature/revision is >>> pointless. >>> >>> The cpu_request_microcode() hooks need the signature only. The BSP gathers >>> this in early_microcode_init(), the APs and S3 in microcode_update_cpu(). >> That's microcode_update_one() after 502478bc1d9d if I'm not mistaken. > > I wouldn't necessarily say "after". My point was merely that there's no microcode_update_cpu() anymore, as of that commit. > microcode_update_cpu() has been the way the APs and S3 get this > information for ages, whether its in the function directly, or in an > immediate callee. > >> In the >> course of determining that I'm afraid I also found the first sentence of this >> paragraph rather misleading than helpful: > > Do you mean "The cpu_request_microcode() hooks need the signature only" ? Yes. >> While it is true what is being said, >> in both cases it is collect_cpu_info() that is being invoked, retrieving both >> signature and revision. IOW logic needing the signature only doesn't really >> matter here (and the sentence made me hunt for cases where we would read just >> the signature, aiming at verifying that leaving the revision field unset >> would indeed not be a problem). > > It probably doesn't come as a surprise that I'm intending to rework > collect_cpu_info() entirely. It's a mess. > > The signature and platform flags are invariants for a CPU. (In fact, > Platform Flags had better be the same for an entire system). The > revision does change with type, but apply_microcode() keeps it up to date. > > Yet we had logic which was throwing the details away and re-gathering > (which is quite expensive) for basically every microcode operation. > > > What I'm trying to express is "this information is collected once at the > start of day, and kept up to date, so collect_cpu_info() should not be > called under any other circumstance". > > Perhaps I should just say that directly? That may be a good thing, yes. The main point still being though that the way that 1st sentence in the paragraph was written, it ended up confusing. Jan
On 14/11/2024 4:15 pm, Jan Beulich wrote: > On 14.11.2024 16:59, Andrew Cooper wrote: >> On 14/11/2024 11:11 am, Jan Beulich wrote: >>> On 12.11.2024 22:19, Andrew Cooper wrote: >>>> With the tangle of logic starting to come under control, it is now plain to >>>> see that parse_blob()'s side effect of re-gathering the signature/revision is >>>> pointless. >>>> >>>> The cpu_request_microcode() hooks need the signature only. The BSP gathers >>>> this in early_microcode_init(), the APs and S3 in microcode_update_cpu(). >>> That's microcode_update_one() after 502478bc1d9d if I'm not mistaken. >> I wouldn't necessarily say "after". > My point was merely that there's no microcode_update_cpu() anymore, as of > that commit. Oh. I totally missed that. I'll fix it, and make the rest of the paragraph a little bit more direct. ~Andrew
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c index fd4b08b45388..5897ec54032a 100644 --- a/xen/arch/x86/cpu/microcode/core.c +++ b/xen/arch/x86/cpu/microcode/core.c @@ -189,8 +189,6 @@ static struct patch_with_flags nmi_patch = */ static struct microcode_patch *parse_blob(const char *buf, size_t len) { - alternative_vcall(ucode_ops.collect_cpu_info); - return alternative_call(ucode_ops.cpu_request_microcode, buf, len, true); }
With the tangle of logic starting to come under control, it is now plain to see that parse_blob()'s side effect of re-gathering the signature/revision is pointless. The cpu_request_microcode() hooks need the signature only. The BSP gathers this in early_microcode_init(), the APs and S3 in microcode_update_cpu(). For good measure, the apply_microcode() hooks also keep the revision correct as load attempts are made. This finally gets us down to a single call per CPU on boot / S3 resume, and no calls during late-load hypercalls. No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/cpu/microcode/core.c | 2 -- 1 file changed, 2 deletions(-)