| Message ID | 20241009181742.1128779-6-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Clean up MSR_IA32_APICBASE_BASE code | expand |
On Wed, 2024-10-09 at 11:17 -0700, Sean Christopherson wrote: > Move kvm_set_apic_base() to lapic.c so that the bulk of KVM's local APIC > code resides in lapic.c, regardless of whether or not KVM is emulating the > local APIC in-kernel. This will also allow making various helpers visible > only to lapic.c. > > No functional change intended. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/lapic.c | 21 +++++++++++++++++++++ > arch/x86/kvm/x86.c | 21 --------------------- > 2 files changed, 21 insertions(+), 21 deletions(-) > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index fe30f465611f..6239cfd89aad 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -2628,6 +2628,27 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value) > } > } > > +int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > +{ > + enum lapic_mode old_mode = kvm_get_apic_mode(vcpu); > + enum lapic_mode new_mode = kvm_apic_mode(msr_info->data); > + u64 reserved_bits = kvm_vcpu_reserved_gpa_bits_raw(vcpu) | 0x2ff | > + (guest_cpuid_has(vcpu, X86_FEATURE_X2APIC) ? 0 : X2APIC_ENABLE); > + > + if ((msr_info->data & reserved_bits) != 0 || new_mode == LAPIC_MODE_INVALID) > + return 1; > + if (!msr_info->host_initiated) { > + if (old_mode == LAPIC_MODE_X2APIC && new_mode == LAPIC_MODE_XAPIC) > + return 1; > + if (old_mode == LAPIC_MODE_DISABLED && new_mode == LAPIC_MODE_X2APIC) > + return 1; > + } > + > + kvm_lapic_set_base(vcpu, msr_info->data); > + kvm_recalculate_apic_map(vcpu->kvm); > + return 0; > +} Nit: It is a little bit weird to use 'struct msr_data *msr_info' as function parameter if kvm_set_apic_base() is in lapic.c. Maybe we can change to take apic_base and host_initialized directly. A side gain is we can get rid of using the 'struct msr_data apic_base_msr' local variable in __set_sregs_common() when calling kvm_apic_set_base(): static int __set_sregs_common(...) { struct msr_data apic_base_msr; ... apic_base_msr.data = sregs->apic_base; apic_base_msr.host_initiated = true; if (kvm_set_apic_base(vcpu, &apic_base_msr)) return -EINVAL; ... }
On Mon, Oct 14, 2024, Kai Huang wrote: > On Wed, 2024-10-09 at 11:17 -0700, Sean Christopherson wrote: > > Move kvm_set_apic_base() to lapic.c so that the bulk of KVM's local APIC > > code resides in lapic.c, regardless of whether or not KVM is emulating the > > local APIC in-kernel. This will also allow making various helpers visible > > only to lapic.c. > > > > No functional change intended. > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > --- > > arch/x86/kvm/lapic.c | 21 +++++++++++++++++++++ > > arch/x86/kvm/x86.c | 21 --------------------- > > 2 files changed, 21 insertions(+), 21 deletions(-) > > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > > index fe30f465611f..6239cfd89aad 100644 > > --- a/arch/x86/kvm/lapic.c > > +++ b/arch/x86/kvm/lapic.c > > @@ -2628,6 +2628,27 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value) > > } > > } > > > > +int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > > +{ > > + enum lapic_mode old_mode = kvm_get_apic_mode(vcpu); > > + enum lapic_mode new_mode = kvm_apic_mode(msr_info->data); > > + u64 reserved_bits = kvm_vcpu_reserved_gpa_bits_raw(vcpu) | 0x2ff | > > + (guest_cpuid_has(vcpu, X86_FEATURE_X2APIC) ? 0 : X2APIC_ENABLE); > > + > > + if ((msr_info->data & reserved_bits) != 0 || new_mode == LAPIC_MODE_INVALID) > > + return 1; > > + if (!msr_info->host_initiated) { > > + if (old_mode == LAPIC_MODE_X2APIC && new_mode == LAPIC_MODE_XAPIC) > > + return 1; > > + if (old_mode == LAPIC_MODE_DISABLED && new_mode == LAPIC_MODE_X2APIC) > > + return 1; > > + } > > + > > + kvm_lapic_set_base(vcpu, msr_info->data); > > + kvm_recalculate_apic_map(vcpu->kvm); > > + return 0; > > +} > > Nit: > > It is a little bit weird to use 'struct msr_data *msr_info' as function > parameter if kvm_set_apic_base() is in lapic.c. Maybe we can change to take > apic_base and host_initialized directly. > > A side gain is we can get rid of using the 'struct msr_data apic_base_msr' local > variable in __set_sregs_common() when calling kvm_apic_set_base(): Ooh, nice. I agree, it'd be better to pass in separate parameters. Gah, and looking at this with fresh eyes reminded me why I even started poking at this code in the first place. Patch 1's changelog does a poor job of calling it out, but the main impetus for this series was to avoid kvm_recalculate_apic_map() when doing KVM_SET_SREGS without modifying APIC_BASE. That's _mostly_ handled by patch 1, but it doesn't completely fix things because if the map is already DIRTY, then KVM will unnecessarily fall into kvm_recalculate_apic_map()'s slow path, even though some other vCPU/task is responsible for refreshing the calculation. I'll send a v2 with your suggested change, a better changelog for patch 1, and another patch at the end to short-circuit kvm_apic_set_base() (not just the inner helper) if the new value is the same as the old value. Thanks Kai! > static int __set_sregs_common(...) > { > struct msr_data apic_base_msr; > ... > > apic_base_msr.data = sregs->apic_base; > apic_base_msr.host_initiated = true; > if (kvm_set_apic_base(vcpu, &apic_base_msr)) > return -EINVAL; > ... > } >
On Mon, Oct 14, 2024, Sean Christopherson wrote: > On Mon, Oct 14, 2024, Kai Huang wrote: > > On Wed, 2024-10-09 at 11:17 -0700, Sean Christopherson wrote: > > > Move kvm_set_apic_base() to lapic.c so that the bulk of KVM's local APIC > > > code resides in lapic.c, regardless of whether or not KVM is emulating the > > > local APIC in-kernel. This will also allow making various helpers visible > > > only to lapic.c. > > > > > > No functional change intended. > > > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > > --- > > > arch/x86/kvm/lapic.c | 21 +++++++++++++++++++++ > > > arch/x86/kvm/x86.c | 21 --------------------- > > > 2 files changed, 21 insertions(+), 21 deletions(-) > > > > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > > > index fe30f465611f..6239cfd89aad 100644 > > > --- a/arch/x86/kvm/lapic.c > > > +++ b/arch/x86/kvm/lapic.c > > > @@ -2628,6 +2628,27 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value) > > > } > > > } > > > > > > +int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > > > +{ > > > + enum lapic_mode old_mode = kvm_get_apic_mode(vcpu); > > > + enum lapic_mode new_mode = kvm_apic_mode(msr_info->data); > > > + u64 reserved_bits = kvm_vcpu_reserved_gpa_bits_raw(vcpu) | 0x2ff | > > > + (guest_cpuid_has(vcpu, X86_FEATURE_X2APIC) ? 0 : X2APIC_ENABLE); > > > + > > > + if ((msr_info->data & reserved_bits) != 0 || new_mode == LAPIC_MODE_INVALID) > > > + return 1; > > > + if (!msr_info->host_initiated) { > > > + if (old_mode == LAPIC_MODE_X2APIC && new_mode == LAPIC_MODE_XAPIC) > > > + return 1; > > > + if (old_mode == LAPIC_MODE_DISABLED && new_mode == LAPIC_MODE_X2APIC) > > > + return 1; > > > + } > > > + > > > + kvm_lapic_set_base(vcpu, msr_info->data); > > > + kvm_recalculate_apic_map(vcpu->kvm); > > > + return 0; > > > +} > > > > Nit: > > > > It is a little bit weird to use 'struct msr_data *msr_info' as function > > parameter if kvm_set_apic_base() is in lapic.c. Maybe we can change to take > > apic_base and host_initialized directly. > > > > A side gain is we can get rid of using the 'struct msr_data apic_base_msr' local > > variable in __set_sregs_common() when calling kvm_apic_set_base(): > > Ooh, nice. I agree, it'd be better to pass in separate parameters. > > Gah, and looking at this with fresh eyes reminded me why I even started poking at > this code in the first place. Patch 1's changelog does a poor job of calling it > out, Duh, because patch 1 doesn't change any of that. KVM already skips setting the map DIRTY if neither MSR_IA32_APICBASE_ENABLE nor X2APIC_ENABLE is toggled. So it's really just the (IIRC, rare) collision with an already-dirty map that's nice to avoid. > but the main impetus for this series was to avoid kvm_recalculate_apic_map() > when doing KVM_SET_SREGS without modifying APIC_BASE. That's _mostly_ handled by > patch 1, but it doesn't completely fix things because if the map is already DIRTY, > then KVM will unnecessarily fall into kvm_recalculate_apic_map()'s slow path, even > though some other vCPU/task is responsible for refreshing the calculation. > > I'll send a v2 with your suggested change, a better changelog for patch 1, and > another patch at the end to short-circuit kvm_apic_set_base() (not just the inner > helper) if the new value is the same as the old value. > > Thanks Kai! > > > static int __set_sregs_common(...) > > { > > struct msr_data apic_base_msr; > > ... > > > > apic_base_msr.data = sregs->apic_base; > > apic_base_msr.host_initiated = true; > > if (kvm_set_apic_base(vcpu, &apic_base_msr)) > > return -EINVAL; > > ... > > } > >
On 15/10/2024 8:40 am, Sean Christopherson wrote: > On Mon, Oct 14, 2024, Sean Christopherson wrote: >> On Mon, Oct 14, 2024, Kai Huang wrote: >>> On Wed, 2024-10-09 at 11:17 -0700, Sean Christopherson wrote: >>>> Move kvm_set_apic_base() to lapic.c so that the bulk of KVM's local APIC >>>> code resides in lapic.c, regardless of whether or not KVM is emulating the >>>> local APIC in-kernel. This will also allow making various helpers visible >>>> only to lapic.c. >>>> >>>> No functional change intended. >>>> >>>> Signed-off-by: Sean Christopherson <seanjc@google.com> >>>> --- >>>> arch/x86/kvm/lapic.c | 21 +++++++++++++++++++++ >>>> arch/x86/kvm/x86.c | 21 --------------------- >>>> 2 files changed, 21 insertions(+), 21 deletions(-) >>>> >>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c >>>> index fe30f465611f..6239cfd89aad 100644 >>>> --- a/arch/x86/kvm/lapic.c >>>> +++ b/arch/x86/kvm/lapic.c >>>> @@ -2628,6 +2628,27 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value) >>>> } >>>> } >>>> >>>> +int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info) >>>> +{ >>>> + enum lapic_mode old_mode = kvm_get_apic_mode(vcpu); >>>> + enum lapic_mode new_mode = kvm_apic_mode(msr_info->data); >>>> + u64 reserved_bits = kvm_vcpu_reserved_gpa_bits_raw(vcpu) | 0x2ff | >>>> + (guest_cpuid_has(vcpu, X86_FEATURE_X2APIC) ? 0 : X2APIC_ENABLE); >>>> + >>>> + if ((msr_info->data & reserved_bits) != 0 || new_mode == LAPIC_MODE_INVALID) >>>> + return 1; >>>> + if (!msr_info->host_initiated) { >>>> + if (old_mode == LAPIC_MODE_X2APIC && new_mode == LAPIC_MODE_XAPIC) >>>> + return 1; >>>> + if (old_mode == LAPIC_MODE_DISABLED && new_mode == LAPIC_MODE_X2APIC) >>>> + return 1; >>>> + } >>>> + >>>> + kvm_lapic_set_base(vcpu, msr_info->data); >>>> + kvm_recalculate_apic_map(vcpu->kvm); >>>> + return 0; >>>> +} >>> >>> Nit: >>> >>> It is a little bit weird to use 'struct msr_data *msr_info' as function >>> parameter if kvm_set_apic_base() is in lapic.c. Maybe we can change to take >>> apic_base and host_initialized directly. >>> >>> A side gain is we can get rid of using the 'struct msr_data apic_base_msr' local >>> variable in __set_sregs_common() when calling kvm_apic_set_base(): >> >> Ooh, nice. I agree, it'd be better to pass in separate parameters. >> >> Gah, and looking at this with fresh eyes reminded me why I even started poking at >> this code in the first place. Patch 1's changelog does a poor job of calling it >> out, > > Duh, because patch 1 doesn't change any of that. KVM already skips setting the > map DIRTY if neither MSR_IA32_APICBASE_ENABLE nor X2APIC_ENABLE is toggled. So > it's really just the (IIRC, rare) collision with an already-dirty map that's nice > to avoid. > I think if the map is already dirty, the other thread that makes the map dirty must be responsible for calling kvm_recalculate_apic_map() after that. For updating 'apic_base' path, IIUC it makes sense anyway to avoid everything when 'apic_base' doesn't change. Calling kvm_recalculate_apic_map() when 'apic_base' is not changed has no harm, but logically I don't think there's need to do that, even the map is already dirty.
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index fe30f465611f..6239cfd89aad 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -2628,6 +2628,27 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value) } } +int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info) +{ + enum lapic_mode old_mode = kvm_get_apic_mode(vcpu); + enum lapic_mode new_mode = kvm_apic_mode(msr_info->data); + u64 reserved_bits = kvm_vcpu_reserved_gpa_bits_raw(vcpu) | 0x2ff | + (guest_cpuid_has(vcpu, X86_FEATURE_X2APIC) ? 0 : X2APIC_ENABLE); + + if ((msr_info->data & reserved_bits) != 0 || new_mode == LAPIC_MODE_INVALID) + return 1; + if (!msr_info->host_initiated) { + if (old_mode == LAPIC_MODE_X2APIC && new_mode == LAPIC_MODE_XAPIC) + return 1; + if (old_mode == LAPIC_MODE_DISABLED && new_mode == LAPIC_MODE_X2APIC) + return 1; + } + + kvm_lapic_set_base(vcpu, msr_info->data); + kvm_recalculate_apic_map(vcpu->kvm); + return 0; +} + void kvm_apic_update_apicv(struct kvm_vcpu *vcpu) { struct kvm_lapic *apic = vcpu->arch.apic; diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index c70ee3b33b93..a2a2a6126e67 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -667,27 +667,6 @@ static void drop_user_return_notifiers(void) kvm_on_user_return(&msrs->urn); } -int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info) -{ - enum lapic_mode old_mode = kvm_get_apic_mode(vcpu); - enum lapic_mode new_mode = kvm_apic_mode(msr_info->data); - u64 reserved_bits = kvm_vcpu_reserved_gpa_bits_raw(vcpu) | 0x2ff | - (guest_cpuid_has(vcpu, X86_FEATURE_X2APIC) ? 0 : X2APIC_ENABLE); - - if ((msr_info->data & reserved_bits) != 0 || new_mode == LAPIC_MODE_INVALID) - return 1; - if (!msr_info->host_initiated) { - if (old_mode == LAPIC_MODE_X2APIC && new_mode == LAPIC_MODE_XAPIC) - return 1; - if (old_mode == LAPIC_MODE_DISABLED && new_mode == LAPIC_MODE_X2APIC) - return 1; - } - - kvm_lapic_set_base(vcpu, msr_info->data); - kvm_recalculate_apic_map(vcpu->kvm); - return 0; -} - /* * Handle a fault on a hardware virtualization (VMX or SVM) instruction. *
Move kvm_set_apic_base() to lapic.c so that the bulk of KVM's local APIC code resides in lapic.c, regardless of whether or not KVM is emulating the local APIC in-kernel. This will also allow making various helpers visible only to lapic.c. No functional change intended. Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/lapic.c | 21 +++++++++++++++++++++ arch/x86/kvm/x86.c | 21 --------------------- 2 files changed, 21 insertions(+), 21 deletions(-)