Messages in this thread | | | Subject | Re: [PATCH v4 1/1] x86/acpi, x86/boot: Add multiprocessor wake-up support | From | "Kuppuswamy, Sathyanarayanan" <> | Date | Fri, 21 May 2021 08:14:45 -0700 |
| |
On 5/21/21 7:45 AM, Peter Zijlstra wrote: > 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
Yes. I will fix it.
> >> + /* >> + * 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.
I think we can use smp_store_release(). Let me test and add it in next version.
> >> + >> + /* >> + * 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?
Read mailbox memory, timeout dec and then pause. Its more like busy wait loop.
And timeout count is decided based on our experiments. Once spec defines a standard, we can modify it.
> >> + >> + 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 */
-- Sathyanarayanan Kuppuswamy Linux Kernel Developer
| |