Re: [Xen-devel] [PATCH v2 01/11] x86/domctl: Add XEN_DOMCTL_set_avail_vcpus

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/11/16 15:29, Boris Ostrovsky wrote:
> On 11/09/2016 10:04 AM, Andrew Cooper wrote:
>> On 09/11/16 14:39, Boris Ostrovsky wrote:
>>> This domctl is called when a VCPU is hot-(un)plugged to a guest (via
>>> 'xl vcpu-set'). While this currently is only intended to be needed by
>>> PVH guests we will call this domctl for all (x86) guests for consistency.
>>>
>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
>>> Acked-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
>>> ---
>>> Changes in v2:
>>> * Added comment in arch_do_domctl() stating that the domctl is only required
>>>   for PVH guests
>> I am not happy with this change until we understand why it is needed.
>>
>> Are we genuinely saying that there is no current enforcement in the
>> PV-hotplug mechanism?
> That's right. Don't call setup_cpu_watcher() in Linux and you will be
> running with maxvcpus.

/sigh - Quality engineering there...

Yes - lets take the time to actually do this properly.

>
>>> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
>>> index 2a2fe04..b736d4c 100644
>>> --- a/xen/arch/x86/domctl.c
>>> +++ b/xen/arch/x86/domctl.c
>>> @@ -1430,6 +1430,23 @@ long arch_do_domctl(
>>>          }
>>>          break;
>>>  
>>> +    case XEN_DOMCTL_set_avail_vcpus:
>>> +    {
>>> +        unsigned int num = domctl->u.avail_vcpus.num;
>>> +
>>> +        /*
>>> +         * This is currently only needed by PVH guests (but
>>> +         * any guest is free to make this call).
>>> +         */
>>> +        ret = -EINVAL;
>>> +        if ( num > d->max_vcpus )
>>> +            break;
>>> +
>>> +        d->arch.avail_vcpus = num;
>>> +        ret = 0;
>>> +        break;
>>> +    }
>> What do you actually mean by "avail_vcpus"?  What happens if a vcpu
>> higher than the new number is currently online and running?  What
>> happens to the already-existing vcpus-at-startup value?
> It shouldn't happen: we set avail_vcpus at domain creation time to
> vcpus-at-startup.
>
> The name is not great. It would have been better to have it online_vcpus
> but that usually means that VPF_down is set (which can happen, for
> example, when the guest offlines a VCPU).

How about an availability bitmap instead, which always has max_vcpus
bits in it?  Xen should consult the bitmap before allowing a VM to
online a new vcpu.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel