On Fri, Jan 12, 2018 at 10:33 PM, Willy Tarreau w@1wt.eu wrote:
On Fri, Jan 12, 2018 at 10:08:20PM -0800, Andy Lutomirski wrote:
In fact, it looks like this code is totally bogus and has never been correct at all. Even in:
commit 4b1d5ae3b103eda43f9d0f85c355bb6995b03a30 Author: Peter Zijlstra peterz@infradead.org Date: Mon Dec 4 15:07:59 2017 +0100
x86/mm: Use/Fix PCID to optimize user/kernel switchesWe have:
.macro SWITCH_TO_USER_CR3_NOSTACK scratch_reg:req scratch_reg2:req ALTERNATIVE "jmp .Lend_@", "", X86_FEATURE_PTI mov %cr3, \scratch_reg
ALTERNATIVE "jmp .Lwrcr3_\@", "", X86_FEATURE_PCID...
.Lwrcr3_@: /* Flip the PGD and ASID to the user version */ orq $(PTI_SWITCH_MASK), \scratch_reg mov \scratch_reg, %cr3 .Lend_@:
That's bogus. PTI_SWITCH_MASK is 0x1800, which has PCID = 0x800.
This should probably use an alternative to select between 0x1000 and 0x800 depending on X86_FEATURE_PCID or just use an entirely different label for the !PCID case.
FWIW, this bit in SAVE_AND_SWITCH_TO_KERNEL_CR3
testq $(PTI_SWITCH_MASK), \scratch_reg jz .Ldone_\@is a bit silly, too. It's *correct* (I think), but shouldn't that just be bt $(PTI_SWITCH_PGTABLES_BIT), \scratch_reg, with the obvious caveat that the headers don't actually define PTI_SWITCH_PGTABLES_BIT?
I wondered the same initially when reading this but thought there was surely a good reason that I could not understand due to my lack of knowledge and stopped wondering. BTW your PTI_SWITCH_PGTABLES_BIT would in fact be PAGE_SHIFT :-)
Trying to inventory this stuff scattered all over the place:
#define PTI_PGTABLE_SWITCH_BIT PAGE_SHIFT #define PTI_SWITCH_PGTABLES_MASK (1<<PAGE_SHIFT) # define X86_CR3_PTI_SWITCH_BIT 11 #define PTI_SWITCH_MASK (PTI_SWITCH_PGTABLES_MASK|(1<<X86_CR3_PTI_SWITCH_BIT))
Blech. I wouldn't be terribly surprised if I missed a few as well. How about:
PTI_USER_PGTABLE_BIT = PAGE_SHIFT PTI_USER_PGTABLE_MASK = 1 << PTI_USER_PGTABLE_BIT PTI_USER_PCID_BIT = 11 PTI_USER_PCID_MASK = 1 << PTI_USER_PCID_BIT PTI_USER_PGTABLE_AND_PCID_MASK = PTI_USER_PCID_MASK | PTI_USER_PGTABLE_MASK
This naming would make the apparently buggy code look fishy, as it should. I will give this a shot some time soon if no one beats me to it.