Changeset View
Standalone View
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; | |||||
| void | |||||
| hw_tsx_recalculate(void) | |||||
| { | |||||
| uint32_t regs[4]; | |||||
| /* Check CPUID.07h.EBX.HLE and RTM for the presence of TSX */ | |||||
| /* XXX should we used the cached cpu_stdext_feature value? */ | |||||
kib: yes, please use the cpu_stdext_feature. This var can be overwritten by user in emergency as… | |||||
| do_cpuid(0x07, regs); | |||||
| if ((regs[2] & CPUID_STDEXT_HLE) == 0 || | |||||
| (regs[2] & CPUID_STDEXT_RTM) == 0) { | |||||
| /* TSX is not present */ | |||||
| return; | |||||
| } | |||||
| if ((cpu_ia32_arch_caps & IA32_ARCH_CAP_TAA_NO) && | |||||
| (hw_tsx_disable == 1)) { | |||||
| hw_tsx_disable = 0; | |||||
| } else if (cpu_ia32_arch_caps & IA32_ARCH_CAP_MDS_NO) { | |||||
| if (cpu_ia32_arch_caps & IA32_ARCH_CAP_TSX_CTRL) | |||||
| hw_tsx_disable = 1; | |||||
| else { | |||||
| hw_mds_disable = 1; | |||||
kibUnsubmitted Not Done Inline ActionsI 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 ActionsMy 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… | |||||
| hw_mds_recalculate(); | |||||
| 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"); | |||||
| /* | /* | ||||
| * 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) | ||||
| { | { | ||||
Done Inline Actionsmaybe /* CPU is not susceptible to TAA */ emaste: maybe `/* CPU is not susceptible to TAA */` | |||||
| u_int cr0; | u_int cr0; | ||||
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 */` | |||||
Done Inline ActionsWhat you've described are two states that can be mutually exclusive. scottl: What you've described are two states that can be mutually exclusive. | |||||
Done Inline ActionsBut 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… | |||||
| cr0 = rcr0(); | cr0 = rcr0(); | ||||
| if ((cr0 & CR0_WP) == 0) | if ((cr0 & CR0_WP) == 0) | ||||
| return (false); | return (false); | ||||
| load_cr0(cr0 & ~CR0_WP); | load_cr0(cr0 & ~CR0_WP); | ||||
| return (true); | return (true); | ||||
| } | } | ||||
| void | void | ||||
| Show All 11 Lines | #ifdef DEV_ACPI | ||||
| ACPI_TABLE_FADT *fadt; | ACPI_TABLE_FADT *fadt; | ||||
| vm_paddr_t physaddr; | vm_paddr_t physaddr; | ||||
| physaddr = acpi_find_table(ACPI_SIG_FADT); | physaddr = acpi_find_table(ACPI_SIG_FADT); | ||||
| if (physaddr == 0) | if (physaddr == 0) | ||||
| return (false); | return (false); | ||||
| fadt = acpi_map_table(physaddr, ACPI_SIG_FADT); | fadt = acpi_map_table(physaddr, ACPI_SIG_FADT); | ||||
| if (fadt == NULL) | if (fadt == NULL) | ||||
| return (false); | return (false); | ||||
Done Inline ActionsWhy 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. | |||||
| *flagsp = fadt->BootFlags; | *flagsp = fadt->BootFlags; | ||||
| acpi_unmap_table(fadt); | acpi_unmap_table(fadt); | ||||
| return (true); | return (true); | ||||
| #else | #else | ||||
| return (false); | return (false); | ||||
| #endif | #endif | ||||
| } | } | ||||
Not Done Inline ActionsI'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"? | |||||
Done Inline ActionsI think that this is handled by the OID renaming scottl: I think that this is handled by the OID renaming | |||||
Not Done Inline Actionshw_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… | |||||
Done Inline ActionsHow 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… | |||||
Not Done Inline ActionsIn 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… | |||||
Done Inline ActionsI 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… | |||||
Done Inline ActionsJust "VERW"? emaste: Just "VERW"? | |||||
yes, please use the cpu_stdext_feature. This var can be overwritten by user in emergency as well.