Re: [PATCH 09/14] platform/x86/intel/ifs: Use generic microcode headers and functions

Sohil Mehta
2022-11-01T18:37:46Z

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