Hi Junhui, I have some follow-up to add to the discussion,

On 10/4/26 21:25, Junhui Liu wrote:
> Hi E,
> 
> On Mon Oct 5, 2026 at 8:40 AM CST, E Shattow wrote:
>> Hi Junhui,
...
>>
>> The previous list-table of offsets is okay for review and diff output,
>> however the following list-table is not acceptable.
>>
>>> +
>>> +   * - Area
>>> +     - K1
>>> +     - K3 non-secure boot mode
>>> +     - K3 secure boot mode
>>> +   * - Root and OEM keys
>>> +     - RSA-2048 moduli with an eFuse root-hash check when secure boot is
>>> +       enabled
>>> +     - Unused
>>> +     - RSA-2048 moduli with an eFuse root-hash check
>>> +   * - Header CRC
>>> +     - Not checked separately
>>> +     - header1 verified
>>> +
>>> +       header0 ignored
>>> +     - Both verified and covered by RSA
>>> +   * - Image CRC
>>> +     - Not checked separately
>>> +     - Payload CRC32 in header1
>>> +     - Covered by RSA but not checked separately
>>> +   * - signature0
>>> +     - Verified with the root key over ``[0x100, 0xb00)``
>>> +     - Unused
>>> +     - Verified with the root key over ``[0x100, 0xb00)``
>>
>> Why the mixed-use of square brackets and parenthesis?
> 
> I think this is a fairly common mathematical notation for a half-open
> interval. If you find it less readable in this context, I can change it
> to [0x100, 0xaff] instead.

Let's qualify the interval notation with verbose "interval", for example:

-     - Verified with the root key over ``[0x100, 0xb00)``
+     - Verified with the root key over ``[0x100, 0xb00)`` interval

With this verbose qualifier I have some hope to search Wikipedia for
"interval" and learn about the symbolic representation. I assume here a
half-open interval representation is one of many forms of a
classification of "interval", so simply adding "interval" is enough context.

> 
>>
>>> +   * - signature1
>>> +     - Verified with the SPL key over header1 and the aligned payload
>>> +     - Unused
>>> +     - Verified with the SPL key over header1 and the aligned payload
>>> +
>>
>> I don't know what is preferred here by documentation reviewers but this
>> above list-table is not reviewable as-is. Maybe try to flip the axis, or
>> split into a series of tables as one-per-name of K1, K3 non-secure boot
>> mode, and K3 secure boot mode?  The purpose of restructuring this should
>> be readability before being rendered, and minimal 'diff' impact for
>> future changes.
> 
> I combined the three cases in one table intentionally, so readers can
> compare the treatment of each field across the K1, K3 non-secure, and K3
> secure paths side by side. Splitting it into three tables would make
> that comparison less direct and require readers to move back and forth
> between them. It would also duplicate the field names across the three
> tables.
> 
> Given that trade-off, I would prefer to keep the current table layout.
> 

No complaint about the rendered output it is very useful.

What I am meaning by "flip the axis" is for example:

.. list-table:: Authentication policy
   :header-rows: 1

   * - Image Variant
     - Root and OEM keys
     - Header CRC
     - Image CRC
     - signature0
     - signature1
   * - K1
     - RSA-2048 moduli with an eFuse root-hash check when secure boot is
       enabled
     - Not checked separately
     - Not checked separately
     - Verified with the root key over ``[0x100, 0xb00)`` interval
     - Verified with the SPL key over header1 and the aligned payload
   * - K3 non-secure
     - Unused
     - | header1 verified
       | header0 ignored
     - Payload CRC32 in header1
     - Unused
     - Unused
   * - K3 secure
     - RSA-2048 moduli with an eFuse root-hash check
     - Both verified and covered by RSA
     - Covered by RSA but not checked separately
     - Verified with the root key over ``[0x100, 0xb00)``
     - Verified with the SPL key over header1 and the aligned payload

Reviewability is improved yet it remains confusing and is less readable
on the rendered output.

Next, to then delete the column headers, move column descriptions to
natural-language descriptions, and keep a single-table format:

.. list-table:: Authentication policy

   * - K1
     - RSA-2048 moduli root and OEM keys with an eFuse root-hash check
       when secure boot is enabled
     - No additional header verification
     - No additional image checksum
     - Verified signature0 with the root key over ``[0x100, 0xb00)``
       interval
     - Verified signature1 with the SPL key over header1 and the aligned
       payload
   * - K3 non-secure
     - Unused root and OEM keys
     - header0 ignored, header1 verified
     - Image payload CRC32 in header1
     - Unused signature0
     - Unused signature1
   * - K3 secure
     - RSA-2048 moduli root and OEM keys with an eFuse root-hash check
     - header0 and header1 both verified and covered by RSA
     - No additional image checksum
     - Verified signature0 with the root key over ``[0x100, 0xb00)``
       interval
     - Verified signature1 with the SPL key over header1 and the aligned
       payload

I can now read this comfortably both in source and in rendered output.
What do you think?

>>
>>> +K1 verifies both signatures even without a programmed root-key hash. In 
>>> this
>>> +case, the root key comes from the image and is not authenticated by 
>>> hardware,
>>> +so the image, keys, and signatures can be replaced together.
>>> +
>>> +K3 uses CRC32 in non-secure boot mode and hardware-rooted RSA in secure 
>>> boot
>>> +mode.
>>> +
>>> +Creating an image
>>> +-----------------
>>> +
>>> +U-Boot currently creates only K3 images for non-secure boot mode. Package 
>>> an
>>> +SPL payload with::
>>> +
>>> +    $ tools/mkimage -T smtimage -n k3 -d u-boot-spl.bin FSBL.bin
>>
>> Use lowercase letters for the output filename if it is not any CONFIG_
>> symbol or preprocessor define symbol, and re-use an existing filename
>> stem as for example any of:
>>
>> u-boot-spl.smtimage
>> u-boot-spl-mkimage.bin
>> u-boot-spl.bin.smtimage
>> spl.bin
> 
> Okay. I prefer .bin as the filename suffix. I think I will use
> u-boot-spl-smtimage.bin.

Let's re-use "u-boot-spl-mkimage.bin" and
"u-boot-spl-mkimage.signed.bin" since you prefer ".bin" suffix and not
"smtimage" suffix?

`uniq` counts:

4  "spl/boot.bin"
4  "u-boot-spl-mkimage.bin"
4  "u-boot-spl-mkimage.signed.bin"
8  "spl.bin"
17  "u-boot-spl-ddr.bin"

SpacemiT K1 boards do use "u-boot-spl-ddr.bin" filename for the
processed SPL image but that is a binman artifact of some DDR training
blob from before support for smtimage is added to mkimage.

It may be a benefit for distro packagers and CI/CD workflow to have
distinct filenames for unsigned and signed output, so
"u-boot-spl-mkimage.bin" and "u-boot-spl-mkimage.signed.bin", otherwise
I would say "spl.bin" is short and popular for re-use.

> 
>>
>> A distro package of u-boot or later user of the build system may copy
>> this output to "FSBL.bin" but that is nothing to do with us here. In
>> fact for documentation purpose here it is useful to retain "FSBL.bin" as
>> a distinct description of the vendor firmware SPL and not get this
>> confused for mainline u-boot.
>>
>>> +
>>> +K1 images and RSA-authenticated K3 images are not yet supported.
>>>
>>
>> This statement may be simply deleted. It is obvious that support does
>> not exist when it is not described here. If you would like to keep this
>> line it is not any problem. Similarly for the sentence "U-Boot currently
>> creates only K3 images for non-secure boot mode." which is not really
>> accurate to say, as there is no makefile target or binman setup for any
>> of this yet. Technically U-Boot does nothing at all regarding this
>> documentation. It would just have to be more diff lines to review later
>> and can be omitted now.
> 
> I will remove it.
> 
>>
>> -E
> 

With all that, and I'll confirm my R-by tag again in the next round so
not to delay the series,

Reviewed-by: E Shattow <[email protected]>

Reply via email to