Re: [PATCH v2 2/6] x86/kvm: add boot parameter for adding vcpu-id bits

[Juergen Gross <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 2844 bytes --]

On 03.09.21 21:48, Eduardo Habkost wrote:
> On Fri, Sep 03, 2021 at 03:08:03PM +0200, Juergen Gross wrote:
>> Today the maximum vcpu-id of a kvm guest's vcpu on x86 systems is set
>> via a #define in a header file.
>>
>> In order to support higher vcpu-ids without generally increasing the
>> memory consumption of guests on the host (some guest structures contain
>> arrays sized by KVM_MAX_VCPU_ID) add a boot parameter for adding some
>> bits to the vcpu-id. Additional bits are needed as the vcpu-id is
>> constructed via bit-wise concatenation of socket-id, core-id, etc.
>> As those ids maximum values are not always a power of 2, the vcpu-ids
>> are sparse.
>>
>> The additional number of bits needed is basically the number of
>> topology levels with a non-power-of-2 maximum value, excluding the top
>> most level.
>>
>> The default value of the new parameter will be to take the correct
>> setting from the host's topology.
> 
> Having the default depend on the host topology makes the host
> behaviour unpredictable (which might be a problem when migrating
> VMs from another host with a different topology).  Can't we just
> default to 2?

Okay, fine with me.

> 
>>
>> Calculating the maximum vcpu-id dynamically requires to allocate the
>> arrays using KVM_MAX_VCPU_ID as the size dynamically.
>>
>> Signed-of-by: Juergen Gross <jgross@suse.com>
>> ---
>> V2:
>> - switch to specifying additional bits (based on comment by Vitaly
>>    Kuznetsov)
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
> [...]
>>   #define KVM_MAX_VCPUS 288
>>   #define KVM_SOFT_MAX_VCPUS 240
>> -#define KVM_MAX_VCPU_ID 1023
>> +#define KVM_MAX_VCPU_ID kvm_max_vcpu_id()
> [...]
>> +unsigned int kvm_max_vcpu_id(void)
>> +{
>> +	int n_bits = fls(KVM_MAX_VCPUS - 1);
>> +
>> +	if (vcpu_id_add_bits < -1 || vcpu_id_add_bits > (32 - n_bits)) {
>> +		pr_err("Invalid value of vcpu_id_add_bits=%d parameter!\n",
>> +		       vcpu_id_add_bits);
>> +		vcpu_id_add_bits = -1;
>> +	}
>> +
>> +	if (vcpu_id_add_bits >= 0) {
>> +		n_bits += vcpu_id_add_bits;
>> +	} else {
>> +		n_bits++;		/* One additional bit for core level. */
>> +		if (topology_max_die_per_package() > 1)
>> +			n_bits++;	/* One additional bit for die level. */
>> +	}
>> +
>> +	if (!n_bits)
>> +		n_bits = 1;
>> +
>> +	return (1U << n_bits) - 1;
> 
> The largest possible VCPU ID is not KVM_MAX_VCPU_ID,
> it's (KVM_MAX_VCPU_ID - 1).  This is enforced by
> kvm_vm_ioctl_create_vcpu().
> 
> That would mean KVM_MAX_VCPU_ID should be (1 << n_bits) instead
> of ((1 << n_bits) - 1), wouldn't it?

Oh, indeed. I have been fooled by the IMO bad naming of this macro.

The current value 1023 suggests it is not only me having been fooled.

Shouldn't it be named "KVM_MAX_VCPU_IDS" instead?


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]