Re: [PATCH v1 2/2] spl: Add support for booting from ESP
On Thu, Sep 21, 2023 at 05:03:37PM +0530, mchit...@ventanamicro.com wrote:
> On Thu, 2023-09-14 at 12:29 -0400, Tom Rini wrote:
> > On Thu, Sep 14, 2023 at 03:38:21PM +0530, Mayuresh Chitale wrote:
> >
> > > Some platforms as described by EBBR specification may store images
> > > in
> > > the FIRMWARE directory of the UEFI system partition(ESP). Add
> > > support
> > > to boot from the EFI system partition if it is enabled for a
> > > platform.
> > >
> > > Signed-off-by: Mayuresh Chitale
> > [snip]
> > > +config SPL_ESP_BOOT
> > > + bool "Load next stage boot image from the UEFI system
> > > partition"
> > > + select SPL_PARTITION_TYPE_GUID
> > > + help
> > > + When enabled, first try to boot from the UEFI system
> > > partition as
> > > + described in the Ch.4 of the EBBR specification.
> >
> > We need to select this by perhaps BOOT_DEFAULTS or BOOTMETH_DISTRO,
> > whatever is supposed to signify that yes, this is going to be a
> > SystemReady IR (or higher) compliant build.
> I am not sure about the SystemReady compliance but I think the config
> option can be enabled for those platforms which require it.
It's "nearly everyone" that needs the option, which is why I'm saying it
needs to be part of the high level options that a board selects to say
they want to be compliant.
> > [snip]
> > > int spl_blk_load_image(struct spl_image_info *spl_image,
> > > struct spl_boot_device *bootdev,
> > > enum uclass_id uclass_id, int devnum, int
> > > partnum)
> > > {
> > > const char *filename = CONFIG_SPL_FS_LOAD_PAYLOAD_NAME;
> > > - struct legacy_img_hdr *header;
> > [snip]
> > > @@ -63,24 +88,26 @@ int spl_blk_load_image(struct spl_image_info
> > > *spl_image,
> > > }
> > >
> > > blk_show_device(uclass_id, devnum);
> > > - header = spl_get_load_buffer(-sizeof(*header),
> > > sizeof(*header));
> > > -
> > [snip]
> >
> > Is this an artifact of Sean's patch series where "header" is no
> > longer
> > actually used? As-is this won't compile on top of next (which is
> > where
> > given Sean's feedback, the series needs to be rebased upon anyhow).
> Yes, I will rebase on the latest next without Sean's series and resend
> the patch.
OK.
> > > + /*
> > > + * First try to boot from EFI System partition. In case of
> > > failure,
> > > + * fall back to the configured partition.
> > > + */
> >
> > I don't know that this is the right behavior. If we're configured to
> > boot from partition X, we boot from partition X. If we're configured
> > to
> > find the ESP and use that, we find and use that.
>
> This is to maintain compatibility and make more attempts to boot. So
> the same SPL binary can load images from devices with or without the
> ESP.
Yes, I don't think that's what we want. It should be an either/or not a
a then b.
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH v1 2/2] spl: Add support for booting from ESP
On Thu, 2023-09-14 at 12:29 -0400, Tom Rini wrote:
> On Thu, Sep 14, 2023 at 03:38:21PM +0530, Mayuresh Chitale wrote:
>
> > Some platforms as described by EBBR specification may store images
> > in
> > the FIRMWARE directory of the UEFI system partition(ESP). Add
> > support
> > to boot from the EFI system partition if it is enabled for a
> > platform.
> >
> > Signed-off-by: Mayuresh Chitale
> [snip]
> > +config SPL_ESP_BOOT
> > + bool "Load next stage boot image from the UEFI system
> > partition"
> > + select SPL_PARTITION_TYPE_GUID
> > + help
> > + When enabled, first try to boot from the UEFI system
> > partition as
> > + described in the Ch.4 of the EBBR specification.
>
> We need to select this by perhaps BOOT_DEFAULTS or BOOTMETH_DISTRO,
> whatever is supposed to signify that yes, this is going to be a
> SystemReady IR (or higher) compliant build.
I am not sure about the SystemReady compliance but I think the config
option can be enabled for those platforms which require it.
> [snip]
> > int spl_blk_load_image(struct spl_image_info *spl_image,
> >struct spl_boot_device *bootdev,
> >enum uclass_id uclass_id, int devnum, int
> > partnum)
> > {
> > const char *filename = CONFIG_SPL_FS_LOAD_PAYLOAD_NAME;
> > - struct legacy_img_hdr *header;
> [snip]
> > @@ -63,24 +88,26 @@ int spl_blk_load_image(struct spl_image_info
> > *spl_image,
> > }
> >
> > blk_show_device(uclass_id, devnum);
> > - header = spl_get_load_buffer(-sizeof(*header),
> > sizeof(*header));
> > -
> [snip]
>
> Is this an artifact of Sean's patch series where "header" is no
> longer
> actually used? As-is this won't compile on top of next (which is
> where
> given Sean's feedback, the series needs to be rebased upon anyhow).
Yes, I will rebase on the latest next without Sean's series and resend
the patch.
> > + /*
> > +* First try to boot from EFI System partition. In case of
> > failure,
> > +* fall back to the configured partition.
> > +*/
>
> I don't know that this is the right behavior. If we're configured to
> boot from partition X, we boot from partition X. If we're configured
> to
> find the ESP and use that, we find and use that.
This is to maintain compatibility and make more attempts to boot. So
the same SPL binary can load images from devices with or without the
ESP.
Re: [PATCH v1 2/2] spl: Add support for booting from ESP
On Thu, Sep 14, 2023 at 03:38:21PM +0530, Mayuresh Chitale wrote:
> Some platforms as described by EBBR specification may store images in
> the FIRMWARE directory of the UEFI system partition(ESP). Add support
> to boot from the EFI system partition if it is enabled for a platform.
>
> Signed-off-by: Mayuresh Chitale
[snip]
> +config SPL_ESP_BOOT
> + bool "Load next stage boot image from the UEFI system partition"
> + select SPL_PARTITION_TYPE_GUID
> + help
> + When enabled, first try to boot from the UEFI system partition as
> + described in the Ch.4 of the EBBR specification.
We need to select this by perhaps BOOT_DEFAULTS or BOOTMETH_DISTRO,
whatever is supposed to signify that yes, this is going to be a
SystemReady IR (or higher) compliant build.
[snip]
> int spl_blk_load_image(struct spl_image_info *spl_image,
> struct spl_boot_device *bootdev,
> enum uclass_id uclass_id, int devnum, int partnum)
> {
> const char *filename = CONFIG_SPL_FS_LOAD_PAYLOAD_NAME;
> - struct legacy_img_hdr *header;
[snip]
> @@ -63,24 +88,26 @@ int spl_blk_load_image(struct spl_image_info *spl_image,
> }
>
> blk_show_device(uclass_id, devnum);
> - header = spl_get_load_buffer(-sizeof(*header), sizeof(*header));
> -
[snip]
Is this an artifact of Sean's patch series where "header" is no longer
actually used? As-is this won't compile on top of next (which is where
given Sean's feedback, the series needs to be rebased upon anyhow).
> + /*
> + * First try to boot from EFI System partition. In case of failure,
> + * fall back to the configured partition.
> + */
I don't know that this is the right behavior. If we're configured to
boot from partition X, we boot from partition X. If we're configured to
find the ESP and use that, we find and use that.
--
Tom
signature.asc
Description: PGP signature
