Re: [Xen-devel] [PATCH 1/3] x86/ACPI: re-park previously parked CPUs upon resume from S3

From: "Jan Beulich" <JBeulich@suse.com>
To: "Julien Grall" <julien.grall@arm.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>, WeiLiu <wl@xen.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <Ian.Jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
	xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [Xen-devel] [PATCH 1/3] x86/ACPI: re-park previously parked CPUs upon resume from S3
Date: Mon, 17 Jun 2019 00:40:18 -0600	[thread overview]
Message-ID: <5D0735D20200007800238B1B@prv1-mh.provo.novell.com> (raw)
In-Reply-To: <6c6fcf56-f6d1-e6b7-dcf8-3889a772d03e@arm.com>

>>> On 14.06.19 at 18:52, <julien.grall@arm.com> wrote:
> The title and commit message are a bit odd to read because you are modifying 
> common code but everything is geared towards x86.

Indeed. There's no caller of {en,dis}able_nonboot_cpus() in Arm code
at present, afaics. Hence the code changed (but not the file) is truly
x86-specific at the moment. I've explicitly thought about the
"inconsistency" between title and contents, but I've deliberately put it
as is: The change _is_ x86 / ACPI only, _despite_ touching common
code (and hence needing a REST maintainer ack).

>> --- a/xen/common/cpu.c
>> +++ b/xen/common/cpu.c
>> @@ -105,7 +105,7 @@ int cpu_down(unsigned int cpu)
>>       if ( err )
>>           goto fail;
>>   
>> -    if ( unlikely(system_state < SYS_STATE_active) )
>> +    if ( system_state < SYS_STATE_active || system_state == SYS_STATE_resume )
> 
> So this change here is necessary because enable_nonboot_cpus() may call 
> cpu_down(), am I right?

Yes (albeit likely s/necessary/wanted/).

> If so, could you please mention it in the commit message?

Hmm, I could. But this is just paralleling what we're already doing for
the boot path, so it didn't seem imperative to me to call it out. But
anyway, I've added a sentence.

>> @@ -207,15 +207,19 @@ void enable_nonboot_cpus(void)
>>   
>>       printk("Enabling non-boot CPUs  ...\n");
>>   
>> -    for_each_cpu ( cpu, &frozen_cpus )
>> +    for_each_present_cpu ( cpu )
>>       {
>> +        if ( park_offline_cpus ? cpu == smp_processor_id()
> 
> park_offline_cpus is x86 specific, so it will not build on Arm.

And that's intentional, even more so that (as said above) Arm doesn't
call here in the first place. And even if it did - whether to do things the
"new" way would then still (intentionally) depend on whether Arm had
any way of park_offline_cpus being "true".

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel