Page MenuHomeFreeBSD
Differential D32958

aarch64: Add static asssert for context size
ClosedPublic
Actions

Authored by imp on Nov 12 2021, 6:38 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 17, 8:51 AM
Unknown Object (File)
Thu, Mar 6, 7:52 PM
Unknown Object (File)
Mon, Mar 3, 4:28 AM
Unknown Object (File)
Mon, Feb 24, 8:08 AM
Unknown Object (File)
Feb 15 2025, 12:04 AM
Unknown Object (File)
Jan 28 2025, 10:11 PM
Unknown Object (File)
Jan 28 2025, 6:49 PM
Unknown Object (File)
Jan 26 2025, 10:21 PM
View All 95 Files
Subscribers
andrew
emaste
kib

Details

Reviewers
manu
jhb
andrew
kib
Commits
rG3988ca5aabb2: aarch64: Add static asssert for context size
Summary

Add a static assert for the mcontext{,32}_t and ucontext32_t
sizes. These are de-factor ABI options and cannot change size ever.

Sponsored by: Netflix

Test Plan

Found issues with mcontext size in qemu bsd-user, so thought I'd add asserts here to ensure the size doesn't change, and to document said size.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

imp created this revision.Nov 12 2021, 6:38 AM
Herald added a reviewer: manu. · View Herald TranscriptNov 12 2021, 6:38 AM
Herald added subscribers: emaste, andrew. · View Herald Transcript
imp requested review of this revision.Nov 12 2021, 6:38 AM
Harbormaster completed remote builds in B42749: Diff 98399.Nov 12 2021, 6:38 AM
imp edited the test plan for this revision. (Show Details)Nov 12 2021, 6:39 AM
imp added a reviewer: jhb.
andrew accepted this revision.Nov 12 2021, 11:20 AM

These should also be true in CheriBSD running FreeBSD/arm64 binaries. I'll have to look how we could handle the new extensions where userspace may have access to larger register files, e.g. SVE has 32 registers that could be up to 2048 bytes each. In this case I expect we'll store them in their own variable sized structure that one of the spare fields point to.

This revision is now accepted and ready to land.Nov 12 2021, 11:20 AM
kib added a subscriber: kib.Nov 12 2021, 2:05 PM

I believe that the general preference is to put this kind of asserts into .c file and not in .h. It is enough to get single error from the build, instead of 40+ from each instance of compiler in the parallel build.

Andrew, WRT to new extensions, please look how Intel extensions are handled on x86, in particular getcontextx(3). Indeed we store a pointer to extended part of XSAVE area into existing mcontext(32).

jhb accepted this revision.Nov 12 2021, 5:32 PM

I would be fine with moving the assertions to a C file (probably arm64/arm64/machdep.c next to get_mcontext? The 32-bit assertion could perhaps be next to get_mcontext32)

imp added a comment.Nov 12 2021, 5:42 PM
In D32958#744229, @jhb wrote:

I would be fine with moving the assertions to a C file (probably arm64/arm64/machdep.c next to get_mcontext? The 32-bit assertion could perhaps be next to get_mcontext32)

I was going to add several more. My plan was just to add them all to the top of exec_machdep.c on all the architectures, with appropriate ifdefs for 32-bit compat stuff since I was also going to assert on ucontext_t size and sigframe as well.

imp updated this revision to Diff 102513.Feb 8 2022, 9:23 PM

update, per review

This revision now requires review to proceed.Feb 8 2022, 9:23 PM
Harbormaster completed remote builds in B44344: Diff 102513.Feb 8 2022, 9:23 PM
imp added a comment.Feb 8 2022, 9:44 PM

exec_machdep.c seems to be the place that has all the MS signal handling on all the platforms, so it seems like the right place for these. I did one as an example, but will do the others in a similar manner once this gets signed off on.
I can also add an assert for the UCONTEXT too. bsd-user just cares about mcontext at compile time (though arguably it should care about both).

imp updated this revision to Diff 102517.Feb 8 2022, 9:51 PM

Add ucontext too...

Harbormaster completed remote builds in B44345: Diff 102517.Feb 8 2022, 9:51 PM
kib added inline comments.Feb 8 2022, 11:12 PM
sys/arm64/arm64/exec_machdep.c
65

As John suggested, I agree that arm64/arm64/freebsd32_machdep.c is the better place for COMPAT32 asserts.

imp updated this revision to Diff 102531.Feb 8 2022, 11:59 PM

update per review

Harbormaster completed remote builds in B44356: Diff 102531.Feb 8 2022, 11:59 PM
kib accepted this revision.Feb 9 2022, 12:40 AM
This revision is now accepted and ready to land.Feb 9 2022, 12:40 AM
andrew accepted this revision.Feb 9 2022, 2:34 PM
andrew added inline comments.Feb 9 2022, 2:37 PM
sys/arm64/arm64/exec_machdep.c
64

Shouldn't this message be ucontext_t ...?

imp updated this revision to Diff 102619.Feb 10 2022, 7:30 PM

update per review

This revision now requires review to proceed.Feb 10 2022, 7:30 PM
Harbormaster completed remote builds in B44393: Diff 102619.Feb 10 2022, 7:30 PM
This revision was not accepted when it landed; it landed in state Needs Review.Feb 10 2022, 9:34 PM
Closed by commit rG3988ca5aabb2: aarch64: Add static asssert for context size (authored by imp). · Explain Why
This revision was automatically updated to reflect the committed changes.
imp added a commit: rG3988ca5aabb2: aarch64: Add static asssert for context size.

Revision Contents
Changeset List

PathSize
sys/
arm64/
arm64/
4 lines
4 lines

Diff 102638

View Options

sys/arm64/arm64/exec_machdep.c

  • This file was added.
/*-
* Copyright (c) 2013 Ruslan Bukin <br@bsdpad.com>
* Copyright (c) 2013 Semihalf
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
* are met:
* 1. Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* 2. Redistributions in binary form must reproduce the above copyright
* notice, this list of conditions and the following disclaimer in the
* documentation and/or other materials provided with the distribution.
*
* THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
* ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
* OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE.
*
*/
#include <sys/cdefs.h>
__FBSDID("$FreeBSD$");
#include <sys/param.h>
#include <sys/systm.h>
#include <sys/bus.h>
#include <sys/lock.h>
#include <sys/mutex.h>
#include <sys/smp.h>
#include <sys/cpuset.h>
#include <machine/smp.h>
#include <machine/fdt.h>
#include <machine/intr.h>
#include <dev/fdt/fdt_common.h>
#define AL_CPU_RESUME_WATERMARK_REG 0x00
#define AL_CPU_RESUME_FLAGS_REG 0x04
#define AL_CPU_RESUME_PCPU_RADDR_REG(cpu) (0x08 + 0x04 + 8*(cpu))
#define AL_CPU_RESUME_PCPU_FLAGS(cpu) (0x08 + 8*(cpu))
/* Per-CPU flags */
#define AL_CPU_RESUME_FLG_PERCPU_DONT_RESUME (1 << 2)
/* The expected magic number for validating the resume addresses */
#define AL_CPU_RESUME_MAGIC_NUM 0xf0e1d200
#define AL_CPU_RESUME_MAGIC_NUM_MASK 0xffffff00
/* The expected minimal version number for validating the capabilities */
#define AL_CPU_RESUME_MIN_VER 0x000000c3
#define AL_CPU_RESUME_MIN_VER_MASK 0x000000ff
/* Field controlling the boot-up of companion cores */
#define AL_NB_INIT_CONTROL (0x8)
#define AL_NB_CONFIG_STATUS_PWR_CTRL(cpu) (0x2020 + (cpu)*0x100)
extern void mpentry(void);
int alpine_serdes_resource_get(uint32_t group, bus_space_tag_t *tag,
bus_addr_t *baddr);
static int platform_mp_get_core_cnt(void);
static int alpine_get_cpu_resume_base(uint32_t *pbase, uint32_t *psize);
static int alpine_get_nb_base(uint32_t *pbase, uint32_t *psize);
static int alpine_get_serdes_base(uint32_t *pbase, uint32_t *psize);
int alpine_serdes_resource_get(uint32_t group, bus_space_tag_t *tag,
bus_addr_t *baddr);
static int
platform_mp_get_core_cnt(void)
{
static int ncores = 0;
phandle_t node;
phandle_t child;
int nchilds;
/* Calculate ncores value only once */
impUnsubmitted
Done
Inline Actions

Shouldn't this be lower case?

imp: Shouldn't this be lower case?
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

While this is case insensitive comparison you are right that the practice is lower case. Will fix it.

jpa-semihalf.com: While this is case insensitive comparison you are right that the practice is lower case. Will…
if (ncores)
return (ncores);
__asm __volatile("mrc p15, 1, %0, c9, c0, 2\n" : "=r" (ncores));
ncores = ((ncores >> 24) & 3) + 1;
andrewUnsubmitted
Done
Inline Actions

This needs a newline:

boolean_t
alpine_validate_cpu(...)

Should it be static?

andrew: This needs a newline: boolean_t alpine_validate_cpu(...) Should it be static?
/* Check if number of CPU cores should be limited in SMP mode */
andrewUnsubmitted
Done
Inline Actions

You should use ofw_cpu_early_foreach for this.

andrew: You should use ofw_cpu_early_foreach for this.
if ((node = OF_finddevice("/cpus")) == -1)
goto end;
nchilds = 0;
for (child = OF_child(node); child != 0; child = OF_peer(child)) {
if (fdt_is_compatible(child, "ARM,CORTEX-A15")) {
andrewUnsubmitted
Done
Inline Actions

This should use a function from cpu-v6.h

andrew: This should use a function from cpu-v6.h
nchilds++;
andrewUnsubmitted
Done
Inline Actions

You should add something like the following to armreg.h, then use it here.

#define L2CTLR_NPROC_SHIFT 24
#define L2CTLR_NPROC(r) ((((r) >> L2CTLR_NPROC_SHIFT) & 3) + 1)
andrew: You should add something like the following to armreg.h, then use it here. #define…
}
}
/* Limit CPUs if DTS has configured less than available */
if ((nchilds > 0) && (nchilds < ncores)) {
printf("SMP: limiting number of active CPUs to %d out of %d\n",
nchilds, ncores);
ncores = nchilds;
}
ianUnsubmitted
Done
Inline Actions

the number of cores should also be limited by the hw.ncpu tunable so that you can control it from loader(8).

ian: the number of cores should also be limited by the hw.ncpu tunable so that you can control it…
jpa-semihalf.comAuthorUnsubmitted
Not Done
Inline Actions

We are not supporting loader(8) so that would be dead code.

jpa-semihalf.com: We are not supporting loader(8) so that would be dead code.
ianUnsubmitted
Done
Inline Actions

I don't view "we are not supporting loader(8)" as a valid position without some sort of really strong justification. It's not 2011 anymore where we commit code for a new platform as soon as it can boot a bare kernel.bin that supports a serial console.

loader(8) is part of freebsd and is the standard way to load a kernel. why do you claim to not support it?

ian: I don't view "we are not supporting loader(8)" as a valid position without some sort of really…
impUnsubmitted
Done
Inline Actions

These boards are embedded network processors that live on a PCIe card. They have very limited space for extra stuff not related to the task at hand... While I generally agree we need loader(8) support, this board may be a reasonable exception to that rule.

imp: These boards are embedded network processors that live on a PCIe card. They have very limited…
ianUnsubmitted
Done
Inline Actions

Ah, I didn't realize these were not general-purpose systems oriented towards the end user. For that case, compiling in the dtb and skipping loader(8) seems reasonable.

ian: Ah, I didn't realize these were not general-purpose systems oriented towards the end user. For…
end:
return (ncores);
}
void
platform_mp_init_secondary(void)
{
arm_init_secondary_ic();
}
void
platform_mp_setmaxid(void)
{
int core_cnt;
core_cnt = platform_mp_get_core_cnt();
mp_maxid = core_cnt - 1;
}
int
platform_mp_probe(void)
{
mp_ncpus = platform_mp_get_core_cnt();
return (1);
}
static int
alpine_get_cpu_resume_base(uint32_t *pbase, uint32_t *psize)
{
phandle_t node;
u_long base=0;
u_long size=0;
if (!pbase || !psize)
impUnsubmitted
Not Done
Inline Actions

Shouldn't this be a kassert?

imp: Shouldn't this be a kassert?
jpa-semihalf.comAuthorUnsubmitted
Not Done
Inline Actions

In general that would be the way to go, but other errors are handled with same panic message anyway, so I would rather not duplicate code unless necessary.

jpa-semihalf.com: In general that would be the way to go, but other errors are handled with same panic message…
return (EINVAL);
if ((node = OF_finddevice("/")) == -1)
return (EFAULT);
andrewUnsubmitted
Done
Inline Actions

Do these need to be fdt_ functions, or could you call an ofw_ function?

andrew: Do these need to be fdt_ functions, or could you call an ofw_ function?
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

I don't have device yet, so the alternative option is to use pure OF_ set of functions to find one, for example wrapped into ofw_search_tree_compatible_device("/simple-bus/al-cpu-resume");

jpa-semihalf.com: I don't have device yet, so the alternative option is to use pure OF_ set of functions to find…
if ((node = fdt_find_compatible(node, "simple-bus", 1)) == 0)
return (EFAULT);
if ((node =
fdt_find_compatible(node, "annapurna-labs,al-cpu-resume", 1)) == 0)
return (EFAULT);
if (fdt_regsize(node, &base, &size))
return (EFAULT);
*pbase = base;
*psize = size;
return (0);
}
static int
alpine_get_nb_base(uint32_t *pbase, uint32_t *psize)
{
phandle_t node;
u_long base=0;
u_long size=0;
if (!pbase || !psize)
impUnsubmitted
Done
Inline Actions

kassert?

imp: kassert?
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

Same as above.

jpa-semihalf.com: Same as above.
return (EINVAL);
if ((node = OF_finddevice("/")) == -1)
andrewUnsubmitted
Done
Inline Actions

Missing space, should be base = 0;, and the same for size.

andrew: Missing space, should be `base = 0;`, and the same for size.
return (EFAULT);
andrewUnsubmitted
Done
Inline Actions
if (pbase == NULL || psize == NULL)
andrew: if (pbase == NULL || psize == NULL)
if ((node = fdt_find_compatible(node, "simple-bus", 1)) == 0)
return (EFAULT);
if ((node =
fdt_find_compatible(node, "annapurna-labs,al-nb-service", 1)) == 0)
return (EFAULT);
if (fdt_regsize(node, &base, &size))
return (EFAULT);
*pbase = base;
*psize = size;
return (0);
}
void
platform_mp_start_ap(void)
{
uint32_t physaddr;
vm_offset_t vaddr;
uint32_t val;
uint32_t start_mask;
uint32_t cpu_resume_base;
uint32_t nb_base;
andrewUnsubmitted
Done
Inline Actions

uint32_t is the wrong type for a number of these.

andrew: uint32_t is the wrong type for a number of these.
uint32_t cpu_resume_size;
uint32_t nb_size;
bus_addr_t cpu_resume_baddr;
bus_addr_t nb_baddr;
int a;
if (alpine_get_cpu_resume_base(&cpu_resume_base, &cpu_resume_size))
panic("Couldn't resolve cpu_resume_base address\n");
if (alpine_get_nb_base(&nb_base, &nb_size))
panic("Couldn't resolve_nb_base address\n");
/* Proceed with start addresses for additional CPUs */
ianUnsubmitted
Done
Inline Actions

Unless this platform has mapping control registers that allow repositioning the window for accessing hardware regs, none of these bus_space_map() calls should be using fdt_immr_pa, they should just be using the physical addresses from the fdt data.

As far as I know, only Marvel chips have such a feature (and maybe not even all of them). fdt_immr_pa should never appear outside of arm/mv (and some polution where it leaks out into dev/fdt that we really need to fix).

ian: Unless this platform has mapping control registers that allow repositioning the window for…
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

I removed all references to fdt_immr_pa and used the preferred way of setting addresses (arm_devmap_add_entry())

jpa-semihalf.com: I removed all references to fdt_immr_pa and used the preferred way of setting addresses…
if (bus_space_map(fdtbus_bs_tag, fdt_immr_pa + cpu_resume_base,
cpu_resume_size, 0, &cpu_resume_baddr))
panic("Couldn't map CPU-resume area");
if (bus_space_map(fdtbus_bs_tag, fdt_immr_pa + nb_base,
nb_size, 0, &nb_baddr))
panic("Couldn't map NB-service area");
/* Proceed with start addresses for additional CPUs */
val = bus_space_read_4(fdtbus_bs_tag, cpu_resume_baddr,
AL_CPU_RESUME_WATERMARK_REG);
if (((val & AL_CPU_RESUME_MAGIC_NUM_MASK) != AL_CPU_RESUME_MAGIC_NUM) ||
((val & AL_CPU_RESUME_MIN_VER_MASK) < AL_CPU_RESUME_MIN_VER)) {
panic("CPU-resume device is not compatible");
}
vaddr = (vm_offset_t)mpentry;
physaddr = pmap_kextract(vaddr);
for (a = 1; a < platform_mp_get_core_cnt(); a++) {
/* Power up the core */
bus_space_write_4(fdtbus_bs_tag, nb_baddr,
AL_NB_CONFIG_STATUS_PWR_CTRL(a), 0);
mb();
/* Enable resume */
val = bus_space_read_4(fdtbus_bs_tag, cpu_resume_baddr,
AL_CPU_RESUME_PCPU_FLAGS(a));
val &= ~AL_CPU_RESUME_FLG_PERCPU_DONT_RESUME;
bus_space_write_4(fdtbus_bs_tag, cpu_resume_baddr,
AL_CPU_RESUME_PCPU_FLAGS(a), val);
mb();
/* Set resume physical address */
bus_space_write_4(fdtbus_bs_tag, cpu_resume_baddr,
AL_CPU_RESUME_PCPU_RADDR_REG(a), physaddr);
mb();
}
/* Release cores from reset */
if (bus_space_map(fdtbus_bs_tag, fdt_immr_pa + nb_base,
nb_size, 0, &nb_baddr))
panic("Couldn't map NB-service area");
start_mask = (1 << platform_mp_get_core_cnt()) - 1;
/* Release cores from reset */
val = bus_space_read_4(fdtbus_bs_tag, nb_baddr, AL_NB_INIT_CONTROL);
val |= start_mask;
andrewUnsubmitted
Done
Inline Actions

Is a dmb enough here?

andrew: Is a dmb enough here?
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

armv7_sev() issues dsb internally, so I think it is even not needed here, but for the sake of future changes to armv7_sev() I will replace the mb() accordingly.

jpa-semihalf.com: armv7_sev() issues dsb internally, so I think it is even not needed here, but for the sake of…
bus_space_write_4(fdtbus_bs_tag, nb_baddr, AL_NB_INIT_CONTROL, val);
ianUnsubmitted
Done
Inline Actions

Why do we even do a SEV here? I have the feeling this has been pasted into every soc mp file and doesn't do anything useful on any of them.

ian: Why do we even do a SEV here? I have the feeling this has been pasted into every soc mp file…
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

I removed it and so far so good. On the other hand I kept the dsb().

jpa-semihalf.com: I removed it and so far so good. On the other hand I kept the dsb().
mb();
armv7_sev();
bus_space_unmap(fdtbus_bs_tag, nb_baddr, nb_size);
bus_space_unmap(fdtbus_bs_tag, cpu_resume_baddr, cpu_resume_size);
}
void
platform_ipi_send(cpuset_t cpus, u_int ipi)
{
pic_ipi_send(cpus, ipi);
}
andrewUnsubmitted
Done
Inline Actions

This has the same style issues as alpine_get_nb_base.

andrew: This has the same style issues as alpine_get_nb_base.
View Options

sys/arm64/arm64/freebsd32_machdep.c

  • This file was added.
/*-
* Copyright (c) 2013 Ruslan Bukin <br@bsdpad.com>
* Copyright (c) 2013 Semihalf
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
* are met:
* 1. Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
* 2. Redistributions in binary form must reproduce the above copyright
* notice, this list of conditions and the following disclaimer in the
* documentation and/or other materials provided with the distribution.
*
* THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
* ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
* OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE.
*
*/
#include <sys/cdefs.h>
__FBSDID("$FreeBSD$");
#include <sys/param.h>
#include <sys/systm.h>
#include <sys/bus.h>
#include <sys/lock.h>
#include <sys/mutex.h>
#include <sys/smp.h>
#include <sys/cpuset.h>
#include <machine/smp.h>
#include <machine/fdt.h>
#include <machine/intr.h>
#include <dev/fdt/fdt_common.h>
#define AL_CPU_RESUME_WATERMARK_REG 0x00
#define AL_CPU_RESUME_FLAGS_REG 0x04
#define AL_CPU_RESUME_PCPU_RADDR_REG(cpu) (0x08 + 0x04 + 8*(cpu))
#define AL_CPU_RESUME_PCPU_FLAGS(cpu) (0x08 + 8*(cpu))
/* Per-CPU flags */
#define AL_CPU_RESUME_FLG_PERCPU_DONT_RESUME (1 << 2)
/* The expected magic number for validating the resume addresses */
#define AL_CPU_RESUME_MAGIC_NUM 0xf0e1d200
#define AL_CPU_RESUME_MAGIC_NUM_MASK 0xffffff00
/* The expected minimal version number for validating the capabilities */
#define AL_CPU_RESUME_MIN_VER 0x000000c3
#define AL_CPU_RESUME_MIN_VER_MASK 0x000000ff
/* Field controlling the boot-up of companion cores */
#define AL_NB_INIT_CONTROL (0x8)
#define AL_NB_CONFIG_STATUS_PWR_CTRL(cpu) (0x2020 + (cpu)*0x100)
extern void mpentry(void);
int alpine_serdes_resource_get(uint32_t group, bus_space_tag_t *tag,
bus_addr_t *baddr);
static int platform_mp_get_core_cnt(void);
static int alpine_get_cpu_resume_base(uint32_t *pbase, uint32_t *psize);
static int alpine_get_nb_base(uint32_t *pbase, uint32_t *psize);
static int alpine_get_serdes_base(uint32_t *pbase, uint32_t *psize);
int alpine_serdes_resource_get(uint32_t group, bus_space_tag_t *tag,
bus_addr_t *baddr);
static int
platform_mp_get_core_cnt(void)
{
static int ncores = 0;
phandle_t node;
phandle_t child;
int nchilds;
/* Calculate ncores value only once */
impUnsubmitted
Done
Inline Actions

Shouldn't this be lower case?

imp: Shouldn't this be lower case?
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

While this is case insensitive comparison you are right that the practice is lower case. Will fix it.

jpa-semihalf.com: While this is case insensitive comparison you are right that the practice is lower case. Will…
if (ncores)
return (ncores);
__asm __volatile("mrc p15, 1, %0, c9, c0, 2\n" : "=r" (ncores));
ncores = ((ncores >> 24) & 3) + 1;
andrewUnsubmitted
Done
Inline Actions

This needs a newline:

boolean_t
alpine_validate_cpu(...)

Should it be static?

andrew: This needs a newline: boolean_t alpine_validate_cpu(...) Should it be static?
/* Check if number of CPU cores should be limited in SMP mode */
andrewUnsubmitted
Done
Inline Actions

You should use ofw_cpu_early_foreach for this.

andrew: You should use ofw_cpu_early_foreach for this.
if ((node = OF_finddevice("/cpus")) == -1)
goto end;
nchilds = 0;
for (child = OF_child(node); child != 0; child = OF_peer(child)) {
if (fdt_is_compatible(child, "ARM,CORTEX-A15")) {
andrewUnsubmitted
Done
Inline Actions

This should use a function from cpu-v6.h

andrew: This should use a function from cpu-v6.h
nchilds++;
andrewUnsubmitted
Done
Inline Actions

You should add something like the following to armreg.h, then use it here.

#define L2CTLR_NPROC_SHIFT 24
#define L2CTLR_NPROC(r) ((((r) >> L2CTLR_NPROC_SHIFT) & 3) + 1)
andrew: You should add something like the following to armreg.h, then use it here. #define…
}
}
/* Limit CPUs if DTS has configured less than available */
if ((nchilds > 0) && (nchilds < ncores)) {
printf("SMP: limiting number of active CPUs to %d out of %d\n",
nchilds, ncores);
ncores = nchilds;
}
ianUnsubmitted
Done
Inline Actions

the number of cores should also be limited by the hw.ncpu tunable so that you can control it from loader(8).

ian: the number of cores should also be limited by the hw.ncpu tunable so that you can control it…
jpa-semihalf.comAuthorUnsubmitted
Not Done
Inline Actions

We are not supporting loader(8) so that would be dead code.

jpa-semihalf.com: We are not supporting loader(8) so that would be dead code.
ianUnsubmitted
Done
Inline Actions

I don't view "we are not supporting loader(8)" as a valid position without some sort of really strong justification. It's not 2011 anymore where we commit code for a new platform as soon as it can boot a bare kernel.bin that supports a serial console.

loader(8) is part of freebsd and is the standard way to load a kernel. why do you claim to not support it?

ian: I don't view "we are not supporting loader(8)" as a valid position without some sort of really…
impUnsubmitted
Done
Inline Actions

These boards are embedded network processors that live on a PCIe card. They have very limited space for extra stuff not related to the task at hand... While I generally agree we need loader(8) support, this board may be a reasonable exception to that rule.

imp: These boards are embedded network processors that live on a PCIe card. They have very limited…
ianUnsubmitted
Done
Inline Actions

Ah, I didn't realize these were not general-purpose systems oriented towards the end user. For that case, compiling in the dtb and skipping loader(8) seems reasonable.

ian: Ah, I didn't realize these were not general-purpose systems oriented towards the end user. For…
end:
return (ncores);
}
void
platform_mp_init_secondary(void)
{
arm_init_secondary_ic();
}
void
platform_mp_setmaxid(void)
{
int core_cnt;
core_cnt = platform_mp_get_core_cnt();
mp_maxid = core_cnt - 1;
}
int
platform_mp_probe(void)
{
mp_ncpus = platform_mp_get_core_cnt();
return (1);
}
static int
alpine_get_cpu_resume_base(uint32_t *pbase, uint32_t *psize)
{
phandle_t node;
u_long base=0;
u_long size=0;
if (!pbase || !psize)
impUnsubmitted
Not Done
Inline Actions

Shouldn't this be a kassert?

imp: Shouldn't this be a kassert?
jpa-semihalf.comAuthorUnsubmitted
Not Done
Inline Actions

In general that would be the way to go, but other errors are handled with same panic message anyway, so I would rather not duplicate code unless necessary.

jpa-semihalf.com: In general that would be the way to go, but other errors are handled with same panic message…
return (EINVAL);
if ((node = OF_finddevice("/")) == -1)
return (EFAULT);
andrewUnsubmitted
Done
Inline Actions

Do these need to be fdt_ functions, or could you call an ofw_ function?

andrew: Do these need to be fdt_ functions, or could you call an ofw_ function?
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

I don't have device yet, so the alternative option is to use pure OF_ set of functions to find one, for example wrapped into ofw_search_tree_compatible_device("/simple-bus/al-cpu-resume");

jpa-semihalf.com: I don't have device yet, so the alternative option is to use pure OF_ set of functions to find…
if ((node = fdt_find_compatible(node, "simple-bus", 1)) == 0)
return (EFAULT);
if ((node =
fdt_find_compatible(node, "annapurna-labs,al-cpu-resume", 1)) == 0)
return (EFAULT);
if (fdt_regsize(node, &base, &size))
return (EFAULT);
*pbase = base;
*psize = size;
return (0);
}
static int
alpine_get_nb_base(uint32_t *pbase, uint32_t *psize)
{
phandle_t node;
u_long base=0;
u_long size=0;
if (!pbase || !psize)
impUnsubmitted
Done
Inline Actions

kassert?

imp: kassert?
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

Same as above.

jpa-semihalf.com: Same as above.
return (EINVAL);
if ((node = OF_finddevice("/")) == -1)
andrewUnsubmitted
Done
Inline Actions

Missing space, should be base = 0;, and the same for size.

andrew: Missing space, should be `base = 0;`, and the same for size.
return (EFAULT);
andrewUnsubmitted
Done
Inline Actions
if (pbase == NULL || psize == NULL)
andrew: if (pbase == NULL || psize == NULL)
if ((node = fdt_find_compatible(node, "simple-bus", 1)) == 0)
return (EFAULT);
if ((node =
fdt_find_compatible(node, "annapurna-labs,al-nb-service", 1)) == 0)
return (EFAULT);
if (fdt_regsize(node, &base, &size))
return (EFAULT);
*pbase = base;
*psize = size;
return (0);
}
void
platform_mp_start_ap(void)
{
uint32_t physaddr;
vm_offset_t vaddr;
uint32_t val;
uint32_t start_mask;
uint32_t cpu_resume_base;
uint32_t nb_base;
andrewUnsubmitted
Done
Inline Actions

uint32_t is the wrong type for a number of these.

andrew: uint32_t is the wrong type for a number of these.
uint32_t cpu_resume_size;
uint32_t nb_size;
bus_addr_t cpu_resume_baddr;
bus_addr_t nb_baddr;
int a;
if (alpine_get_cpu_resume_base(&cpu_resume_base, &cpu_resume_size))
panic("Couldn't resolve cpu_resume_base address\n");
if (alpine_get_nb_base(&nb_base, &nb_size))
panic("Couldn't resolve_nb_base address\n");
/* Proceed with start addresses for additional CPUs */
ianUnsubmitted
Done
Inline Actions

Unless this platform has mapping control registers that allow repositioning the window for accessing hardware regs, none of these bus_space_map() calls should be using fdt_immr_pa, they should just be using the physical addresses from the fdt data.

As far as I know, only Marvel chips have such a feature (and maybe not even all of them). fdt_immr_pa should never appear outside of arm/mv (and some polution where it leaks out into dev/fdt that we really need to fix).

ian: Unless this platform has mapping control registers that allow repositioning the window for…
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

I removed all references to fdt_immr_pa and used the preferred way of setting addresses (arm_devmap_add_entry())

jpa-semihalf.com: I removed all references to fdt_immr_pa and used the preferred way of setting addresses…
if (bus_space_map(fdtbus_bs_tag, fdt_immr_pa + cpu_resume_base,
cpu_resume_size, 0, &cpu_resume_baddr))
panic("Couldn't map CPU-resume area");
if (bus_space_map(fdtbus_bs_tag, fdt_immr_pa + nb_base,
nb_size, 0, &nb_baddr))
panic("Couldn't map NB-service area");
/* Proceed with start addresses for additional CPUs */
val = bus_space_read_4(fdtbus_bs_tag, cpu_resume_baddr,
AL_CPU_RESUME_WATERMARK_REG);
if (((val & AL_CPU_RESUME_MAGIC_NUM_MASK) != AL_CPU_RESUME_MAGIC_NUM) ||
((val & AL_CPU_RESUME_MIN_VER_MASK) < AL_CPU_RESUME_MIN_VER)) {
panic("CPU-resume device is not compatible");
}
vaddr = (vm_offset_t)mpentry;
physaddr = pmap_kextract(vaddr);
for (a = 1; a < platform_mp_get_core_cnt(); a++) {
/* Power up the core */
bus_space_write_4(fdtbus_bs_tag, nb_baddr,
AL_NB_CONFIG_STATUS_PWR_CTRL(a), 0);
mb();
/* Enable resume */
val = bus_space_read_4(fdtbus_bs_tag, cpu_resume_baddr,
AL_CPU_RESUME_PCPU_FLAGS(a));
val &= ~AL_CPU_RESUME_FLG_PERCPU_DONT_RESUME;
bus_space_write_4(fdtbus_bs_tag, cpu_resume_baddr,
AL_CPU_RESUME_PCPU_FLAGS(a), val);
mb();
/* Set resume physical address */
bus_space_write_4(fdtbus_bs_tag, cpu_resume_baddr,
AL_CPU_RESUME_PCPU_RADDR_REG(a), physaddr);
mb();
}
/* Release cores from reset */
if (bus_space_map(fdtbus_bs_tag, fdt_immr_pa + nb_base,
nb_size, 0, &nb_baddr))
panic("Couldn't map NB-service area");
start_mask = (1 << platform_mp_get_core_cnt()) - 1;
/* Release cores from reset */
val = bus_space_read_4(fdtbus_bs_tag, nb_baddr, AL_NB_INIT_CONTROL);
val |= start_mask;
andrewUnsubmitted
Done
Inline Actions

Is a dmb enough here?

andrew: Is a dmb enough here?
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

armv7_sev() issues dsb internally, so I think it is even not needed here, but for the sake of future changes to armv7_sev() I will replace the mb() accordingly.

jpa-semihalf.com: armv7_sev() issues dsb internally, so I think it is even not needed here, but for the sake of…
bus_space_write_4(fdtbus_bs_tag, nb_baddr, AL_NB_INIT_CONTROL, val);
ianUnsubmitted
Done
Inline Actions

Why do we even do a SEV here? I have the feeling this has been pasted into every soc mp file and doesn't do anything useful on any of them.

ian: Why do we even do a SEV here? I have the feeling this has been pasted into every soc mp file…
jpa-semihalf.comAuthorUnsubmitted
Done
Inline Actions

I removed it and so far so good. On the other hand I kept the dsb().

jpa-semihalf.com: I removed it and so far so good. On the other hand I kept the dsb().
mb();
armv7_sev();
bus_space_unmap(fdtbus_bs_tag, nb_baddr, nb_size);
bus_space_unmap(fdtbus_bs_tag, cpu_resume_baddr, cpu_resume_size);
}
void
platform_ipi_send(cpuset_t cpus, u_int ipi)
{
pic_ipi_send(cpus, ipi);
}
andrewUnsubmitted
Done
Inline Actions

This has the same style issues as alpine_get_nb_base.

andrew: This has the same style issues as alpine_get_nb_base.