Date: Wed, 14 Dec 2016 18:15:36 +0100
From: Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: David Hildenbrand <david@redhat.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org, 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: <20161214171535.GA6799@potion>
References: <20161213163001.4567-1-rkrcmar@redhat.com>
 <20161213163001.4567-3-rkrcmar@redhat.com>
 <304d7935-6a0a-2296-b611-02177c888bb5@redhat.com>
 <35f6b1f8-6769-935e-633f-9013e496af16@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <35f6b1f8-6769-935e-633f-9013e496af16@redhat.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1633
Lines: 47

2016-12-14 17:59+0100, Paolo Bonzini:
> On 14/12/2016 17:15, David Hildenbrand wrote:
>> 
>>>      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)
> 
> Radim, what do you think?  I wanted to get these in before Christmas,
> but it's your call.

There was a reason why it was so ugly ... it's not a hack for nothing.

I can hope to make the patches/code more understandable, but the
function shouldn't change, unless we want to take a different approach.