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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 11/09/2016 02:23 PM, Andrew Cooper wrote:
> 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.

We could indeed use bitmap (and then it will actually be easier to
handle io request as we can just copy appropriate bytes of the map
instead of going bit-by-bit). This will still require the new domctl.

I am not convinced though that we can start enforcing this new VCPU
count, at least for PV guests. They expect to start all max VCPUs and
then offline them. This, of course, can be fixed but all non-updated
kernels will stop booting.
-boris


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