| Message ID | 20211110212001.3745914-11-aaronlewis@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Run access test in an L2 guest | expand |
Hi, This patch caused the regression. Here is the failure. Failure seems to happen only on 5-level paging. Still investigating.. Let me know if you have any idea, #./tests/access BUILD_HEAD=f3e081d7 timeout -k 1s --foreground 180 /usr/local/bin/qemu-system-x86_64 --no-reboot -nodefaults -device pc-testdev -device isa-debug-exit,iobase=0xf4,iosize=0x4 -vnc none -serial stdio -device pci-testdev -machine accel=kvm -kernel /tmp/tmp.w1JL6jhyfN -smp 1 -cpu max # -initrd /tmp/tmp.9coF1FJSwD enabling apic starting test starting 5-level paging test. run ............................FAIL access ============================= Git bisect log. git bisect log git bisect start # bad: [68aa4e32f2b717b991e4dce7dfdde2247366abbc] x86: do not run vmx_pf_exception_test twice git bisect bad 68aa4e32f2b717b991e4dce7dfdde2247366abbc # good: [c90c646d7381c99ac7d9d7812bd8535214458978] access: treat NX as reserved if EFER.NXE=0 git bisect good c90c646d7381c99ac7d9d7812bd8535214458978 # good: [c90c646d7381c99ac7d9d7812bd8535214458978] access: treat NX as reserved if EFER.NXE=0 git bisect good c90c646d7381c99ac7d9d7812bd8535214458978 # good: [c73cc92d8060dd79b71cbd2ded1454a6e924b771] s390x: uv-host: Fence a destroy cpu test on z15 git bisect good c73cc92d8060dd79b71cbd2ded1454a6e924b771 # good: [c73cc92d8060dd79b71cbd2ded1454a6e924b771] s390x: uv-host: Fence a destroy cpu test on z15 git bisect good c73cc92d8060dd79b71cbd2ded1454a6e924b771 # good: [2e88ad238a19253b94e9f410e4c86ed632c134a0] unify field names and definitions for GDT descriptors git bisect good 2e88ad238a19253b94e9f410e4c86ed632c134a0 # good: [91abf0b9aa0bac4ca17df0be63871ca6e1562eac] Merge branch 'gdt-idt-cleanup' into master git bisect good 91abf0b9aa0bac4ca17df0be63871ca6e1562eac # bad: [0f10d9aea13631a414a3023699dd2dfd47dfd02f] x86: Prepare access test for running in L2 git bisect bad 0f10d9aea13631a414a3023699dd2dfd47dfd02f # good: [7a14c1d9468626d4cdd0d883097c7caaa36a91bf] x86: Fix operand size for lldt git bisect good 7a14c1d9468626d4cdd0d883097c7caaa36a91bf # bad: [f3e081d74812ee05be7e744eb8be3f04a2f65c87] x86: Look up the PTEs rather than assuming them git bisect bad f3e081d74812ee05be7e744eb8be3f04a2f65c87 # good: [f7599ce50db691c4281479002f03d611927bed1c] x86: Add a regression test for L1 LDTR persistence bug git bisect good f7599ce50db691c4281479002f03d611927bed1c # first bad commit: [f3e081d74812ee05be7e744eb8be3f04a2f65c87] x86: Look up the PTEs rather than assuming them Thanks Babu On 11/10/21 3:19 PM, Aaron Lewis wrote: > Rather than assuming which PTEs the SMEP test runs on, look them up to > ensure they are correct. If this test were to run on a different page > table (ie: run in an L2 test) the wrong PTEs would be set. Switch to > looking up the PTEs to avoid this from happening. > > Signed-off-by: Aaron Lewis <aaronlewis@google.com> > --- > lib/libcflat.h | 1 + > lib/x86/vm.c | 21 +++++++++++++++++++++ > lib/x86/vm.h | 3 +++ > x86/access.c | 26 ++++++++++++++++++-------- > x86/cstart64.S | 1 - > x86/flat.lds | 1 + > 6 files changed, 44 insertions(+), 9 deletions(-) > > diff --git a/lib/libcflat.h b/lib/libcflat.h > index 9bb7e08..c1fd31f 100644 > --- a/lib/libcflat.h > +++ b/lib/libcflat.h > @@ -35,6 +35,7 @@ > #define __ALIGN_MASK(x, mask) (((x) + (mask)) & ~(mask)) > #define __ALIGN(x, a) __ALIGN_MASK(x, (typeof(x))(a) - 1) > #define ALIGN(x, a) __ALIGN((x), (a)) > +#define ALIGN_DOWN(x, a) __ALIGN((x) - ((a) - 1), (a)) > #define IS_ALIGNED(x, a) (((x) & ((typeof(x))(a) - 1)) == 0) > > #define MIN(a, b) ((a) < (b) ? (a) : (b)) > diff --git a/lib/x86/vm.c b/lib/x86/vm.c > index 5cd2ee4..6a70ef6 100644 > --- a/lib/x86/vm.c > +++ b/lib/x86/vm.c > @@ -281,3 +281,24 @@ void force_4k_page(void *addr) > if (pte & PT_PAGE_SIZE_MASK) > split_large_page(ptep, 2); > } > + > +/* > + * Call the callback on each page from virt to virt + len. > + */ > +void walk_pte(void *virt, size_t len, pte_callback_t callback) > +{ > + pgd_t *cr3 = current_page_table(); > + uintptr_t start = (uintptr_t)virt; > + uintptr_t end = (uintptr_t)virt + len; > + struct pte_search search; > + size_t page_size; > + uintptr_t curr; > + > + for (curr = start; curr < end; curr = ALIGN_DOWN(curr + page_size, page_size)) { > + search = find_pte_level(cr3, (void *)curr, 1); > + assert(found_leaf_pte(search)); > + page_size = 1ul << PGDIR_BITS(search.level); > + > + callback(search, (void *)curr); > + } > +} > diff --git a/lib/x86/vm.h b/lib/x86/vm.h > index d9753c3..4c6dff9 100644 > --- a/lib/x86/vm.h > +++ b/lib/x86/vm.h > @@ -52,4 +52,7 @@ struct vm_vcpu_info { > u64 cr0; > }; > > +typedef void (*pte_callback_t)(struct pte_search search, void *va); > +void walk_pte(void *virt, size_t len, pte_callback_t callback); > + > #endif > diff --git a/x86/access.c b/x86/access.c > index a781a0c..8e3a718 100644 > --- a/x86/access.c > +++ b/x86/access.c > @@ -201,10 +201,24 @@ static void set_cr0_wp(int wp) > } > } > > +static void clear_user_mask(struct pte_search search, void *va) > +{ > + *search.pte &= ~PT_USER_MASK; > +} > + > +static void set_user_mask(struct pte_search search, void *va) > +{ > + *search.pte |= PT_USER_MASK; > + > + /* Flush to avoid spurious #PF */ > + invlpg(va); > +} > + > static unsigned set_cr4_smep(int smep) > { > + extern char stext, etext; > + size_t len = (size_t)&etext - (size_t)&stext; > unsigned long cr4 = shadow_cr4; > - extern u64 ptl2[]; > unsigned r; > > cr4 &= ~CR4_SMEP_MASK; > @@ -214,14 +228,10 @@ static unsigned set_cr4_smep(int smep) > return 0; > > if (smep) > - ptl2[2] &= ~PT_USER_MASK; > + walk_pte(&stext, len, clear_user_mask); > r = write_cr4_checking(cr4); > - if (r || !smep) { > - ptl2[2] |= PT_USER_MASK; > - > - /* Flush to avoid spurious #PF */ > - invlpg((void *)(2 << 21)); > - } > + if (r || !smep) > + walk_pte(&stext, len, set_user_mask); > if (!r) > shadow_cr4 = cr4; > return r; > diff --git a/x86/cstart64.S b/x86/cstart64.S > index ddb83a0..ff79ae7 100644 > --- a/x86/cstart64.S > +++ b/x86/cstart64.S > @@ -17,7 +17,6 @@ stacktop: > .data > > .align 4096 > -.globl ptl2 > ptl2: > i = 0 > .rept 512 * 4 > diff --git a/x86/flat.lds b/x86/flat.lds > index a278b56..337bc44 100644 > --- a/x86/flat.lds > +++ b/x86/flat.lds > @@ -3,6 +3,7 @@ SECTIONS > . = 4M + SIZEOF_HEADERS; > stext = .; > .text : { *(.init) *(.text) *(.text.*) } > + etext = .; > . = ALIGN(4K); > .data : { > *(.data)
On Mon, Nov 29, 2021, Babu Moger wrote: > Hi, > > This patch caused the regression. Here is the failure. Failure seems to > happen only on 5-level paging. Still investigating.. Let me know if you > have any idea, Fix posted, though you may need to take the entire series up to that point for things to actually work. https://lore.kernel.org/all/20211125012857.508243-11-seanjc@google.com/
On 11/29/21 2:43 PM, Sean Christopherson wrote: > On Mon, Nov 29, 2021, Babu Moger wrote: >> Hi, >> >> This patch caused the regression. Here is the failure. Failure seems to >> happen only on 5-level paging. Still investigating.. Let me know if you >> have any idea, > Fix posted, though you may need to take the entire series up to that point for > things to actually work. Sure. Will test and let you know. Thanks
On 11/29/21 2:43 PM, Sean Christopherson wrote: > On Mon, Nov 29, 2021, Babu Moger wrote: >> Hi, >> >> This patch caused the regression. Here is the failure. Failure seems to >> happen only on 5-level paging. Still investigating.. Let me know if you >> have any idea, > Fix posted, though you may need to take the entire series up to that point for > things to actually work. Yes. Verified the fix. Looks good. Thanks
diff --git a/lib/libcflat.h b/lib/libcflat.h index 9bb7e08..c1fd31f 100644 --- a/lib/libcflat.h +++ b/lib/libcflat.h @@ -35,6 +35,7 @@ #define __ALIGN_MASK(x, mask) (((x) + (mask)) & ~(mask)) #define __ALIGN(x, a) __ALIGN_MASK(x, (typeof(x))(a) - 1) #define ALIGN(x, a) __ALIGN((x), (a)) +#define ALIGN_DOWN(x, a) __ALIGN((x) - ((a) - 1), (a)) #define IS_ALIGNED(x, a) (((x) & ((typeof(x))(a) - 1)) == 0) #define MIN(a, b) ((a) < (b) ? (a) : (b)) diff --git a/lib/x86/vm.c b/lib/x86/vm.c index 5cd2ee4..6a70ef6 100644 --- a/lib/x86/vm.c +++ b/lib/x86/vm.c @@ -281,3 +281,24 @@ void force_4k_page(void *addr) if (pte & PT_PAGE_SIZE_MASK) split_large_page(ptep, 2); } + +/* + * Call the callback on each page from virt to virt + len. + */ +void walk_pte(void *virt, size_t len, pte_callback_t callback) +{ + pgd_t *cr3 = current_page_table(); + uintptr_t start = (uintptr_t)virt; + uintptr_t end = (uintptr_t)virt + len; + struct pte_search search; + size_t page_size; + uintptr_t curr; + + for (curr = start; curr < end; curr = ALIGN_DOWN(curr + page_size, page_size)) { + search = find_pte_level(cr3, (void *)curr, 1); + assert(found_leaf_pte(search)); + page_size = 1ul << PGDIR_BITS(search.level); + + callback(search, (void *)curr); + } +} diff --git a/lib/x86/vm.h b/lib/x86/vm.h index d9753c3..4c6dff9 100644 --- a/lib/x86/vm.h +++ b/lib/x86/vm.h @@ -52,4 +52,7 @@ struct vm_vcpu_info { u64 cr0; }; +typedef void (*pte_callback_t)(struct pte_search search, void *va); +void walk_pte(void *virt, size_t len, pte_callback_t callback); + #endif diff --git a/x86/access.c b/x86/access.c index a781a0c..8e3a718 100644 --- a/x86/access.c +++ b/x86/access.c @@ -201,10 +201,24 @@ static void set_cr0_wp(int wp) } } +static void clear_user_mask(struct pte_search search, void *va) +{ + *search.pte &= ~PT_USER_MASK; +} + +static void set_user_mask(struct pte_search search, void *va) +{ + *search.pte |= PT_USER_MASK; + + /* Flush to avoid spurious #PF */ + invlpg(va); +} + static unsigned set_cr4_smep(int smep) { + extern char stext, etext; + size_t len = (size_t)&etext - (size_t)&stext; unsigned long cr4 = shadow_cr4; - extern u64 ptl2[]; unsigned r; cr4 &= ~CR4_SMEP_MASK; @@ -214,14 +228,10 @@ static unsigned set_cr4_smep(int smep) return 0; if (smep) - ptl2[2] &= ~PT_USER_MASK; + walk_pte(&stext, len, clear_user_mask); r = write_cr4_checking(cr4); - if (r || !smep) { - ptl2[2] |= PT_USER_MASK; - - /* Flush to avoid spurious #PF */ - invlpg((void *)(2 << 21)); - } + if (r || !smep) + walk_pte(&stext, len, set_user_mask); if (!r) shadow_cr4 = cr4; return r; diff --git a/x86/cstart64.S b/x86/cstart64.S index ddb83a0..ff79ae7 100644 --- a/x86/cstart64.S +++ b/x86/cstart64.S @@ -17,7 +17,6 @@ stacktop: .data .align 4096 -.globl ptl2 ptl2: i = 0 .rept 512 * 4 diff --git a/x86/flat.lds b/x86/flat.lds index a278b56..337bc44 100644 --- a/x86/flat.lds +++ b/x86/flat.lds @@ -3,6 +3,7 @@ SECTIONS . = 4M + SIZEOF_HEADERS; stext = .; .text : { *(.init) *(.text) *(.text.*) } + etext = .; . = ALIGN(4K); .data : { *(.data)
Rather than assuming which PTEs the SMEP test runs on, look them up to ensure they are correct. If this test were to run on a different page table (ie: run in an L2 test) the wrong PTEs would be set. Switch to looking up the PTEs to avoid this from happening. Signed-off-by: Aaron Lewis <aaronlewis@google.com> --- lib/libcflat.h | 1 + lib/x86/vm.c | 21 +++++++++++++++++++++ lib/x86/vm.h | 3 +++ x86/access.c | 26 ++++++++++++++++++-------- x86/cstart64.S | 1 - x86/flat.lds | 1 + 6 files changed, 44 insertions(+), 9 deletions(-)