| Message ID | 20210424004645.3950558-18-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: vCPU RESET/INIT fixes and consolidation | expand |
On Fri, Apr 23, 2021 at 5:51 PM Sean Christopherson <seanjc@google.com> wrote: > > Stuff vcpu->arch.apic_base and apic->base_address directly during APIC > reset, as opposed to bouncing through kvm_set_apic_base() while fudging > the ENABLE bit during creation to avoid the other, unwanted side effects. > > This is a step towards consolidating the APIC RESET logic across x86, > VMX, and SVM. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/lapic.c | 15 ++++++--------- > 1 file changed, 6 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index b088f6984b37..b1366df46d1d 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -2305,7 +2305,6 @@ EXPORT_SYMBOL_GPL(kvm_apic_update_apicv); > void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event) > { > struct kvm_lapic *apic = vcpu->arch.apic; > - u64 msr_val; > int i; > > if (!apic) > @@ -2315,10 +2314,13 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event) > hrtimer_cancel(&apic->lapic_timer.timer); > > if (!init_event) { > - msr_val = APIC_DEFAULT_PHYS_BASE | MSR_IA32_APICBASE_ENABLE; > + vcpu->arch.apic_base = APIC_DEFAULT_PHYS_BASE | > + MSR_IA32_APICBASE_ENABLE; > if (kvm_vcpu_is_reset_bsp(vcpu)) > - msr_val |= MSR_IA32_APICBASE_BSP; > - kvm_lapic_set_base(vcpu, msr_val); > + vcpu->arch.apic_base |= MSR_IA32_APICBASE_BSP; > + > + apic->base_address = MSR_IA32_APICBASE_ENABLE; I think you wanted to make the code above set apic->base_address to APIC_DEFAULT_PHYS_BASE (not MSR_IA32_APICBASE_ENABLE). Thanks, Reiji
On Wed, May 26, 2021, Reiji Watanabe wrote: > On Fri, Apr 23, 2021 at 5:51 PM Sean Christopherson <seanjc@google.com> wrote: > > > > Stuff vcpu->arch.apic_base and apic->base_address directly during APIC > > reset, as opposed to bouncing through kvm_set_apic_base() while fudging > > the ENABLE bit during creation to avoid the other, unwanted side effects. > > > > This is a step towards consolidating the APIC RESET logic across x86, > > VMX, and SVM. > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > --- > > arch/x86/kvm/lapic.c | 15 ++++++--------- > > 1 file changed, 6 insertions(+), 9 deletions(-) > > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > > index b088f6984b37..b1366df46d1d 100644 > > --- a/arch/x86/kvm/lapic.c > > +++ b/arch/x86/kvm/lapic.c > > @@ -2305,7 +2305,6 @@ EXPORT_SYMBOL_GPL(kvm_apic_update_apicv); > > void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event) > > { > > struct kvm_lapic *apic = vcpu->arch.apic; > > - u64 msr_val; > > int i; > > > > if (!apic) > > @@ -2315,10 +2314,13 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event) > > hrtimer_cancel(&apic->lapic_timer.timer); > > > > if (!init_event) { > > - msr_val = APIC_DEFAULT_PHYS_BASE | MSR_IA32_APICBASE_ENABLE; > > + vcpu->arch.apic_base = APIC_DEFAULT_PHYS_BASE | > > + MSR_IA32_APICBASE_ENABLE; > > if (kvm_vcpu_is_reset_bsp(vcpu)) > > - msr_val |= MSR_IA32_APICBASE_BSP; > > - kvm_lapic_set_base(vcpu, msr_val); > > + vcpu->arch.apic_base |= MSR_IA32_APICBASE_BSP; > > + > > + apic->base_address = MSR_IA32_APICBASE_ENABLE; > > I think you wanted to make the code above set apic->base_address > to APIC_DEFAULT_PHYS_BASE (not MSR_IA32_APICBASE_ENABLE). Indeed! It also means I need to double check that I'm testing a guest without x2apic enabled. Thanks much!
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index b088f6984b37..b1366df46d1d 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -2305,7 +2305,6 @@ EXPORT_SYMBOL_GPL(kvm_apic_update_apicv); void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event) { struct kvm_lapic *apic = vcpu->arch.apic; - u64 msr_val; int i; if (!apic) @@ -2315,10 +2314,13 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event) hrtimer_cancel(&apic->lapic_timer.timer); if (!init_event) { - msr_val = APIC_DEFAULT_PHYS_BASE | MSR_IA32_APICBASE_ENABLE; + vcpu->arch.apic_base = APIC_DEFAULT_PHYS_BASE | + MSR_IA32_APICBASE_ENABLE; if (kvm_vcpu_is_reset_bsp(vcpu)) - msr_val |= MSR_IA32_APICBASE_BSP; - kvm_lapic_set_base(vcpu, msr_val); + vcpu->arch.apic_base |= MSR_IA32_APICBASE_BSP; + + apic->base_address = MSR_IA32_APICBASE_ENABLE; + kvm_apic_set_xapic_id(apic, vcpu->vcpu_id); } kvm_apic_set_version(apic->vcpu); @@ -2461,11 +2463,6 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu, int timer_advance_ns) lapic_timer_advance_dynamic = false; } - /* - * APIC is created enabled. This will prevent kvm_lapic_set_base from - * thinking that APIC state has changed. - */ - vcpu->arch.apic_base = MSR_IA32_APICBASE_ENABLE; static_branch_inc(&apic_sw_disabled.key); /* sw disabled at reset */ kvm_iodevice_init(&apic->dev, &apic_mmio_ops);
Stuff vcpu->arch.apic_base and apic->base_address directly during APIC reset, as opposed to bouncing through kvm_set_apic_base() while fudging the ENABLE bit during creation to avoid the other, unwanted side effects. This is a step towards consolidating the APIC RESET logic across x86, VMX, and SVM. Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/lapic.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)