Re: [PATCH v2 11/12] x86/paravirt: Don't use pv_ops vector for MSR access functions

[Jürgen Groß <jgross@xxxxxxxx>]

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

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature