linux-kernel - Re: [PATCH v2 11/12] x86/paravirt: Don't use pv

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <1541b670-8b29-42a5-a58d-34d85197751d@suse.com>
Date: Tue, 30 Sep 2025 11:02:52 +0200
From: Jürgen Groß <jgross@...e.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: linux-kernel@...r.kernel.org, x86@...nel.org,
 virtualization@...ts.linux.dev, xin@...or.com,
 Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
 Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>,
 "H. Peter Anvin" <hpa@...or.com>, Ajay Kaher <ajay.kaher@...adcom.com>,
 Alexey Makhalov <alexey.makhalov@...adcom.com>,
 Broadcom internal kernel review list
 <bcm-kernel-feedback-list@...adcom.com>,
 Boris Ostrovsky <boris.ostrovsky@...cle.com>, xen-devel@...ts.xenproject.org
Subject: Re: [PATCH v2 11/12] x86/paravirt: Don't use pv_ops vector for MSR
 access functions

On 30.09.25 10:38, Peter Zijlstra wrote:
> On Tue, Sep 30, 2025 at 09:03:55AM +0200, Juergen Gross wrote:
> 
>> +static __always_inline u64 read_msr(u32 msr)
>> +{
>> +	if (cpu_feature_enabled(X86_FEATURE_XENPV))
>> +		return xen_read_msr(msr);
>> +
>> +	return native_rdmsrq(msr);
>> +}
>> +
>> +static __always_inline int read_msr_safe(u32 msr, u64 *p)
>> +{
>> +	if (cpu_feature_enabled(X86_FEATURE_XENPV))
>> +		return xen_read_msr_safe(msr, p);
>> +
>> +	return native_read_msr_safe(msr, p);
>> +}
>> +
>> +static __always_inline void write_msr(u32 msr, u64 val)
>> +{
>> +	if (cpu_feature_enabled(X86_FEATURE_XENPV))
>> +		xen_write_msr(msr, val);
>> +	else
>> +		native_wrmsrq(msr, val);
>> +}
>> +
>> +static __always_inline int write_msr_safe(u32 msr, u64 val)
>> +{
>> +	if (cpu_feature_enabled(X86_FEATURE_XENPV))
>> +		return xen_write_msr_safe(msr, val);
>> +
>> +	return native_write_msr_safe(msr, val);
>> +}
>> +
>> +static __always_inline u64 rdpmc(int counter)
>> +{
>> +	if (cpu_feature_enabled(X86_FEATURE_XENPV))
>> +		return xen_read_pmc(counter);
>> +
>> +	return native_read_pmc(counter);
>> +}
> 
> Egads, didn't we just construct giant ALTERNATIVE()s for the native_
> things? Why wrap that in a cpu_feature_enabled() instead of just adding
> one more case to the ALTERNATIVE() ?

The problem I encountered with using pv_ops was to implement the *_safe()
variants. There is no simple way to do that using ALTERNATIVE_<n>(), as
in the Xen PV case the call will remain, and I didn't find a way to
specify a sane interface between the call-site and the called Xen function
to return the error indicator. Remember that at the call site the main
interface is the one of the RDMSR/WRMSR instructions. They lack an error
indicator.

In Xin's series there was a patch written initially by you to solve such
a problem by adding the _ASM_EXTABLE_FUNC_REWIND() exception table method.
I think this is a dead end, as it will break when using a shadow stack.

Additionally I found a rather ugly hack only to avoid re-iterating most of
the bare metal ALTERNATIVE() for the paravirt case. It is possible, but the
bare metal case is gaining one additional ALTERNATIVE level, resulting in
patching the original instruction with an identical copy first.

Another benefit of my approach is the dropping of "#include <asm/paravirt.h>"
from msr.h, which is making life a little bit easier.

Juergen

Download attachment "OpenPGP_0xB0DE9DD628BF132F.asc" of type "application/pgp-keys" (3684 bytes)

Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (496 bytes)