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:47 PM, Boris Ostrovsky wrote:
> 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.


How about we don't clear _VPF_down if the bit in the availability bitmap
is not set?


-boris

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