Provide support for this insn, which is a prereq to FRED. CPUID-wise
introduce both its and FRED's bit at this occasion, thus allowing to
also express the dependency right away.
While adding a testcase, also add a SWAPGS one. In order to not affect
the behavior of pre-existing tests, install write_{segment,msr} hooks
only transiently.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
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.
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).
---
v5: Re-base.
v3: Add dependency on LM. Re-base.
v2: Use X86_EXC_*. Add comments.
--- a/tools/tests/x86_emulator/predicates.c
+++ b/tools/tests/x86_emulator/predicates.c
@@ -326,6 +326,7 @@ static const struct {
{ { 0x00, 0x18 }, { 2, 2 }, T, R }, /* ltr */
{ { 0x00, 0x20 }, { 2, 2 }, T, R }, /* verr */
{ { 0x00, 0x28 }, { 2, 2 }, T, R }, /* verw */
+ { { 0x00, 0x30 }, { 0, 2 }, T, R, pfx_f2 }, /* lkgs */
{ { 0x01, 0x00 }, { 2, 2 }, F, W }, /* sgdt */
{ { 0x01, 0x08 }, { 2, 2 }, F, W }, /* sidt */
{ { 0x01, 0x10 }, { 2, 2 }, F, R }, /* lgdt */
--- a/tools/tests/x86_emulator/test_x86_emulator.c
+++ b/tools/tests/x86_emulator/test_x86_emulator.c
@@ -670,6 +670,10 @@ static int blk(
return x86_emul_blk((void *)offset, p_data, bytes, eflags, state, ctxt);
}
+#ifdef __x86_64__
+static unsigned long gs_base, gs_base_shadow;
+#endif
+
static int read_segment(
enum x86_segment seg,
struct segment_register *reg,
@@ -679,8 +683,30 @@ static int read_segment(
return X86EMUL_UNHANDLEABLE;
memset(reg, 0, sizeof(*reg));
reg->p = 1;
+
+#ifdef __x86_64__
+ if ( seg == x86_seg_gs )
+ reg->base = gs_base;
+#endif
+
+ return X86EMUL_OKAY;
+}
+
+#ifdef __x86_64__
+static int write_segment(
+ enum x86_segment seg,
+ const struct segment_register *reg,
+ struct x86_emulate_ctxt *ctxt)
+{
+ if ( !is_x86_user_segment(seg) )
+ return X86EMUL_UNHANDLEABLE;
+
+ if ( seg == x86_seg_gs )
+ gs_base = reg->base;
+
return X86EMUL_OKAY;
}
+#endif
static int read_msr(
unsigned int reg,
@@ -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 */
+ if ( ctxt->addr_size < 64 )
+ break;
+ *val = gs_base;
+ return X86EMUL_OKAY;
+
+ case 0xc0000102: /* SHADOW_GS_BASE */
+ if ( ctxt->addr_size < 64 )
+ break;
+ *val = gs_base_shadow;
+ return X86EMUL_OKAY;
+#endif
+
case 0xc0000103: /* TSC_AUX */
#define TSC_AUX_VALUE 0xCACACACA
*val = TSC_AUX_VALUE;
@@ -702,6 +742,31 @@ static int read_msr(
return X86EMUL_UNHANDLEABLE;
}
+#ifdef __x86_64__
+static int write_msr(
+ unsigned int reg,
+ uint64_t val,
+ struct x86_emulate_ctxt *ctxt)
+{
+ switch ( reg )
+ {
+ case 0xc0000101: /* GS_BASE */
+ if ( ctxt->addr_size < 64 || !is_canonical_address(val) )
+ break;
+ gs_base = val;
+ return X86EMUL_OKAY;
+
+ case 0xc0000102: /* SHADOW_GS_BASE */
+ if ( ctxt->addr_size < 64 || !is_canonical_address(val) )
+ break;
+ gs_base_shadow = val;
+ return X86EMUL_OKAY;
+ }
+
+ return X86EMUL_UNHANDLEABLE;
+}
+#endif
+
#define INVPCID_ADDR 0x12345678
#define INVPCID_PCID 0x123
@@ -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;
+ emulops.write_segment = NULL;
+ emulops.write_msr = NULL;
#endif
printf("okay\n");
--- a/tools/tests/x86_emulator/x86-emulate.c
+++ b/tools/tests/x86_emulator/x86-emulate.c
@@ -85,6 +85,7 @@ bool emul_test_init(void)
cp.feat.invpcid = true;
cp.feat.adx = true;
cp.feat.rdpid = true;
+ cp.feat.lkgs = true;
cp.feat.wrmsrns = true;
cp.extd.clzero = true;
--- 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;
+ }
break;
}
break;
--- a/xen/arch/x86/x86_emulate/private.h
+++ b/xen/arch/x86/x86_emulate/private.h
@@ -592,6 +592,7 @@ amd_like(const struct x86_emulate_ctxt *
#define vcpu_has_sm4() (ctxt->cpuid->feat.sm4)
#define vcpu_has_avx_vnni() (ctxt->cpuid->feat.avx_vnni)
#define vcpu_has_avx512_bf16() (ctxt->cpuid->feat.avx512_bf16)
+#define vcpu_has_lkgs() (ctxt->cpuid->feat.lkgs)
#define vcpu_has_wrmsrns() (ctxt->cpuid->feat.wrmsrns)
#define vcpu_has_avx_ifma() (ctxt->cpuid->feat.avx_ifma)
#define vcpu_has_avx_vnni_int8() (ctxt->cpuid->feat.avx_vnni_int8)
--- 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,
+ 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;
+ }
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 */
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],
}
deep_features = tuple(sorted(deps.keys()))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
On 04.09.2024 16:24, Andrew Cooper wrote:
> 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.
I'm afraid it hasn't become clear to me which of your further comments
are the "broken corners".
>> --- 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 ?
We are doing so already. Just not in this function. And since there
were hex numbers with comments here, I (blindly) added more. I'll submit
a cleanup patch to change the pre-existing ones, and I've already
switched over this and further patches to use the named constants
instead.
>> @@ -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.
That was sitting on top, and I was kind of hoping that I could avoid the
re-basing ahead. But I've meanwhile done so, including the committing of
the result, as you've probably seen.
>> --- 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.
We already handle modrm_reg 0 and 1 differently. I'm not convinced of
making 7 match 6 without need. We can't predict what Intel will put
there - JMPE (which I'm not really concerned about here, and which
the logic being added also doesn't exclude) already didn't match the
reg/mem16 pattern.
>> --- 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?
The function writes the full base, but what we retrieved via
protmode_load_seg() is only 32 bits wide. Hence the parenthesization
in the comment. I can add e.g. "zero-extended" if you think that makes
things more clear?
>> + 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?
Since you say "all" I can only assume you mean both the write_segment()
and the write_msr(). We need the former, as we replaced the segment
base if protmode_load_seg() succeeded. It's only the write_msr() which
is debatable, yet as indicated that matches SWAPGS handling. I'd like
to keep the two as similar as possible.
> 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".
Indeed.
> 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.)
Well, I'm looking at this differently anyway: The MSR is merely an alias
for the segment base. Just like LFS/LGS won't trigger respective MSR
intercepts, LKGS shouldn't either.
> AFAICT, we're only performing the read in order to do the best-effort
> unwind, so I think that path needs dropping.
No, as said - we need to put back the correct base of the "real" GS.
>> --- 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?
Sure.
> Also, as we're going for CPUID bits more generally these days, bit 20 is
> NMI_SRC also from the FRED spec.
I can add that, sure. It just seemed unrelated to me. I wanted to have
FRED to put in place the dependency in gen-cpuid.py. What isn't quite
clear to me is whether there should then also be a dependency recorded
between FRED and NMI_SRC.
>> @@ -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.
This is what I'm meaning to say with the comment. Whereas ...
> So, perhaps "There is no hard dependency, but the spec requires that
> LKGS is available in FRED systems" ?
... this is weaker than what I think is wanted/needed.
Jan
© 2016 - 2024 Red Hat, Inc.