linux-kernel - Re: [PATCH v2 08/12] KVM: SVM: Update AVIC settings when changing APIC mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <abb93e2d73b7ada6cbabcd3ebbf7b38e4701ec57.camel@redhat.com>
Date:   Mon, 18 Apr 2022 15:55:02 +0300
From:   Maxim Levitsky <mlevitsk@...hat.com>
To:     Suravee Suthikulpanit <suravee.suthikulpanit@....com>,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Cc:     pbonzini@...hat.com, seanjc@...gle.com, joro@...tes.org,
        jon.grimm@....com, wei.huang2@....com, terry.bowman@....com
Subject: Re: [PATCH v2 08/12] KVM: SVM: Update AVIC settings when changing
 APIC mode

On Tue, 2022-04-12 at 06:58 -0500, Suravee Suthikulpanit wrote:
> When APIC mode is updated (e.g. disabled, xAPIC, or x2APIC),
> KVM needs to call kvm_vcpu_update_apicv() to update AVIC settings
> accordingly.
> 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@....com>
> ---
>  arch/x86/kvm/svm/avic.c | 15 +++++++++++++++
>  arch/x86/kvm/svm/svm.c  |  1 +
>  2 files changed, 16 insertions(+)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 22ee1098e2a5..01392b8364f4 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -616,6 +616,21 @@ 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);

I think it makes sense to call avic_refresh_apicv_exec_ctrl directly here.
 
I am not sure that kvm_vcpu_update_apicv will even call it
because it has an optimization of doing nothing when inhibition status
didn't change.
 
 
Another semi-related note:
 
the current way the x2avic msrs are configured creates slight performance 
problem for nesting:
 
The problem is that when entering a nested guest, AVIC on the current vCPU
is inhibited, but this is done only so that this vCPU *peers* don't
try to use AVIC to send IPIs to it, so there is no need to update vmcb01
msr interception bitmap, and vmcb02 should have all these msrs intercepted always.
Same with returning to host.

It also should be checked that during nested entry, at least vmcb01 msr bitmap
is updated - TL;DR - please check that x2avic works when there is a nested guest running.
 

My avic nested coexistence patches are already upstream (kvm/queue) so this is already
relevant.
 
With nested x2avic (which will be very easy to do after my nested AVIC code is
upstream, vmcb02 will need to have x2avic msrs not intercepted as long as L1
doesn't want to intercept them if nested x2avic is enabled).

Best regards,
	Maxim Levitsky


> +}
> +
>  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 c85663b62d4e..b7dbd8bb2c0a 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4606,6 +4606,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,