On Tue, Apr 4, 2023 at 12:53 AM Dave Hansen <dave.hansen@xxxxxxxxx> wrote: > > On 4/3/23 07:05, Lai Jiangshan wrote: > > Documentation/x86/kernel-stacks.rst | 2 + > > arch/x86/entry/Makefile | 3 + > > arch/x86/entry/entry_64.S | 615 +++++++------------------- > > arch/x86/entry/ist_entry.c | 299 +++++++++++++ > > arch/x86/include/asm/cpu_entry_area.h | 8 +- > > arch/x86/include/asm/idtentry.h | 15 +- > > arch/x86/include/asm/sev.h | 14 - > > arch/x86/include/asm/traps.h | 1 - > > arch/x86/kernel/asm-offsets_64.c | 7 + > > arch/x86/kernel/callthunks.c | 2 + > > arch/x86/kernel/dumpstack_64.c | 6 +- > > arch/x86/kernel/nmi.c | 8 - > > arch/x86/kernel/sev.c | 108 ----- > > arch/x86/kernel/traps.c | 43 -- > > arch/x86/kvm/vmx/vmx.c | 441 +++++++++++++++++- > > arch/x86/kvm/x86.c | 10 +- > > arch/x86/mm/cpu_entry_area.c | 2 +- > > tools/objtool/check.c | 7 +- > > 18 files changed, 937 insertions(+), 654 deletions(-) > > create mode 100644 arch/x86/entry/ist_entry.c > > Some high-level comments... > > The diffstat looks a lot nastier because of the testing patch. If you > that patch and trim the diffstat a bit, it starts to look a _lot_ more > appealing: > > > arch/x86/entry/entry_64.S | 615 ++++++++---------------------------- > > arch/x86/entry/ist_entry.c | 299 +++++++++++++++++ > > arch/x86/kernel/sev.c | 108 ------ > > arch/x86/kernel/traps.c | 43 -- > ... > > 16 files changed, 490 insertions(+), 650 deletions(-) > > It removes more code than it adds and also removes a bunch of assembly. > If it were me posting this, I'd have shouted that from the rooftops > instead of obscuring it with a testing patch and leaving it as an > exercise to the reader to figure out. The cover letter has 800+ lines of comments. About 100-300 lines of comments will be moved into the code which would make the diffstat not so appealing. P.S. One of the reasons I didn't move them is that the comments are much more complicated than the code which is a sign of improvement-required. I'm going to change the narration from save-touch-replicate-copy-commit to save-copy-build-commit and avoid using "replicate". copy=copy_outmost(), build=build_interrupted(), the new narration will change the comments mainly, and it will not change the actual work. If the new narration is not simpler, I will not send it out to add any confusion. > * Flesh out the testing story. What kind of hardware was this tested > on? How much was bare metal vs. VMs, etc...? It is tested on bare metal and VM. It is hard to stress the bare metal on atomic-IST-entry. The testing code tests it in VM and the super exceptions can be injected at will. > * Reinforce what the benefits to end users are here. Are folks > _actually_ running into the #VC fragility, for instance? > > Also, let's say we queue this, it starts getting linux-next testing, and > we start getting bug reports of hangs. It'll have to get reverted if we > can't find the bug fast. > > How much of a pain would it be to make atomic-IST-entry _temporarily_ > selectable at boot time? It would obviously need to keep the old code > around and would not have the nice diffstat. But that way, folks would > at least have a workaround while we're bug hunting. It is easy to make atomic-IST-entry selectable at boot time since IDT is an indirect table. I will do it and temporarily keep the old code.
