linux-kernel - Re: [PATCH v4 1/1] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <YKfHiu/a/V/0DS3V@hirez.programming.kicks-ass.net>
Date:   Fri, 21 May 2021 16:45:30 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>
Cc:     Rafael J Wysocki <rjw@...ysocki.net>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H . Peter Anvin" <hpa@...or.com>, linux-kernel@...r.kernel.org,
        x86@...nel.org,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Andi Kleen <ak@...ux.intel.com>
Subject: Re: [PATCH v4 1/1] x86/acpi, x86/boot: Add multiprocessor wake-up
 support

On Thu, May 13, 2021 at 02:37:32PM -0700, Kuppuswamy Sathyanarayanan wrote:
> +static int acpi_wakeup_cpu(int apicid, unsigned long start_ip)
> +{
> +	u8 timeout = 0xFF;
> +
> +	/* Remap mailbox memory only for the first call to acpi_wakeup_cpu() */
> +	if (physids_empty(apic_id_wakemap))
> +		acpi_mp_wake_mailbox = memremap(acpi_mp_wake_mailbox_paddr,
> +						sizeof(*acpi_mp_wake_mailbox),
> +						MEMREMAP_WB);

{ } for being multi-line

> +	/*
> +	 * According to the ACPI specification r6.4, sec 5.2.12.19, the
> +	 * mailbox-based wakeup mechanism cannot be used more than once
> +	 * for the same CPU, so skip sending wake commands to already
> +	 * awake CPU.
> +	 */
> +	if (physid_isset(apicid, apic_id_wakemap)) {
> +		pr_err("CPU already awake (APIC ID %x), skipping wakeup\n",
> +		       apicid);
> +		return -EINVAL;
> +	}
> +
> +
> +	/*
> +	 * Mailbox memory is shared between firmware and OS. Firmware will
> +	 * listen on mailbox command address, and once it receives the wakeup
> +	 * command, CPU associated with the given apicid will be booted. So,
> +	 * the value of apic_id and wakeup_vector has to be set before updating
> +	 * the wakeup command. So use WRITE_ONCE to let the compiler know about
> +	 * it and preserve the order of writes.
> +	 */
> +	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);

Do those want to be smp_store_release(), in addition to being a volatile
write, those also include compiler barriers to make sure the compiler
doesn't lift stuff around.

> +
> +	/*
> +	 * After writing wakeup command, wait for maximum timeout of 0xFF
> +	 * for firmware to reset the command address back zero to indicate
> +	 * the successful reception of command.
> +	 * NOTE: 255 as timeout value is decided based on our experiments.
> +	 *
> +	 * XXX: Change the timeout once ACPI specification comes up with
> +	 *      standard maximum timeout value.
> +	 */
> +	while (READ_ONCE(acpi_mp_wake_mailbox->command) && timeout--)
> +		cpu_relax();

What's the unit of the timeout? The mailbox reads, the PAUSE
instruction?

> +
> +	if (timeout) {
> +		/*
> +		 * If the CPU wakeup process is successful, store the
> +		 * status in apic_id_wakemap to prevent re-wakeup
> +		 * requests.
> +		 */
> +		physid_set(apicid, apic_id_wakemap);
> +		return 0;
> +	}
> +
> +	/* If timed out (timeout == 0), return error */
> +	return -EIO;
> +}
> +
>  #endif				/*CONFIG_X86_LOCAL_APIC */