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

[Bin Meng]

Hi Simon,

On Tue, Aug 11, 2015 at 11:15 AM, Simon Glass <sjg at chromium.org> wrote:
> 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.

As I said before, this is a kind of limitation exposed by certain
processors. On some processors, without first applying microcode
updates, the CAR just won't work. Thus it is required to load
microcode update before initializing CAR and that's why Intel FSP
implementation chose doing this way. I believe the correct way should
be that Intel stops manufacturing malfunctional processors like this
:-)

> 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.

I am afraid that, keeping two copies with multiple microcode blobs
will make U-Boot does not fit the 1MB ROM size. Yes, it is not a
problem since we have large SPI flash, but keeping U-Boot ROM small is
one of our goals, isn't it?

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

I still think my series seems to be the easiest way to do this. But if
you want to use ifdtool, I am OK with that too.

Regards,
Bin