| Message ID | 20200323101724.15655-2-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/ucode: Cleanup and fixes - Part 3/n (Intel) | expand |
On 23.03.2020 11:17, Andrew Cooper wrote: > ... and struct cpu_signature for good measure. > > No comment is passed on the suitability of the behaviour... > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Wei Liu <wl@xen.org> > CC: Roger Pau Monné <roger.pau@citrix.com> > --- > xen/arch/x86/cpu/microcode/private.h | 46 ++++++++++++++++++++++++++++++++++++ > xen/include/asm-x86/microcode.h | 5 ++++ > 2 files changed, 51 insertions(+) > > diff --git a/xen/arch/x86/cpu/microcode/private.h b/xen/arch/x86/cpu/microcode/private.h > index e64168a502..a2aec53047 100644 > --- a/xen/arch/x86/cpu/microcode/private.h > +++ b/xen/arch/x86/cpu/microcode/private.h > @@ -14,14 +14,60 @@ enum microcode_match_result { > struct microcode_patch; /* Opaque */ > > struct microcode_ops { > + /* > + * Parse a microcode container. Format is vendor-specific. > + * > + * Search within the container for the patch, suitable for the current > + * CPU, which has the highest revision. (Note: May be a patch which is > + * older that what is running in the CPU. This is a feature, to better > + * cope with corner cases from buggy firmware.) > + * > + * If one is found, allocate and return a struct microcode_patch > + * encapsulating the appropriate microcode patch. Does not alias the > + * original buffer. > + * > + * If one is not found, (nothing matches the current CPU), return NULL. > + * Also may return ERR_PTR(-err), e.g. bad container, out of memory. > + */ > struct microcode_patch *(*cpu_request_microcode)(const void *buf, > size_t size); > + > + /* Obtain microcode-relevant details for the current CPU. */ > int (*collect_cpu_info)(struct cpu_signature *csig); > + > + /* > + * Attempt to load the provided patch into the CPU. Returns -EIO if > + * anything didn't go as expected. > + */ > int (*apply_microcode)(const struct microcode_patch *patch); While at present -EIO may be the only error that may come back here, do we want to risk the comment going stale when another error return gets added? IOW - perhaps add "e.g." or some such? > --- a/xen/include/asm-x86/microcode.h > +++ b/xen/include/asm-x86/microcode.h > @@ -7,8 +7,13 @@ > #include <public/xen.h> > > struct cpu_signature { > + /* CPU signature (CPUID.1.EAX). Only written on Intel. */ > unsigned int sig; > + > + /* Platform Flags (only actually 1 bit). Only applicable to Intel. */ > unsigned int pf; To me "only actually 1 bit" makes it an implication that this is the lowest bit (like in a bool represented in a 32-bit memory location). I didn't think this was the case though, so unless I'm wrong, could you clarify this a little? Jan
On 23/03/2020 12:33, Jan Beulich wrote: > On 23.03.2020 11:17, Andrew Cooper wrote: >> ... and struct cpu_signature for good measure. >> >> No comment is passed on the suitability of the behaviour... >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Wei Liu <wl@xen.org> >> CC: Roger Pau Monné <roger.pau@citrix.com> >> --- >> xen/arch/x86/cpu/microcode/private.h | 46 ++++++++++++++++++++++++++++++++++++ >> xen/include/asm-x86/microcode.h | 5 ++++ >> 2 files changed, 51 insertions(+) >> >> diff --git a/xen/arch/x86/cpu/microcode/private.h b/xen/arch/x86/cpu/microcode/private.h >> index e64168a502..a2aec53047 100644 >> --- a/xen/arch/x86/cpu/microcode/private.h >> +++ b/xen/arch/x86/cpu/microcode/private.h >> @@ -14,14 +14,60 @@ enum microcode_match_result { >> struct microcode_patch; /* Opaque */ >> >> struct microcode_ops { >> + /* >> + * Parse a microcode container. Format is vendor-specific. >> + * >> + * Search within the container for the patch, suitable for the current >> + * CPU, which has the highest revision. (Note: May be a patch which is >> + * older that what is running in the CPU. This is a feature, to better >> + * cope with corner cases from buggy firmware.) >> + * >> + * If one is found, allocate and return a struct microcode_patch >> + * encapsulating the appropriate microcode patch. Does not alias the >> + * original buffer. >> + * >> + * If one is not found, (nothing matches the current CPU), return NULL. >> + * Also may return ERR_PTR(-err), e.g. bad container, out of memory. >> + */ >> struct microcode_patch *(*cpu_request_microcode)(const void *buf, >> size_t size); >> + >> + /* Obtain microcode-relevant details for the current CPU. */ >> int (*collect_cpu_info)(struct cpu_signature *csig); >> + >> + /* >> + * Attempt to load the provided patch into the CPU. Returns -EIO if >> + * anything didn't go as expected. >> + */ >> int (*apply_microcode)(const struct microcode_patch *patch); > While at present -EIO may be the only error that may come back here, do > we want to risk the comment going stale when another error return gets > added? IOW - perhaps add "e.g." or some such? Can do. > >> --- a/xen/include/asm-x86/microcode.h >> +++ b/xen/include/asm-x86/microcode.h >> @@ -7,8 +7,13 @@ >> #include <public/xen.h> >> >> struct cpu_signature { >> + /* CPU signature (CPUID.1.EAX). Only written on Intel. */ >> unsigned int sig; >> + >> + /* Platform Flags (only actually 1 bit). Only applicable to Intel. */ >> unsigned int pf; > To me "only actually 1 bit" makes it an implication that this is the > lowest bit (like in a bool represented in a 32-bit memory location). > I didn't think this was the case though, so unless I'm wrong, could > you clarify this a little? There will be a single bit within the bottom 8 set (the 1 << MSR_PLATFORM_ID[52:50]), despite this field being called "Platform Flags". ~Andrew
On 23.03.2020 14:26, Andrew Cooper wrote: > On 23/03/2020 12:33, Jan Beulich wrote: >> On 23.03.2020 11:17, Andrew Cooper wrote: >>> --- a/xen/include/asm-x86/microcode.h >>> +++ b/xen/include/asm-x86/microcode.h >>> @@ -7,8 +7,13 @@ >>> #include <public/xen.h> >>> >>> struct cpu_signature { >>> + /* CPU signature (CPUID.1.EAX). Only written on Intel. */ >>> unsigned int sig; >>> + >>> + /* Platform Flags (only actually 1 bit). Only applicable to Intel. */ >>> unsigned int pf; >> To me "only actually 1 bit" makes it an implication that this is the >> lowest bit (like in a bool represented in a 32-bit memory location). >> I didn't think this was the case though, so unless I'm wrong, could >> you clarify this a little? > > There will be a single bit within the bottom 8 set (the 1 << > MSR_PLATFORM_ID[52:50]), despite this field being called "Platform Flags". That's what I recalled. Could I talk you into s/only actually 1 bit/only actually a single set bit/ or some such? If so, Acked-by: Jan Beulich <jbeulich@suse.com> Jan
diff --git a/xen/arch/x86/cpu/microcode/private.h b/xen/arch/x86/cpu/microcode/private.h index e64168a502..a2aec53047 100644 --- a/xen/arch/x86/cpu/microcode/private.h +++ b/xen/arch/x86/cpu/microcode/private.h @@ -14,14 +14,60 @@ enum microcode_match_result { struct microcode_patch; /* Opaque */ struct microcode_ops { + /* + * Parse a microcode container. Format is vendor-specific. + * + * Search within the container for the patch, suitable for the current + * CPU, which has the highest revision. (Note: May be a patch which is + * older that what is running in the CPU. This is a feature, to better + * cope with corner cases from buggy firmware.) + * + * If one is found, allocate and return a struct microcode_patch + * encapsulating the appropriate microcode patch. Does not alias the + * original buffer. + * + * If one is not found, (nothing matches the current CPU), return NULL. + * Also may return ERR_PTR(-err), e.g. bad container, out of memory. + */ struct microcode_patch *(*cpu_request_microcode)(const void *buf, size_t size); + + /* Obtain microcode-relevant details for the current CPU. */ int (*collect_cpu_info)(struct cpu_signature *csig); + + /* + * Attempt to load the provided patch into the CPU. Returns -EIO if + * anything didn't go as expected. + */ int (*apply_microcode)(const struct microcode_patch *patch); + + /* + * Optional. If provided and applicable to the specific update attempt, + * is run once by the initiating CPU. Returning an error will abort the + * load attempt. + */ int (*start_update)(void); + + /* + * Optional. If provided, called on every CPU which completes a microcode + * load. May be called in the case of some errors, and not others. May + * be called even if start_update() wasn't. + */ void (*end_update_percpu)(void); + + /* Free a patch previously allocated by cpu_request_microcode(). */ void (*free_patch)(struct microcode_patch *patch); + + /* + * Is the microcode patch applicable for the current CPU, and newer than + * the currently running patch? + */ bool (*match_cpu)(const struct microcode_patch *patch); + + /* + * Given two patches, are they both applicable to the current CPU, and is + * new a higher revision than old? + */ enum microcode_match_result (*compare_patch)( const struct microcode_patch *new, const struct microcode_patch *old); }; diff --git a/xen/include/asm-x86/microcode.h b/xen/include/asm-x86/microcode.h index 89b9aaa02d..41e85a24d2 100644 --- a/xen/include/asm-x86/microcode.h +++ b/xen/include/asm-x86/microcode.h @@ -7,8 +7,13 @@ #include <public/xen.h> struct cpu_signature { + /* CPU signature (CPUID.1.EAX). Only written on Intel. */ unsigned int sig; + + /* Platform Flags (only actually 1 bit). Only applicable to Intel. */ unsigned int pf; + + /* Microcode Revision. */ unsigned int rev; };
... and struct cpu_signature for good measure. No comment is passed on the suitability of the behaviour... Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/cpu/microcode/private.h | 46 ++++++++++++++++++++++++++++++++++++ xen/include/asm-x86/microcode.h | 5 ++++ 2 files changed, 51 insertions(+)