linux-kernel - Re: [PATCH 3/3] x86/acpi, x86/boot: Add multiprocessor wake-up support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 22 Apr 2021 16:39:46 -0700
From:   "Kuppuswamy, Sathyanarayanan" 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>
To:     Thomas Gleixner <tglx@...utronix.de>,
        Rafael J Wysocki <rjw@...ysocki.net>,
        Ingo Molnar <mingo@...hat.com>,
        "H . Peter Anvin" <hpa@...or.com>,
        Peter Zijlstra <peterz@...radead.org>
Cc:     Len Brown <lenb@...nel.org>, Robert Moore <robert.moore@...el.com>,
        Erik Kaneda <erik.kaneda@...el.com>,
        linux-acpi@...r.kernel.org, devel@...ica.org,
        linux-kernel@...r.kernel.org, x86@...nel.org,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Andi Kleen <ak@...ux.intel.com>
Subject: Re: [PATCH 3/3] x86/acpi, x86/boot: Add multiprocessor wake-up
 support



On 4/22/21 4:04 PM, Thomas Gleixner wrote:
> Kuppuswamy!
> 
> On Thu, Apr 22 2021 at 12:24, Kuppuswamy Sathyanarayanan wrote:
>> +static int acpi_wakeup_cpu(int apicid, unsigned long start_ip)
>> +{
>> +	acpi_mp_wake_mailbox_init();
>> +
>> +	if (!acpi_mp_wake_mailbox)
>> +		return -EINVAL;
>> +
>> +	WRITE_ONCE(acpi_mp_wake_mailbox->apic_id, apicid);
>> +	WRITE_ONCE(acpi_mp_wake_mailbox->wakeup_vector, start_ip);
>> +	WRITE_ONCE(acpi_mp_wake_mailbox->command, ACPI_MP_WAKE_COMMAND_WAKEUP);
> 
> What's the point of using WRITE_ONCE() here? Where is the required
> READ_ONCE() counterpart and the required documentation in form of a
> comment?

mailbox memory is shared between firmware and OS. We use WRITE_ONCE to notify
compiler about it, and also to preserve the order of writes to these
addresses. As per MADT Wake protocol, once OS update the mailbox command
address with ACPI_MP_*_WAKEUP command, firmware will be bring the AP out sleep
state and trigger the boot process.
Since its a one way communication, we don't need to read the mailbox address
again. So there is no corresponding READ_ONCE() call.

I will add some comments about it in next version.

> 
>> +static int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
>> +				      const unsigned long end)
>> +{
> ...
>> +	acpi_wake_cpu_handler_update(acpi_wakeup_cpu);
> ...
> 
>> +++ b/arch/x86/kernel/apic/probe_32.c
>> @@ -207,3 +207,11 @@ int __init default_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
>>   	}
>>   	return 0;
>>   }
>> +
>> +void __init acpi_wake_cpu_handler_update(wakeup_cpu_handler handler)
>> +{
>> +	struct apic **drv;
>> +
>> +	for (drv = __apicdrivers; drv < __apicdrivers_end; drv++)
>> +		(*drv)->wakeup_secondary_cpu = handler;
>> +}
>> diff --git a/arch/x86/kernel/apic/probe_64.c b/arch/x86/kernel/apic/probe_64.c
>> index c46720f185c0..986dbb68d3c4 100644
>> --- a/arch/x86/kernel/apic/probe_64.c
>> +++ b/arch/x86/kernel/apic/probe_64.c
>> @@ -50,3 +50,11 @@ int __init default_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
>>   	}
>>   	return 0;
>>   }
>> +
>> +void __init acpi_wake_cpu_handler_update(wakeup_cpu_handler handler)
>> +{
>> +	struct apic **drv;
>> +
>> +	for (drv = __apicdrivers; drv < __apicdrivers_end; drv++)
>> +		(*drv)->wakeup_secondary_cpu = handler;
>> +}
> 
> What's the reason for having two verbatim copies of the same function
> which has no dependency on CONFIG_*_32/64 at all?

Initially I thought all ACPI related functions are grouped in probe_*.c.
After some review, now I know its incorrect. I will move the function
definition to apic.c

> 
> Thanks,
> 
>          tglx
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer