From: Sean Christopherson <seanjc@google.com>
To: Yang Weijiang <weijiang.yang@intel.com>
Cc: pbonzini@redhat.com, kvm@vger.kernel.org
Subject: Re: [kvm-unit-tests PATCH] x86/access: Fixed test stuck issue on new 52bit machine
Date: Mon, 11 Jan 2021 14:25:59 -0800 [thread overview]
Message-ID: <X/zQdznwyBXHoout@google.com> (raw)
In-Reply-To: <20210110091942.12835-1-weijiang.yang@intel.com>
On Sun, Jan 10, 2021, Yang Weijiang wrote:
> When the application is tested on a machine with 52bit-physical-address, the
> synthesized 52bit GPA triggers EPT(4-Level) fast_page_fault infinitely.
That doesn't sound right, KVM should use 5-level EPT if guest maxpa > 48.
Hmm, unless the CPU doesn't support 5-level EPT, but I didn't think such CPUs
(maxpa=52 w/o 5-level EPT) existed? Ah, but it would be possible with nested
VMX, and initial KVM 5-level support didn't allow nested 5-level EPT. Any
chance you're running this test in a VM with 5-level EPT disabled, but maxpa=52?
> On the other hand, there's no reserved bits in 51:max_physical_address on
> machines with 52bit-physical-address.
>
> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> ---
> x86/access.c | 20 +++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/x86/access.c b/x86/access.c
> index 7dc9eb6..bec1c4d 100644
> --- a/x86/access.c
> +++ b/x86/access.c
> @@ -15,6 +15,7 @@ static _Bool verbose = false;
> typedef unsigned long pt_element_t;
> static int invalid_mask;
> static int page_table_levels;
> +static int max_phyaddr;
>
> #define PT_BASE_ADDR_MASK ((pt_element_t)((((pt_element_t)1 << 36) - 1) & PAGE_MASK))
> #define PT_PSE_BASE_ADDR_MASK (PT_BASE_ADDR_MASK & ~(1ull << 21))
> @@ -394,9 +395,10 @@ static void ac_emulate_access(ac_test_t *at, unsigned flags)
> if (!F(AC_PDE_ACCESSED))
> at->ignore_pde = PT_ACCESSED_MASK;
>
> - pde_valid = F(AC_PDE_PRESENT)
> - && !F(AC_PDE_BIT51) && !F(AC_PDE_BIT36) && !F(AC_PDE_BIT13)
> + pde_valid = F(AC_PDE_PRESENT) && !F(AC_PDE_BIT36) && !F(AC_PDE_BIT13)
> && !(F(AC_PDE_NX) && !F(AC_CPU_EFER_NX));
> + if (max_phyaddr < 52)
> + pde_valid &= !F(AC_PDE_BIT51);
>
> if (!pde_valid) {
> at->expected_fault = 1;
> @@ -420,9 +422,10 @@ static void ac_emulate_access(ac_test_t *at, unsigned flags)
>
> at->expected_pde |= PT_ACCESSED_MASK;
>
> - pte_valid = F(AC_PTE_PRESENT)
> - && !F(AC_PTE_BIT51) && !F(AC_PTE_BIT36)
> + pte_valid = F(AC_PTE_PRESENT) && !F(AC_PTE_BIT36)
> && !(F(AC_PTE_NX) && !F(AC_CPU_EFER_NX));
> + if (max_phyaddr < 52)
> + pte_valid &= !F(AC_PTE_BIT51);
This _should_ be unnecessary. As below, AC_*_BIT51_MASK will be set in
invalid_mask, and so ac_test_bump_one() will skip tests that try to set bit 51.
> if (!pte_valid) {
> at->expected_fault = 1;
> @@ -964,13 +967,11 @@ static int ac_test_run(void)
> shadow_cr4 = read_cr4();
> shadow_efer = rdmsr(MSR_EFER);
>
> - if (cpuid_maxphyaddr() >= 52) {
> - invalid_mask |= AC_PDE_BIT51_MASK;
> - invalid_mask |= AC_PTE_BIT51_MASK;
> - }
> - if (cpuid_maxphyaddr() >= 37) {
> + if (max_phyaddr >= 37 && max_phyaddr < 52) {
> invalid_mask |= AC_PDE_BIT36_MASK;
> invalid_mask |= AC_PTE_BIT36_MASK;
> + invalid_mask |= AC_PDE_BIT51_MASK;
> + invalid_mask |= AC_PTE_BIT51_MASK;
> }
This change is incorrect. "invalid_mask" is misleading in this context as it
means "bits that can't be tested because they're legal". So setting the bit 51
flags in invalid_mask if 'maxpa >= 52' is correct, as it states those tests are
"invalid" because setting bit 51 will not fault.
All that being said, it's also entirely possible I'm misreading this test, I've
done it many times before :-)
> if (this_cpu_has(X86_FEATURE_PKU)) {
> @@ -1038,6 +1039,7 @@ int main(void)
> int r;
>
> printf("starting test\n\n");
> + max_phyaddr = cpuid_maxphyaddr();
> page_table_levels = 4;
> r = ac_test_run();
>
> --
> 2.17.2
>
next prev parent reply other threads:[~2021-01-11 22:27 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-10 9:19 [kvm-unit-tests PATCH] x86/access: Fixed test stuck issue on new 52bit machine Yang Weijiang
2021-01-11 22:25 ` Sean Christopherson [this message]
2021-01-12 9:04 ` Yang Weijiang
2021-01-12 17:01 ` Sean Christopherson
2021-01-13 9:07 ` Yang Weijiang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=X/zQdznwyBXHoout@google.com \
--to=seanjc@google.com \
--cc=kvm@vger.kernel.org \
--cc=pbonzini@redhat.com \
--cc=weijiang.yang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).