Re: [PATCH v4 04/14] x86/boot: Document the ordering dependency of _svm_cpu_up()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 27/02/2026 11:16 pm, Andrew Cooper wrote:
> Lets just say this took an unreasoanble amount of time and effort to track
> down, when trying to move traps_init() earlier during boot.
>
> When the SYSCALL linkage MSRs are not configured ahead of _svm_cpu_up() on the
> BSP, the first context switch into PV uses svm_load_segs() and clobbers the
> later-set-up linkage with the 0's cached here, causing hypercalls issues by
> the PV guest to enter at 0 in supervisor mode on the user stack.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>
> v4:
>  * New
>
> It occurs to me that it's not actually 0's we cache here.  It's whatever
> context was left from prior to Xen.  We still don't reliably clean unused
> MSRs.
> ---
>  xen/arch/x86/hvm/svm/svm.c | 16 ++++++++++++++++
>  xen/arch/x86/setup.c       |  2 +-
>  2 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 18ba837738c6..f1e02d919cae 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -35,6 +35,7 @@
>  #include <asm/p2m.h>
>  #include <asm/paging.h>
>  #include <asm/processor.h>
> +#include <asm/traps.h>
>  #include <asm/vm_event.h>
>  #include <asm/x86_emulate.h>
>  
> @@ -1581,6 +1582,21 @@ static int _svm_cpu_up(bool bsp)
>      /* Initialize OSVW bits to be used by guests */
>      svm_host_osvw_init();
>  
> +    /*
> +     * VMSAVE writes out the current full FS, GS, LDTR and TR segments, and
> +     * the GS_SHADOW, SYSENTER and SYSCALL linkage MSRs.
> +     *
> +     * The segment data gets modified by the svm_load_segs() optimisation for
> +     * PV context switches, but all values get reloaded at that point, as 
> well
> +     * as during context switch from SVM.
> +     *
> +     * If PV guests are available (and FRED is not in use), it is critical
> +     * that the SYSCALL linkage MSRs been configured at this juncture.
> +     */
> +    ASSERT(opt_fred >= 0); /* Confirm that FRED-ness has been resolved */
> +    if ( IS_ENABLED(CONFIG_PV) && !opt_fred )
> +        ASSERT(rdmsr(MSR_LSTAR));

It has occurred to me that this is subtly wrong.  While FRED doesn't use
LSTAR/SFMASK, it does reuse STAR.

So this needs to be:

    if ( IS_ENABLED(CONFIG_PV) )
        ASSERT(rdmsr(MSR_STAR));

with the include dropped, as the final sentence adjusted to say "even
with FRED".

~Andrew