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

[Kuppuswamy, Sathyanarayanan]

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