Re: [RFC PATCH 03/20] x86/virt/seamldr: Introduce a wrapper for P-SEAMLDR SEAMCALLs

[Nikolay Borisov]

On 6/9/25 10:53, Chao Gao wrote:
> > > +config INTEL_TDX_MODULE_UPDATE
> > > +	bool "Intel TDX module runtime update"
> > > +	depends on INTEL_TDX_HOST
> > > +	help
> > > +	  This enables the kernel to support TDX module runtime update. This allows
> > > +	  the admin to upgrade the TDX module to a newer one without the need to
> > > +	  terminate running TDX guests.
> > > +
> > > +	  If unsure, say N.
> > > +
> >
> > WHy should this be conditional?
>
> Good question. I don't have a strong reason, but here are my considerations:
>
> 1. Runtime updates aren't strictly necessary for TDX functionalities. Users can
>     update the TDX module via BIOS updates and reboot if service downtime isn't
>     a concern.
>
> 2. Selecting TDX module updates requires selecting FW_UPLOAD and FW_LOADER,
>     which I think will significantly increase the kernel size if FW_UPLOAD/LOADER
>     won't otherwise be selected.

If size is a consideration (but given the size of machines that are 
likely to run CoCo guests I'd say it's not) then don't make this a 
user-configurable option but rather make it depend on TDX being selected 
and FW_UPLOAD/FW_LOADER being selected.

I'd rather keep the user visible options to a minimum, especially 
something such as this update functionality.

But in any case I'd like to hear other opinions as well.


> It may or may not be wise to assume that most TDX users will enable TDX module
> updates. so, I'm taking a conservative approach by making it optional. The
> resulting code isn't that complex, as CONFIG_INTEL_TDX_MODULE_UPDATE
> appears in only two places:
>
> 1. in the Makefile:
>
>    obj-y += seamcall.o tdx.o
>    obj-$(CONFIG_INTEL_TDX_MODULE_UPDATE) += seamldr.o
>
> 2. in the seamldr.h:
>
>    #ifdef CONFIG_INTEL_TDX_MODULE_UPDATE
>    extern struct attribute_group seamldr_group;
>    #define SEAMLDR_GROUP (&seamldr_group)
>    int get_seamldr_info(void);
>    void seamldr_init(struct device *dev);
>    #else
>    #define SEAMLDR_GROUP NULL
>    static inline int get_seamldr_info(void) { return 0; }
>    static inline void seamldr_init(struct device *dev) { }
>    #endif
>
> That said, I'm open to keeping or dropping the Kconfig option.