[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8cb7050c-d8e4-08ff-fb34-1df207a738db@intel.com>
Date: Tue, 1 Nov 2022 11:37:31 -0700
From: Sohil Mehta <sohil.mehta@...el.com>
To: Jithu Joseph <jithu.joseph@...el.com>, <hdegoede@...hat.com>,
<markgross@...nel.org>
CC: <tglx@...utronix.de>, <mingo@...hat.com>, <bp@...en8.de>,
<dave.hansen@...ux.intel.com>, <x86@...nel.org>, <hpa@...or.com>,
<gregkh@...uxfoundation.org>, <ashok.raj@...el.com>,
<tony.luck@...el.com>, <linux-kernel@...r.kernel.org>,
<platform-driver-x86@...r.kernel.org>, <patches@...ts.linux.dev>,
<ravi.v.shankar@...el.com>, <thiago.macieira@...el.com>,
<athenas.jimenez.gonzalez@...el.com>
Subject: Re: [PATCH 09/14] platform/x86/intel/ifs: Use generic microcode
headers and functions
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
Powered by blists - more mailing lists