On Thu, Jan 30, 2025 at 11:45:01AM -0800, Vishal Annapurve wrote:
On Thu, Jan 30, 2025 at 10:48 AM Kirill A. Shutemov kirill@shutemov.name wrote:
...
I think it is worth to putting this into a separate patch and not backport. The rest of the patch is bugfix and this doesn't belong.
Otherwise, looks good to me:
Reviewed-by: Kirill A. Shutemov kirill.shutemov@linux.intel.com@linux.intel.com>
-- Kiryl Shutsemau / Kirill A. Shutemov
Thanks Kirill for the review.
Thinking more about this fix, now I am wondering why the efforts [1] to move halt/safe_halt under CONFIG_PARAVIRT were abandoned. Currently proposed fix is incomplete as it would not handle scenarios where CONFIG_PARAVIRT_XXL is disabled. I am tilting towards reviving [1] and requiring CONFIG_PARAVIRT for TDX VMs. WDYT?
[1] https://lore.kernel.org/lkml/20210517235008.257241-1-sathyanarayanan.kuppusw...
Many people dislike paravirt callbacks. We tried to avoid relying on them for core TDX enabling.
Can you explain the issue you see with CONFIG_PARAVIRT_XXL being disabled? I don't think I follow.
Relevant callers of *_safe_halt() are:
- kvm_wait() -> safe_halt() -> raw_safe_halt() -> arch_safe_halt()
Okay, I didn't realized that CONFIG_PARAVIRT_SPINLOCKS doesn't depend on CONFIG_PARAVIRT_XXL.
It would be interesting to check if paravirtualized spinlocks make sense for TDX given the cost of TD exit.
Maybe we should avoid advertising KVM_FEATURE_PV_UNHALT to the TDX guests?
- acpi_safe_halt() -> safe_halt() -> raw_safe_halt() -> arch_safe_halt()
Have you checked why you get there? I don't see a reason for TDX guest to get into ACPI idle stuff. We don't have C-states to manage.