[U-Boot] [PATCH 1/4] x86: microcode-tool: Write pure data to the dtsi file

[Simon Glass]

Hi Bin,

On 10 August 2015 at 21:05, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Simon,
>
> On Tue, Aug 11, 2015 at 10:51 AM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Bin,
>>
>> On 10 August 2015 at 20:45, Bin Meng <bmeng.cn at gmail.com> wrote:
>>> Hi Simon,
>>>
>>> On Mon, Aug 10, 2015 at 11:48 PM, Simon Glass <sjg at chromium.org> wrote:
>>>> Hi Bin,
>>>>
>>>> On 7 August 2015 at 18:47, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>>> Hi Simon,
>>>>>
>>>>> On Sat, Aug 8, 2015 at 3:26 AM, Simon Glass <sjg at chromium.org> wrote:
>>>>>> Hi Bin,
>>>>>>
>>>>>> On 7 August 2015 at 03:28, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>>>>> Currently the microcode-tool writes microcode into a data block as
>>>>>>> well as the device tree properties which represents the first 48
>>>>>>> bytes in the microcode data. Now we change the tool to only write
>>>>>>> the microcode without device tree stuff so that multiple microcode
>>>>>>> data blocks can be included in a single property.
>>>>>>>
>>>>>>> Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
>>>>>>> ---
>>>>>>>
>>>>>>>  tools/microcode-tool.py | 23 ++++++++++++-----------
>>>>>>>  1 file changed, 12 insertions(+), 11 deletions(-)
>>>>>>
>>>>>> I would rather than we use a tool to pack the microcode together (e.g.
>>>>>> ifdtool) rather than changing the source data. I realise that the FSP
>>>>>> requires this packing, but I quite dislike the approach of making the
>>>>>> source files fit the object files.
>>>>>>
>>>>>
>>>>> Could you roughly describe how you want to do using ifdtool? Is the
>>>>> microcode source still from dtb?
>>>>
>>>> Yes I think it can be done by adding an option to generate the
>>>> microcode blob (since for non-FSP platforms it is unnecessary). It can
>>>> scan the available microcode nodes and pack them into a single block,
>>>> then put a pointer to it into the ROM.
>>>>
>>>
>>> This way we have duplicated microcodes in the ROM. One in the device
>>> tree and the other one created by ifdtool as a single block. This
>>> causes waste of ROM space.
>>>
>>>> We already add the pointer with this:
>>>>
>>>> IFDTOOL_FLAGS += -m 0x$(shell $(NM) u-boot |grep _dt_ucode_base_size
>>>> |cut -d' ' -f1)
>>>>
>>>> but now it will need to go in a separate place in the ROM. I suggest
>>>> immediately above the device tree.
>>>>
>>>> See the write_uboot() which does all sorts of wacky stuff already.
>>>> Hopefully we can just replace that code with something that creates
>>>> the blob.
>>>>
>>>>>
>>>>>> If you like I can take a look at adding this feature to ifdtool.
>>>>>>
>>>>
>>>
>>> And I also would like to clean up the microcode dtsi files. The
>>> properties below:
>>>
>>> intel,header-version = <1>;
>>> intel,update-revision = <0x105>;
>>> intel,date-code = <0x7182011>;
>>> intel,processor-signature = <0x20661>;
>>> intel,checksum = <0x52558795>;
>>> intel,loader-revision = <1>;
>>> intel,processor-flags = <0x2>;
>>>
>>> I think we can remove them. It provides duplicated information as the
>>> data property. As data property has to be parsed by us anyway, with
>>> these additional properties it adds no value.
>>
>> Don't you think these are much easier to read than the binary header?
>>
>
> Yes, but I still think duplication is not good. And if we want to do
> duplication, we should add two more properties
> (intel,microcode-data-size and intel,microcode-total-size) to fully
> describe the first 48 bytes header. Right now the description is
> somewhere in between.

Well at least one of those could be found by looking at the length of
the 'data' property. But yes I suppose we may as well include
everything.

>
>> Also we could avoid repeating the data in the data property if we
>> wanted to, since ifdtool can fix this. I only added it because I was
>> unable to get microcode setup to work later on in U-Boot.
>>
>
> If we use ifdtool, the header data cannot be avoided as FSP needs the
> header. It looks like we may add some special handling in ifdtool when
> generating FSP based ROM. These microcode blocks can consume lots of
> ROM space if we keep two copies. We can have ifdtool to remove the
> microcode nodes in the dtb to avoid duplication, but I don't like that
> way. I think putting microcode in the device tree is good for future
> reference from U-Boot.

Yes and hopefully one day Intel will make microcode update from FSP
easier and we can go back to using device tree normally.

I agree two copies is a waste but it doesn't seem like a problem for
now. I agree that dropping the microcode from the fdt isn't worth it,
but if we want to later, then fdtgrep can do it pretty easily.

Mostly I just think that the fdt microcode files are much more
friendly and readable than the binary dumps.

Regards,
Simon