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

Paolo Bonzini
2016-12-14T16:59:31Z

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.

Paolo

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