| Message ID | 20241107122117.4073266-4-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/ucode: Simplify/fix loading paths further | expand |
On 07/11/2024 12:21 pm, 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(). 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> > > Slightly RFC. > > Just before posting, I've realised that cpu_request_microcode() does actually > use the current CPU revision, and it's buggy, and it's the cause of `xen-ucode > --force` not working as expected. > > I'm tempted to do another series cleaning that up in isolation, such that this > patch becomes true in this form. Actually no. Having tried a bit, I think it's easier to do with patch 2 already in place. So instead I'm tempted to edit the middle paragraph to note that it currently uses the revision but that's going to be fixed shortly. The rest of the paragraph explains why it's still safe anyway. ~Andrew
On 07/11/2024 9:58 pm, Andrew Cooper wrote: > On 07/11/2024 12:21 pm, 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(). 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> >> >> Slightly RFC. >> >> Just before posting, I've realised that cpu_request_microcode() does actually >> use the current CPU revision, and it's buggy, and it's the cause of `xen-ucode >> --force` not working as expected. >> >> I'm tempted to do another series cleaning that up in isolation, such that this >> patch becomes true in this form. > Actually no. Having tried a bit, I think it's easier to do with patch 2 > already in place. > > So instead I'm tempted to edit the middle paragraph to note that it > currently uses the revision but that's going to be fixed shortly. The > rest of the paragraph explains why it's still safe anyway. So, after the latter series, this patch happens to be accurate. cpu_request_microcode() does read the revision, but discards the result of the calculation which used it. ~Andrew
On 12.11.2024 11:36, Andrew Cooper wrote: > On 07/11/2024 9:58 pm, Andrew Cooper wrote: >> On 07/11/2024 12:21 pm, 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(). 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> >>> >>> Slightly RFC. >>> >>> Just before posting, I've realised that cpu_request_microcode() does actually >>> use the current CPU revision, and it's buggy, and it's the cause of `xen-ucode >>> --force` not working as expected. >>> >>> I'm tempted to do another series cleaning that up in isolation, such that this >>> patch becomes true in this form. >> Actually no. Having tried a bit, I think it's easier to do with patch 2 >> already in place. >> >> So instead I'm tempted to edit the middle paragraph to note that it >> currently uses the revision but that's going to be fixed shortly. The >> rest of the paragraph explains why it's still safe anyway. > > So, after the latter series, this patch happens to be accurate. > > cpu_request_microcode() does read the revision, but discards the result > of the calculation which used it. What's the intended overall sequence of patches then? With two series that (aiui) now have grown some sort of dependency, and with this series have gained a 4/3 patch, having a clear picture would certainly help. Might it be best if you merge both series and re-submit as a single one? Jan
On 12/11/2024 10:49 am, Jan Beulich wrote: > On 12.11.2024 11:36, Andrew Cooper wrote: >> On 07/11/2024 9:58 pm, Andrew Cooper wrote: >>> On 07/11/2024 12:21 pm, 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(). 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> >>>> >>>> Slightly RFC. >>>> >>>> Just before posting, I've realised that cpu_request_microcode() does actually >>>> use the current CPU revision, and it's buggy, and it's the cause of `xen-ucode >>>> --force` not working as expected. >>>> >>>> I'm tempted to do another series cleaning that up in isolation, such that this >>>> patch becomes true in this form. >>> Actually no. Having tried a bit, I think it's easier to do with patch 2 >>> already in place. >>> >>> So instead I'm tempted to edit the middle paragraph to note that it >>> currently uses the revision but that's going to be fixed shortly. The >>> rest of the paragraph explains why it's still safe anyway. >> So, after the latter series, this patch happens to be accurate. >> >> cpu_request_microcode() does read the revision, but discards the result >> of the calculation which used it. > What's the intended overall sequence of patches then? With two series that > (aiui) now have grown some sort of dependency, and with this series have > gained a 4/3 patch, having a clear picture would certainly help. Might it > be best if you merge both series and re-submit as a single one? The order turns out to be as emailed out and threaded. ~Andrew
On 12.11.2024 11:57, Andrew Cooper wrote: > On 12/11/2024 10:49 am, Jan Beulich wrote: >> On 12.11.2024 11:36, Andrew Cooper wrote: >>> On 07/11/2024 9:58 pm, Andrew Cooper wrote: >>>> On 07/11/2024 12:21 pm, 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(). 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> >>>>> >>>>> Slightly RFC. >>>>> >>>>> Just before posting, I've realised that cpu_request_microcode() does actually >>>>> use the current CPU revision, and it's buggy, and it's the cause of `xen-ucode >>>>> --force` not working as expected. >>>>> >>>>> I'm tempted to do another series cleaning that up in isolation, such that this >>>>> patch becomes true in this form. >>>> Actually no. Having tried a bit, I think it's easier to do with patch 2 >>>> already in place. >>>> >>>> So instead I'm tempted to edit the middle paragraph to note that it >>>> currently uses the revision but that's going to be fixed shortly. The >>>> rest of the paragraph explains why it's still safe anyway. >>> So, after the latter series, this patch happens to be accurate. >>> >>> cpu_request_microcode() does read the revision, but discards the result >>> of the calculation which used it. >> What's the intended overall sequence of patches then? With two series that >> (aiui) now have grown some sort of dependency, and with this series have >> gained a 4/3 patch, having a clear picture would certainly help. Might it >> be best if you merge both series and re-submit as a single one? > > The order turns out to be as emailed out and threaded. Yet above you said "after the latter series, this patch happens to be accurate." Which suggest to me that at least part of the latter series needs to be in place for the change here to be correct. IOW - I'm confused now. Jan
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> Slightly RFC. Just before posting, I've realised that cpu_request_microcode() does actually use the current CPU revision, and it's buggy, and it's the cause of `xen-ucode --force` not working as expected. I'm tempted to do another series cleaning that up in isolation, such that this patch becomes true in this form. --- xen/arch/x86/cpu/microcode/core.c | 2 -- 1 file changed, 2 deletions(-)