Re: [PATCH v7 1/2] x86, apic: Add disable_cpu_apicid kernel parameter

[HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com>]

(2013/11/28 0:10), Vivek Goyal wrote:
> On Wed, Nov 27, 2013 at 11:00:48AM +0900, HATAYAMA Daisuke wrote:
>> Add disable_cpu_apicid kernel parameter. To use this kernel parameter,
>> specify an initial APIC ID of the corresponding CPU you want to
>> disable.
>>
>> This is mostly used for the kdump 2nd kernel to disable BSP to wake up
>> multiple CPUs without causing system reset or hang due to sending INIT
>> from AP to BSP.
>>
>> Kdump users first figure out initial APIC ID of the BSP, CPU0 in the
>> 1st kernel, for example from /proc/cpuinfo and then set up this kernel
>> parameter for the 2nd kernel using the obtained APIC ID.
>>
>> However, doing this procedure at each boot time manually is awkward,
>> which should be automatically done by user-land service scripts, for
>> example, kexec-tools on fedora/RHEL distributions.
>>
>> This design is more flexible than disabling BSP in kernel boot time
>> automatically in that in kernel boot time we have no choice but
>> referring to ACPI/MP table to obtain initial APIC ID for BSP, meaning
>> that the method is not applicable to the systems without such BIOS
>> tables.
>>
>> One assumption behind this design is that users get initial APIC ID of
>> the BSP in still healthy state and so BSP is uniquely kept in
>> CPU0. Thus, through the kernel parameter, only one initial APIC ID can
>> be specified.
>>
>> boot_cpu_physical_apicid is designed to have the apicid returned by
>> read_apic_id(). However, on some platforms, it is temporarilly
>> modified by the apicid reported as BSP through MP table. This function
>> is called with the modified boot_cpu_physical_apicid.
>>
>> On the platforms, boot_cpu_physical_apicid is always forced to be the
>> apicid of the BSP. Then, disabled_cpu_apicid kernel parameter doesn't
>> work well for apicids for APs.
>>
>> Here we leave improvement of handling boot_cpu_physical_apicid as
>> another work for now since the temporary modification is done on
>> differnet platforms and tests on each platform are required. As a
>> workaround, we directly update boot_cpu_physical_apicid by
>> read_apic_id().
>>
>> Signed-off-by: HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com>
>> ---
>>   arch/x86/kernel/apic/apic.c |   49 +++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 49 insertions(+)
>>
>> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
>> index d278736..a94f618 100644
>> --- a/arch/x86/kernel/apic/apic.c
>> +++ b/arch/x86/kernel/apic/apic.c
>> @@ -75,6 +75,13 @@ unsigned int max_physical_apicid;
>>   physid_mask_t phys_cpu_present_map;
>>
>>   /*
>> + * Processor to be disabled specified by kernel parameter
>> + * disable_cpu_apicid=<int>, mostly used for the kdump 2nd kernel to
>> + * avoid undefined behaviour caused by sending INIT from AP to BSP.
>> + */
>> +unsigned int disabled_cpu_apicid = BAD_APICID;
>> +
>> +/*
>>    * Map cpu index to physical APIC ID
>>    */
>>   DEFINE_EARLY_PER_CPU_READ_MOSTLY(u16, x86_cpu_to_apicid, BAD_APICID);
>> @@ -2111,9 +2118,42 @@ void disconnect_bsp_APIC(int virt_wire_setup)
>>   int generic_processor_info(int apicid, int version)
>>   {
>>   	int cpu, max = nr_cpu_ids;
>> +	/*
>> +	 * boot_cpu_physical_apicid is designed to have the apicid
>> +	 * returned by read_apic_id(). However, on some platforms, it
>> +	 * is temporarilly modified by the apicid reported as BSP
>> +	 * through MP table. Concretely:
>> +	 *
>> +	 * - arch/x86/kernel/mpparse.c: MP_processor_info()
>> +	 * - arch/x86/mm/amdtopology.c: amd_numa_init()
>> +	 * - arch/x86/platform/visws/visws_quirks.c: MP_processor_info()
>> +	 *
>> +	 * This function is executed with the modified
>> +	 * boot_cpu_physical_apicid. So, disabled_cpu_apicid kernel
>> +	 * parameter doesn't work.
>> +	 *
>> +	 * Since fixing handling of boot_cpu_physical_apicid requires
>> +	 * another discussion and tests on each platform, we leave it
>> +	 * for now and here we use read_apic_id() directly by updating
>> +	 * global scope of boot_cpu_physical_id with the local one.
>> +	 */
>> +	unsigned int boot_cpu_physical_apicid = read_apic_id();
>
> I think this is confusing. Why are you trying to define a local variable
> with same name as global variable. There is no need. I think if we
> simply put the comment there that why are we not making use of
> boot_cpu_physical_apicid, that is good enough and directly read
> the apic id.
>
> 	if (disabled_cpu_apicid != BAD_APICID &&
> 	    disabled_cpu_apicid != read_apic_id() &&
> 	    disabled_cpu_apicid == apicid) {
>
> 	}
>

Because there are also other occurrences of boot_cpu_physical_apicid in the
same function, and defining the local variable with the same name needs
one line only, and it's a temporary workaround change, I think it unnatural
to replace boot_cpu_physical_apicid even temporarily.

-- 
Thanks.
HATAYAMA, Daisuke


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec