Re: [PATCH v14 04/19] x86: Secure Launch main header file

[ross . philipson]

On 4/24/25 5:29 AM, Huang, Kai wrote:
> On Mon, 2025-04-21 at 09:26 -0700, Ross Philipson wrote:
> > Introduce the main Secure Launch header file used in the early SL stub
> > and the early setup code.
> >
> > This header file contains the following categories:
> >   - Secure Launch implementation specific structures and definitions.
> >   - Intel TXT architecture specific DRTM structures, definitions and functions
> > used by Secure Launch.
> >   - DRTM TPM event logging definitions and helper functions.
>
> Looking at the actual code in this patch, seems >90% code in the
> <linux/slaunch.h> is Intel specific, e.g., TXT specific macro/structure
> definitions.  It doesn't seem to be the right way to organize the code.
>
> E.g., following the current pattern, when we need to add support for another TXT
> similar vendor-specific technology, we will need to add yet-another set of
> macro/structure definitions for that technology to <linux/slaunch.h> as well.
>
> That would be a giant mess IMHO.
>
> Could we make <linux/slaunch.h> only contain the common things, and move Intel
> specific things to some x86 header(s), e.g., <asm/intel-txt.h> or <asm/txt.h>?

Yes this looks to be a good idea. I think it has been something we have 
thought of before. We will look into it.


> [...]
>
> > +/*
> > + * External functions available in mainline kernel.
> > + */
> > +void slaunch_setup_txt(void);
> > +void slaunch_fixup_jump_vector(void);
> > +u32 slaunch_get_flags(void);
> > +struct sl_ap_wake_info *slaunch_get_ap_wake_info(void);
> > +struct acpi_table_header *slaunch_get_dmar_table(struct acpi_table_header *dmar);
> > +void __noreturn slaunch_txt_reset(void __iomem *txt,
> > +					 const char *msg, u64 error);
> > +void slaunch_finalize(int do_sexit);
> > +
> > +static inline bool slaunch_is_txt_launch(void)
> > +{
> > +	u32 mask = SL_FLAG_ACTIVE | SL_FLAG_ARCH_TXT;
> > +
> > +	return (slaunch_get_flags() & mask) == mask;
> > +}
> > +
> > +#else
> > +
> > +static inline void slaunch_setup_txt(void)
> > +{
> > +}
> > +
> > +static inline void slaunch_fixup_jump_vector(void)
> > +{
> > +}
> > +
> > +static inline u32 slaunch_get_flags(void)
> > +{
> > +	return 0;
> > +}
> > +
> > +static inline struct acpi_table_header *slaunch_get_dmar_table(struct acpi_table_header *dmar)
> > +{
> > +	return dmar;
> > +}
> > +
> > +static inline void slaunch_finalize(int do_sexit)
> > +{
> > +}
> > +
> > +static inline bool slaunch_is_txt_launch(void)
> > +{
> > +	return false;
> > +}
>
> .. btw it's not clear which part of the code is common code.
>
> Looking at the abvoe code, it seems those functions are common functions called
> from common code.  E.g., slaunch_finalize() is called from kernel/kexec_core.c,
> which means it is a concept in the kernel common code and must be available for
> all ARCHs (I haven't read how other functions are called, though).
>
> But the name slaunch_setup_txt(), slaunch_get_dmar_table() and
> slaunch_is_txt_launch() are quite Intel specific.  So it seems to me this patch
> _tries_ to support Secure Launch at the arch agnostic level but it doesn't do a
> good job at the abstraction?

If we do what you suggest, I think these ambiguities will go away.

Thank you,
Ross