Messages in this thread | | | Date | Tue, 1 Nov 2022 11:37:31 -0700 | Subject | Re: [PATCH 09/14] platform/x86/intel/ifs: Use generic microcode headers and functions | From | Sohil Mehta <> |
| |
On 10/21/2022 1:34 PM, Jithu Joseph wrote: > Newer IFS test image headers will use microcode_header_intel->hdrver = 2, > so as to distinguish it from microcode images and older IFS test images. >
IIUC, older IFS test images would no longer be supported. Have they been released publicly?
What would happen if someone tries to load one? I am guessing one of the error checks would catch it. It might be useful to describe this error signature in the commit message.
> > - if ((data_size + MC_HEADER_SIZE > total_size) || (total_size % sizeof(u32))) { > - dev_err(dev, "bad ifs data file size.\n"); > + if (data->hdrver != IFS_HEADER_VER) { > + dev_err(dev, "Header version %d not supported\n", data->hdrver); > return -EINVAL; > } > > - if (mc_header->ldrver != 1 || mc_header->hdrver != 1) { > - dev_err(dev, "invalid/unknown ifs update format.\n"); > + if (microcode_intel_sanity_check((void *)data, true, IFS_HEADER_VER)) {
I referred to this in a another patch. The data->hdrver is already verified above, why is there a need to pass it as a parameter as well.
> + dev_err(dev, "sanity check failed\n"); > return -EINVAL; > } > > - mc = (u32 *)mc_header; > - sum = 0; > - for (int i = 0; i < total_size / sizeof(u32); i++) > - sum += mc[i]; > + intel_cpu_collect_info(&uci); > > - if (sum) { > - dev_err(dev, "bad ifs data checksum, aborting.\n"); > + if (!microcode_intel_find_matching_signature((void *)data, > + uci.cpu_sig.sig, > + uci.cpu_sig.pf)) { > + dev_err(dev, "cpu signature, pf not matching\n");
What does pf stand for? It would be good to avoid abbreviations for error logging.
> /* > * Load ifs image. Before loading ifs module, the ifs image must be located > * in /lib/firmware/intel/ifs and named as {family/model/stepping}.{testname}. > @@ -252,12 +189,11 @@ int ifs_load_firmware(struct device *dev) > goto done; > } > > - if (!ifs_image_sanity_check(dev, (struct microcode_header_intel *)fw->data)) { > - dev_err(dev, "ifs header sanity check failed\n"); > + ret = ifs_image_sanity_check(dev, (struct microcode_header_intel *)fw->data); > + if (ret) > goto release; > - } > > - ifs_header_ptr = (struct ifs_header *)fw->data; > + ifs_header_ptr = (struct microcode_header_intel *)fw->data;
The use of a global ifs_header_ptr seems problematic. The semaphore operation before calling ifs_load_firmware() makes it seem concurrency is expected. Can ifs_load_firmware() really be called concurrently?
If that is not true can we use a mutex for synchronization?
Sohil
| |