Re: [PATCH v5 1/7] x86emul: support LKGS

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/09/2024 1:28 pm, Jan Beulich wrote:
> ---
> Instead of ->read_segment() we could of course also use ->read_msr() to
> fetch the original GS base. I don't think I can see a clear advantage of
> either approach; the way it's done it matches how we handle SWAPGS.

It turns out this is littered with broken corners.  See below.

> For PV save_segments() would need adjustment, but the insn being
> restricted to ring 0 means PV guests can't use it anyway (unless we
> wanted to emulate it as another privileged insn).

PV guests can't have architectural FRED anyway (although I'd really like
to do paravirt FRED to simplify Linux as x86 PV is already ~80% there).

We already handle SWAPGS automatically on kernel/user transitions, and
have a dedicated hypercall for setting GSKERN, so I don't see any need
to provide LKGS even in a future where there's some kind of paravirt-FRED.

> --- a/tools/tests/x86_emulator/test_x86_emulator.c
> +++ b/tools/tests/x86_emulator/test_x86_emulator.c
> @@ -693,6 +719,20 @@ static int read_msr(
>          *val = ctxt->addr_size > 32 ? 0x500 /* LME|LMA */ : 0;
>          return X86EMUL_OKAY;
>  
> +#ifdef __x86_64__
> +    case 0xc0000101: /* GS_BASE */

It's only just occurred to me, but given x86-defns.h, isn't msr-index.h
suitably usable too ?

> @@ -1335,6 +1400,41 @@ int main(int argc, char **argv)
>          printf("%u bytes read - ", bytes_read);
>          goto fail;
>      }
> +    printf("okay\n");
> +
> +    emulops.write_segment = write_segment;
> +    emulops.write_msr     = write_msr;
> +
> +    printf("%-40s", "Testing swapgs...");
> +    instr[0] = 0x0f; instr[1] = 0x01; instr[2] = 0xf8;
> +    regs.eip = (unsigned long)&instr[0];
> +    gs_base = 0xffffeeeecccc8888UL;
> +    gs_base_shadow = 0x0000111122224444UL;
> +    rc = x86_emulate(&ctxt, &emulops);
> +    if ( (rc != X86EMUL_OKAY) ||
> +         (regs.eip != (unsigned long)&instr[3]) ||
> +         (gs_base != 0x0000111122224444UL) ||
> +         (gs_base_shadow != 0xffffeeeecccc8888UL) )
> +        goto fail;
> +    printf("okay\n");
> +
> +    printf("%-40s", "Testing lkgs 2(%rdx)...");
> +    instr[0] = 0xf2; instr[1] = 0x0f; instr[2] = 0x00; instr[3] = 0x72; 
> instr[4] = 0x02;
> +    regs.eip = (unsigned long)&instr[0];
> +    regs.edx = (unsigned long)res;
> +    res[0]   = 0x00004444;
> +    res[1]   = 0x8888cccc;
> +    i = cp.extd.nscb; cp.extd.nscb = true; /* for AMD */
> +    rc = x86_emulate(&ctxt, &emulops);
> +    if ( (rc != X86EMUL_OKAY) ||
> +         (regs.eip != (unsigned long)&instr[5]) ||
> +         (gs_base != 0x0000111122224444UL) ||
> +         gs_base_shadow )
> +        goto fail;
> +
> +    cp.extd.nscb = i;

I think I acked the patches to rename this?

I'd suggest putting those in now, rather than creating more (re)work later.

> --- a/xen/arch/x86/x86_emulate/decode.c
> +++ b/xen/arch/x86/x86_emulate/decode.c
> @@ -743,8 +743,12 @@ decode_twobyte(struct x86_emulate_state
>          case 0:
>              s->desc |= DstMem | SrcImplicit | Mov;
>              break;
> +        case 6:
> +            if ( !(s->modrm_reg & 1) && mode_64bit() )
> +            {
>          case 2: case 4:
> -            s->desc |= SrcMem16;
> +                s->desc |= SrcMem16;
> +            }

Well - not something I was expecting, but I've just had to go and find
the Itanium instruction manuals...

Do we really need this complexity?  JMPE is a decoding wrinkle of
Itanium processors, which I think we can reasonably ignore.

IMO we should treat Grp6 as uniformly Reg/Mem16, and rely on the
!mode_64bit() to exclude the encodings commonly used as JMPE.

... not that I see any evidence that an Itanium processor wouldn't
decode LKGS as REP JMPE.

It is curious that Intel picked this encoding space for LKGS which, like
JMPE, is a CPL0-only instruction.  I assume it usefully shares a
group-decode row/column, but I've not had enough coffee for
higher-dimentional decode reasoning.


> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -2870,8 +2870,35 @@ x86_emulate(
>                  break;
>              }
>              break;
> -        default:
> -            generate_exception_if(true, X86_EXC_UD);
> +        case 6: /* lkgs */
> +            generate_exception_if((modrm_reg & 1) || vex.pfx != vex_f2,
> +                                  X86_EXC_UD);
> +            generate_exception_if(!mode_64bit() || !mode_ring0(), 
> X86_EXC_UD);
> +            vcpu_must_have(lkgs);
> +            fail_if(!ops->read_segment || !ops->read_msr ||
> +                    !ops->write_segment || !ops->write_msr);
> +            if ( (rc = ops->read_msr(MSR_SHADOW_GS_BASE, &msr_val,
> +                                     ctxt)) != X86EMUL_OKAY ||
> +                 (rc = ops->read_segment(x86_seg_gs, &sreg,
> +                                         ctxt)) != X86EMUL_OKAY )
> +                goto done;
> +            dst.orig_val = sreg.base; /* Preserve full GS Base. */
> +            if ( (rc = protmode_load_seg(x86_seg_gs, src.val, false, &sreg,
> +                                         ctxt, ops)) != X86EMUL_OKAY ||
> +                 /* Write (32-bit) base into SHADOW_GS. */
> +                 (rc = ops->write_msr(MSR_SHADOW_GS_BASE, sreg.base,

The comment says 32-bit, but that's the full base, isn't it?

> +                                      ctxt)) != X86EMUL_OKAY )
> +                goto done;
> +            sreg.base = dst.orig_val; /* Reinstate full GS Base. */
> +            if ( (rc = ops->write_segment(x86_seg_gs, &sreg,
> +                                          ctxt)) != X86EMUL_OKAY )
> +            {
> +                /* Best effort unwind (i.e. no real error checking). */
> +                if ( ops->write_msr(MSR_SHADOW_GS_BASE, msr_val,
> +                                    ctxt) == X86EMUL_EXCEPTION )
> +                    x86_emul_reset_event(ctxt);
> +                goto done;
> +            }

Do we need all of this?

Either protmode_load_seg() fails and there's nothing to unwind, or
write_msr() fails and we only have to unwind GS.

I think?


This is actually a good example of where pipeline microcode has a much
easier time than we do.  Inside the pipeline, there's no such thing as
"can't store to gs & GS_KERN once the checks are done".

Although it does make me wonder.  Would LKGS trigger the MSR
intercepts?  Architecturally, it writes MSR_GS_KERN, so ought to trigger
the Write intercept.

However, version 7 of the FRED spec says:

"Because the base address in the descriptor is only 32 bits, LKGS clears
the upper 32 bits of the 64-bit IA32_KERNEL_GS_BASE MSR."

so I suspect it does not architecturally read MSR_GS_KERN, so would not
trigger the Read intercept (or introspection for that matter.)

AFAICT, we're only performing the read in order to do the best-effort
unwind, so I think that path needs dropping.



>              break;
>          }
>          break;
> --- a/xen/include/public/arch-x86/cpufeatureset.h
> +++ b/xen/include/public/arch-x86/cpufeatureset.h
> @@ -296,6 +296,8 @@ XEN_CPUFEATURE(AVX512_BF16,  10*32+ 5) /
>  XEN_CPUFEATURE(FZRM,         10*32+10) /*A  Fast Zero-length REP MOVSB */
>  XEN_CPUFEATURE(FSRS,         10*32+11) /*A  Fast Short REP STOSB */
>  XEN_CPUFEATURE(FSRCS,        10*32+12) /*A  Fast Short REP CMPSB/SCASB */
> +XEN_CPUFEATURE(FRED,         10*32+17) /*   Flexible Return and Event 
> Delivery */
> +XEN_CPUFEATURE(LKGS,         10*32+18) /*S  Load Kernel GS Base */

Can we please keep this 's' until we've had a play on real hardware?

Also, as we're going for CPUID bits more generally these days, bit 20 is
NMI_SRC also from the FRED spec.

>  XEN_CPUFEATURE(WRMSRNS,      10*32+19) /*S  WRMSR Non-Serialising */
>  XEN_CPUFEATURE(AVX_IFMA,     10*32+23) /*A  AVX-IFMA Instructions */
>  
> --- a/xen/tools/gen-cpuid.py
> +++ b/xen/tools/gen-cpuid.py
> @@ -274,7 +274,7 @@ def crunch_numbers(state):
>          # superpages, PCID and PKU are only available in 4 level paging.
>          # NO_LMSL indicates the absense of Long Mode Segment Limits, which
>          # have been dropped in hardware.
> -        LM: [CX16, PCID, LAHF_LM, PAGE1GB, PKU, NO_LMSL],
> +        LM: [CX16, PCID, LAHF_LM, PAGE1GB, PKU, NO_LMSL, LKGS],
>  
>          # AMD K6-2+ and K6-III processors shipped with 3DNow+, beyond the
>          # standard 3DNow in the earlier K6 processors.
> @@ -338,6 +338,9 @@ def crunch_numbers(state):
>  
>          # The behaviour described by RRSBA depend on eIBRS being active.
>          EIBRS: [RRSBA],
> +
> +        # FRED builds on the LKGS instruction.
> +        LKGS: [FRED],

I'd be tempted to justify this differently.

It is intentional that LKGS is usable with CR4.FRED=0, for the benefit
of FRED-aware-but-not-active OSes running on FRED-capable hardware.

However, FRED=1 systems cannot operate without LKGS.

So, perhaps "There is no hard dependency, but the spec requires that
LKGS is available in FRED systems" ?

~Andrew