Re: git: b9c0003f0fa3 - main - arm64: Initialize x18 for APs earlier during boot

On 11/13/23 7:51 AM, Mark Johnston wrote:
> The branch main has been updated by markj:
> 
> URL: https://cgit.FreeBSD.org/src/commit/?id=b9c0003f0fa39ead4bb3953b9118ae6f08e560f8
> 
> commit b9c0003f0fa39ead4bb3953b9118ae6f08e560f8
> Author:     Mark Johnston <markj@FreeBSD.org>
> AuthorDate: 2023-11-13 15:44:45 +0000
> Commit:     Mark Johnston <markj@FreeBSD.org>
> CommitDate: 2023-11-13 15:44:45 +0000
> 
>      arm64: Initialize x18 for APs earlier during boot
>      
>      When KMSAN is configured, the instrumentation inserts calls to
>      __msan_get_context_state() into all function prologues.  The
>      implementation dereferences curthread and thus assumes that x18 points
>      to the PCPU area.  This applies in particular to init_secondary(), which
>      currently is responsible for initializing x18 for APs.
>      
>      Move initialization into locore to avoid this problem.  No functional
>      change intended.
>      
>      Reviewed by:    kib, andrew
>      MFC after:      2 weeks
>      Sponsored by:   Juniper Networks, Inc.
>      Sponsored by:   Klara, Inc.
>      Differential Revision:  https://reviews.freebsd.org/D42533

(Sorry for just now replying, I'm looking at this in more detail while merging
to CheriBSD)

I think we can remove a bunch of now-dead code from init_secondary after
this, and probably don't even need to pass the cpu number to init_secondary
either?   That is:

void
init_secondary(uint64_t cpu)
{
	struct pcpu *pcpup;
	pmap_t pmap0;
	uint64_t mpidr;

	ptrauth_mp_start(cpu);

	/*
	 * Verify that the value passed in 'cpu' argument (aka context_id) is
	 * valid. Some older U-Boot based PSCI implementations are buggy,
	 * they can pass random value in it.
	 */
	mpidr = READ_SPECIALREG(mpidr_el1) & CPU_AFF_MASK;
	if (cpu >= MAXCPU || cpuid_to_pcpu[cpu] == NULL ||
	    PCPU_GET_MPIDR(cpuid_to_pcpu[cpu]) != mpidr) {
		for (cpu = 0; cpu < mp_maxid; cpu++)
			if (cpuid_to_pcpu[cpu] != NULL &&
			    PCPU_GET_MPIDR(cpuid_to_pcpu[cpu]) == mpidr)
				break;
		if ( cpu >= MAXCPU)
			panic("MPIDR for this CPU is not in pcpu table");
	}

	/*
	 * Identify current CPU. This is necessary to setup
	 * affinity registers and to provide support for
	 * runtime chip identification.
	 *
	 * We need this before signalling the CPU is ready to
	 * let the boot CPU use the results.
	 */
	pcpup = cpuid_to_pcpu[cpu];
	pcpup->pc_midr = get_midr();
	identify_cpu(cpu);

Can I think instead become something like:

void
init_secondary(void)
{
         struct pcpu *pcpup;
         pmap_t pmap0;
         uint64_t cpu;

         cpu = PCPU_GET(cpuid);
         ptrauth_mp_start(cpu);

	/*
	 * Identify current CPU. This is necessary to setup
	 * affinity registers and to provide support for
	 * runtime chip identification.
	 *
	 * We need this before signalling the CPU is ready to
	 * let the boot CPU use the results.
	 */
         pcpup = get_pcpu();
	pcpup->pc_midr = get_midr();
	identify_cpu(cpu);

Maybe we want to keep an explicit panic if PCPU_GET_MPIDR(pcpup) doesn't
match the mpidr from the register?

-- 
John Baldwin