Re: [PATCH 2/9] KVM: x86: dynamic kvm_apic_map

From: "Radim Krčmář" <rkrcmar@redhat.com>
To: Peter Xu <peterx@redhat.com>
Cc: kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
	"Lan, Tianyu" <tianyu.lan@intel.com>,
	Igor Mammedov <imammedo@redhat.com>,
	Jan Kiszka <jan.kiszka@web.de>
Subject: Re: [PATCH 2/9] KVM: x86: dynamic kvm_apic_map
Date: Wed, 25 May 2016 18:15:00 +0200	[thread overview]
Message-ID: <20160525161459.GF14795@potion> (raw)
In-Reply-To: <20160523080401.GC8247@pxdev.xzpeter.org>

2016-05-23 16:04+0800, Peter Xu:
> Hi, Radim,
> 
> Got several questions inline.
> 
> On Fri, May 06, 2016 at 10:53:58PM +0200, Radim Krčmář wrote:
>> x2APIC supports up to 2^32-1 LAPICs, but most guest in coming years will
>> have slighly less VCPUs.  Dynamic size saves memory at the cost of
>> turning one constant into a variable.
> 
> From the manual, I see x2apic only support 2^20-16 processors, not
> 2^32-1.  Which one is correct?

"up to" is a crucial part.  Physical x2APIC can address 2^32-1, logical
x2APIC can address (2^16-1)*16 LAPICs and the OS can choose which mode
to use.

I think that machines with APIC IDs >2^20 will still be able to use
logical x2APIC, but higher APIC IDs are only be addressable with
physical x2APIC.

(Well, broadcasts would make even xAPIC "support" an unbounded amount of
 LAPICs. :])

> From below codes [1], I still see 2^20-16, since we are using high
> 16 bits of dest ID as cluster ID (0-0xfffe, while I guess 0xffff
> should be reserved), and lower 16 bits as processor/lapic mask.
> 
> [...]
> 
>> +static inline bool kvm_apic_map_get_logical_dest(struct kvm_apic_map *map,
>> +		u32 dest_id, struct kvm_lapic ***cluster, u16 *mask) {
>> +	switch (map->mode) {
>> +	case KVM_APIC_MODE_X2APIC: {
>> +		u32 offset = (dest_id >> 16) * 16;
>> +		// XXX: phys_map must be allocated as multiples of 16
>> +		if (offset < map->size) {
>> +			*cluster = &map->phys_map[offset];
>> +			*mask = dest_id & 0xffff;
> 
> [1]

This function is called ...get_LOGICAL_dest, so only logical addresses
are going to be passed to it.  Yes, it cannot handle APIC ID > 2^20-16.

> [...]
> 
>>  static void recalculate_apic_map(struct kvm *kvm)
>> @@ -156,17 +162,28 @@ static void recalculate_apic_map(struct kvm *kvm)
>>  	struct kvm_apic_map *new, *old = NULL;
>>  	struct kvm_vcpu *vcpu;
>>  	int i;
>> -
>> -	new = kzalloc(sizeof(struct kvm_apic_map), GFP_KERNEL);
>> +	u32 size = 255;
>>  
>>  	mutex_lock(&kvm->arch.apic_map_lock);
>>  
>> +	kvm_for_each_vcpu(i, vcpu, kvm)
>> +		if (kvm_apic_present(vcpu))
>> +			size = max(size, kvm_apic_id(vcpu->arch.apic));
>> +
>> +	size++;
>> +	size += (16 - size) & 16;
> 
> Is this same as:
> 
>   size = round_up(size, 16);
> 
> ?

It is, thank you.