| Message ID | 20210713163324.627647-47-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: vCPU RESET/INIT fixes and consolidation | expand |
On Tue, Jul 13, 2021 at 9:35 AM Sean Christopherson <seanjc@google.com> wrote: > > Preserve CR0.CD and CR0.NW on INIT instead of forcing them to '1', as > defined by both Intel's SDM and AMD's APM. > > Note, current versions of Intel's SDM are very poorly written with > respect to INIT behavior. Table 9-1. "IA-32 and Intel 64 Processor > States Following Power-up, Reset, or INIT" quite clearly lists power-up, > RESET, _and_ INIT as setting CR0=60000010H, i.e. CD/NW=1. But the SDM > then attempts to qualify CD/NW behavior in a footnote: > > 2. The CD and NW flags are unchanged, bit 4 is set to 1, all other bits > are cleared. > > Presumably that footnote is only meant for INIT, as the RESET case and > especially the power-up case are rather non-sensical. Another footnote > all but confirms that: > > 6. Internal caches are invalid after power-up and RESET, but left > unchanged with an INIT. > > Bare metal testing shows that CD/NW are indeed preserved on INIT (someone > else can hack their BIOS to check RESET and power-up :-D). > > Reported-by: Reiji Watanabe <reijiw@google.com> > Signed-off-by: Sean Christopherson <seanjc@google.com> Reviewed-by: Reiji Watanabe <reijiw@google.com> Thank you for the fix and checking the CD/NW with the bare metal testing. Regards, Reiji
> On Jul 19, 2021, at 9:37 PM, Reiji Watanabe <reijiw@google.com> wrote: > > On Tue, Jul 13, 2021 at 9:35 AM Sean Christopherson <seanjc@google.com> wrote: >> >> Preserve CR0.CD and CR0.NW on INIT instead of forcing them to '1', as >> defined by both Intel's SDM and AMD's APM. >> >> Note, current versions of Intel's SDM are very poorly written with >> respect to INIT behavior. Table 9-1. "IA-32 and Intel 64 Processor >> States Following Power-up, Reset, or INIT" quite clearly lists power-up, >> RESET, _and_ INIT as setting CR0=60000010H, i.e. CD/NW=1. But the SDM >> then attempts to qualify CD/NW behavior in a footnote: >> >> 2. The CD and NW flags are unchanged, bit 4 is set to 1, all other bits >> are cleared. >> >> Presumably that footnote is only meant for INIT, as the RESET case and >> especially the power-up case are rather non-sensical. Another footnote >> all but confirms that: >> >> 6. Internal caches are invalid after power-up and RESET, but left >> unchanged with an INIT. >> >> Bare metal testing shows that CD/NW are indeed preserved on INIT (someone >> else can hack their BIOS to check RESET and power-up :-D). >> >> Reported-by: Reiji Watanabe <reijiw@google.com> >> Signed-off-by: Sean Christopherson <seanjc@google.com> > > Reviewed-by: Reiji Watanabe <reijiw@google.com> > > Thank you for the fix and checking the CD/NW with the bare metal testing. Interesting. Is there a kvm-unit-test to reproduce the issue by any chance?
On Mon, Jul 26, 2021, Nadav Amit wrote: > > > On Jul 19, 2021, at 9:37 PM, Reiji Watanabe <reijiw@google.com> wrote: > > > > On Tue, Jul 13, 2021 at 9:35 AM Sean Christopherson <seanjc@google.com> wrote: > >> > >> Preserve CR0.CD and CR0.NW on INIT instead of forcing them to '1', as > >> defined by both Intel's SDM and AMD's APM. > >> > >> Note, current versions of Intel's SDM are very poorly written with > >> respect to INIT behavior. Table 9-1. "IA-32 and Intel 64 Processor > >> States Following Power-up, Reset, or INIT" quite clearly lists power-up, > >> RESET, _and_ INIT as setting CR0=60000010H, i.e. CD/NW=1. But the SDM > >> then attempts to qualify CD/NW behavior in a footnote: > >> > >> 2. The CD and NW flags are unchanged, bit 4 is set to 1, all other bits > >> are cleared. > >> > >> Presumably that footnote is only meant for INIT, as the RESET case and > >> especially the power-up case are rather non-sensical. Another footnote > >> all but confirms that: > >> > >> 6. Internal caches are invalid after power-up and RESET, but left > >> unchanged with an INIT. > >> > >> Bare metal testing shows that CD/NW are indeed preserved on INIT (someone > >> else can hack their BIOS to check RESET and power-up :-D). > >> > >> Reported-by: Reiji Watanabe <reijiw@google.com> > >> Signed-off-by: Sean Christopherson <seanjc@google.com> > > > > Reviewed-by: Reiji Watanabe <reijiw@google.com> > > > > Thank you for the fix and checking the CD/NW with the bare metal testing. > > Interesting. > > Is there a kvm-unit-test to reproduce the issue by any chance? No :-/
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index f35dd8192c32..3f0226259496 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -10787,6 +10787,7 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu) void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event) { unsigned long old_cr0 = kvm_read_cr0(vcpu); + unsigned long new_cr0; u32 eax, dummy; kvm_lapic_reset(vcpu, init_event); @@ -10873,7 +10874,18 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event) kvm_set_rflags(vcpu, X86_EFLAGS_FIXED); kvm_rip_write(vcpu, 0xfff0); - static_call(kvm_x86_set_cr0)(vcpu, X86_CR0_NW | X86_CR0_CD | X86_CR0_ET); + /* + * CR0.CD/NW are set on RESET, preserved on INIT. Note, some versions + * of Intel's SDM list CD/NW as being set on INIT, but they contradict + * (or qualify) that with a footnote stating that CD/NW are preserved. + */ + new_cr0 = X86_CR0_ET; + if (init_event) + new_cr0 |= (old_cr0 & (X86_CR0_NW | X86_CR0_CD)); + else + new_cr0 |= X86_CR0_NW | X86_CR0_CD; + + static_call(kvm_x86_set_cr0)(vcpu, new_cr0); static_call(kvm_x86_set_cr4)(vcpu, 0); static_call(kvm_x86_set_efer)(vcpu, 0); static_call(kvm_x86_update_exception_bitmap)(vcpu);
Preserve CR0.CD and CR0.NW on INIT instead of forcing them to '1', as defined by both Intel's SDM and AMD's APM. Note, current versions of Intel's SDM are very poorly written with respect to INIT behavior. Table 9-1. "IA-32 and Intel 64 Processor States Following Power-up, Reset, or INIT" quite clearly lists power-up, RESET, _and_ INIT as setting CR0=60000010H, i.e. CD/NW=1. But the SDM then attempts to qualify CD/NW behavior in a footnote: 2. The CD and NW flags are unchanged, bit 4 is set to 1, all other bits are cleared. Presumably that footnote is only meant for INIT, as the RESET case and especially the power-up case are rather non-sensical. Another footnote all but confirms that: 6. Internal caches are invalid after power-up and RESET, but left unchanged with an INIT. Bare metal testing shows that CD/NW are indeed preserved on INIT (someone else can hack their BIOS to check RESET and power-up :-D). Reported-by: Reiji Watanabe <reijiw@google.com> Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/x86.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)