Re: [PATCH v1 7/7] spapr.c: consider CPU core online state before allowing unplug

[Daniel Henrique Barboza <danielhb413@gmail.com>]

On 1/17/21 10:18 PM, David Gibson wrote:
> On Fri, Jan 15, 2021 at 03:52:56PM -0300, Daniel Henrique Barboza wrote:
>>
>>
>> On 1/15/21 2:22 PM, Greg Kurz wrote:
>>> On Thu, 14 Jan 2021 15:06:28 -0300
>>> Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
>>>
>>>> The only restriction we have when unplugging CPUs is to forbid unplug of
>>>> the boot cpu core. spapr_core_unplug_possible() does not contemplate the
>>
>> I'll look into it.
>>
>>>
>>> I can't remember why this restriction was introduced in the first place...
>>> This should be investigated and documented if the limitation still stands.
>>>
>>>> possibility of some cores being offlined by the guest, meaning that we're
>>>> rolling the dice regarding on whether we're unplugging the last online
>>>> CPU core the guest has.
>>>>
>>>
>>> Trying to unplug the last CPU is obviously something that deserves
>>> special care. LoPAPR is quite explicit on the outcome : this should
>>> terminate the partition.
>>>
>>> 13.7.4.1.1. Isolation of CPUs
>>>
>>> The isolation of a CPU, in all cases, is preceded by the stop-self
>>> RTAS function for all processor threads, and the OS insures that all
>>> the CPU’s threads are in the RTAS stopped state prior to isolating the
>>> CPU. Isolation of a processor that is not stopped produces unpredictable
>>> results. The stopping of the last processor thread of a LPAR partition
>>> effectively kills the partition, and at that point, ownership of all
>>> partition resources reverts to the platform firmware.
>>
>>
>> I was just investigating a reason why we should check for all thread
>> states before unplugging the core, like David suggested in his reply.
>> rtas_stop_self() was setting 'cs->halted = 1' without a thread activity
>> check like ibm,suspend-me() does and I was wondering why. This text you sent
>> explains it, quoting:
>>
>> "> The isolation of a CPU, in all cases, is preceded by the stop-self
>>   RTAS function for all processor threads, and the OS insures that all
>>   the CPU’s threads are in the RTAS stopped state prior to isolating the
>>   CPU."
>>
>>
>> This seems to be corroborated by arch/powerpc/platform/pseries/hotplug-cpu.c:
> 
> Um... this seems like you're overcomplicating this.  The crucial point
> here is that 'start-cpu' and 'stop-self' operate on individual
> threads, where as dynamic reconfiguration hotplug and unplug works on
> whole cores.
> 
>> static void pseries_cpu_offline_self(void)
>> {
>> 	unsigned int hwcpu = hard_smp_processor_id();
>>
>> 	local_irq_disable();
>> 	idle_task_exit();
>> 	if (xive_enabled())
>> 		xive_teardown_cpu();
>> 	else
>> 		xics_teardown_cpu();
>>
>> 	unregister_slb_shadow(hwcpu);
>> 	rtas_stop_self();
>>
>> 	/* Should never get here... */
>> 	BUG();
>> 	for(;;);
>> }
>>
>>
>> IIUC this means that we can rely on cs->halted = 1 since it's coming right
>> after a rtas_stop_self() call. This is still a bit confusing though and I
>> wouldn't mind standardizing the 'CPU core is offline' condition with what
>> ibm,suspend-me is already doing.
> 
> At the moment we have no tracking of whether a core is online.  We
> kinda-sorta track whether a *thread* is online through stop-self /
> start-cpu.
> 
>> David, what do you think?
> 
> I think we can rely on cs->halted = 1 when the thread is offline.
> What I'm much less certain about is whether we can count on the thread
> being offline when cs->halted = 1.

I guess we should just stick with your first suggestion then. I'll check for
both cs->halted and msr to assert whether a whole core if offline, like
ibm,suspend-me is doing:

         if (!cs->halted || (e->msr & (1ULL << MSR_EE))) {
             rtas_st(rets, 0, H_MULTI_THREADS_ACTIVE);
             return;
         }

Another question not necessarily related to this fix: we do a similar check
in the start of do_client_architecture_support(), returning the same
H_MULTI_THREADS_ACTIVE error, but we're not checking e->msr:


     /* CAS is supposed to be called early when only the boot vCPU is active. */
     CPU_FOREACH(cs) {
         if (cs == CPU(cpu)) {
             continue;
         }
         if (!cs->halted) {
             warn_report("guest has multiple active vCPUs at CAS, which is not allowed");
             return H_MULTI_THREADS_ACTIVE;
         }
     }


Should we bother changing this logic to check for e->msr as well? If there is no
possible harm done I'd rather have the same check returning H_MULTI_THREADS_ACTIVE
in both places. If there is a special condition in early boot where this check in
do_client_architecture_support() is enough, I would like to put a comment in there
to make it clear why.



Thanks,


DHB


> 
>>> R1-13.7.4.1.1-1. For the LRDR option: Prior to issuing the RTAS
>>> set-indicator specifying isolate isolation-state of a CPU DR
>>> connector type, all the CPU threads must be in the RTAS stopped
>>> state.
>>>
>>> R1-13.7.4.1.1-2. For the LRDR option: Stopping of the last processor
>>> thread of a LPAR partition with the stop-self RTAS function, must kill
>>> the partition, with ownership of all partition resources reverting to
>>> the platform firmware.
>>>
>>> This is clearly not how things work today : linux doesn't call
>>> "stop-self" on the last vCPU and even if it did, QEMU doesn't
>>> terminate the VM.
>>>
>>> If there's a valid reason to not implement this PAPR behavior, I'd like
>>> it to be documented.
>>
>>
>> I can only speculate. This would create a unorthodox way of shutting down
>> the guest, when the user can just shutdown the whole thing gracefully.
>>
>> Unless we're considering borderline cases, like the security risk mentioned
>> in the kernel docs (Documentation/core-api/cpu_hotplug.rst):
>>
>> "Such advances require CPUs available to a kernel to be removed either for
>> provisioning reasons, or for RAS purposes to keep an offending CPU off
>> system execution path. Hence the need for CPU hotplug support in the
>> Linux kernel."
>>
>> In this extreme scenario I can see a reason to kill the partition/guest
>> by offlining the last online CPU - if it's compromising the host we'd
>> rather terminate immediately instead of waiting for graceful
>> shutdown.
> 
> I'm a bit confused by this comment.  You seem to be conflating
> online/offline operations (start-cpu/stop-self) with hot plug/unplug
> operations - they're obviously related, but they're not the same
> thing.
> 
>>>> If we hit the jackpot, we're going to detach the core DRC and pulse the
>>>> hotplug IRQ, but the guest OS will refuse to release the CPU. Our
>>>> spapr_core_unplug() DRC release callback will never be called and the CPU
>>>> core object will keep existing in QEMU. No error message will be sent
>>>> to the user, but the CPU core wasn't unplugged from the guest.
>>>>
>>>> If the guest OS onlines the CPU core again we won't be able to hotunplug it
>>>> either. 'dmesg' inside the guest will report a failed attempt to offline an
>>>> unknown CPU:
>>>>
>>>> [  923.003994] pseries-hotplug-cpu: Failed to offline CPU <NULL>, rc: -16
>>>>
>>>> This is the result of stopping the DRC state transition in the middle in the
>>>> first failed attempt.
>>>>
>>>
>>> Yes, at this point only a machine reset can fix things up.
>>>
>>> Given this is linux's choice not to call "stop-self" as it should do, I'm not
>>> super fan of hardcoding this logic in QEMU, unless there are really good
>>> reasons to do so.
>>
>> I understand where are you coming from and I sympathize. Not sure about how users
>> would feel about that though. They expect a somewhat compatible behavior of
>> multi-arch features like hotplug/hotunplug, and x86 will neither hotunplug nor offline
>> the last CPU as well.
>>
>> There is a very high chance that, even if we pull this design off, I'll need to go to
>> Libvirt and disable it because we broke compatibility with how vcpu unplug operated
>> earlier.
>