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.