linux-kernel - Re: [PATCH 08/19] KVM: SVM: Remove redundant cluster calculation that also creates a shadow

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: <7281e42323b53a016cf8545b7a4547d70d87efce.camel@redhat.com>
Date:   Wed, 31 Aug 2022 13:19:50 +0300
From:   Maxim Levitsky <mlevitsk@...hat.com>
To:     Sean Christopherson <seanjc@...gle.com>,
        Paolo Bonzini <pbonzini@...hat.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Suravee Suthikulpanit <suravee.suthikulpanit@....com>,
        Li RongQing <lirongqing@...du.com>
Subject: Re: [PATCH 08/19] KVM: SVM: Remove redundant cluster calculation
 that also creates a shadow

On Wed, 2022-08-31 at 00:34 +0000, Sean Christopherson wrote:
> Drop the redundant "cluster" calculation and its horrific shadowing in
> the x2avic logical mode path.  The "cluster" that is declared at an outer
> scope is derived using the exact same calculation and has performed the
> left-shift.

Actually I think we should just revert the commit 
'KVM: SVM: Use target APIC ID to complete x2AVIC IRQs when possible'


I know that the patch that intially introduced the the avic_kick_target_vcpus_fast had
x2apic/x2avic code, and then it was split to avoid adding x2avic code before it was merged,
resulting in this patch to add the x2apic specific code.

But when I fixed most issues of avic_kick_target_vcpus_fast in my 
'KVM: x86: SVM: fix avic_kick_target_vcpus_fast', I added back x2apic code because
it was just natural to do since I had to calculate cluster/bitmap masks anyway.

I expected this patch to be dropped because of this but I guess it was not noticed,
or patches were merged in the wrong order.

This is the reason of shadowing, duplicate calculations/etc.

Patch 9 and 7 of your series can be dropped as well then.

Best regards,
	Maxim Levitsky


> 
> No functional change intended.
> 
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>  arch/x86/kvm/svm/avic.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 14f567550a1e..8c9cad96008e 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -410,10 +410,9 @@ static int avic_kick_target_vcpus_fast(struct kvm *kvm, struct kvm_lapic *source
>  			 * For x2APIC logical mode, cannot leverage the index.
>  			 * Instead, calculate physical ID from logical ID in ICRH.
>  			 */
> -			int cluster = (icrh & 0xffff0000) >> 16;
>  			int apic = ffs(bitmap) - 1;
>  
> -			l1_physical_id = (cluster << 4) + apic;
> +			l1_physical_id = cluster + apic;
>  		}
>  	}
>