Re: [PATCH v2 1/1] spl: allow loading via partition type GUID

[Heinrich Schuchardt <heinrich.schuchardt@canonical.com>]

On 2/17/23 03:55, Simon Glass wrote:
> " properHi Heinrich,
> 
> On Thu, 16 Feb 2023 at 14:31, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>>
>>
>> On 2/16/23 21:17, Simon Glass wrote:
>>> Hi Heinrich,
>>>
>>> On Thu, 16 Feb 2023 at 08:30, Heinrich Schuchardt
>>> <heinrich.schuchardt@canonical.com> wrote:
>>>>
>>>> Some boards provide main U-Boot as a dedicated partition to SPL.
>>>> Currently we can define either a fixed partition number or an MBR
>>>> partition type to define which partition is to be used.
>>>>
>>>> Partition numbers tend to conflict with established partitioning schemes
>>>> of Linux distros. MBR partitioning is more and more replaced by GPT
>>>> partitioning.
>>>>
>>>> Allow defining a partition type GUID identifying the partition to load
>>>> main U-Boot from.
>>>>
>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>> ---
>>>> v2:
>>>>           avoid if/endif in Kconfig
>>>> ---
>>>>    common/spl/Kconfig   | 27 ++++++++++++++++++++++-----
>>>>    common/spl/spl_mmc.c | 13 +++++++++++++
>>>>    2 files changed, 35 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/common/spl/Kconfig b/common/spl/Kconfig
>>>> index 3c2af453ab..9d12b48297 100644
>>>> --- a/common/spl/Kconfig
>>>> +++ b/common/spl/Kconfig
>>>> @@ -514,19 +514,36 @@ config SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION
>>>>             used in raw mode
>>>>
>>>>    config SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION_TYPE
>>>> -       bool "MMC raw mode: by partition type"
>>>> +       bool "MMC raw mode: by MBR partition type"
>>>>           depends on DOS_PARTITION && SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION
>>>>           help
>>>> -         Use partition type for specifying U-Boot partition on MMC/SD in
>>>> +         Use MBR partition type for specifying U-Boot partition on MMC/SD in
>>>>             raw mode. U-Boot will be loaded from the first partition of this
>>>>             type to be found.
>>>>
>>>>    config SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION_TYPE
>>>> -       hex "Partition Type on the MMC to load U-Boot from"
>>>> +       hex "MBR Partition Type on the MMC to load U-Boot from"
>>>>           depends on SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION_TYPE
>>>>           help
>>>> -         Partition Type on the MMC to load U-Boot from, when the MMC is being
>>>> -         used in raw mode.
>>>> +         MBR Partition Type on the MMC to load U-Boot from, when the MMC is
>>>> +         being used in raw mode.
>>>> +
>>>> +config SYS_MMCSD_RAW_MODE_U_BOOT_USE_GPT_PARTITION_TYPE
>>>> +       bool "MMC raw mode: GPT by partition type"
>>>> +       depends on PARTITION_TYPE_GUID && SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION
>>>> +       help
>>>> +         Use GPT partition type for specifying U-Boot partition on MMC/SD in
>>>> +         raw mode. U-Boot will be loaded from the first partition of this
>>>> +         type to be found.
>>>> +
>>>> +config SYS_MMCSD_RAW_MODE_U_BOOT_GPT_PARTITION_TYPE
>>>> +       string "GPT Partition Type on the MMC to load U-Boot from"
>>>> +       depends on SYS_MMCSD_RAW_MODE_U_BOOT_USE_GPT_PARTITION_TYPE
>>>> +       default d2f002f8-e4e7-4269-b8ac-3bb6fabeaff6
>>>
>>> What is this? Can we have a register of these hideous things and call
>>> them by name?
> 
> Further, I don't see any documentation on this in U-Boot. Could you at
> least add a list of these things?
> 
>>>
>>>> +       help
>>>> +         GPT Partition Type on the MMC to load U-Boot from, when the MMC is
>>>> +         being used in raw mode. The GUID must be lower case, low endian,
>>>> +         and formatted like d2f002f8-e4e7-4269-b8ac-3bb6fabeaff6.
>>>>
>>>>    config SUPPORT_EMMC_BOOT_OVERRIDE_PART_CONFIG
>>>>           bool "Override eMMC EXT_CSC_PART_CONFIG by user defined partition"
>>>> diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
>>>> index e4135b2048..69bf1d6e98 100644
>>>> --- a/common/spl/spl_mmc.c
>>>> +++ b/common/spl/spl_mmc.c
>>>> @@ -191,6 +191,19 @@ static int mmc_load_image_raw_partition(struct spl_image_info *spl_image,
>>>>           struct disk_partition info;
>>>>           int err;
>>>>
>>>> +#ifdef CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_GPT_PARTITION_TYPE
>>>> +       for (int i = 1; i <= MAX_SEARCH_PARTITIONS; ++i) {
>>>> +               err = part_get_info(mmc_get_blk_desc(mmc), i, &info);
>>>> +               if (err)
>>>> +                       continue;
>>>> +               if (!strncmp(info.type_guid,
>>>> +                            CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_GPT_PARTITION_TYPE,
>>>> +                            UUID_STR_LEN)) {
>>>> +                       partition = i;
>>>> +                       break;
>>>> +               }
>>>> +       }
>>>> +#endif
>>>>    #ifdef CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION_TYPE
>>>>           int type_part;
>>>>           /* Only support MBR so DOS_ENTRY_NUMBERS */
>>>> --
>>>> 2.38.1
>>>>
>>>
>>> Is it possible to avoid using #ifdef here?
>>
>> Unfortunately not. Field 'type_guid' is restricted by an #ifdef. So
>> unconditional compilation would fail.
> 
> Do you think it is worth adding an accessor as we have done with some
> global_data things?

There are other places like disk/part_efi.c where we could trade #ifdef 
for if with such an accessor. The accessor itself would require an #ifdef.

We should be careful about the value returned for 
CONFIG_PARTITION_TYPE_GUID=n. Returning NULL would probably lead to 
warnings from GCC and Coverity. We would better return a dummy string 
like "00000000-0000-0000-0000-000000000000".

> 
>>
>>>
>>> Longer term, I wonder if we can add a DT schema for all of
>>> this...these CONFIG options for boot selection seem to be getting out
>>> of hand!
>>
>> Tom just moved a lot of constants hard coded in C code to Kconfig with a
>> big effort. Now you want to move Kconfig values to hard coded constants
>> in device-tree.
>>
>> Running in circles does not sound like a winning strategy.
> 
> The values seem to be common across certain boards of the same type, SoC, etc.

As I described above using the same GUID value for different boards only 
makes sense if they are supported by the very same U-Boot binary.

On boards with the same SoC (even from different vendors) I would very 
much prefer a single U-Boot SPL selecting a board specific device-tree 
to having multiple binaries. We should push developers into this direction.

> 
> I'm not sure, but at some point this is all going to get out of hand.
> Already we have these options:
> 
> common/spl/Kconfig:config SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR
> common/spl/Kconfig:config SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR
> common/spl/Kconfig:config SYS_MMCSD_RAW_MODE_U_BOOT_DATA_PART_OFFSET
> common/spl/Kconfig:config SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION
> common/spl/Kconfig:config SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION
> common/spl/Kconfig:config SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION_TYPE
> common/spl/Kconfig:config SYS_MMCSD_RAW_MODE_U_BOOT_PARTITION_TYPE
> common/spl/Kconfig:config SYS_MMCSD_RAW_MODE_EMMC_BOOT_PARTITION
> common/spl/Kconfig:config SYS_MMCSD_FS_BOOT_PARTITION
> common/spl/Kconfig:config SYS_MMCSD_RAW_MODE_KERNEL_SECTOR
> common/spl/Kconfig:config SYS_MMCSD_RAW_MODE_ARGS_SECTOR
> common/spl/Kconfig:config SYS_MMCSD_RAW_MODE_ARGS_SECTORS
> 
> That is just for MMC raw mode.
> 
> For environment we have SYS_MMC_ENV_DEV and _PART. If you look around
> you'll see loads of these options.
> 
> I see that rockchip uses u-boot,spl-boot-order as a way to determine
> the boot order. This makes it configurable without rebuilding
> U-Boot...although I don't think we need to make the MMC stuff
> configurable, since I am assuming that the boot ROM determines at
> least some of it...?

This patch is about SPL loading main U-Boot. So the boot ROM is not 
involved.

> 
> It seems that the whole thing is crying out for a bit of organisation
> and a proper schema.

The discussion was about hard-coding the values vs configuration.

OS distributions should have enough flexibility to deliver an 
installation image with U-Boot for multiple boards on the same medium. 
For the build process it is preferable to use different configurations 
instead of patching source code per U-Boot which might be required if 
hard-coded values for partition GUIDs in the device-trees are used.

I think Tom's approach is right. The U-Boot documentation should give 
guidance on how new boards should find U-Boot SPL and main U-Boot.

Best regards

Heinrich