Re: [PATCH] KVM: VMX: Postpone IPIv setup after successful vCPU creation

[Huang, Kai]

> 
> In other words, I want to fix this entire class of bugs, not play a game of
> whack-a-mole with bugs that humans are all but guaranteed to overlook.

I was thinking yet another xarray could make the overall logic more complicated,
but yeah, agreed that rejecting the duplicated vcpu_id asap is the right
solution to avoid such bugs (my first glance too in the first reply).

> 
> My other concern with the proposed change is that the vCPU becomes reachable
> before long before kvm_arch_vcpu_postcreate().  Which should be fine" for IPI
> virtualization, but sets a precedence I'd rather not exist, because initializing
> vCPU state _after_ it's reachable is rarely correct.  E.g. msr_kvm_poll_control
> is initialized in postcreate for some reason, and that's technically buggy
> because it's possible, albeit extremely unlikely, that MSR_KVM_POLL_CONTROL could
> be written by userspace before postcreate() runs.

Fair enough.  This didn't ring a bell to me.  Thanks for mentioning it.

> 
> E.g. for the xarray approacy (sketch only, completely untested):
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index fdbd697d0337..e34ca5920393 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -791,6 +791,7 @@ struct kvm {
>         /* The current active memslot set for each address space */
>         struct kvm_memslots __rcu *memslots[KVM_MAX_NR_ADDRESS_SPACES];
>         struct xarray vcpu_array;
> +       struct xarray pending_vcpu_ids;
>         /*
>          * Protected by slots_lock, but can be read outside if an
>          * incorrect answer is acceptable.
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 2df8ee9ecf6c..8bc4f7ffd76d 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -4179,6 +4179,12 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
>                 return r;
>         }
>  
> +       r = xa_insert(&kvm->pending_vcpu_ids, id, xa_mk_value(id), GFP_KERNEL);
> +       if (r) {
> +               mutex_unlock(&kvm->lock);
> +               return r;
> +       }
> +

(No preference between xarray vs bitmap)

Should we move this before kvm_arch_vcpu_precreate() to avoid any "precreate"
for duplicated vcpu_id?