Re: [Xen-devel] [PATCH] xen/x86: Move microcode into its own directory

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] xen/x86: Move microcode into its own directory
Date: Thu, 19 Mar 2020 10:21:59 +0100	[thread overview]
Message-ID: <9168f174-1f75-86cd-267d-0b4bcd3a07e5@suse.com> (raw)
In-Reply-To: <080ba22f-f79d-1e50-71ba-ef3d2653e920@citrix.com>

On 18.03.2020 22:42, Andrew Cooper wrote:
> On 18/03/2020 21:05, Andrew Cooper wrote:
>> Split the existing asm/microcode.h in half, keeping the per-cpu cpu_sig
>> available to external users, and moving everything else into private.h
>>
>> Take the opportunity to trim and clean up the include lists for all 3 source
>> files, all of which include rather more than necessary.
>>
>> No functional change.
>>
>> 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>
>>
>> Inclusion of asm/flushtlb.h in isolation was broken by c/s 80943aa40e, and the
>> commit message even states this breakage.  I'm surprised it got accepted.
>>
>> Either this needs fixing, or the 23(!) other files including asm/flushtlb.h
>> should be adjusted.  Personally I don't think it is reasonable to require
>> including xen/mm.h just to get at tlb flushing functionality, but I also can't
>> spot an obvious way to untangle the dependencies (hence the TODO).
> 
> Actually, I've found that microcode_free_patch() has no external callers.
> 
> I've folded the following delta in, to avoid moving a useless function
> declaration
> 
> diff --git a/xen/arch/x86/microcode/core.c b/xen/arch/x86/microcode/core.c
> index e99f4ab06c..19e1d4b221 100644
> --- a/xen/arch/x86/microcode/core.c
> +++ b/xen/arch/x86/microcode/core.c
> @@ -243,7 +243,7 @@ static struct microcode_patch *parse_blob(const char
> *buf, size_t len)
>      return NULL;
>  }
>  
> -void microcode_free_patch(struct microcode_patch *microcode_patch)
> +static void microcode_free_patch(struct microcode_patch *microcode_patch)
>  {
>      microcode_ops->free_patch(microcode_patch->mc);
>      xfree(microcode_patch);
> diff --git a/xen/arch/x86/microcode/private.h
> b/xen/arch/x86/microcode/private.h
> index 97c7405dad..2e3be79eaf 100644
> --- a/xen/arch/x86/microcode/private.h
> +++ b/xen/arch/x86/microcode/private.h
> @@ -34,6 +34,4 @@ struct microcode_ops {
>  
>  extern const struct microcode_ops *microcode_ops;
>  
> -void microcode_free_patch(struct microcode_patch *patch);
> -
>  #endif /* ASM_X86_MICROCODE_PRIVATE_H */
> 
> 
> Alternatively, I might consider pulling this and the similar change to
> early_microcode_update_cpu() into an earlier patch, to separate the
> static-ing of functions from the general moving of code/declarations.
> 
> Thoughts?

Either way is fine by me, and can have my ack right away.

Jan

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