Re: [PATCH v2 2/2] x86/efi: Implement support for embedding SBAT data for x86

[Ard Biesheuvel <ardb@kernel.org>]

On Mon, 12 May 2025 at 16:02, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>
> Ard Biesheuvel <ardb@kernel.org> writes:
>
> > On Mon, 5 May 2025 at 17:46, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> >>
...
> >> +       .ascii  ".text\0\0\0"
> >> +#ifdef CONFIG_EFI_SBAT
> >> +       .long   ZO__sbat                        # VirtualSize
> >> +       .long   setup_size                      # VirtualAddress
> >> +       .long   ZO__sbat                        # SizeOfRawData
> >> +#else
> >> +       .long   ZO__data                        # VirtualSize
> >> +       .long   setup_size                      # VirtualAddress
> >> +       .long   ZO__data                        # SizeOfRawData
> >> +#endif
> >> +       .long   setup_size                      # PointerToRawData
> >
> > Would it work if we do the following here
> >
> > #ifdef CONFIG_EFI_SBAT
> >   .set .Ltextsize, ZO__sbat
> > #else
> >   .set .Ltextsize, ZO__data
> > #endif
> >
> > and keep a single section definition for .text
> >
> >   .ascii  ".text\0\0\0"
> >   .long   .Ltextsize                  # VirtualSize
> >   .long   setup_size                  # VirtualAddress
> >   .long   .Ltextsize                  # SizeOfRawData
> >   .long   setup_size                  # PointerToRawData
> >
>
> As we already have '#ifdef CONFIG_EFI_SBAT' below I'd suggest we set
> textsize there, basically:
>
> @@ -199,16 +194,20 @@ pecompat_fstart:
>                 IMAGE_SCN_MEM_EXECUTE           # Characteristics
>
>  #ifdef CONFIG_EFI_SBAT
> -       .ascii ".sbat\0\0\0"
> -       .long   ZO__esbat - ZO__sbat            # VirtualSize
> -       .long   setup_size + ZO__sbat           # VirtualAddress
> -       .long   ZO__esbat - ZO__sbat            # SizeOfRawData
> -       .long   setup_size + ZO__sbat           # PointerToRawData
> +       .ascii  ".sbat\0\0\0"
> +       .long   ZO__esbat - ZO__sbat            # VirtualSize
> +       .long   setup_size + ZO__sbat           # VirtualAddress
> +       .long   ZO__esbat - ZO__sbat            # SizeOfRawData
> +       .long   setup_size + ZO__sbat           # PointerToRawData
>
>         .long   0, 0, 0
>         .long   IMAGE_SCN_CNT_INITIALIZED_DATA  | \
>                 IMAGE_SCN_MEM_READ              | \
>                 IMAGE_SCN_MEM_DISCARDABLE       # Characteristics
> +
> +       .set textsize, ZO__sbat
> +#else
> +       .set textsize, ZO__data
>  #endif
>
>         .ascii  ".data\0\0\0"
>
> and nobody seems to care that we use it first and define/set it later.
>

Yeah that looks fine.

> BTW, does '.L' prefix you suggest has a meaning here? I see we don't use
> it for e.g. 'pecompat_fstart', 'section_count'.
>

The .L prefix prevents the symbols from ending up in the ELF file -
you can drop it if you like, it doesn't really matter one way or the
other.