| Message ID | 20200318210540.5602-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/x86: Move microcode into its own directory | expand |
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? ~Andrew
On 18.03.2020 22: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> Acked-by: Jan Beulich <jbeulich@suse.com> albeit preferably with ... > --- > xen/arch/x86/Makefile | 4 +-- > xen/arch/x86/microcode/Makefile | 3 ++ > xen/arch/x86/{microcode_amd.c => microcode/amd.c} | 12 ++++---- > xen/arch/x86/{microcode.c => microcode/core.c} | 15 +++------- > .../x86/{microcode_intel.c => microcode/intel.c} | 9 ++---- > .../microcode.h => arch/x86/microcode/private.h} | 19 ++++--------- ... these going into xen/arch/x86/cpu/microcode/. Thoughts? Jan
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
On 19/03/2020 09:21, Jan Beulich wrote: > On 18.03.2020 22: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> > Acked-by: Jan Beulich <jbeulich@suse.com> > albeit preferably with ... > >> --- >> xen/arch/x86/Makefile | 4 +-- >> xen/arch/x86/microcode/Makefile | 3 ++ >> xen/arch/x86/{microcode_amd.c => microcode/amd.c} | 12 ++++---- >> xen/arch/x86/{microcode.c => microcode/core.c} | 15 +++------- >> .../x86/{microcode_intel.c => microcode/intel.c} | 9 ++---- >> .../microcode.h => arch/x86/microcode/private.h} | 19 ++++--------- > ... these going into xen/arch/x86/cpu/microcode/. Thoughts? TBH, I've always found the cpu/ directory redundant. Everything in arch/x86 is part of the CPU, and these days, even drivers/passthrough is part of the CPU. I'm happy to put it wherever makes sense, so long as there is a clear understanding of why. ~Andrew
On 19.03.2020 10:52, Andrew Cooper wrote: > On 19/03/2020 09:21, Jan Beulich wrote: >> On 18.03.2020 22: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> >> Acked-by: Jan Beulich <jbeulich@suse.com> >> albeit preferably with ... >> >>> --- >>> xen/arch/x86/Makefile | 4 +-- >>> xen/arch/x86/microcode/Makefile | 3 ++ >>> xen/arch/x86/{microcode_amd.c => microcode/amd.c} | 12 ++++---- >>> xen/arch/x86/{microcode.c => microcode/core.c} | 15 +++------- >>> .../x86/{microcode_intel.c => microcode/intel.c} | 9 ++---- >>> .../microcode.h => arch/x86/microcode/private.h} | 19 ++++--------- >> ... these going into xen/arch/x86/cpu/microcode/. Thoughts? > > TBH, I've always found the cpu/ directory redundant. Everything in > arch/x86 is part of the CPU, and these days, even drivers/passthrough is > part of the CPU. I'm surprised of you saying so - certainly e.g. memory management stuff also interfaces with the CPU, but is imo still helpful to be separated. Likewise while IOMMU stuff may today be part of the CPU package, it's still not core CPU functionality imo. > I'm happy to put it wherever makes sense, so long as there is a clear > understanding of why. The boundaries are always going to be fuzzy, I think. As said, I'd prefer the alternative place, but I'm not going to insist. Jan
On 19/03/2020 09:59, Jan Beulich wrote: > On 19.03.2020 10:52, Andrew Cooper wrote: >> On 19/03/2020 09:21, Jan Beulich wrote: >>> On 18.03.2020 22: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> >>> Acked-by: Jan Beulich <jbeulich@suse.com> >>> albeit preferably with ... >>> >>>> --- >>>> xen/arch/x86/Makefile | 4 +-- >>>> xen/arch/x86/microcode/Makefile | 3 ++ >>>> xen/arch/x86/{microcode_amd.c => microcode/amd.c} | 12 ++++---- >>>> xen/arch/x86/{microcode.c => microcode/core.c} | 15 +++------- >>>> .../x86/{microcode_intel.c => microcode/intel.c} | 9 ++---- >>>> .../microcode.h => arch/x86/microcode/private.h} | 19 ++++--------- >>> ... these going into xen/arch/x86/cpu/microcode/. Thoughts? >> TBH, I've always found the cpu/ directory redundant. Everything in >> arch/x86 is part of the CPU, and these days, even drivers/passthrough is >> part of the CPU. > I'm surprised of you saying so - certainly e.g. memory management > stuff also interfaces with the CPU, but is imo still helpful to be > separated. I can see an argument for things like domain.c not living in cpu/, but where do we draw the line? Should traps.c be considered cpu/ or not? What about FPU handling? > Likewise while IOMMU stuff may today be part of the > CPU package, it's still not core CPU functionality imo. Sure, for small systems, but considering it is effectively mandatory for a >255 cpu system, I'd no longer agree. After all, we know its not safe running an Intel system until you've turned on every thread's CR4.MCE, even if you don't actually want to use the thread. > >> I'm happy to put it wherever makes sense, so long as there is a clear >> understanding of why. > The boundaries are always going to be fuzzy, I think. As said, I'd > prefer the alternative place, but I'm not going to insist. All I'm looking for is some kind of clarity on what this boundary might be. ~Andrew
On 19.03.2020 11:41, Andrew Cooper wrote: > On 19/03/2020 09:59, Jan Beulich wrote: >> On 19.03.2020 10:52, Andrew Cooper wrote: >>> On 19/03/2020 09:21, Jan Beulich wrote: >>>> On 18.03.2020 22: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> >>>> Acked-by: Jan Beulich <jbeulich@suse.com> >>>> albeit preferably with ... >>>> >>>>> --- >>>>> xen/arch/x86/Makefile | 4 +-- >>>>> xen/arch/x86/microcode/Makefile | 3 ++ >>>>> xen/arch/x86/{microcode_amd.c => microcode/amd.c} | 12 ++++---- >>>>> xen/arch/x86/{microcode.c => microcode/core.c} | 15 +++------- >>>>> .../x86/{microcode_intel.c => microcode/intel.c} | 9 ++---- >>>>> .../microcode.h => arch/x86/microcode/private.h} | 19 ++++--------- >>>> ... these going into xen/arch/x86/cpu/microcode/. Thoughts? >>> TBH, I've always found the cpu/ directory redundant. Everything in >>> arch/x86 is part of the CPU, and these days, even drivers/passthrough is >>> part of the CPU. >> I'm surprised of you saying so - certainly e.g. memory management >> stuff also interfaces with the CPU, but is imo still helpful to be >> separated. > > I can see an argument for things like domain.c not living in cpu/, but > where do we draw the line? > > Should traps.c be considered cpu/ or not? Perhaps partly here and there. > What about FPU handling? Yes, this would belong under cpu/ imo. >> Likewise while IOMMU stuff may today be part of the >> CPU package, it's still not core CPU functionality imo. > > Sure, for small systems, but considering it is effectively mandatory for > a >255 cpu system, I'd no longer agree. That still doesn't make the IOMMU part of the core CPU. Nor is it technically impossible to run >255 CPU systems without IOMMU, it's just not very efficient interrupt distribution wise. > After all, we know its not safe running an Intel system until you've > turned on every thread's CR4.MCE, even if you don't actually want to use > the thread. Well, CR4.MCE and in fact all MCA handling is CPU stuff, and hence imo validly lives under cpu/mcheck/. Jan
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index ed709e2373..fa87520c66 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -43,9 +43,7 @@ obj-$(CONFIG_INDIRECT_THUNK) += indirect-thunk.o obj-y += ioport_emulate.o obj-y += irq.o obj-$(CONFIG_KEXEC) += machine_kexec.o -obj-y += microcode_amd.o -obj-y += microcode_intel.o -obj-y += microcode.o +obj-y += microcode/ obj-y += mm.o x86_64/mm.o obj-$(CONFIG_HVM) += monitor.o obj-y += mpparse.o diff --git a/xen/arch/x86/microcode/Makefile b/xen/arch/x86/microcode/Makefile new file mode 100644 index 0000000000..aae235245b --- /dev/null +++ b/xen/arch/x86/microcode/Makefile @@ -0,0 +1,3 @@ +obj-y += amd.o +obj-y += core.o +obj-y += intel.o diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode/amd.c similarity index 99% rename from xen/arch/x86/microcode_amd.c rename to xen/arch/x86/microcode/amd.c index bc7459416c..9028889813 100644 --- a/xen/arch/x86/microcode_amd.c +++ b/xen/arch/x86/microcode/amd.c @@ -16,16 +16,14 @@ #include <xen/err.h> #include <xen/init.h> -#include <xen/kernel.h> -#include <xen/lib.h> -#include <xen/sched.h> -#include <xen/smp.h> -#include <xen/spinlock.h> +#include <xen/mm.h> /* TODO: Fix asm/tlbflush.h breakage */ +#include <asm/hvm/svm/svm.h> #include <asm/msr.h> #include <asm/processor.h> -#include <asm/microcode.h> -#include <asm/hvm/svm/svm.h> +#include <asm/system.h> + +#include "private.h" #define pr_debug(x...) ((void)0) diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode/core.c similarity index 99% rename from xen/arch/x86/microcode.c rename to xen/arch/x86/microcode/core.c index 6907b312cf..e99f4ab06c 100644 --- a/xen/arch/x86/microcode.c +++ b/xen/arch/x86/microcode/core.c @@ -22,29 +22,22 @@ */ #include <xen/cpu.h> +#include <xen/earlycpio.h> #include <xen/err.h> +#include <xen/guest_access.h> #include <xen/init.h> -#include <xen/kernel.h> -#include <xen/lib.h> -#include <xen/notifier.h> #include <xen/param.h> -#include <xen/sched.h> -#include <xen/smp.h> -#include <xen/softirq.h> #include <xen/spinlock.h> #include <xen/stop_machine.h> -#include <xen/tasklet.h> -#include <xen/guest_access.h> -#include <xen/earlycpio.h> #include <xen/watchdog.h> #include <asm/apic.h> #include <asm/delay.h> -#include <asm/msr.h> #include <asm/nmi.h> #include <asm/processor.h> #include <asm/setup.h> -#include <asm/microcode.h> + +#include "private.h" /* * Before performing a late microcode update on any thread, we diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode/intel.c similarity index 98% rename from xen/arch/x86/microcode_intel.c rename to xen/arch/x86/microcode/intel.c index 91b7d473f7..90fb006c94 100644 --- a/xen/arch/x86/microcode_intel.c +++ b/xen/arch/x86/microcode/intel.c @@ -23,15 +23,12 @@ #include <xen/err.h> #include <xen/init.h> -#include <xen/kernel.h> -#include <xen/lib.h> -#include <xen/sched.h> -#include <xen/smp.h> -#include <xen/spinlock.h> #include <asm/msr.h> #include <asm/processor.h> -#include <asm/microcode.h> +#include <asm/system.h> + +#include "private.h" #define pr_debug(x...) ((void)0) diff --git a/xen/include/asm-x86/microcode.h b/xen/arch/x86/microcode/private.h similarity index 79% copy from xen/include/asm-x86/microcode.h copy to xen/arch/x86/microcode/private.h index 7d5a1f8e8a..97c7405dad 100644 --- a/xen/include/asm-x86/microcode.h +++ b/xen/arch/x86/microcode/private.h @@ -1,7 +1,9 @@ -#ifndef ASM_X86__MICROCODE_H -#define ASM_X86__MICROCODE_H +#ifndef ASM_X86_MICROCODE_PRIVATE_H +#define ASM_X86_MICROCODE_PRIVATE_H -#include <xen/percpu.h> +#include <xen/types.h> + +#include <asm/microcode.h> enum microcode_match_result { OLD_UCODE, /* signature matched, but revision id is older or equal */ @@ -9,8 +11,6 @@ enum microcode_match_result { MIS_UCODE, /* signature mismatched */ }; -struct cpu_signature; - struct microcode_patch { union { struct microcode_intel *mc_intel; @@ -32,15 +32,8 @@ struct microcode_ops { const struct microcode_patch *new, const struct microcode_patch *old); }; -struct cpu_signature { - unsigned int sig; - unsigned int pf; - unsigned int rev; -}; - -DECLARE_PER_CPU(struct cpu_signature, cpu_sig); extern const struct microcode_ops *microcode_ops; void microcode_free_patch(struct microcode_patch *patch); -#endif /* ASM_X86__MICROCODE_H */ +#endif /* ASM_X86_MICROCODE_PRIVATE_H */ diff --git a/xen/include/asm-x86/microcode.h b/xen/include/asm-x86/microcode.h index 7d5a1f8e8a..9b6ff7db08 100644 --- a/xen/include/asm-x86/microcode.h +++ b/xen/include/asm-x86/microcode.h @@ -3,35 +3,6 @@ #include <xen/percpu.h> -enum microcode_match_result { - OLD_UCODE, /* signature matched, but revision id is older or equal */ - NEW_UCODE, /* signature matched, but revision id is newer */ - MIS_UCODE, /* signature mismatched */ -}; - -struct cpu_signature; - -struct microcode_patch { - union { - struct microcode_intel *mc_intel; - struct microcode_amd *mc_amd; - void *mc; - }; -}; - -struct microcode_ops { - struct microcode_patch *(*cpu_request_microcode)(const void *buf, - size_t size); - int (*collect_cpu_info)(struct cpu_signature *csig); - int (*apply_microcode)(const struct microcode_patch *patch); - int (*start_update)(void); - void (*end_update_percpu)(void); - void (*free_patch)(void *mc); - bool (*match_cpu)(const struct microcode_patch *patch); - enum microcode_match_result (*compare_patch)( - const struct microcode_patch *new, const struct microcode_patch *old); -}; - struct cpu_signature { unsigned int sig; unsigned int pf; @@ -39,8 +10,5 @@ struct cpu_signature { }; DECLARE_PER_CPU(struct cpu_signature, cpu_sig); -extern const struct microcode_ops *microcode_ops; - -void microcode_free_patch(struct microcode_patch *patch); #endif /* ASM_X86__MICROCODE_H */
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). --- xen/arch/x86/Makefile | 4 +-- xen/arch/x86/microcode/Makefile | 3 ++ xen/arch/x86/{microcode_amd.c => microcode/amd.c} | 12 ++++---- xen/arch/x86/{microcode.c => microcode/core.c} | 15 +++------- .../x86/{microcode_intel.c => microcode/intel.c} | 9 ++---- .../microcode.h => arch/x86/microcode/private.h} | 19 ++++--------- xen/include/asm-x86/microcode.h | 32 ---------------------- 7 files changed, 22 insertions(+), 72 deletions(-) create mode 100644 xen/arch/x86/microcode/Makefile rename xen/arch/x86/{microcode_amd.c => microcode/amd.c} (99%) rename xen/arch/x86/{microcode.c => microcode/core.c} (99%) rename xen/arch/x86/{microcode_intel.c => microcode/intel.c} (98%) copy xen/{include/asm-x86/microcode.h => arch/x86/microcode/private.h} (79%)