| Message ID | 20220207051202.577951-2-manali.shukla@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nSVM: Add testing for routing L2 exceptions | expand |
On Mon, Feb 07, 2022, Manali Shukla wrote: > Add following 2 routines : > 1) set_user_mask_all() - set PT_USER_MASK for all the levels of page tables > 2) clear_user_mask_all - clear PT_USER_MASK for all the levels of page tables > > commit 916635a813e975600335c6c47250881b7a328971 > (nSVM: Add test for NPT reserved bit and #NPF error code behavior) > clears PT_USER_MASK for all svm testcases. Any tests that requires > usermode access will fail after this commit. Gah, I took the easy route and it burned us. I would rather we start breaking up the nSVM and nVMX monoliths, e.g. add a separate NPT test and clear the USER flag only in that test, not the "common" nSVM test. If we don't do that, this should really use walk_pte() instead of adding yet another PTE walker.
On 2/15/2022 1:00 AM, Sean Christopherson wrote: > On Mon, Feb 07, 2022, Manali Shukla wrote: >> Add following 2 routines : >> 1) set_user_mask_all() - set PT_USER_MASK for all the levels of page tables >> 2) clear_user_mask_all - clear PT_USER_MASK for all the levels of page tables >> >> commit 916635a813e975600335c6c47250881b7a328971 >> (nSVM: Add test for NPT reserved bit and #NPF error code behavior) >> clears PT_USER_MASK for all svm testcases. Any tests that requires >> usermode access will fail after this commit. > > Gah, I took the easy route and it burned us. I would rather we start breaking up > the nSVM and nVMX monoliths, e.g. add a separate NPT test and clear the USER flag > only in that test, not the "common" nSVM test. Yeah. I agree. I will try to set/clear User flag in svm_npt_rsvd_bits_test() and set User flag by default for all the test cases by calling setup_vm() and use walk_pte() to set/clear User flag in svm_npt_rsvd_bits_test(). Walk_pte() routine uses start address and length to walk over the memory region where flag needs to be set/clear. So start address and length need to be figured out to use this routine. I will work on this. > > If we don't do that, this should really use walk_pte() instead of adding yet > another PTE walker. -Manali
On Thu, Feb 17, 2022 at 3:55 AM Shukla, Manali <mashukla@amd.com> wrote: > > > > On 2/15/2022 1:00 AM, Sean Christopherson wrote: > > On Mon, Feb 07, 2022, Manali Shukla wrote: > >> Add following 2 routines : > >> 1) set_user_mask_all() - set PT_USER_MASK for all the levels of page tables > >> 2) clear_user_mask_all - clear PT_USER_MASK for all the levels of page tables > >> > >> commit 916635a813e975600335c6c47250881b7a328971 > >> (nSVM: Add test for NPT reserved bit and #NPF error code behavior) > >> clears PT_USER_MASK for all svm testcases. Any tests that requires > >> usermode access will fail after this commit. > > > > Gah, I took the easy route and it burned us. I would rather we start breaking up > > the nSVM and nVMX monoliths, e.g. add a separate NPT test and clear the USER flag > > only in that test, not the "common" nSVM test. > > Yeah. I agree. I will try to set/clear User flag in svm_npt_rsvd_bits_test() and > set User flag by default for all the test cases by calling setup_vm() > and use walk_pte() to set/clear User flag in svm_npt_rsvd_bits_test(). > > Walk_pte() routine uses start address and length to walk over the memory > region where flag needs to be set/clear. So start address and length need to be > figured out to use this routine. For the start address and length you can use 'stext' and 'etext' to compute those. There's an example in access.c set_cr4_smep(), however, it uses walk_ptes() as opposed to walk_pte() (similar, but different). > > I will work on this. > > > > > If we don't do that, this should really use walk_pte() instead of adding yet > > another PTE walker. > > -Manali
On Thu, Feb 17, 2022, Shukla, Manali wrote: > > On 2/15/2022 1:00 AM, Sean Christopherson wrote: > > On Mon, Feb 07, 2022, Manali Shukla wrote: > >> Add following 2 routines : > >> 1) set_user_mask_all() - set PT_USER_MASK for all the levels of page tables > >> 2) clear_user_mask_all - clear PT_USER_MASK for all the levels of page tables > >> > >> commit 916635a813e975600335c6c47250881b7a328971 > >> (nSVM: Add test for NPT reserved bit and #NPF error code behavior) > >> clears PT_USER_MASK for all svm testcases. Any tests that requires > >> usermode access will fail after this commit. > > > > Gah, I took the easy route and it burned us. I would rather we start breaking up > > the nSVM and nVMX monoliths, e.g. add a separate NPT test and clear the USER flag > > only in that test, not the "common" nSVM test. > > Yeah. I agree. I will try to set/clear User flag in svm_npt_rsvd_bits_test() and > set User flag by default for all the test cases by calling setup_vm() > and use walk_pte() to set/clear User flag in svm_npt_rsvd_bits_test(). I was thinking of something more drastic. The only reason the nSVM tests are "incompatible" with usermode is this snippet in main(): int main(int ac, char **av) { /* Omit PT_USER_MASK to allow tested host.CR4.SMEP=1. */ pteval_t opt_mask = 0; int i = 0; ac--; av++; __setup_vm(&opt_mask); ... } Change that to setup_vm() and KUT will build the test with PT_USER_MASK set on all PTEs. My thought (might be a bad one) is to move the nNPT tests to their own file/test so that the tests don't need to fiddle with page tables midway through. The quick and dirty approach would be to turn the current main() into a small helper, minus its call to __setup_vm(). Longer term, I think it'd make sense to add svm/ + vmx/ subdirectories, and turn much of the common code into proper libraries, e.g. test_wanted() can and should be common helper, probably with more glue code to allow declaring a set of subtests. But for now I think we can just add svm_npt.c or whatever.
On 2/17/2022 8:04 PM, Aaron Lewis wrote: > On Thu, Feb 17, 2022 at 3:55 AM Shukla, Manali <mashukla@amd.com> wrote: >> >> >> >> On 2/15/2022 1:00 AM, Sean Christopherson wrote: >>> On Mon, Feb 07, 2022, Manali Shukla wrote: >>>> Add following 2 routines : >>>> 1) set_user_mask_all() - set PT_USER_MASK for all the levels of page tables >>>> 2) clear_user_mask_all - clear PT_USER_MASK for all the levels of page tables >>>> >>>> commit 916635a813e975600335c6c47250881b7a328971 >>>> (nSVM: Add test for NPT reserved bit and #NPF error code behavior) >>>> clears PT_USER_MASK for all svm testcases. Any tests that requires >>>> usermode access will fail after this commit. >>> >>> Gah, I took the easy route and it burned us. I would rather we start breaking up >>> the nSVM and nVMX monoliths, e.g. add a separate NPT test and clear the USER flag >>> only in that test, not the "common" nSVM test. >> >> Yeah. I agree. I will try to set/clear User flag in svm_npt_rsvd_bits_test() and >> set User flag by default for all the test cases by calling setup_vm() >> and use walk_pte() to set/clear User flag in svm_npt_rsvd_bits_test(). >> >> Walk_pte() routine uses start address and length to walk over the memory >> region where flag needs to be set/clear. So start address and length need to be >> figured out to use this routine. > > For the start address and length you can use 'stext' and 'etext' to > compute those. There's an example in access.c set_cr4_smep(), > however, it uses walk_ptes() as opposed to walk_pte() (similar, but > different). > Yeah PT_USER_MASK is only set for text segment in set_cr4_smep(). In #AC exception test, PT_USER_MASK needs to be set for user stack pages. So, setting PT_USER_MASK for length('etext' - 'stext') of memory will not resolve the problem. Please let me know if I am missing something. >> >> I will work on this. >> >>> >>> If we don't do that, this should really use walk_pte() instead of adding yet >>> another PTE walker. >> >> -Manali
On 2/17/2022 8:50 PM, Sean Christopherson wrote: > On Thu, Feb 17, 2022, Shukla, Manali wrote: >> >> On 2/15/2022 1:00 AM, Sean Christopherson wrote: >>> On Mon, Feb 07, 2022, Manali Shukla wrote: >>>> Add following 2 routines : >>>> 1) set_user_mask_all() - set PT_USER_MASK for all the levels of page tables >>>> 2) clear_user_mask_all - clear PT_USER_MASK for all the levels of page tables >>>> >>>> commit 916635a813e975600335c6c47250881b7a328971 >>>> (nSVM: Add test for NPT reserved bit and #NPF error code behavior) >>>> clears PT_USER_MASK for all svm testcases. Any tests that requires >>>> usermode access will fail after this commit. >>> >>> Gah, I took the easy route and it burned us. I would rather we start breaking up >>> the nSVM and nVMX monoliths, e.g. add a separate NPT test and clear the USER flag >>> only in that test, not the "common" nSVM test. >> >> Yeah. I agree. I will try to set/clear User flag in svm_npt_rsvd_bits_test() and >> set User flag by default for all the test cases by calling setup_vm() >> and use walk_pte() to set/clear User flag in svm_npt_rsvd_bits_test(). > > I was thinking of something more drastic. The only reason the nSVM tests are > "incompatible" with usermode is this snippet in main(): > > int main(int ac, char **av) > { > /* Omit PT_USER_MASK to allow tested host.CR4.SMEP=1. */ > pteval_t opt_mask = 0; > int i = 0; > > ac--; > av++; > > __setup_vm(&opt_mask); > > ... > } > > Change that to setup_vm() and KUT will build the test with PT_USER_MASK set on > all PTEs. My thought (might be a bad one) is to move the nNPT tests to their own > file/test so that the tests don't need to fiddle with page tables midway through. > > The quick and dirty approach would be to turn the current main() into a small > helper, minus its call to __setup_vm(). Yeah this seems to be a better idea. Based on your suggestions, I am planning to do following. 1) Move svm_npt_rsvd_bits_test() to file svm_npt.c and run it with __setup_vm() 2) There are 7 more npt test cases in svm_tests.c, move them to svm_npt.c Below are the test cases: npt_nx, npt_np, npt_us, npt_rw, npt_rw_pfwalk, npt_l1mmio, npt_rw_l1mmio 3) Change __setup_vm() to setup_vm() in svm_tests.c ( after doing this #AC exception test will work fine without the need of set_user_mask_all and clear_user_mask_all) > > Longer term, I think it'd make sense to add svm/ + vmx/ subdirectories, and turn > much of the common code into proper libraries, e.g. test_wanted() can and should > be common helper, probably with more glue code to allow declaring a set of subtests. > But for now I think we can just add svm_npt.c or whatever. I agree, we can do something similar in future.
diff --git a/lib/x86/vm.c b/lib/x86/vm.c index 56be57b..f3a7ae8 100644 --- a/lib/x86/vm.c +++ b/lib/x86/vm.c @@ -37,6 +37,60 @@ pteval_t *install_pte(pgd_t *cr3, return &pt[offset]; } +/* + * set PT_USER_MASK bit for all levels of page tables + */ + +void set_user_mask_all(pteval_t *pt, int level) +{ + pteval_t pte, *ptep; + int i; + + if (level == PAGE_LEVEL) + pte_opt_mask |= PT_USER_MASK; + + for (i = 0; i < 512; i++) { + ptep = &pt[i]; + pte = *ptep; + + if ((pte & PT_PRESENT_MASK) && !(pte & PT_USER_MASK)) { + *ptep |= PT_USER_MASK; + + if (level == 1 || pte & PT_PAGE_SIZE_MASK) + continue; + + set_user_mask_all(phys_to_virt(pte & 0xffffffffff000ull), level - 1); + } + } +} + + +/* + * clear PT_USER_MASK bit for all levels of page tables + */ + +void clear_user_mask_all(pteval_t *pt, int level) +{ + pteval_t pte, *ptep; + int i; + + for (i = 0; i < 512; i++) { + ptep = &pt[i]; + pte = *ptep; + + if ((pte & PT_PRESENT_MASK) && (pte & PT_USER_MASK)) { + *ptep &= ~PT_USER_MASK; + + if (level == 1 || pte & PT_PAGE_SIZE_MASK) + continue; + + clear_user_mask_all(phys_to_virt(pte & 0xffffffffff000ull), level - 1); + } + } + if (level == PAGE_LEVEL) + pte_opt_mask &= ~PT_USER_MASK; +} + /* * Finds last PTE in the mapping of @virt that's at or above @lowest_level. The * returned PTE isn't necessarily present, but its parent is. diff --git a/lib/x86/vm.h b/lib/x86/vm.h index 4c6dff9..75715e5 100644 --- a/lib/x86/vm.h +++ b/lib/x86/vm.h @@ -38,6 +38,9 @@ pteval_t *install_large_page(pgd_t *cr3, phys_addr_t phys, void *virt); void install_pages(pgd_t *cr3, phys_addr_t phys, size_t len, void *virt); bool any_present_pages(pgd_t *cr3, void *virt, size_t len); +void set_user_mask_all(pteval_t *pt, int level); +void clear_user_mask_all(pteval_t *pt, int level); + static inline void *current_page_table(void) { return phys_to_virt(read_cr3());
Add following 2 routines : 1) set_user_mask_all() - set PT_USER_MASK for all the levels of page tables 2) clear_user_mask_all - clear PT_USER_MASK for all the levels of page tables commit 916635a813e975600335c6c47250881b7a328971 (nSVM: Add test for NPT reserved bit and #NPF error code behavior) clears PT_USER_MASK for all svm testcases. Any tests that requires usermode access will fail after this commit. Usermode function needs to be called from L2 guest to generate AC exception and calling usermode function with supervisor pages generates #PF exception with error code 0004 Add solution to above mentioned problem which is to set PT_USER_MASK for all pages in the initialization of #AC exception test and clear them in uninitialization of #AC exception at last. AC exception test works fine without hampering other test cases with this solution. Signed-off-by: Manali Shukla <manali.shukla@amd.com> --- lib/x86/vm.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++ lib/x86/vm.h | 3 +++ 2 files changed, 57 insertions(+)