| Message ID | 20221021203413.1220137-8-jithu.joseph@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | IFS multi test image support and misc changes | expand |
On 10/21/2022 1:34 PM, Jithu Joseph wrote: > Refactor header version as a parameter and expose this function. Isn't the header version part of the microcode data itself? Microcode Format +----------------------+ Base |Header Version | +----------------------+ |Update revision | +----------------------+ If so, why the need to pass it as a parameter to sanity_check()? > > No functional change intended. Maybe skip this statement. Apart from adding a parameter to an newly exported function, there is a change in an error print as well. > diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c > index 5473b094baee..bc3f33a25d7a 100644 > --- a/arch/x86/kernel/cpu/microcode/intel.c > +++ b/arch/x86/kernel/cpu/microcode/intel.c > @@ -37,6 +37,8 @@ > #include <asm/setup.h> > #include <asm/msr.h> > > +#define MICROCODE_HEADER_VER 1 > + Should this define be in a central location, like microcode_intel.h? You would soon be adding a define for IFS_HEADER_VER. Having them defined together would make it easier to follow. Sohil
On 11/1/2022 12:28 AM, Sohil Mehta wrote: > On 10/21/2022 1:34 PM, Jithu Joseph wrote: > >> Refactor header version as a parameter and expose this function. > > Isn't the header version part of the microcode data itself? > Header forms the first 48 bytes in a microcode file followed by contents > Microcode Format > +----------------------+ Base > |Header Version | > +----------------------+ > |Update revision | > +----------------------+ > > If so, why the need to pass it as a parameter to sanity_check()? > It is for sanity checking if the contents of the input file matches with the usage scenario. All microcode files have 1 in the header and IFS test images will use 2. Existing usages check if the microcode file has Header version 1. This (along with other sanity checks) is done by the below code microcode_intel_sanity_check(data, false, MICROCODE_HEADER_VER) and IFS usages check whether the file has Header version 2 (along with other sanity checks shared with microcode files) microcode_intel_sanity_check(data, true, IFS_HEADER_VER)) In other words the sanity check will flag somebody mistakenly passing in a microcode file as IFS test image (and vice-versa) >> >> No functional change intended. > > Maybe skip this statement. Apart from adding a parameter to an newly exported function, there is a change in an error print as well. > >> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c >> index 5473b094baee..bc3f33a25d7a 100644 >> --- a/arch/x86/kernel/cpu/microcode/intel.c >> +++ b/arch/x86/kernel/cpu/microcode/intel.c >> @@ -37,6 +37,8 @@ >> #include <asm/setup.h> >> #include <asm/msr.h> >> +#define MICROCODE_HEADER_VER 1 >> + > > Should this define be in a central location, like microcode_intel.h? > It can be moved there > You would soon be adding a define for IFS_HEADER_VER. Having them defined together would make it easier to follow. > > Sohil Jithu
On Fri, Oct 21, 2022 at 01:34:06PM -0700, Jithu Joseph wrote: > IFS test image carries the same microcode header as regular Intel > microcode blobs. Microcode blobs use header version of 1, > whereas IFS test images will use header version of 2. > > microcode_sanity_check() can be used by IFS driver to perform > sanity check of the IFS test images too. So I'm very sceptical about such reuse. The moment we decide to change something in the microcode loader, we're going to have to * test the IFS driver too * and I suspect we won't even be able to because we'd probably need special hardware and those special blobs which you probably don't distribute freely. And yes, right now that function should be doing the SDM-sanctioned dance about verifying the table and thus should also be generic but judging from past experience, things do get different in time and implementations do get changed so even if it is a trivial change to microcode_sanity_check(), we would still need to test the IFS driver too. So I'm wondering if it wouldn't be simply easier on everyone involved if you just copy the bits you need into your driver and use them there as you wish. Then the testing burden won't be an issue and there won't be any potential cross-breakages.
On Thu, Nov 03, 2022 at 12:33:50PM +0100, Borislav Petkov wrote: > On Fri, Oct 21, 2022 at 01:34:06PM -0700, Jithu Joseph wrote: > > IFS test image carries the same microcode header as regular Intel > > microcode blobs. Microcode blobs use header version of 1, > > whereas IFS test images will use header version of 2. > > > > microcode_sanity_check() can be used by IFS driver to perform > > sanity check of the IFS test images too. > > So I'm very sceptical about such reuse. The generic parts of the microcode is completely common between IFS and CPU microcode. The parts that differ are in the meta-data. CPU microcode has stayed the same for over 15 years roughly. Changing formats isn't something that's done lightly especially since there is cost of enabling in all OS's. Both are in the same class of microcode, consumed by the CPU. Inside Intel the format change is also pretty strict that they have several levels of approval within the HW teams and also needs review and approval from SW teams. What probably is best is that we should document that the external formats are shared in the header and probably in the shared routines. Changing names as you suggested to be more generic is in the right track. > Then the testing burden won't be an issue and there won't be any > potential cross-breakages. Even if its only a few hundred lines of code, it seems odd to simply duplicate for the sake of fear of testing.
On Thu, Nov 03, 2022 at 12:25:32PM -0700, Ashok Raj wrote: > CPU microcode has stayed the same for over 15 years roughly. Changing > formats isn't something that's done lightly especially since there is cost > of enabling in all OS's. Yeah, because it has been that way for the last 15 years and that means it'll not change in the future either. Yeah, right. > Even if its only a few hundred lines of code, it seems odd to simply > duplicate for the sake of fear of testing. I don't think I ever said "fear" but rather that it would be real hard to test. But it's not like you're reading my mails properly. But don't worry about it. We'll change the microcode loader and if we break the IFS thing in the process, then, oh well, sh*t happens.
On 11/3/2022 4:33 AM, Borislav Petkov wrote: > The moment we decide to change something in the microcode loader, we're > going to have to > > * test the IFS driver too > > * and I suspect we won't even be able to because we'd probably need > special hardware and those special blobs which you probably don't > distribute freely. We hear your concern that IFS test images are not yet publicly available for testing. We are working on it and it should be available in the near future. > > And yes, right now that function should be doing the SDM-sanctioned > dance about verifying the table and thus should also be generic but > judging from past experience, things do get different in time and > implementations do get changed so even if it is a trivial change to > microcode_sanity_check(), we would still need to test the IFS driver > too. > > So I'm wondering if it wouldn't be simply easier on everyone involved if > you just copy the bits you need into your driver and use them there as > you wish. > > Then the testing burden won't be an issue and there won't be any > potential cross-breakages. > We further hear your concern of potential cross-breakages due to IFS and Intel Microcode update driver sharing common functionality (like find_matching_signature() and microcode_sanity_check()). Within Intel, microcode and IFS folks do work closely, limiting the chances of this being introduced from Intel side. If these doesn’t alleviate your concern, I will post v2 without exporting the aforementioned functions and implementing them separately in IFS driver as you suggested. Jithu
On Thu, Nov 03, 2022 at 11:15:12PM -0700, Joseph, Jithu wrote: > If these doesn’t alleviate your concern, I will post v2 without > exporting the aforementioned functions and implementing them > separately in IFS driver as you suggested. So tglx persuaded me yesterday that we should do code sharing after all, so that all blob loading remains consistent. So let's try the cpu/intel.c thing and see what breaks, how and when. As to patch 8, that metadata checking should not be part of microcode_intel_sanity_check() but a separate function. Along with microcode_intel_find_meta_data() - all those should go into the IFS thing. When microcode loading ends up really needing metadata, *then* that functionality should be lifted into a more fitting place like cpu/intel.c Thx.
On 11/4/2022 3:50 AM, Borislav Petkov wrote: > As to patch 8, that metadata checking should not be part of > microcode_intel_sanity_check() but a separate function. Along with > microcode_intel_find_meta_data() - all those should go into the IFS > thing. > > When microcode loading ends up really needing metadata, *then* > that functionality should be lifted into a more fitting place like > cpu/intel.c Thanks for the pointer. I will move the microcode_intel_find_meta_data() function into IFS driver and also drop the metadata checking from the exported microcode_intel_sanity_check() Wanted to check with you, if it is okay to rename the first "reserved" field in microcode_header_intel to "metasize" today (as shown in the diff below) or would you prefer to do that too at a later point ? (doing so today will help to avoid redefining an IFS specific header struct, with this as the only change ) diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h index a4d2ed43193c..bec23c11ca52 100644 --- a/arch/x86/include/asm/microcode_intel.h +++ b/arch/x86/include/asm/microcode_intel.h @@ -14,7 +14,8 @@ struct microcode_header_intel { unsigned int pf; unsigned int datasize; unsigned int totalsize; - unsigned int reserved[3]; + unsigned int metasize; + unsigned int reserved[2]; }; Jithu
On Fri, Nov 04, 2022 at 03:02:42PM -0700, Joseph, Jithu wrote: > Wanted to check with you, if it is okay to rename the first "reserved" > field in microcode_header_intel to "metasize" today (as shown in the > diff below) or would you prefer to do that too at a later point ? > (doing so today will help to avoid redefining an IFS specific header > struct, with this as the only change ) No objections - it is not used by the microcode loader anyway. Thx.
diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h index 33db2a62ed34..27eba991c6b6 100644 --- a/arch/x86/include/asm/microcode_intel.h +++ b/arch/x86/include/asm/microcode_intel.h @@ -75,6 +75,7 @@ extern void show_ucode_info_early(void); extern int __init save_microcode_in_initrd_intel(void); void reload_ucode_intel(void); int microcode_intel_find_matching_signature(void *mc, unsigned int csig, int cpf); +int microcode_intel_sanity_check(void *mc, bool print_err, int hdr_ver); #else static inline __init void load_ucode_intel_bsp(void) {} static inline void load_ucode_intel_ap(void) {} @@ -83,6 +84,8 @@ static inline int __init save_microcode_in_initrd_intel(void) { return -EINVAL; static inline void reload_ucode_intel(void) {} static inline int microcode_intel_find_matching_signature(void *mc, unsigned int csig, int cpf) { return 0; } +static inline int microcode_intel_sanity_check(void *mc, bool print_err, int hdr_ver) + { return -EINVAL; } #endif #endif /* _ASM_X86_MICROCODE_INTEL_H */ diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c index 5473b094baee..bc3f33a25d7a 100644 --- a/arch/x86/kernel/cpu/microcode/intel.c +++ b/arch/x86/kernel/cpu/microcode/intel.c @@ -37,6 +37,8 @@ #include <asm/setup.h> #include <asm/msr.h> +#define MICROCODE_HEADER_VER 1 + static const char ucode_path[] = "kernel/x86/microcode/GenuineIntel.bin"; /* Current microcode patch used in early patching on the APs. */ @@ -164,7 +166,7 @@ static void save_microcode_patch(struct ucode_cpu_info *uci, void *data, unsigne intel_ucode_patch = p->data; } -static int microcode_sanity_check(void *mc, bool print_err) +int microcode_intel_sanity_check(void *mc, bool print_err, int hdr_ver) { unsigned long total_size, data_size, ext_table_size; struct microcode_header_intel *mc_header = mc; @@ -181,9 +183,10 @@ static int microcode_sanity_check(void *mc, bool print_err) return -EINVAL; } - if (mc_header->ldrver != 1 || mc_header->hdrver != 1) { + if (mc_header->ldrver != 1 || mc_header->hdrver != hdr_ver) { if (print_err) - pr_err("Error: invalid/unknown microcode update format.\n"); + pr_err("Error: invalid/unknown microcode update format. Header version %d\n", + mc_header->hdrver); return -EINVAL; } @@ -261,6 +264,7 @@ static int microcode_sanity_check(void *mc, bool print_err) } return 0; } +EXPORT_SYMBOL_GPL(microcode_intel_sanity_check); /* * Get microcode matching with BSP's model. Only CPUs with the same model as @@ -282,7 +286,7 @@ scan_microcode(void *data, size_t size, struct ucode_cpu_info *uci, bool save) mc_size = get_totalsize(mc_header); if (!mc_size || mc_size > size || - microcode_sanity_check(data, false) < 0) + microcode_intel_sanity_check(data, false, MICROCODE_HEADER_VER) < 0) break; size -= mc_size; @@ -821,7 +825,7 @@ static enum ucode_state generic_load_microcode(int cpu, struct iov_iter *iter) memcpy(mc, &mc_header, sizeof(mc_header)); data = mc + sizeof(mc_header); if (!copy_from_iter_full(data, data_size, iter) || - microcode_sanity_check(mc, true) < 0) { + microcode_intel_sanity_check(mc, true, MICROCODE_HEADER_VER) < 0) { break; }