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

[ross.philipson@oracle.com]

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