Date: Wed, 14 Dec 2016 18:12:32 +0100
From: Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>
To: David Hildenbrand <david@redhat.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        Paolo Bonzini <pbonzini@redhat.com>,
        Igor Mammedov <imammedo@redhat.com>
Subject: Re: [PATCH v2 2/4] KVM: x86: replace kvm_apic_id with
 kvm_{x,x2}apic_id
Message-ID: <20161214171232.GA5594@potion>
References: <20161213163001.4567-1-rkrcmar@redhat.com>
 <20161213163001.4567-3-rkrcmar@redhat.com>
 <304d7935-6a0a-2296-b611-02177c888bb5@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <304d7935-6a0a-2296-b611-02177c888bb5@redhat.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2940
Lines: 86

2016-12-14 17:15+0100, David Hildenbrand:
>>  	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.

It is neccesassary to write an entry for both IDs and I wanted to split
it before [4/4], because doing both changes at once seemed hard to
grasp.

Putting it here didn't work well either ... is a separate patch for the
hunk below better, or would you prefer to have it in [4/4]?

> 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)

The trick is that we want to do the following even in xAPIC mode:

  new->phys_map[kvm_x2apic_id(apic)] = apic;

This is the main idea of the hotplug hack -- to allow unique addressing
of processors that were reset in xAPIC mode.  (And I add a disgusting
"x2apic_id > 0xff" condition in [4/4], because we still allow guests to
change xAPIC IDs, which wouldn't play nice with this.)

Hardware does a superset of this, because it only looks at lower 8 bits
of the desination ID when delivering to xAPIC.

When kvm_x2apic_id(apic) != kvm_xapic_id(apic), then the APIC is in
xAPIC mode so we definitely want to keep xAPIC working, hence

  if (!apic_x2apic_mode(apic))
  	new->phys_map[kvm_xapic_id(apic)] = apic;

Two writes are necessary.

And there can already be another_apic "kvm_x2apic_id(another_apic) ==
kvm_xapic_id(apic)" so we prevent hotplug from breaking existing x2APIC
setups by doing "!new->phys_map[aid]" when setting xAPIC ID.

I hope we get a better solution in the future, but it would have to be
done at hardware (QEMU) level, because even firmware (seabios) doesn't
have standard ways to deal with this situation ...

>> -		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.