On Thu, Jan 30, 2025 at 10:48 AM Kirill A. Shutemov <kirill@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>@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.kuppuswamy@xxxxxxxxxxxxxxx/ > > 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: 1) kvm_wait() -> safe_halt() -> raw_safe_halt() -> arch_safe_halt() 2) acpi_safe_halt() -> safe_halt() -> raw_safe_halt() -> arch_safe_halt() arch_safe_halt() can get routed to native_safe_halt if CONFIG_PARAVIRT_XXL is disabled and will use "sti; hlt" combination which is unsafe for TDX VMs as of now. Either patch suggested by Sean [1] earlier or the implementation [2] to implement safe_halt always for TDX VMs seem functionally more correct to me. [2] being better where it avoids #VEs altogether. I haven't come across configurations where CONFIG_PARAVIRT_XXL is disabled but I don't see any guarantees around keeping it enabled for TDX VMs. [1] https://lore.kernel.org/lkml/Z5l6L3Hen9_Y3SGC@xxxxxxxxxx/ [2] https://lore.kernel.org/lkml/20210517235008.257241-1-sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx/ > > -- > Kiryl Shutsemau / Kirill A. Shutemov
