| Message ID | 20200312051107.1454880-10-damien.lemoal@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Kendryte k210 SoC boards support | expand |
On 3/12/20 1:11 AM, Damien Le Moal wrote: > Commit c68a9032299e ("riscv: set pmp configuration if kernel is running > in M-mode") added PMP initialization to M-Mode. While this patch is > valid for any SoC following the ratified riscv specifications, the > Kendryte K210 SoC is based on earlier unstable specifications and does > not seem to support PMP initialization (the SoC crashes if CSR_PMPADDR0 > or CSR_PMPCFG0 are accessed). The PMP bit has its polarity inverted in the v1.9 specification, and is called the SUM or Supervisor User Memory Access bit. --Sean
On Thu, Mar 12, 2020 at 8:20 AM Sean Anderson <seanga2@gmail.com> wrote: > > On 3/12/20 1:11 AM, Damien Le Moal wrote: > > Commit c68a9032299e ("riscv: set pmp configuration if kernel is running > > in M-mode") added PMP initialization to M-Mode. While this patch is > > valid for any SoC following the ratified riscv specifications, the > > Kendryte K210 SoC is based on earlier unstable specifications and does > > not seem to support PMP initialization (the SoC crashes if CSR_PMPADDR0 > > or CSR_PMPCFG0 are accessed). > > The PMP bit has its polarity inverted in the v1.9 specification, and is > called the SUM or Supervisor User Memory Access bit. > > --Sean > I don't think supporting old specs in Linux is a good idea. As per the patch guideline for RISC-V Linux, patches for only "frozen" or "ratified" specifications are allowed.
> I don't think supporting old specs in Linux is a good idea. As per the > patch guideline > for RISC-V Linux, patches for only "frozen" or "ratified" > specifications are allowed. > Well this CPU follows the v1.9 spec. It's real hardware, if it is to be be supported, then the 1.9 spec needs to be as well. --Sean
On Thu, Mar 12, 2020 at 11:14 AM Sean Anderson <seanga2@gmail.com> wrote: > > > > I don't think supporting old specs in Linux is a good idea. As per the > > patch guideline > > for RISC-V Linux, patches for only "frozen" or "ratified" > > specifications are allowed. > > > > Well this CPU follows the v1.9 spec. It's real hardware, if it is to be > be supported, then the 1.9 spec needs to be as well. > As RISC-V is an open ISA and it's continuously evolving, there will be some hardware that will implement old specifications or non-backward compatible features. I fear the number of hardware with such features/implementations will grow in the future. If Linux is going to support all of them, it may be a maintenance nightmare. > --Sean
On 3/12/20 2:29 PM, Atish Patra wrote: > On Thu, Mar 12, 2020 at 11:14 AM Sean Anderson <seanga2@gmail.com> wrote: >> >> >>> I don't think supporting old specs in Linux is a good idea. As per the >>> patch guideline >>> for RISC-V Linux, patches for only "frozen" or "ratified" >>> specifications are allowed. >>> >> >> Well this CPU follows the v1.9 spec. It's real hardware, if it is to be >> be supported, then the 1.9 spec needs to be as well. >> > > As RISC-V is an open ISA and it's continuously evolving, there will be > some hardware > that will implement old specifications or non-backward compatible features. > I fear the number of hardware with such features/implementations will > grow in the future. > If Linux is going to support all of them, it may be a maintenance nightmare. I agree. There is also no standard way to communicate the implemented privileged spec level e.g. in the device tree. The base integer instruction set version can be specified in the riscv,isa property, such as riscv,isa = "rv64i2p1..." However, there is no "extension" for the privileged specification. A method to specify this would be helpful, especially since the bootloader may need to enable the MMU before loading Linux since there is no way to enable it from S-mode until v1.10. On the other hand, there is relatively little changed from v1.9 to the current revision. The following list has the differences from the current spec: * The PMP has flipped polarity * The mcounteren CSRs are split * sfence.vma is sfence.vm (though this should be handled by the sbi anyway) * satp has a different name, and mode no longer exists in the top four bits. Since these bits used to be part of ASID, it's fine to write the mode to those bits. If linux never switches from (e.g.) sv39 to something else, there will be no observed difference either. Everything else is mostly forwards-compatible, as far as I can tell. That is, assuming new behaviour on old hardware won't cause problems. A sufficiently smart kernel could even detect the version at runtime by intentionally triggering behaviour which is illegal depending on the privileged version, and then checking for an exception. --Sean
On Thu, Mar 12, 2020 at 11:49 AM Sean Anderson <seanga2@gmail.com> wrote: > > On 3/12/20 2:29 PM, Atish Patra wrote: > > On Thu, Mar 12, 2020 at 11:14 AM Sean Anderson <seanga2@gmail.com> wrote: > >> > >> > >>> I don't think supporting old specs in Linux is a good idea. As per the > >>> patch guideline > >>> for RISC-V Linux, patches for only "frozen" or "ratified" > >>> specifications are allowed. > >>> > >> > >> Well this CPU follows the v1.9 spec. It's real hardware, if it is to be > >> be supported, then the 1.9 spec needs to be as well. > >> > > > > As RISC-V is an open ISA and it's continuously evolving, there will be > > some hardware > > that will implement old specifications or non-backward compatible features. > > I fear the number of hardware with such features/implementations will > > grow in the future. > > If Linux is going to support all of them, it may be a maintenance nightmare. > > I agree. There is also no standard way to communicate the implemented > privileged spec level e.g. in the device tree. The base integer > instruction set version can be specified in the riscv,isa property, such > as > > riscv,isa = "rv64i2p1..." > > However, there is no "extension" for the privileged specification. > A method to specify this would be helpful, especially since the > bootloader may need to enable the MMU before loading Linux since there > is no way to enable it from S-mode until v1.10. > > On the other hand, there is relatively little changed from v1.9 to the > current revision. The following list has the differences from the > current spec: > > * The PMP has flipped polarity > * The mcounteren CSRs are split > * sfence.vma is sfence.vm (though this should be handled by the sbi > anyway) > * satp has a different name, and mode no longer exists in the top four > bits. Since these bits used to be part of ASID, it's fine to write the > mode to those bits. If linux never switches from (e.g.) sv39 to > something else, there will be no observed difference either. > > Everything else is mostly forwards-compatible, as far as I can tell. > That is, assuming new behaviour on old hardware won't cause problems. > Even if the changes are minimal and we can easily hide under macro magic, it will create a bad precedent for the future. What if somebody sends a patch for a non-standard extension and cites kendryte support as an example. > A sufficiently smart kernel could even detect the version at runtime by > intentionally triggering behaviour which is illegal depending on the > privileged version, and then checking for an exception. > That's the maintenance nightmare I was talking about. For kendryte, it is only few incompatible changes in privilege specification but what if some hardware implements a variation of hypervisor spec or vector extension. > --Sean >
On 3/12/20 3:12 PM, Atish Patra wrote: > On Thu, Mar 12, 2020 at 11:49 AM Sean Anderson <seanga2@gmail.com> wrote: >> >> On 3/12/20 2:29 PM, Atish Patra wrote: >>> On Thu, Mar 12, 2020 at 11:14 AM Sean Anderson <seanga2@gmail.com> wrote: >>>> >>>> >>>>> I don't think supporting old specs in Linux is a good idea. As per the >>>>> patch guideline >>>>> for RISC-V Linux, patches for only "frozen" or "ratified" >>>>> specifications are allowed. >>>>> >>>> >>>> Well this CPU follows the v1.9 spec. It's real hardware, if it is to be >>>> be supported, then the 1.9 spec needs to be as well. >>>> >>> >>> As RISC-V is an open ISA and it's continuously evolving, there will be >>> some hardware >>> that will implement old specifications or non-backward compatible features. >>> I fear the number of hardware with such features/implementations will >>> grow in the future. >>> If Linux is going to support all of them, it may be a maintenance nightmare. >> >> I agree. There is also no standard way to communicate the implemented >> privileged spec level e.g. in the device tree. The base integer >> instruction set version can be specified in the riscv,isa property, such >> as >> >> riscv,isa = "rv64i2p1..." >> >> However, there is no "extension" for the privileged specification. >> A method to specify this would be helpful, especially since the >> bootloader may need to enable the MMU before loading Linux since there >> is no way to enable it from S-mode until v1.10. >> >> On the other hand, there is relatively little changed from v1.9 to the >> current revision. The following list has the differences from the >> current spec: >> >> * The PMP has flipped polarity >> * The mcounteren CSRs are split >> * sfence.vma is sfence.vm (though this should be handled by the sbi >> anyway) >> * satp has a different name, and mode no longer exists in the top four >> bits. Since these bits used to be part of ASID, it's fine to write the >> mode to those bits. If linux never switches from (e.g.) sv39 to >> something else, there will be no observed difference either. >> >> Everything else is mostly forwards-compatible, as far as I can tell. >> That is, assuming new behaviour on old hardware won't cause problems. >> > Even if the changes are minimal and we can easily hide under macro magic, > it will create a bad precedent for the future. What if somebody sends > a patch for > a non-standard extension and cites kendryte support as an example. I think there are substantial differences between a non-standard extension, and what we would need for the K210. First, the changes we would need are for the official specification. At the time this chip was designed, this was *the* authritative privileged spec. I think if a hardware vendor makes the effort to comply with the specification as it exists at the time, then we should support that. In addition, the incompatibilities are within the core boot process. Most non-standard extensions will be optional extras which can be completely ignored. For example, the GAP8 processor has a non-standard extension which adds some instructions for complex number arithmetic (and other operations). These instructions have no effect on the usual boot process, and (if there was an MMU) Linux could run fine on that board with no knowledge of these extensions. Lastly, these non-standard instructions can be documented in a standard way through the isa version string. This incompatibility in the spec has no standard way to be documented. >> A sufficiently smart kernel could even detect the version at runtime by >> intentionally triggering behaviour which is illegal depending on the >> privileged version, and then checking for an exception. >> > That's the maintenance nightmare I was talking about. For kendryte, it > is only few incompatible changes > in privilege specification but what if some hardware implements a > variation of hypervisor spec or vector extension. Hopefully that will not happen, but I think given the long development period of the vector spec, it is inevitable that a chip will be released with some subtle (or not-so-subtle) incompatibilities. As far as I can tell, the restriction on non-ratified extensions is to prevent work towards experimental specifications which may never have real hardware which uses them. However, I think if real hardware is incompatible small but fundamental way, then we should make the effort to support it, especially when the patches already exist. --Sean
On Thu, 2020-03-12 at 11:10 -0700, Atish Patra wrote: > On Thu, Mar 12, 2020 at 8:20 AM Sean Anderson <seanga2@gmail.com> wrote: > > On 3/12/20 1:11 AM, Damien Le Moal wrote: > > > Commit c68a9032299e ("riscv: set pmp configuration if kernel is running > > > in M-mode") added PMP initialization to M-Mode. While this patch is > > > valid for any SoC following the ratified riscv specifications, the > > > Kendryte K210 SoC is based on earlier unstable specifications and does > > > not seem to support PMP initialization (the SoC crashes if CSR_PMPADDR0 > > > or CSR_PMPCFG0 are accessed). > > > > The PMP bit has its polarity inverted in the v1.9 specification, and is > > called the SUM or Supervisor User Memory Access bit. > > > > --Sean > > > I don't think supporting old specs in Linux is a good idea. As per the > patch guideline > for RISC-V Linux, patches for only "frozen" or "ratified" > specifications are allowed. Yes, I agree on this point. However, this should be taken a little more lightly in my opinion. As long as we do not try to run the K210 with MMU turned on, this is the only known tiny deviation from the current ratified privileged specs v1.11. Since this is really a small one, I am inclined to consider this in the same way as a hardware bug, for which software exceptions are perfectly acceptable in the kernel. There are tons of "quirk" bits defined for ratified specifications such as NVMe or AHCI for example, due to hardware bugs. Even though the hardware intend to follow the specifications, quirks are sometimes necessary as work-around for hardware implementation bugs which are far more difficult to fix than software. Of course there are limits to this and sometimes, the decision has to be "go fix your hardware". In this case, I do not think we are at that level of seriousness, again assuming we never run with MMU on (because then we would need to implement support for the 1.9 specs, and that would seriously mess things up). Without this fix, the K210 cannot boot, at all. So if this small change is refused, we may as well just give up on the entire effort. That would really be too bad since the K210 boards are really cheap and would enable a lot of hobbyist and students to start playing with Linux on RISC-V... In the end, I think this is Palmer's decision. I gave my 2 cents above. If there is too much push-back on this, I will stop the effort because I do not see how to work around this one within 1.11 specs. Cheers.
On Fri, Mar 13, 2020 at 12:19 AM Sean Anderson <seanga2@gmail.com> wrote: > > On 3/12/20 2:29 PM, Atish Patra wrote: > > On Thu, Mar 12, 2020 at 11:14 AM Sean Anderson <seanga2@gmail.com> wrote: > >> > >> > >>> I don't think supporting old specs in Linux is a good idea. As per the > >>> patch guideline > >>> for RISC-V Linux, patches for only "frozen" or "ratified" > >>> specifications are allowed. > >>> > >> > >> Well this CPU follows the v1.9 spec. It's real hardware, if it is to be > >> be supported, then the 1.9 spec needs to be as well. > >> > > > > As RISC-V is an open ISA and it's continuously evolving, there will be > > some hardware > > that will implement old specifications or non-backward compatible features. > > I fear the number of hardware with such features/implementations will > > grow in the future. > > If Linux is going to support all of them, it may be a maintenance nightmare. > > I agree. There is also no standard way to communicate the implemented > privileged spec level e.g. in the device tree. The base integer > instruction set version can be specified in the riscv,isa property, such > as > > riscv,isa = "rv64i2p1..." > > However, there is no "extension" for the privileged specification. > A method to specify this would be helpful, especially since the > bootloader may need to enable the MMU before loading Linux since there > is no way to enable it from S-mode until v1.10. > > On the other hand, there is relatively little changed from v1.9 to the > current revision. The following list has the differences from the > current spec: > > * The PMP has flipped polarity > * The mcounteren CSRs are split > * sfence.vma is sfence.vm (though this should be handled by the sbi > anyway) > * satp has a different name, and mode no longer exists in the top four > bits. Since these bits used to be part of ASID, it's fine to write the > mode to those bits. If linux never switches from (e.g.) sv39 to > something else, there will be no observed difference either. > > Everything else is mostly forwards-compatible, as far as I can tell. > That is, assuming new behaviour on old hardware won't cause problems. > > A sufficiently smart kernel could even detect the version at runtime by > intentionally triggering behaviour which is illegal depending on the > privileged version, and then checking for an exception. As-per Linux RISC-V patch acceptance policy, patches for "Frozen" or "Ratified" spec will only be accepted. The KVM RISC-V and Vector support patches are on-hold for same reason. (Refer, Documentation/riscv/patch-acceptance.rst) The amount of change is not the question here it more about policy. Linux RISC-V NOMMU kernel has carefully avoided this policy issues by only touching CSRs in a way which is compliant with the latest "Ratified" v1.11 spec. Regards, Anup
On Thu, Mar 12, 2020 at 10:25 PM Damien Le Moal <Damien.LeMoal@wdc.com> wrote: > > On Thu, 2020-03-12 at 11:10 -0700, Atish Patra wrote: > > On Thu, Mar 12, 2020 at 8:20 AM Sean Anderson <seanga2@gmail.com> wrote: > > > On 3/12/20 1:11 AM, Damien Le Moal wrote: > > > > Commit c68a9032299e ("riscv: set pmp configuration if kernel is running > > > > in M-mode") added PMP initialization to M-Mode. While this patch is > > > > valid for any SoC following the ratified riscv specifications, the > > > > Kendryte K210 SoC is based on earlier unstable specifications and does > > > > not seem to support PMP initialization (the SoC crashes if CSR_PMPADDR0 > > > > or CSR_PMPCFG0 are accessed). > > > > > > The PMP bit has its polarity inverted in the v1.9 specification, and is > > > called the SUM or Supervisor User Memory Access bit. > > > > > > --Sean > > > > > I don't think supporting old specs in Linux is a good idea. As per the > > patch guideline > > for RISC-V Linux, patches for only "frozen" or "ratified" > > specifications are allowed. > > Yes, I agree on this point. However, this should be taken a little more > lightly in my opinion. As long as we do not try to run the K210 with > MMU turned on, this is the only known tiny deviation from the current > ratified privileged specs v1.11. Since this is really a small one, I am > inclined to consider this in the same way as a hardware bug, for which > software exceptions are perfectly acceptable in the kernel. There are > tons of "quirk" bits defined for ratified specifications such as NVMe > or AHCI for example, due to hardware bugs. Even though the hardware > intend to follow the specifications, quirks are sometimes necessary as > work-around for hardware implementation bugs which are far more > difficult to fix than software. > > Of course there are limits to this and sometimes, the decision has to > be "go fix your hardware". In this case, I do not think we are at that > level of seriousness, again assuming we never run with MMU on (because > then we would need to implement support for the 1.9 specs, and that > would seriously mess things up). > > Without this fix, the K210 cannot boot, at all. So if this small change > is refused, we may as well just give up on the entire effort. That > would really be too bad since the K210 boards are really cheap and > would enable a lot of hobbyist and students to start playing with Linux > on RISC-V... > I think there is some confusion. I was suggesting that supporting v1.9 spec is not a good idea. That means allowing PMP access as per v1.9 spec instead of this patch as Sean suggested. This patch is fine as it just ignores touching the PMP CSR for kendryte board. There is no issue with this patch and it doesn't violate the patch policy. > In the end, I think this is Palmer's decision. I gave my 2 cents above. > If there is too much push-back on this, I will stop the effort because > I do not see how to work around this one within 1.11 specs. > > Cheers. > > -- > Damien Le Moal > Western Digital Research
On Thu, 2020-03-12 at 12:12 -0700, Atish Patra wrote: > On Thu, Mar 12, 2020 at 11:49 AM Sean Anderson <seanga2@gmail.com> wrote: > > On 3/12/20 2:29 PM, Atish Patra wrote: > > > On Thu, Mar 12, 2020 at 11:14 AM Sean Anderson <seanga2@gmail.com> wrote: > > > > > > > > > I don't think supporting old specs in Linux is a good idea. As per the > > > > > patch guideline > > > > > for RISC-V Linux, patches for only "frozen" or "ratified" > > > > > specifications are allowed. > > > > > > > > > > > > > Well this CPU follows the v1.9 spec. It's real hardware, if it is to be > > > > be supported, then the 1.9 spec needs to be as well. > > > > > > > > > > As RISC-V is an open ISA and it's continuously evolving, there will be > > > some hardware > > > that will implement old specifications or non-backward compatible features. > > > I fear the number of hardware with such features/implementations will > > > grow in the future. > > > If Linux is going to support all of them, it may be a maintenance nightmare. > > > > I agree. There is also no standard way to communicate the implemented > > privileged spec level e.g. in the device tree. The base integer > > instruction set version can be specified in the riscv,isa property, such > > as > > > > riscv,isa = "rv64i2p1..." > > > > However, there is no "extension" for the privileged specification. > > A method to specify this would be helpful, especially since the > > bootloader may need to enable the MMU before loading Linux since there > > is no way to enable it from S-mode until v1.10. > > > > On the other hand, there is relatively little changed from v1.9 to the > > current revision. The following list has the differences from the > > current spec: > > > > * The PMP has flipped polarity > > * The mcounteren CSRs are split > > * sfence.vma is sfence.vm (though this should be handled by the sbi > > anyway) > > * satp has a different name, and mode no longer exists in the top four > > bits. Since these bits used to be part of ASID, it's fine to write the > > mode to those bits. If linux never switches from (e.g.) sv39 to > > something else, there will be no observed difference either. > > > > Everything else is mostly forwards-compatible, as far as I can tell. > > That is, assuming new behaviour on old hardware won't cause problems. > > > Even if the changes are minimal and we can easily hide under macro magic, > it will create a bad precedent for the future. What if somebody sends > a patch for > a non-standard extension and cites kendryte support as an example. Come on ! ISA extensions are much bigger beasts ! And refusing upstream support for non-ratified extensions is fine. This case is nowhere near as complex. And as I mentioned in an earlier email, it is so simple that we could consider it as a hardware bug rather than spec deviation (which hardware bugs are in fact, even though they are not intentional). > > A sufficiently smart kernel could even detect the version at runtime by > > intentionally triggering behaviour which is illegal depending on the > > privileged version, and then checking for an exception. > > > That's the maintenance nightmare I was talking about. For kendryte, it > is only few incompatible changes > in privilege specification but what if some hardware implements a > variation of hypervisor spec or vector extension. If you add "what if" like that, we will never be able to make progress and innovate with RISC-V. I for one do not expect the specs 1.11 to be "THE" RISC-V specs forever. There surely will be someday an evolution of it. We most likely will have to support someday RISC-V 2.0. And we will need to keep support for 1.11 as well when that day comes. Support nightmare most often come from code getting out of hand in terms of cleanliness/structure (in my opinion). As long as tiny changes are not invasive, clearly very limited in scope and clean, I am perfectly fine with exceptions. Again, we will have to deal with various hardware bugs as we get more HW anyway. That will be the same problem... > > > --Sean > > > >
diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S index 52ed11b4fda6..1a5defdc33d2 100644 --- a/arch/riscv/kernel/head.S +++ b/arch/riscv/kernel/head.S @@ -58,11 +58,17 @@ _start_kernel: /* Reset all registers except ra, a0, a1 */ call reset_regs - /* Setup a PMP to permit access to all of memory. */ + /* + * For M-mode, setup PMP to permit access to all of memory. + * This should however not be done for the Kendryte K210 SoC as this + * causes a crash (this SoC follows older unstable specification). + */ +#ifndef CONFIG_SOC_KENDRYTE li a0, -1 csrw CSR_PMPADDR0, a0 li a0, (PMP_A_NAPOT | PMP_R | PMP_W | PMP_X) csrw CSR_PMPCFG0, a0 +#endif /* * The hartid in a0 is expected later on, and we have no firmware
Commit c68a9032299e ("riscv: set pmp configuration if kernel is running in M-mode") added PMP initialization to M-Mode. While this patch is valid for any SoC following the ratified riscv specifications, the Kendryte K210 SoC is based on earlier unstable specifications and does not seem to support PMP initialization (the SoC crashes if CSR_PMPADDR0 or CSR_PMPCFG0 are accessed). Since all physical memory access works fine on this SoC without any explicit PMP initialization, avoid the SoC crash by not touching the PMP CSRs. Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> --- arch/riscv/kernel/head.S | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)