Update and refresh AVIC settings when guest APIC mode is updated
(e.g. changing between disabled, xAPIC, or x2APIC).
Signed-off-by: Suravee Suthikulpanit <[email protected]>
---
arch/x86/kvm/svm/avic.c | 16 ++++++++++++++++
arch/x86/kvm/svm/svm.c | 1 +
2 files changed, 17 insertions(+)
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 3ebeea19b487..d185dd8ddf17 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -691,6 +691,22 @@ void avic_apicv_post_state_restore(struct kvm_vcpu *vcpu)
avic_handle_ldr_update(vcpu);
}
+void avic_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
+{
+ struct vcpu_svm *svm = to_svm(vcpu);
+
+ if (!lapic_in_kernel(vcpu) || (avic_mode == AVIC_MODE_NONE))
+ return;
+
+ if (kvm_get_apic_mode(vcpu) == LAPIC_MODE_INVALID) {
+ WARN_ONCE(true, "Invalid local APIC state (vcpu_id=%d)", vcpu->vcpu_id);
+ return;
+ }
+
+ kvm_vcpu_update_apicv(&svm->vcpu);
+ avic_refresh_apicv_exec_ctrl(&svm->vcpu);
+}
+
static int avic_set_pi_irte_mode(struct kvm_vcpu *vcpu, bool activate)
{
int ret = 0;
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 314628b6bff4..9066568fd19d 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4692,6 +4692,7 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
.enable_nmi_window = svm_enable_nmi_window,
.enable_irq_window = svm_enable_irq_window,
.update_cr8_intercept = svm_update_cr8_intercept,
+ .set_virtual_apic_mode = avic_set_virtual_apic_mode,
.refresh_apicv_exec_ctrl = avic_refresh_apicv_exec_ctrl,
.check_apicv_inhibit_reasons = avic_check_apicv_inhibit_reasons,
.apicv_post_state_restore = avic_apicv_post_state_restore,
--
2.25.1
On Wed, 2022-05-04 at 02:31 -0500, Suravee Suthikulpanit wrote:
> Update and refresh AVIC settings when guest APIC mode is updated
> (e.g. changing between disabled, xAPIC, or x2APIC).
>
> Signed-off-by: Suravee Suthikulpanit <[email protected]>
> ---
> arch/x86/kvm/svm/avic.c | 16 ++++++++++++++++
> arch/x86/kvm/svm/svm.c | 1 +
> 2 files changed, 17 insertions(+)
>
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 3ebeea19b487..d185dd8ddf17 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -691,6 +691,22 @@ void avic_apicv_post_state_restore(struct kvm_vcpu *vcpu)
> avic_handle_ldr_update(vcpu);
> }
>
> +void avic_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
> +{
> + struct vcpu_svm *svm = to_svm(vcpu);
> +
> + if (!lapic_in_kernel(vcpu) || (avic_mode == AVIC_MODE_NONE))
> + return;
> +
> + if (kvm_get_apic_mode(vcpu) == LAPIC_MODE_INVALID) {
> + WARN_ONCE(true, "Invalid local APIC state (vcpu_id=%d)", vcpu->vcpu_id);
> + return;
> + }
> +
> + kvm_vcpu_update_apicv(&svm->vcpu);
Why to have this call? I think that all that is needed is only to call the
avic_refresh_apicv_exec_ctrl.
Best regards,
Maxim Levitsky
> + avic_refresh_apicv_exec_ctrl(&svm->vcpu);
> +}
> +
> static int avic_set_pi_irte_mode(struct kvm_vcpu *vcpu, bool activate)
> {
> int ret = 0;
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 314628b6bff4..9066568fd19d 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4692,6 +4692,7 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
> .enable_nmi_window = svm_enable_nmi_window,
> .enable_irq_window = svm_enable_irq_window,
> .update_cr8_intercept = svm_update_cr8_intercept,
> + .set_virtual_apic_mode = avic_set_virtual_apic_mode,
> .refresh_apicv_exec_ctrl = avic_refresh_apicv_exec_ctrl,
> .check_apicv_inhibit_reasons = avic_check_apicv_inhibit_reasons,
> .apicv_post_state_restore = avic_apicv_post_state_restore,
Maxim,
On 5/4/22 7:19 PM, Maxim Levitsky wrote:
> On Wed, 2022-05-04 at 02:31 -0500, Suravee Suthikulpanit wrote:
>> Update and refresh AVIC settings when guest APIC mode is updated
>> (e.g. changing between disabled, xAPIC, or x2APIC).
>>
>> Signed-off-by: Suravee Suthikulpanit<[email protected]>
>> ---
>> arch/x86/kvm/svm/avic.c | 16 ++++++++++++++++
>> arch/x86/kvm/svm/svm.c | 1 +
>> 2 files changed, 17 insertions(+)
>>
>> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
>> index 3ebeea19b487..d185dd8ddf17 100644
>> --- a/arch/x86/kvm/svm/avic.c
>> +++ b/arch/x86/kvm/svm/avic.c
>> @@ -691,6 +691,22 @@ void avic_apicv_post_state_restore(struct kvm_vcpu *vcpu)
>> avic_handle_ldr_update(vcpu);
>> }
>>
>> +void avic_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
>> +{
>> + struct vcpu_svm *svm = to_svm(vcpu);
>> +
>> + if (!lapic_in_kernel(vcpu) || (avic_mode == AVIC_MODE_NONE))
>> + return;
>> +
>> + if (kvm_get_apic_mode(vcpu) == LAPIC_MODE_INVALID) {
>> + WARN_ONCE(true, "Invalid local APIC state (vcpu_id=%d)", vcpu->vcpu_id);
>> + return;
>> + }
>> +
>> + kvm_vcpu_update_apicv(&svm->vcpu);
> Why to have this call? I think that all that is needed is only to call the
> avic_refresh_apicv_exec_ctrl.
When APIC mode is updated on each vCPU, we need to check and update
vcpu->arch.apicv_active accordingly, which happens in the kvm_vcpu_update_apicv()
One test case that would fail w/o the kvm_vcpu_update_apicv() is when
we boot a Linux guest w/ guest kernel option _nox2apic_, which Linux forces APIC
mode of vCPUs with APIC ID 255 and higher to disable. W/o this line of code, the VM
would not boot w/ more than 255 vCPUs.
Regards,
Suravee
On Thu, 2022-05-05 at 08:38 +0700, Suravee Suthikulpanit wrote:
> Maxim,
>
> On 5/4/22 7:19 PM, Maxim Levitsky wrote:
> > On Wed, 2022-05-04 at 02:31 -0500, Suravee Suthikulpanit wrote:
> > > Update and refresh AVIC settings when guest APIC mode is updated
> > > (e.g. changing between disabled, xAPIC, or x2APIC).
> > >
> > > Signed-off-by: Suravee Suthikulpanit<[email protected]>
> > > ---
> > > arch/x86/kvm/svm/avic.c | 16 ++++++++++++++++
> > > arch/x86/kvm/svm/svm.c | 1 +
> > > 2 files changed, 17 insertions(+)
> > >
> > > diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> > > index 3ebeea19b487..d185dd8ddf17 100644
> > > --- a/arch/x86/kvm/svm/avic.c
> > > +++ b/arch/x86/kvm/svm/avic.c
> > > @@ -691,6 +691,22 @@ void avic_apicv_post_state_restore(struct kvm_vcpu *vcpu)
> > > avic_handle_ldr_update(vcpu);
> > > }
> > >
> > > +void avic_set_virtual_apic_mode(struct kvm_vcpu *vcpu)
> > > +{
> > > + struct vcpu_svm *svm = to_svm(vcpu);
> > > +
> > > + if (!lapic_in_kernel(vcpu) || (avic_mode == AVIC_MODE_NONE))
> > > + return;
> > > +
> > > + if (kvm_get_apic_mode(vcpu) == LAPIC_MODE_INVALID) {
> > > + WARN_ONCE(true, "Invalid local APIC state (vcpu_id=%d)", vcpu->vcpu_id);
> > > + return;
> > > + }
> > > +
> > > + kvm_vcpu_update_apicv(&svm->vcpu);
> > Why to have this call? I think that all that is needed is only to call the
> > avic_refresh_apicv_exec_ctrl.
>
> When APIC mode is updated on each vCPU, we need to check and update
> vcpu->arch.apicv_active accordingly, which happens in the kvm_vcpu_update_apicv()
This makes sense, but IMHO it would be better then to call kvm_vcpu_update_apicv
from the common code when apic mode changes then, because this logic should apply
to APICv as well.
In fact that logic of not activating AVIC was added in patch 12
(and on second thought I think it should be split to a separate patch),
was added to common code, thus calling kvm_vcpu_update_apicv when the condition
of 'apic is disabled on this vCPU' should also be done by the common code.
Best regards,
Maxim Levitsky
>
> One test case that would fail w/o the kvm_vcpu_update_apicv() is when
> we boot a Linux guest w/ guest kernel option _nox2apic_, which Linux forces APIC
> mode of vCPUs with APIC ID 255 and higher to disable. W/o this line of code, the VM
> would not boot w/ more than 255 vCPUs.
>
> Regards,
> Suravee
>