On 14.06.24 18:35, Sean Christopherson wrote:
> On Wed, Jun 12, 2024, Mathias Krause wrote:
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 082ac6d95a3a..8bc7b8b2dfc5 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -7220,10 +7220,16 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
>> case KVM_SET_BOOT_CPU_ID:
>> r = 0;
>> mutex_lock(&kvm->lock);
>> - if (kvm->created_vcpus)
>> + if (kvm->created_vcpus) {
>> r = -EBUSY;
>> - else
>> - kvm->arch.bsp_vcpu_id = arg;
>> + goto set_boot_cpu_id_out;
>> + }
>> + if (arg > KVM_MAX_VCPU_IDS) {
>> + r = -EINVAL;
>> + goto set_boot_cpu_id_out;
>> + }
>> + kvm->arch.bsp_vcpu_id = arg;
>> +set_boot_cpu_id_out:
>
> Any reason not to check kvm->arch.max_vcpu_ids? It's not super pretty, but it's
> also not super hard.
I explicitly excluded it as there's no strict requirement for calling
KVM_ENABLE_CAP(KVM_CAP_MAX_VCPU_ID) before KVM_SET_BOOT_CPU_ID, so
kvm->arch.max_vcpu_ids would not yet be set. But yeah, checking if it
was already set prior to narrowing the compare to it is a neat way to
solve that. Good idea!
> And rather than use gotos, this can be done with if-elif-else, e.g. with the
> below delta get to:
>
> r = 0;
> mutex_lock(&kvm->lock);
> if (kvm->created_vcpus)
> r = -EBUSY;
> else if (arg > KVM_MAX_VCPU_IDS ||
> (kvm->arch.max_vcpu_ids && arg > kvm->arch.max_vcpu_ids))
> r = -EINVAL;
> else
> kvm->arch.bsp_vcpu_id = arg;
> mutex_unlock(&kvm->lock);
> break;
Heh, I had the if-else ladder before but went for the goto version after
looking around, attempting not to deviate from the "style" used for all
the other cases . :|
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 6e6f3d31cff0..994aa281b07d 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6707,7 +6707,9 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
> break;
>
> mutex_lock(&kvm->lock);
> - if (kvm->arch.max_vcpu_ids == cap->args[0]) {
> + if (kvm->arch.bsp_vcpu_id > cap->args[0]) {
> + ;
> + } else if (kvm->arch.max_vcpu_ids == cap->args[0]) {
> r = 0;
> } else if (!kvm->arch.max_vcpu_ids) {
> kvm->arch.max_vcpu_ids = cap->args[0];
This is a separate fix, imho -- not allowing to set
kvm->arch.max_vcpu_ids to a value that would exclude
kvm->arch.bsp_vcpu_id. So I'll put that a separate commit.
However, this still doesn't prevent creating VMs that have no BSP as the
actual vCPU ID assignment only happens later, when vCPUs are created.
But, I guess, that's no real issue. If userland insists on not having a
BSP, so be it.
> @@ -7226,16 +7228,13 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
> case KVM_SET_BOOT_CPU_ID:
> r = 0;
> mutex_lock(&kvm->lock);
> - if (kvm->created_vcpus) {
> + if (kvm->created_vcpus)
> r = -EBUSY;
> - goto set_boot_cpu_id_out;
> - }
> - if (arg > KVM_MAX_VCPU_IDS) {
> + else if (arg > KVM_MAX_VCPU_IDS ||
> + (kvm->arch.max_vcpu_ids && arg > kvm->arch.max_vcpu_ids))
> r = -EINVAL;
> - goto set_boot_cpu_id_out;
> - }
> - kvm->arch.bsp_vcpu_id = arg;
> -set_boot_cpu_id_out:
> + else
> + kvm->arch.bsp_vcpu_id = arg;
> mutex_unlock(&kvm->lock);
> break;
> #ifdef CONFIG_KVM_XEN
Thanks, looking good!
Will prepare a v3.
Mathias
