On Mon, Feb 03, 2025 at 10:06:28AM -0800, Dave Hansen wrote:
> On 1/31/25 18:32, Vishal Annapurve wrote:
> ...
> > Are you hinting towards a model where TDX guest prohibits such call
> > sites from being configured? I am not sure if it's a sustainable model
> > if we just rely on the host not advertising these features as the
> > guest kernel can still add new paths that are not controlled by the
> > host that lead to *_safe_halt().
>
> Let's say we required PARAVIRT_XXL for TDX guests and had TDX setup do:
>
> static const typeof(pv_ops) tdx_irq_ops __initconst = {
> .irq = {
> .safe_halt = tdx_safe_halt,
> },
> };
>
> We could get rid of a _bit_ of what TDX is doing now, like:
>
> } else if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST)) {
> pr_info("using TDX aware idle routine\n");
> static_call_update(x86_idle, tdx_safe_halt);
>
> and it would also fix this issue. Right?
>
> This commit:
>
> bfe6ed0c6727 ("x86/tdx: Add HLT support for TDX guests")
>
> Makes it seem totally possible:
>
> > Alternative choices like PV ops have been considered for adding
> > safe_halt() support. But it was rejected because HLT paravirt calls
> > only exist under PARAVIRT_XXL, and enabling it in TDX guest just for
> > safe_halt() use case is not worth the cost.
>
> and honestly it's seeming more "worth the cost" now since that partial
> approach has a bug and might have more bugs in the future.
If we want to go this path, I would rather move safe_halt out of
PARAVIRT_XXL. PARAVIRT_XXL is kitchen sink, no new code should touch it.
But Sean's proposal with HLT check before enabling interrupts looks better
to me.
--
Kiryl Shutsemau / Kirill A. Shutemov
