Page MenuHomeFreeBSD
Differential D22374 Diff 64393 sys/x86/x86/cpu_machdep.c

Changeset View

Standalone View

View Options

sys/x86/x86/cpu_machdep.c

Show First 20 Lines • Show All 1,129 Lines • ▼ Show 20 Lines
} }
SYSCTL_PROC(_hw, OID_AUTO, mds_disable, CTLTYPE_INT | SYSCTL_PROC(_hw, OID_AUTO, mds_disable, CTLTYPE_INT |
CTLFLAG_RWTUN | CTLFLAG_NOFETCH | CTLFLAG_MPSAFE, NULL, 0, CTLFLAG_RWTUN | CTLFLAG_NOFETCH | CTLFLAG_MPSAFE, NULL, 0,
sysctl_mds_disable_handler, "I", sysctl_mds_disable_handler, "I",
"Microarchitectural Data Sampling Mitigation " "Microarchitectural Data Sampling Mitigation "
"(0 - off, 1 - on VERW, 2 - on SW, 3 - on AUTO"); "(0 - off, 1 - on VERW, 2 - on SW, 3 - on AUTO");
int hw_tsx_disable;
int hw_tsx_state;
enum {
TSX_TAA_NONE = 0,
TSX_TAA_DISABLE = 1,
TSX_TAA_VERW = 2,
TSX_TAA_AUTO = 3
};
kibUnsubmitted
Done
Inline Actions

yes, please use the cpu_stdext_feature. This var can be overwritten by user in emergency as well.

kib: yes, please use the cpu_stdext_feature. This var can be overwritten by user in emergency as…
static void
hw_tsx_set_one(bool enable)
{
uint64_t v;
v = rdmsr(MSR_IA32_TSX_CTRL);
if (enable)
v |= (uint64_t)(IA32_TSX_CTRL_RTM_DISABLE |
IA32_TSX_CTRL_TSX_CPUID_CLEAR);
else
v &= ~(uint64_t)(IA32_TSX_CTRL_RTM_DISABLE |
IA32_TSX_CTRL_TSX_CPUID_CLEAR);
wrmsr(MSR_IA32_TSX_CTRL, v);
kibUnsubmitted
Not Done
Inline Actions

I think you need to check the existing value of hw_mds_disable. If it is non-zero, do not touch it. If it is zero, setting it to 1 (VERW) might not work simply because the right microcode is not loaded. 3 (auto) might be more reasonable value.

kib: I think you need to check the existing value of hw_mds_disable. If it is non-zero, do not…
scottlAuthorUnsubmitted
Done
Inline Actions

My intent was to assume that hw_mds_recalculate() would determine the correct state. If this code asks for VERW but that can't be done, then it'll just fail anyways.

scottl: My intent was to assume that `hw_mds_recalculate()` would determine the correct state. If this…
}
static void
hw_tsx_set(bool enable, bool all)
{
struct thread *td;
int bound_cpu, i, is_bound;
if (all) {
td = curthread;
thread_lock(td);
is_bound = sched_is_bound(td);
bound_cpu = td->td_oncpu;
CPU_FOREACH(i) {
sched_bind(td, i);
hw_tsx_set_one(enable);
}
if (is_bound)
sched_bind(td, bound_cpu);
else
sched_unbind(td);
thread_unlock(td);
} else
hw_tsx_set_one(enable);
}
void
hw_tsx_recalculate(void)
{
static int tsx_saved_mds_disable = 0;
int tsx_need = 0, tsx_state = 0;
int mds_disable = 0, need_mds_recalc = 0;
/* Check CPUID.07h.EBX.HLE and RTM for the presence of TSX */
if ((cpu_stdext_feature & CPUID_STDEXT_HLE) == 0 ||
(cpu_stdext_feature & CPUID_STDEXT_RTM) == 0) {
/* TSX is not present */
hw_tsx_state = 0;
return;
}
/* Check to see what mitigation options the CPU gives us */
if (cpu_ia32_arch_caps & IA32_ARCH_CAP_TAA_NO)
tsx_need = TSX_TAA_NONE;
emasteUnsubmitted
Done
Inline Actions

maybe /* CPU is not susceptible to TAA */

emaste: maybe `/* CPU is not susceptible to TAA */`
else if (cpu_ia32_arch_caps & IA32_ARCH_CAP_TSX_CTRL)
tsx_need = TSX_TAA_DISABLE;
emasteUnsubmitted
Done
Inline Actions

/* CPU is susceptible to TAA and has the ability to disable TSX */

emaste: `/* CPU is susceptible to TAA and has the ability to disable TSX */`
scottlAuthorUnsubmitted
Done
Inline Actions

What you've described are two states that can be mutually exclusive.

scottl: What you've described are two states that can be mutually exclusive.
emasteUnsubmitted
Done
Inline Actions

But this case here (where we set tsx_need = TSX_TAA_DISABLE) is exactly that - the CPU is susceptible to TAA, and can turn off TSX.

emaste: But this case here (where we set tsx_need = TSX_TAA_DISABLE) is exactly that - the CPU is…
else {
/* No TSX specific remedies are available. */
if (hw_tsx_disable == TSX_TAA_DISABLE) {
/* The user asked for the disable option, but
* it's not available. */
if (bootverbose)
printf("TSX control not available\n");
return;
} else
tsx_need = TSX_TAA_VERW;
}
/* Can we automatically take action, or are we being forced? */
if (hw_tsx_disable == TSX_TAA_AUTO)
tsx_state = tsx_need;
else
tsx_state = hw_tsx_disable;
/* No state change, nothing to do */
if (tsx_state == hw_tsx_state) {
if (bootverbose)
printf("No TSX change made\n");
return;
}
/* Does the MSR need to be turned on or off? */
if (tsx_state == TSX_TAA_DISABLE)
hw_tsx_set(1 /* enable */, 1 /* all */);
kibUnsubmitted
Done
Inline Actions

Why use ints and implicit convertions ? We have true/false constant symbols in C99.

kib: Why use ints and implicit convertions ? We have true/false constant symbols in C99.
else if (hw_tsx_state == TSX_TAA_DISABLE)
hw_tsx_set(0 /* disable */, 1 /* all */);
/* Does MDS need to be set to turn on VERW? */
if (tsx_state == TSX_TAA_VERW) {
tsx_saved_mds_disable = hw_mds_disable;
mds_disable = hw_mds_disable = 1;
need_mds_recalc = 1;
} else if (hw_tsx_state == TSX_TAA_VERW) {
mds_disable = hw_mds_disable = tsx_saved_mds_disable;
kibUnsubmitted
Not Done
Inline Actions

hw_mds_disable is user-controlled, and user settings should be honored IMO.

This is one of the reason why I proposed to merge taa and mds recalculation functions.

kib: hw_mds_disable is user-controlled, and user settings should be honored IMO. This is one of the…
scottlAuthorUnsubmitted
Done
Inline Actions

How would it work if a user specified VERW for TAA and SW for MDS? Maybe these can be merged in the future, but for now I would like to keep them separate, and enforce that TAA is a superset of MDS, as stated in the review summary.

scottl: How would it work if a user specified VERW for TAA and SW for MDS? Maybe these can be merged…
kibUnsubmitted
Not Done
Inline Actions

In this case you need somehow merge them, and this is easier to do in the merged function.

But I think that you should either not allow that fine control over taa (e.g. only provide disabled/mds/tsx variants of mitigation).

Or even better, further along lines of merged mds/taa handling, do not introduce separate knob to taa. Consider taa action as continuation of mds. For instance, auto would mean: if both MDS_NO and TAA_NO, do nothing. If MDS requires VERW, _or_ TAA requires VERW, it is VERW. If VERW is not available, it is microarch-specific code sequences.

kib: In this case you need somehow merge them, and this is easier to do in the merged function. But…
scottlAuthorUnsubmitted
Done
Inline Actions

I agree that this maybe could be better, but trying to handle MDS_NO, TAA_NO, TSX_CTRL feature bits (not to mention the MDS and RTM/HLE presence bits) as well as VERW vs software mitigation, is a very complex logic table. I'd like to keep it more simple for now, and consider combining in the future.

scottl: I agree that this maybe could be better, but trying to handle MDS_NO, TAA_NO, TSX_CTRL feature…
need_mds_recalc = 1;
}
if (need_mds_recalc) {
hw_mds_recalculate();
if (mds_disable != hw_mds_disable) {
if (bootverbose)
printf("Cannot change MDS state for TSX\n");
/* Don't update our state */
return;
}
}
hw_tsx_state = tsx_state;
return;
}
static void
hw_tsx_recalculate_boot(void * arg __unused)
{
hw_tsx_recalculate();
}
SYSINIT(tsx_recalc, SI_SUB_SMP, SI_ORDER_ANY, hw_tsx_recalculate_boot, NULL);
static int
sysctl_tsx_disable_handler(SYSCTL_HANDLER_ARGS)
{
int error, val;
val = hw_tsx_disable;
error = sysctl_handle_int(oidp, &val, 0, req);
if (error != 0 || req->newptr == NULL)
return (error);
if (val < 0 || val > 3)
return (EINVAL);
hw_tsx_disable = val;
hw_tsx_recalculate();
return (0);
}
SYSCTL_PROC(_hw, OID_AUTO, tsx_disable, CTLTYPE_INT |
CTLFLAG_RWTUN | CTLFLAG_NOFETCH | CTLFLAG_MPSAFE, NULL, 0,
sysctl_tsx_disable_handler, "I",
"TSX Asynchronous Abort Mitigation "
"(0 - off, 1 - disable TSX, 2 - MDS/VERW, 3 - on AUTO");
static int
sysctl_hw_tsx_disable_state_handler(SYSCTL_HANDLER_ARGS)
{
const char *state;
switch (hw_tsx_state) {
case TSX_TAA_NONE:
state = "inactive";
emasteUnsubmitted
Not Done
Inline Actions

I'm not sure that "inactive" is the right name for this, should probably be "TSX not disabled"?

emaste: I'm not sure that "inactive" is the right name for this, should probably be "TSX not disabled"?
scottlAuthorUnsubmitted
Done
Inline Actions

I think that this is handled by the OID renaming

scottl: I think that this is handled by the OID renaming
break;
case TSX_TAA_DISABLE:
state = "TSX disabled";
break;
case TSX_TAA_VERW:
state = "MDS/VERW";
emasteUnsubmitted
Done
Inline Actions

Just "VERW"?

emaste: Just "VERW"?
break;
default:
state = "unknown";
}
return (SYSCTL_OUT(req, state, strlen(state)));
}
SYSCTL_PROC(_hw, OID_AUTO, tsx_disable_state,
CTLTYPE_STRING | CTLFLAG_RD | CTLFLAG_MPSAFE, NULL, 0,
sysctl_hw_tsx_disable_state_handler, "A",
"Transactional Memory Asynchronous Abort Mitigation state");
/* /*
* Enable and restore kernel text write permissions. * Enable and restore kernel text write permissions.
* Callers must ensure that disable_wp()/restore_wp() are executed * Callers must ensure that disable_wp()/restore_wp() are executed
* without rescheduling on the same core. * without rescheduling on the same core.
*/ */
bool bool
disable_wp(void) disable_wp(void)
{ {
Show All 37 Lines