| Message ID | 1549590681-24125-9-git-send-email-atish.patra@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Various SMP related fixes | expand |
> + * We don't support running Linux on hertergenous ISA systems. > + * But first "okay" processor might not be the boot cpu. > + * Check the ISA of boot cpu. Please use up your available 80 characters per line in comments. > + /* > + * All "okay" hart should have same isa. We don't know how to > + * handle if they don't. Throw a warning for now. > + */ > + if (elf_hwcap && temp_hwcap != elf_hwcap) > + pr_warn("isa mismatch: 0x%lx != 0x%lx\n", > + elf_hwcap, temp_hwcap); > + > + if (hartid == boot_cpu_hartid) > + boot_hwcap = temp_hwcap; > + elf_hwcap = temp_hwcap; So we always set elf_hwcap to the capabilities of the previous cpu. > + temp_hwcap = 0; I think tmp_hwcap should be declared and initialized inside the outer loop instead having to manually reset it like this. > + } > > + elf_hwcap = boot_hwcap; And then reset it here to the boot cpu. Shoudn't we only report the features supported by all cores? Otherwise we'll still have problems if the boot cpu supports a feature, but not others. Something like: for () { unsigned long this_hwcap = 0; for (i = 0; i < strlen(isa); i++) this_hwcap |= isa2hwcap[(unsigned char)(isa[i])]; if (elf_hwcap) elf_hwcap &= this_hwcap; else elf_hwcap = this_hwcap; }
On 2/8/19 1:11 AM, Christoph Hellwig wrote: >> + * We don't support running Linux on hertergenous ISA systems. >> + * But first "okay" processor might not be the boot cpu. >> + * Check the ISA of boot cpu. > > Please use up your available 80 characters per line in comments. > I will fix it. >> + /* >> + * All "okay" hart should have same isa. We don't know how to >> + * handle if they don't. Throw a warning for now. >> + */ >> + if (elf_hwcap && temp_hwcap != elf_hwcap) >> + pr_warn("isa mismatch: 0x%lx != 0x%lx\n", >> + elf_hwcap, temp_hwcap); >> + >> + if (hartid == boot_cpu_hartid) >> + boot_hwcap = temp_hwcap; >> + elf_hwcap = temp_hwcap; > > So we always set elf_hwcap to the capabilities of the previous cpu. > >> + temp_hwcap = 0; > > I think tmp_hwcap should be declared and initialized inside the outer loop > instead having to manually reset it like this. > >> + } >> >> + elf_hwcap = boot_hwcap; > > And then reset it here to the boot cpu. > > Shoudn't we only report the features supported by all cores? Otherwise > we'll still have problems if the boot cpu supports a feature, but not > others. > Hmm. The other side of the argument is boot cpu does have a feature that is not supported by other hart that didn't even boot. The user space may execute something based on boot cpu capability but that won't be enabled. At least, in this way we know that we are compatible completely with boot cpu capabilities. Thoughts ? Regards, Atish > Something like: > > for () { > unsigned long this_hwcap = 0; > > for (i = 0; i < strlen(isa); i++) > this_hwcap |= isa2hwcap[(unsigned char)(isa[i])]; > > if (elf_hwcap) > elf_hwcap &= this_hwcap; > else > elf_hwcap = this_hwcap; > } > >
On Sat, Feb 9, 2019 at 12:03 AM Atish Patra <atish.patra@wdc.com> wrote: > > On 2/8/19 1:11 AM, Christoph Hellwig wrote: > >> + * We don't support running Linux on hertergenous ISA systems. > >> + * But first "okay" processor might not be the boot cpu. > >> + * Check the ISA of boot cpu. > > > > Please use up your available 80 characters per line in comments. > > > I will fix it. > > >> + /* > >> + * All "okay" hart should have same isa. We don't know how to > >> + * handle if they don't. Throw a warning for now. > >> + */ > >> + if (elf_hwcap && temp_hwcap != elf_hwcap) > >> + pr_warn("isa mismatch: 0x%lx != 0x%lx\n", > >> + elf_hwcap, temp_hwcap); > >> + > >> + if (hartid == boot_cpu_hartid) > >> + boot_hwcap = temp_hwcap; > >> + elf_hwcap = temp_hwcap; > > > > So we always set elf_hwcap to the capabilities of the previous cpu. > > > >> + temp_hwcap = 0; > > > > I think tmp_hwcap should be declared and initialized inside the outer loop > > instead having to manually reset it like this. > > > >> + } > >> > >> + elf_hwcap = boot_hwcap; > > > > And then reset it here to the boot cpu. > > > > Shoudn't we only report the features supported by all cores? Otherwise > > we'll still have problems if the boot cpu supports a feature, but not > > others. > > > > Hmm. The other side of the argument is boot cpu does have a feature that > is not supported by other hart that didn't even boot. > The user space may execute something based on boot cpu capability but > that won't be enabled. > > At least, in this way we know that we are compatible completely with > boot cpu capabilities. Thoughts ? There is one example on the market, e.g., Samsung Exynos 9810. Mongoose 3 (big cores) only support ARMv8.0, while Cortex-A55 (little ones) support ARMv8.2 (and that brings atomics support). I think, it's the only ARM SOC that supports different ISA extensions between cores on the same package. Kernel scheduler doesn't know that big cores are missing atomics support or that applications needs it and moves the thread resulting in illegal instruction. E.g., see Golang issue: https://github.com/golang/go/issues/28431 I also recall Jon Masters (Computer Architect at Red Hat) advocating against having cores with mismatched capabilities on the server market. It just causes more problems down the line. david
On Sat, 09 Feb 2019 04:26:07 +0000, David Abdurachmanov <david.abdurachmanov@gmail.com> wrote: > > On Sat, Feb 9, 2019 at 12:03 AM Atish Patra <atish.patra@wdc.com> wrote: > > > > On 2/8/19 1:11 AM, Christoph Hellwig wrote: > > >> + * We don't support running Linux on hertergenous ISA systems. > > >> + * But first "okay" processor might not be the boot cpu. > > >> + * Check the ISA of boot cpu. > > > > > > Please use up your available 80 characters per line in comments. > > > > > I will fix it. > > > > >> + /* > > >> + * All "okay" hart should have same isa. We don't know how to > > >> + * handle if they don't. Throw a warning for now. > > >> + */ > > >> + if (elf_hwcap && temp_hwcap != elf_hwcap) > > >> + pr_warn("isa mismatch: 0x%lx != 0x%lx\n", > > >> + elf_hwcap, temp_hwcap); > > >> + > > >> + if (hartid == boot_cpu_hartid) > > >> + boot_hwcap = temp_hwcap; > > >> + elf_hwcap = temp_hwcap; > > > > > > So we always set elf_hwcap to the capabilities of the previous cpu. > > > > > >> + temp_hwcap = 0; > > > > > > I think tmp_hwcap should be declared and initialized inside the outer loop > > > instead having to manually reset it like this. > > > > > >> + } > > >> > > >> + elf_hwcap = boot_hwcap; > > > > > > And then reset it here to the boot cpu. > > > > > > Shoudn't we only report the features supported by all cores? Otherwise > > > we'll still have problems if the boot cpu supports a feature, but not > > > others. > > > > > > > Hmm. The other side of the argument is boot cpu does have a feature that > > is not supported by other hart that didn't even boot. > > The user space may execute something based on boot cpu capability but > > that won't be enabled. > > > > At least, in this way we know that we are compatible completely with > > boot cpu capabilities. Thoughts ? > > There is one example on the market, e.g., Samsung Exynos 9810. > > Mongoose 3 (big cores) only support ARMv8.0, while Cortex-A55 > (little ones) support ARMv8.2 (and that brings atomics support). > I think, it's the only ARM SOC that supports different ISA extensions > between cores on the same package. > > Kernel scheduler doesn't know that big cores are missing atomics > support or that applications needs it and moves the thread > resulting in illegal instruction. Not quite. The scheduler doesn't have to know (thankfully). The problem is that the Samsung folks tampered with the detection logic in the kernel, and ended up advertising the LSE atomics to userspace (despite only being available on half the cores). If you run a mainline kernel on this things, it will just work, as the LSE atomics are not advertised to userspace at all. > > E.g., see Golang issue: https://github.com/golang/go/issues/28431 > > I also recall Jon Masters (Computer Architect at Red Hat) advocating > against having cores with mismatched capabilities on the server > market. Well, nobody recommends that, server or not. That being said, it is possible to handle it, and the arm64 kernel has been dealing with such thing from day 1. We can have CPUs with different PMUs, implemented page sizes, VA and PA spaces... What it takes is some work in the kernel to sanitize it, and be careful in what you expose to userspace. The thing to realise is that people will build stupid systems, no matter how loud you shout. You can either pretend they don't exist, or try to deal with them. Thanks, M.
On Feb 07 2019, Atish Patra <atish.patra@wdc.com> wrote: > + while ((node = of_find_node_by_type(node, "cpu"))) { > + if (!node) { That can never be true. > + pr_warn("Unable to find \"cpu\" devicetree entry"); > + return; > + } > + > + hartid = riscv_of_processor_hartid(node); > + if (hartid < 0) > + continue; > > - if (of_property_read_string(node, "riscv,isa", &isa)) { > - pr_warning("Unable to find \"riscv,isa\" devicetree entry"); > + if (of_property_read_string(node, "riscv,isa", &isa)) { > + pr_warn("Unable to find \"riscv,isa\" devicetree entry"); > + of_node_put(node); > + return; > + } > of_node_put(node); [ 0.000000] OF: ERROR: Bad of_node_put() on /cpus/cpu@1 [ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 5.0.0-rc6-00020-g5903f30f1310 #12 [ 0.000000] Call Trace: [ 0.000000] [<ffffffe001076812>] walk_stackframe+0x0/0xa4 [ 0.000000] [<ffffffe001076a12>] show_stack+0x2a/0x34 [ 0.000000] [<ffffffe0015cf9ea>] dump_stack+0x62/0x7c [ 0.000000] [<ffffffe00149fed4>] of_node_release+0xbe/0xc0 [ 0.000000] [<ffffffe0015d465a>] kobject_put+0xa6/0x1e8 [ 0.000000] [<ffffffe00149f44e>] of_node_put+0x16/0x20 [ 0.000000] [<ffffffe00149b45e>] of_find_node_by_type+0x66/0xa4 [ 0.000000] [<ffffffe0010755ca>] riscv_fill_hwcap+0x14c/0x1ce [ 0.000000] [<ffffffe0000026d4>] 0xffffffe0000026d4 [ 0.000000] [<ffffffe0000006ec>] 0xffffffe0000006ec [ 0.000000] [<ffffffe000000076>] 0xffffffe000000076 Andreas.
On Fri, 08 Feb 2019 20:26:07 PST (-0800), david.abdurachmanov@gmail.com wrote: > On Sat, Feb 9, 2019 at 12:03 AM Atish Patra <atish.patra@wdc.com> wrote: >> >> On 2/8/19 1:11 AM, Christoph Hellwig wrote: >> >> + * We don't support running Linux on hertergenous ISA systems. >> >> + * But first "okay" processor might not be the boot cpu. >> >> + * Check the ISA of boot cpu. >> > >> > Please use up your available 80 characters per line in comments. >> > >> I will fix it. >> >> >> + /* >> >> + * All "okay" hart should have same isa. We don't know how to >> >> + * handle if they don't. Throw a warning for now. >> >> + */ >> >> + if (elf_hwcap && temp_hwcap != elf_hwcap) >> >> + pr_warn("isa mismatch: 0x%lx != 0x%lx\n", >> >> + elf_hwcap, temp_hwcap); >> >> + >> >> + if (hartid == boot_cpu_hartid) >> >> + boot_hwcap = temp_hwcap; >> >> + elf_hwcap = temp_hwcap; >> > >> > So we always set elf_hwcap to the capabilities of the previous cpu. >> > >> >> + temp_hwcap = 0; >> > >> > I think tmp_hwcap should be declared and initialized inside the outer loop >> > instead having to manually reset it like this. >> > >> >> + } >> >> >> >> + elf_hwcap = boot_hwcap; >> > >> > And then reset it here to the boot cpu. >> > >> > Shoudn't we only report the features supported by all cores? Otherwise >> > we'll still have problems if the boot cpu supports a feature, but not >> > others. >> > >> >> Hmm. The other side of the argument is boot cpu does have a feature that >> is not supported by other hart that didn't even boot. >> The user space may execute something based on boot cpu capability but >> that won't be enabled. >> >> At least, in this way we know that we are compatible completely with >> boot cpu capabilities. Thoughts ? > > There is one example on the market, e.g., Samsung Exynos 9810. > > Mongoose 3 (big cores) only support ARMv8.0, while Cortex-A55 > (little ones) support ARMv8.2 (and that brings atomics support). > I think, it's the only ARM SOC that supports different ISA extensions > between cores on the same package. > > Kernel scheduler doesn't know that big cores are missing atomics > support or that applications needs it and moves the thread > resulting in illegal instruction. > > E.g., see Golang issue: https://github.com/golang/go/issues/28431 > > I also recall Jon Masters (Computer Architect at Red Hat) advocating > against having cores with mismatched capabilities on the server market. > > It just causes more problems down the line. IMO the best bet is to only put extensions in HWCAP that are supported by all the harts that userspace will be scheduled on.
On 2/11/19 11:02 AM, Palmer Dabbelt wrote: > On Fri, 08 Feb 2019 20:26:07 PST (-0800), david.abdurachmanov@gmail.com wrote: >> On Sat, Feb 9, 2019 at 12:03 AM Atish Patra <atish.patra@wdc.com> wrote: >>> >>> On 2/8/19 1:11 AM, Christoph Hellwig wrote: >>>>> + * We don't support running Linux on hertergenous ISA systems. >>>>> + * But first "okay" processor might not be the boot cpu. >>>>> + * Check the ISA of boot cpu. >>>> >>>> Please use up your available 80 characters per line in comments. >>>> >>> I will fix it. >>> >>>>> + /* >>>>> + * All "okay" hart should have same isa. We don't know how to >>>>> + * handle if they don't. Throw a warning for now. >>>>> + */ >>>>> + if (elf_hwcap && temp_hwcap != elf_hwcap) >>>>> + pr_warn("isa mismatch: 0x%lx != 0x%lx\n", >>>>> + elf_hwcap, temp_hwcap); >>>>> + >>>>> + if (hartid == boot_cpu_hartid) >>>>> + boot_hwcap = temp_hwcap; >>>>> + elf_hwcap = temp_hwcap; >>>> >>>> So we always set elf_hwcap to the capabilities of the previous cpu. >>>> >>>>> + temp_hwcap = 0; >>>> >>>> I think tmp_hwcap should be declared and initialized inside the outer loop >>>> instead having to manually reset it like this. >>>> >>>>> + } >>>>> >>>>> + elf_hwcap = boot_hwcap; >>>> >>>> And then reset it here to the boot cpu. >>>> >>>> Shoudn't we only report the features supported by all cores? Otherwise >>>> we'll still have problems if the boot cpu supports a feature, but not >>>> others. >>>> >>> >>> Hmm. The other side of the argument is boot cpu does have a feature that >>> is not supported by other hart that didn't even boot. >>> The user space may execute something based on boot cpu capability but >>> that won't be enabled. >>> >>> At least, in this way we know that we are compatible completely with >>> boot cpu capabilities. Thoughts ? >> >> There is one example on the market, e.g., Samsung Exynos 9810. >> >> Mongoose 3 (big cores) only support ARMv8.0, while Cortex-A55 >> (little ones) support ARMv8.2 (and that brings atomics support). >> I think, it's the only ARM SOC that supports different ISA extensions >> between cores on the same package. >> >> Kernel scheduler doesn't know that big cores are missing atomics >> support or that applications needs it and moves the thread >> resulting in illegal instruction. >> >> E.g., see Golang issue: https://github.com/golang/go/issues/28431 >> >> I also recall Jon Masters (Computer Architect at Red Hat) advocating >> against having cores with mismatched capabilities on the server market. >> >> It just causes more problems down the line. > > IMO the best bet is to only put extensions in HWCAP that are supported by all > the harts that userspace will be scheduled on. > Fair enough. Instead of setting HWCAP in setup_arch() once, we can set it only for boot cpu. It will be updated after every cpu comes up online. Thus, HWCAP will consists all extensions supported by all cpus that are online currently. Regards, Atish
On Mon, 11 Feb 2019 12:03:30 -0800 Atish Patra <atish.patra@wdc.com> wrote: > On 2/11/19 11:02 AM, Palmer Dabbelt wrote: > > On Fri, 08 Feb 2019 20:26:07 PST (-0800), david.abdurachmanov@gmail.com wrote: > >> On Sat, Feb 9, 2019 at 12:03 AM Atish Patra <atish.patra@wdc.com> wrote: > >>> > >>> On 2/8/19 1:11 AM, Christoph Hellwig wrote: > >>>>> + * We don't support running Linux on hertergenous ISA systems. > >>>>> + * But first "okay" processor might not be the boot cpu. > >>>>> + * Check the ISA of boot cpu. > >>>> > >>>> Please use up your available 80 characters per line in comments. > >>>> > >>> I will fix it. > >>> > >>>>> + /* > >>>>> + * All "okay" hart should have same isa. We don't know how to > >>>>> + * handle if they don't. Throw a warning for now. > >>>>> + */ > >>>>> + if (elf_hwcap && temp_hwcap != elf_hwcap) > >>>>> + pr_warn("isa mismatch: 0x%lx != 0x%lx\n", > >>>>> + elf_hwcap, temp_hwcap); > >>>>> + > >>>>> + if (hartid == boot_cpu_hartid) > >>>>> + boot_hwcap = temp_hwcap; > >>>>> + elf_hwcap = temp_hwcap; > >>>> > >>>> So we always set elf_hwcap to the capabilities of the previous cpu. > >>>> > >>>>> + temp_hwcap = 0; > >>>> > >>>> I think tmp_hwcap should be declared and initialized inside the outer loop > >>>> instead having to manually reset it like this. > >>>> > >>>>> + } > >>>>> > >>>>> + elf_hwcap = boot_hwcap; > >>>> > >>>> And then reset it here to the boot cpu. > >>>> > >>>> Shoudn't we only report the features supported by all cores? Otherwise > >>>> we'll still have problems if the boot cpu supports a feature, but not > >>>> others. > >>>> > >>> > >>> Hmm. The other side of the argument is boot cpu does have a feature that > >>> is not supported by other hart that didn't even boot. > >>> The user space may execute something based on boot cpu capability but > >>> that won't be enabled. > >>> > >>> At least, in this way we know that we are compatible completely with > >>> boot cpu capabilities. Thoughts ? > >> > >> There is one example on the market, e.g., Samsung Exynos 9810. > >> > >> Mongoose 3 (big cores) only support ARMv8.0, while Cortex-A55 > >> (little ones) support ARMv8.2 (and that brings atomics support). > >> I think, it's the only ARM SOC that supports different ISA extensions > >> between cores on the same package. > >> > >> Kernel scheduler doesn't know that big cores are missing atomics > >> support or that applications needs it and moves the thread > >> resulting in illegal instruction. > >> > >> E.g., see Golang issue: https://github.com/golang/go/issues/28431 > >> > >> I also recall Jon Masters (Computer Architect at Red Hat) advocating > >> against having cores with mismatched capabilities on the server market. > >> > >> It just causes more problems down the line. > > > IMO the best bet is to only put extensions in HWCAP that are supported by all > > the harts that userspace will be scheduled on. > > Fair enough. Instead of setting HWCAP in setup_arch() once, we can set it only for boot cpu. It will be updated after every cpu comes up online. > > Thus, HWCAP will consists all extensions supported by all cpus that are online currently. You must thus prevent CPUs that have a different set of capabilities from coming up late (once userspace has started). M.
On Mon, 11 Feb 2019 14:13:25 PST (-0800), marc.zyngier@arm.com wrote: > On Mon, 11 Feb 2019 12:03:30 -0800 > Atish Patra <atish.patra@wdc.com> wrote: > >> On 2/11/19 11:02 AM, Palmer Dabbelt wrote: >> > On Fri, 08 Feb 2019 20:26:07 PST (-0800), david.abdurachmanov@gmail.com wrote: >> >> On Sat, Feb 9, 2019 at 12:03 AM Atish Patra <atish.patra@wdc.com> wrote: >> >>> >> >>> On 2/8/19 1:11 AM, Christoph Hellwig wrote: >> >>>>> + * We don't support running Linux on hertergenous ISA systems. >> >>>>> + * But first "okay" processor might not be the boot cpu. >> >>>>> + * Check the ISA of boot cpu. >> >>>> >> >>>> Please use up your available 80 characters per line in comments. >> >>>> >> >>> I will fix it. >> >>> >> >>>>> + /* >> >>>>> + * All "okay" hart should have same isa. We don't know how to >> >>>>> + * handle if they don't. Throw a warning for now. >> >>>>> + */ >> >>>>> + if (elf_hwcap && temp_hwcap != elf_hwcap) >> >>>>> + pr_warn("isa mismatch: 0x%lx != 0x%lx\n", >> >>>>> + elf_hwcap, temp_hwcap); >> >>>>> + >> >>>>> + if (hartid == boot_cpu_hartid) >> >>>>> + boot_hwcap = temp_hwcap; >> >>>>> + elf_hwcap = temp_hwcap; >> >>>> >> >>>> So we always set elf_hwcap to the capabilities of the previous cpu. >> >>>> >> >>>>> + temp_hwcap = 0; >> >>>> >> >>>> I think tmp_hwcap should be declared and initialized inside the outer loop >> >>>> instead having to manually reset it like this. >> >>>> >> >>>>> + } >> >>>>> >> >>>>> + elf_hwcap = boot_hwcap; >> >>>> >> >>>> And then reset it here to the boot cpu. >> >>>> >> >>>> Shoudn't we only report the features supported by all cores? Otherwise >> >>>> we'll still have problems if the boot cpu supports a feature, but not >> >>>> others. >> >>>> >> >>> >> >>> Hmm. The other side of the argument is boot cpu does have a feature that >> >>> is not supported by other hart that didn't even boot. >> >>> The user space may execute something based on boot cpu capability but >> >>> that won't be enabled. >> >>> >> >>> At least, in this way we know that we are compatible completely with >> >>> boot cpu capabilities. Thoughts ? >> >> >> >> There is one example on the market, e.g., Samsung Exynos 9810. >> >> >> >> Mongoose 3 (big cores) only support ARMv8.0, while Cortex-A55 >> >> (little ones) support ARMv8.2 (and that brings atomics support). >> >> I think, it's the only ARM SOC that supports different ISA extensions >> >> between cores on the same package. >> >> >> >> Kernel scheduler doesn't know that big cores are missing atomics >> >> support or that applications needs it and moves the thread >> >> resulting in illegal instruction. >> >> >> >> E.g., see Golang issue: https://github.com/golang/go/issues/28431 >> >> >> >> I also recall Jon Masters (Computer Architect at Red Hat) advocating >> >> against having cores with mismatched capabilities on the server market. >> >> >> >> It just causes more problems down the line. >> > > IMO the best bet is to only put extensions in HWCAP that are supported by all >> > the harts that userspace will be scheduled on. >> > Fair enough. Instead of setting HWCAP in setup_arch() once, we can set it only for boot cpu. It will be updated after every cpu comes up online. >> >> Thus, HWCAP will consists all extensions supported by all cpus that are online currently. > > You must thus prevent CPUs that have a different set of capabilities > from coming up late (once userspace has started). and we have no way to do that. I'd prefer if we just looked through the entire device tree and only showed userspace the features that are on every possible CPU from the start. Otherwise the HWCAP will shift around during a userspace run, which seems odd.
> On Feb 11, 2019, at 2:23 PM, Palmer Dabbelt <palmer@sifive.com> wrote: > > On Mon, 11 Feb 2019 14:13:25 PST (-0800), marc.zyngier@arm.com wrote: >> On Mon, 11 Feb 2019 12:03:30 -0800 >> Atish Patra <atish.patra@wdc.com> wrote: >> >>> On 2/11/19 11:02 AM, Palmer Dabbelt wrote: >>> > On Fri, 08 Feb 2019 20:26:07 PST (-0800), david.abdurachmanov@gmail.com wrote: >>> >> On Sat, Feb 9, 2019 at 12:03 AM Atish Patra <atish.patra@wdc.com> wrote: >>> >>> >>> >>> On 2/8/19 1:11 AM, Christoph Hellwig wrote: >>> >>>>> + * We don't support running Linux on hertergenous ISA systems. >>> >>>>> + * But first "okay" processor might not be the boot cpu. >>> >>>>> + * Check the ISA of boot cpu. >>> >>>> >>> >>>> Please use up your available 80 characters per line in comments. >>> >>>> >>> >>> I will fix it. >>> >>> >>> >>>>> + /* >>> >>>>> + * All "okay" hart should have same isa. We don't know how to >>> >>>>> + * handle if they don't. Throw a warning for now. >>> >>>>> + */ >>> >>>>> + if (elf_hwcap && temp_hwcap != elf_hwcap) >>> >>>>> + pr_warn("isa mismatch: 0x%lx != 0x%lx\n", >>> >>>>> + elf_hwcap, temp_hwcap); >>> >>>>> + >>> >>>>> + if (hartid == boot_cpu_hartid) >>> >>>>> + boot_hwcap = temp_hwcap; >>> >>>>> + elf_hwcap = temp_hwcap; >>> >>>> >>> >>>> So we always set elf_hwcap to the capabilities of the previous cpu. >>> >>>> >>> >>>>> + temp_hwcap = 0; >>> >>>> >>> >>>> I think tmp_hwcap should be declared and initialized inside the outer loop >>> >>>> instead having to manually reset it like this. >>> >>>> >>> >>>>> + } >>> >>>>> >>> >>>>> + elf_hwcap = boot_hwcap; >>> >>>> >>> >>>> And then reset it here to the boot cpu. >>> >>>> >>> >>>> Shoudn't we only report the features supported by all cores? Otherwise >>> >>>> we'll still have problems if the boot cpu supports a feature, but not >>> >>>> others. >>> >>>> >>> >>> >>> >>> Hmm. The other side of the argument is boot cpu does have a feature that >>> >>> is not supported by other hart that didn't even boot. >>> >>> The user space may execute something based on boot cpu capability but >>> >>> that won't be enabled. >>> >>> >>> >>> At least, in this way we know that we are compatible completely with >>> >>> boot cpu capabilities. Thoughts ? >>> >> >>> >> There is one example on the market, e.g., Samsung Exynos 9810. >>> >> >>> >> Mongoose 3 (big cores) only support ARMv8.0, while Cortex-A55 >>> >> (little ones) support ARMv8.2 (and that brings atomics support). >>> >> I think, it's the only ARM SOC that supports different ISA extensions >>> >> between cores on the same package. >>> >> >>> >> Kernel scheduler doesn't know that big cores are missing atomics >>> >> support or that applications needs it and moves the thread >>> >> resulting in illegal instruction. >>> >> >>> >> E.g., see Golang issue: https://github.com/golang/go/issues/28431 >>> >> >>> >> I also recall Jon Masters (Computer Architect at Red Hat) advocating >>> >> against having cores with mismatched capabilities on the server market. >>> >> >>> >> It just causes more problems down the line. >>> > > IMO the best bet is to only put extensions in HWCAP that are supported by all >>> > the harts that userspace will be scheduled on. >>> > Fair enough. Instead of setting HWCAP in setup_arch() once, we can set it only for boot cpu. It will be updated after every cpu comes up online. >>> >>> Thus, HWCAP will consists all extensions supported by all cpus that are online currently. >> >> You must thus prevent CPUs that have a different set of capabilities >> from coming up late (once userspace has started). > > and we have no way to do that. I'd prefer if we just looked through the entire device tree and only showed userspace the features that are on every possible CPU from the start. Otherwise the HWCAP will shift around during a userspace run, which seems odd. ok. I will do this for now. Once we have cpu hotplug enabled, we can revisit this. Regards, Atish
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c index a6e369ed..ed8f0c28 100644 --- a/arch/riscv/kernel/cpufeature.c +++ b/arch/riscv/kernel/cpufeature.c @@ -20,6 +20,7 @@ #include <linux/of.h> #include <asm/processor.h> #include <asm/hwcap.h> +#include <asm/smp.h> unsigned long elf_hwcap __read_mostly; #ifdef CONFIG_FPU @@ -32,6 +33,8 @@ void riscv_fill_hwcap(void) const char *isa; size_t i; static unsigned long isa2hwcap[256] = {0}; + int hartid; + unsigned long temp_hwcap = 0, boot_hwcap = 0; isa2hwcap['i'] = isa2hwcap['I'] = COMPAT_HWCAP_ISA_I; isa2hwcap['m'] = isa2hwcap['M'] = COMPAT_HWCAP_ISA_M; @@ -43,27 +46,44 @@ void riscv_fill_hwcap(void) elf_hwcap = 0; /* - * We don't support running Linux on hertergenous ISA systems. For - * now, we just check the ISA of the first "okay" processor. + * We don't support running Linux on hertergenous ISA systems. + * But first "okay" processor might not be the boot cpu. + * Check the ISA of boot cpu. */ - while ((node = of_find_node_by_type(node, "cpu"))) - if (riscv_of_processor_hartid(node) >= 0) - break; - if (!node) { - pr_warning("Unable to find \"cpu\" devicetree entry"); - return; - } + while ((node = of_find_node_by_type(node, "cpu"))) { + if (!node) { + pr_warn("Unable to find \"cpu\" devicetree entry"); + return; + } + + hartid = riscv_of_processor_hartid(node); + if (hartid < 0) + continue; - if (of_property_read_string(node, "riscv,isa", &isa)) { - pr_warning("Unable to find \"riscv,isa\" devicetree entry"); + if (of_property_read_string(node, "riscv,isa", &isa)) { + pr_warn("Unable to find \"riscv,isa\" devicetree entry"); + of_node_put(node); + return; + } of_node_put(node); - return; - } - of_node_put(node); - for (i = 0; i < strlen(isa); ++i) - elf_hwcap |= isa2hwcap[(unsigned char)(isa[i])]; + for (i = 0; i < strlen(isa); ++i) + temp_hwcap |= isa2hwcap[(unsigned char)(isa[i])]; + /* + * All "okay" hart should have same isa. We don't know how to + * handle if they don't. Throw a warning for now. + */ + if (elf_hwcap && temp_hwcap != elf_hwcap) + pr_warn("isa mismatch: 0x%lx != 0x%lx\n", + elf_hwcap, temp_hwcap); + + if (hartid == boot_cpu_hartid) + boot_hwcap = temp_hwcap; + elf_hwcap = temp_hwcap; + temp_hwcap = 0; + } + elf_hwcap = boot_hwcap; /* We don't support systems with F but without D, so mask those out * here. */ if ((elf_hwcap & COMPAT_HWCAP_ISA_F) && !(elf_hwcap & COMPAT_HWCAP_ISA_D)) {
Currently, we set hwcap based on first valid cpu from DT. This may not be correct always as that CPU might not be current booting cpu. Set hwcap based on the boot cpu instead of first valid CPU from DT. Add a sanity check to identify if any hwcap do not match. Signed-off-by: Atish Patra <atish.patra@wdc.com> --- arch/riscv/kernel/cpufeature.c | 52 +++++++++++++++++++++++++++++------------- 1 file changed, 36 insertions(+), 16 deletions(-)