linux-kernel - Re: [PATCH v2 2/4] KVM: x86: replace kvm_apic_id with kvm_{x,x2}apic

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]

Date:   Wed, 14 Dec 2016 17:15:37 +0100
From:   David Hildenbrand <david@...hat.com>
To:     Radim Krčmář <rkrcmar@...hat.com>,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Igor Mammedov <imammedo@...hat.com>
Subject: Re: [PATCH v2 2/4] KVM: x86: replace kvm_apic_id with
 kvm_{x,x2}apic_id


>  	kvm_for_each_vcpu(i, vcpu, kvm)
>  		if (kvm_apic_present(vcpu))
> -			max_id = max(max_id, kvm_apic_id(vcpu->arch.apic));
> +			max_id = max(max_id, kvm_x2apic_id(vcpu->arch.apic));
>
>  	new = kvm_kvzalloc(sizeof(struct kvm_apic_map) +
>  	                   sizeof(struct kvm_lapic *) * ((u64)max_id + 1));
> @@ -179,16 +189,23 @@ static void recalculate_apic_map(struct kvm *kvm)
>  		struct kvm_lapic *apic = vcpu->arch.apic;
>  		struct kvm_lapic **cluster;
>  		u16 mask;
> -		u32 ldr, aid;
> +		u32 ldr;
> +		u8 xapic_id;
> +		u32 x2apic_id;
>
>  		if (!kvm_apic_present(vcpu))
>  			continue;
>
> -		aid = kvm_apic_id(apic);

think I'd even prefer here a simple

aid = kvm_xapic_id(apic);
if (apic_x2apic_mode(apic))
	aid = kvm_x2apic_id(apic);

that would keep changes minimal and I don't really see any benefit in 
the code when splitting handling up.

Patch 4 then simply can fixup setting code

if (aid <= new->max_apic_id && !new->phys_map[aid])
	new->phys_map[aid] = apic;

(if I am not missing some important corner case here)


> -		ldr = kvm_lapic_get_reg(apic, APIC_LDR);
> +		xapic_id = kvm_xapic_id(apic);
> +		x2apic_id = kvm_x2apic_id(apic);
>
> -		if (aid <= new->max_apic_id)
> -			new->phys_map[aid] = apic;
> +		if (apic_x2apic_mode(apic) &&
> +				x2apic_id <= new->max_apic_id)
> +			new->phys_map[x2apic_id] = apic;
> +		else if (!apic_x2apic_mode(apic))


This looks good to me.

-- 

David