linux-kernel - Re: [PATCH V2 1/1] x86/tdx: Route safe halt execution via tdx_safe

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAGtprH-n=cfH_BJAmiNMoRbqq0XdGCf3RE67TYW8z7RARnsCiQ@mail.gmail.com>
Date: Fri, 31 Jan 2025 18:32:04 -0800
From: Vishal Annapurve <vannapurve@...gle.com>
To: "Kirill A. Shutemov" <kirill@...temov.name>
Cc: x86@...nel.org, linux-kernel@...r.kernel.org, pbonzini@...hat.com, 
	seanjc@...gle.com, erdemaktas@...gle.com, ackerleytng@...gle.com, 
	jxgao@...gle.com, sagis@...gle.com, oupton@...gle.com, pgonda@...gle.com, 
	dave.hansen@...ux.intel.com, linux-coco@...ts.linux.dev, 
	chao.p.peng@...ux.intel.com, isaku.yamahata@...il.com, stable@...r.kernel.org
Subject: Re: [PATCH V2 1/1] x86/tdx: Route safe halt execution via tdx_safe_halt()

On Fri, Jan 31, 2025 at 12:13 AM Kirill A. Shutemov
<kirill@...temov.name> wrote:
>
> 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@...temov.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@...ux.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.kuppuswamy@linux.intel.com/
> > >
> > > 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()
>
> 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?
>

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().

> > 2) 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.

Apparently userspace VMM is advertising pblock_address through SSDT
tables in my configuration which causes guests to enable ACPI cpuidle
drivers. Do you know if future generations of TDX hardware will not
support different c-states for TDX VMs?

>
> --
>   Kiryl Shutsemau / Kirill A. Shutemov