Re: [Xen-devel] [PATCH 1/7] x86/ucode: Document the behaviour of the microcode_ops hooks

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
	"Wei Liu" <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH 1/7] x86/ucode: Document the behaviour of the microcode_ops hooks
Date: Mon, 23 Mar 2020 13:33:04 +0100	[thread overview]
Message-ID: <3a2b459d-2d44-55fc-d72f-3f65afaf02af@suse.com> (raw)
In-Reply-To: <20200323101724.15655-2-andrew.cooper3@citrix.com>

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel