Re: [PATCH 2/2] hotplug-cpu.c: set UNISOLATE on dlpar_cpu_remove() failure

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

On 4/19/21 9:48 AM, Michael Ellerman wrote:
> Daniel Henrique Barboza <danielhb413@gmail.com> writes:
>> The RTAS set-indicator call, when attempting to UNISOLATE a DRC that is
>> already UNISOLATED or CONFIGURED, returns RTAS_OK and does nothing else
>> for both QEMU and phyp. This gives us an opportunity to use this
>> behavior to signal the hypervisor layer when an error during device
>> removal happens, allowing it to do a proper error handling, while not
>> breaking QEMU/phyp implementations that don't have this support.
>>
>> This patch introduces this idea by unisolating all CPU DRCs that failed
>> to be removed by dlpar_cpu_remove_by_index(), when handling the
>> PSERIES_HP_ELOG_ID_DRC_INDEX event. This is being done for this event
>> only because its the only CPU removal event QEMU uses, and there's no
>> need at this moment to add this mechanism for phyp only code.
> 
> Have you also confirmed that phyp is not bothered by it? ie. everything
> seems to continue working when you trigger this path on phyp.

Yes. Daniel Bueso (dbuesom@us.ibm.com) from the partition firmware team
helped me with that. We confirmed that phyp returns RTAS_OK under these
conditions (Unisolating an unisolated/configured DRC).


Thanks,


DHB

> 
> cheers
> 
>> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> index 12cbffd3c2e3..ed66895c2f51 100644
>> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> @@ -802,8 +802,15 @@ int dlpar_cpu(struct pseries_hp_errorlog *hp_elog)
>>   	case PSERIES_HP_ELOG_ACTION_REMOVE:
>>   		if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_COUNT)
>>   			rc = dlpar_cpu_remove_by_count(count);
>> -		else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX)
>> +		else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX) {
>>   			rc = dlpar_cpu_remove_by_index(drc_index);
>> +			/* Setting the isolation state of an UNISOLATED/CONFIGURED
>> +			 * device to UNISOLATE is a no-op, but the hypervison can
>> +			 * use it as a hint that the cpu removal failed.
>> +			 */
>> +			if (rc)
>> +				dlpar_unisolate_drc(drc_index);
>> +		}
>>   		else
>>   			rc = -EINVAL;
>>   		break;
>> -- 
>> 2.30.2