| Message ID | 20230216215550.1011637-2-dbarboza@ventanamicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | make write_misa a no-op and FEATURE_* cleanups | expand |
On 2023/2/17 5:55, Daniel Henrique Barboza wrote: > At this moment, and apparently since ever, we have no way of enabling > RISCV_FEATURE_MISA. This means that all the code from write_misa(), all > the nuts and bolts that handles how to write this CSR, has always been a > no-op as well because write_misa() will always exit earlier. > > This seems to be benign in the majority of cases. Booting an Ubuntu > 'virt' guest and logging all the calls to 'write_misa' shows that no > writes to MISA CSR was attempted. Writing MISA, i.e. enabling/disabling > RISC-V extensions after the machine is powered on, seems to be a niche > use. > > Before proceeding, let's recap what the spec says about MISA. It is a > CSR that is divided in 3 fields: > > - MXL, Machine XLEN, described as "may be writable"; > > - MXLEN, the XLEN in M-mode, which is given by the setting of MXL or a > fixed value if MISA is zero; > > - Extensions is defined as "a WARL field that can contain writable bits > where the implementation allows the supported ISA to be modified" > > Thus what we have today (write_misa() being a no-op) is already a valid > spec implementation. We're not obliged to have a particular set of MISA > writable bits, and at this moment we have none. Hi Daniel, I see there has been a discussion on this topic. And as no-op has no harmfulness for current implementation. However, I still think we should make misa writable as default, which is also a valid spec implementation. One reason is that may be we need to dynamic write access for some cpus in the future. The other is we should make QEMU a more useful implementation, not just a legal implementation. We have done in many aspects on this direction. I prefer your implementation before v4. It's not a complicated implementation. And I think the other extensions on QEMU currently can mostly be configurable already. Your work is a good step towards to unify the configuration and the check. I think two more steps we can go further. 1) Remove RVI/RVF and the similar macros, and add fields for them in the configuration struct. 2) Unify the check about configuration. write_misa and cpu_realize_fn can use the same check function. As we have done these two steps, I think we can go more closely for the profile extension. Zhiwei > Given that allowing the dormant code to write MISA can cause tricky bugs > to solve later on, and we don't have a particularly interesting case of > writing MISA to support today, and we're already not violating the > specification, let's erase all the body of write_misa() and turn it into > an official no-op instead of an accidental one. We'll keep consistent > with what we provide users today but with 50+ less lines to maintain. > > RISCV_FEATURE_MISA enum is erased in the process since there's no one > else using it. > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > Reviewed-by: Bin Meng <bmeng@tinylab.org> > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> > --- > target/riscv/cpu.h | 1 - > target/riscv/csr.c | 55 ---------------------------------------------- > 2 files changed, 56 deletions(-) > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index 7128438d8e..01803a020d 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -89,7 +89,6 @@ enum { > RISCV_FEATURE_MMU, > RISCV_FEATURE_PMP, > RISCV_FEATURE_EPMP, > - RISCV_FEATURE_MISA, > RISCV_FEATURE_DEBUG > }; > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > index 1b0a0c1693..f7862ff4a4 100644 > --- a/target/riscv/csr.c > +++ b/target/riscv/csr.c > @@ -1329,61 +1329,6 @@ static RISCVException read_misa(CPURISCVState *env, int csrno, > static RISCVException write_misa(CPURISCVState *env, int csrno, > target_ulong val) > { > - if (!riscv_feature(env, RISCV_FEATURE_MISA)) { > - /* drop write to misa */ > - return RISCV_EXCP_NONE; > - } > - > - /* 'I' or 'E' must be present */ > - if (!(val & (RVI | RVE))) { > - /* It is not, drop write to misa */ > - return RISCV_EXCP_NONE; > - } > - > - /* 'E' excludes all other extensions */ > - if (val & RVE) { > - /* when we support 'E' we can do "val = RVE;" however > - * for now we just drop writes if 'E' is present. > - */ > - return RISCV_EXCP_NONE; > - } > - > - /* > - * misa.MXL writes are not supported by QEMU. > - * Drop writes to those bits. > - */ > - > - /* Mask extensions that are not supported by this hart */ > - val &= env->misa_ext_mask; > - > - /* Mask extensions that are not supported by QEMU */ > - val &= (RVI | RVE | RVM | RVA | RVF | RVD | RVC | RVS | RVU | RVV); > - > - /* 'D' depends on 'F', so clear 'D' if 'F' is not present */ > - if ((val & RVD) && !(val & RVF)) { > - val &= ~RVD; > - } > - > - /* Suppress 'C' if next instruction is not aligned > - * TODO: this should check next_pc > - */ > - if ((val & RVC) && (GETPC() & ~3) != 0) { > - val &= ~RVC; > - } > - > - /* If nothing changed, do nothing. */ > - if (val == env->misa_ext) { > - return RISCV_EXCP_NONE; > - } > - > - if (!(val & RVF)) { > - env->mstatus &= ~MSTATUS_FS; > - } > - > - /* flush translation cache */ > - tb_flush(env_cpu(env)); > - env->misa_ext = val; > - env->xl = riscv_cpu_mxl(env); > return RISCV_EXCP_NONE; > } >
On 2023/2/17 05:55, Daniel Henrique Barboza wrote: > At this moment, and apparently since ever, we have no way of enabling > RISCV_FEATURE_MISA. This means that all the code from write_misa(), all > the nuts and bolts that handles how to write this CSR, has always been a > no-op as well because write_misa() will always exit earlier. > > This seems to be benign in the majority of cases. Booting an Ubuntu > 'virt' guest and logging all the calls to 'write_misa' shows that no > writes to MISA CSR was attempted. Writing MISA, i.e. enabling/disabling > RISC-V extensions after the machine is powered on, seems to be a niche > use. > > Before proceeding, let's recap what the spec says about MISA. It is a > CSR that is divided in 3 fields: > > - MXL, Machine XLEN, described as "may be writable"; > > - MXLEN, the XLEN in M-mode, which is given by the setting of MXL or a > fixed value if MISA is zero; > > - Extensions is defined as "a WARL field that can contain writable bits > where the implementation allows the supported ISA to be modified" > > Thus what we have today (write_misa() being a no-op) is already a valid > spec implementation. We're not obliged to have a particular set of MISA > writable bits, and at this moment we have none. > > Given that allowing the dormant code to write MISA can cause tricky bugs > to solve later on, and we don't have a particularly interesting case of > writing MISA to support today, and we're already not violating the > specification, let's erase all the body of write_misa() and turn it into > an official no-op instead of an accidental one. We'll keep consistent > with what we provide users today but with 50+ less lines to maintain. > > RISCV_FEATURE_MISA enum is erased in the process since there's no one > else using it. > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > Reviewed-by: Bin Meng <bmeng@tinylab.org> > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> Reviewed-by: Weiwei Li <liweiwei@iscas.ac.cn> Regards, Weiwei Li > --- > target/riscv/cpu.h | 1 - > target/riscv/csr.c | 55 ---------------------------------------------- > 2 files changed, 56 deletions(-) > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index 7128438d8e..01803a020d 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -89,7 +89,6 @@ enum { > RISCV_FEATURE_MMU, > RISCV_FEATURE_PMP, > RISCV_FEATURE_EPMP, > - RISCV_FEATURE_MISA, > RISCV_FEATURE_DEBUG > }; > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > index 1b0a0c1693..f7862ff4a4 100644 > --- a/target/riscv/csr.c > +++ b/target/riscv/csr.c > @@ -1329,61 +1329,6 @@ static RISCVException read_misa(CPURISCVState *env, int csrno, > static RISCVException write_misa(CPURISCVState *env, int csrno, > target_ulong val) > { > - if (!riscv_feature(env, RISCV_FEATURE_MISA)) { > - /* drop write to misa */ > - return RISCV_EXCP_NONE; > - } > - > - /* 'I' or 'E' must be present */ > - if (!(val & (RVI | RVE))) { > - /* It is not, drop write to misa */ > - return RISCV_EXCP_NONE; > - } > - > - /* 'E' excludes all other extensions */ > - if (val & RVE) { > - /* when we support 'E' we can do "val = RVE;" however > - * for now we just drop writes if 'E' is present. > - */ > - return RISCV_EXCP_NONE; > - } > - > - /* > - * misa.MXL writes are not supported by QEMU. > - * Drop writes to those bits. > - */ > - > - /* Mask extensions that are not supported by this hart */ > - val &= env->misa_ext_mask; > - > - /* Mask extensions that are not supported by QEMU */ > - val &= (RVI | RVE | RVM | RVA | RVF | RVD | RVC | RVS | RVU | RVV); > - > - /* 'D' depends on 'F', so clear 'D' if 'F' is not present */ > - if ((val & RVD) && !(val & RVF)) { > - val &= ~RVD; > - } > - > - /* Suppress 'C' if next instruction is not aligned > - * TODO: this should check next_pc > - */ > - if ((val & RVC) && (GETPC() & ~3) != 0) { > - val &= ~RVC; > - } > - > - /* If nothing changed, do nothing. */ > - if (val == env->misa_ext) { > - return RISCV_EXCP_NONE; > - } > - > - if (!(val & RVF)) { > - env->mstatus &= ~MSTATUS_FS; > - } > - > - /* flush translation cache */ > - tb_flush(env_cpu(env)); > - env->misa_ext = val; > - env->xl = riscv_cpu_mxl(env); > return RISCV_EXCP_NONE; > } >
Hey, On 2/16/23 22:42, LIU Zhiwei wrote: > > On 2023/2/17 5:55, Daniel Henrique Barboza wrote: >> At this moment, and apparently since ever, we have no way of enabling >> RISCV_FEATURE_MISA. This means that all the code from write_misa(), all >> the nuts and bolts that handles how to write this CSR, has always been a >> no-op as well because write_misa() will always exit earlier. >> >> This seems to be benign in the majority of cases. Booting an Ubuntu >> 'virt' guest and logging all the calls to 'write_misa' shows that no >> writes to MISA CSR was attempted. Writing MISA, i.e. enabling/disabling >> RISC-V extensions after the machine is powered on, seems to be a niche >> use. >> >> Before proceeding, let's recap what the spec says about MISA. It is a >> CSR that is divided in 3 fields: >> >> - MXL, Machine XLEN, described as "may be writable"; >> >> - MXLEN, the XLEN in M-mode, which is given by the setting of MXL or a >> fixed value if MISA is zero; >> >> - Extensions is defined as "a WARL field that can contain writable bits >> where the implementation allows the supported ISA to be modified" >> >> Thus what we have today (write_misa() being a no-op) is already a valid >> spec implementation. We're not obliged to have a particular set of MISA >> writable bits, and at this moment we have none. > > Hi Daniel, > > I see there has been a discussion on this topic. And as no-op has no harmfulness for current implementation. > However, I still think we should make misa writable as default, which is also a valid spec implementation. > > One reason is that may be we need to dynamic write access for some cpus in the future. The other is we should > make QEMU a more useful implementation, not just a legal implementation. We have done in many aspects on this direction. > > I prefer your implementation before v4. It's not a complicated implementation. And I think the other extensions on QEMU currently > can mostly be configurable already. I don't have a strong opinion in this matter to be honest. My problems with the existing code are: - the code is untested. I cannot say that this was never tested, but I can say that this has been mostly untested ever since introduced. Which is normal for a code that is 'dormant'. - the code is dormant and most likely with bugs, but it's still maintained. For example we have e91a7227 ("target/riscv: Split misa.mxl and misa.ext") that had to make changes here. So we have the upkeep but no benefits. - we don't have an use case for it. Most OSes doesn't seem to care, and afaik no applications seems to care either. All this said, I think we can reach a consensus of keeping it if we can at least come up with a way of testing it. > > Your work is a good step towards to unify the configuration and the check. I think two more steps we can go further. > > 1) Remove RVI/RVF and the similar macros, and add fields for them in the configuration struct. > > 2) Unify the check about configuration. write_misa and cpu_realize_fn can use the same check function. > > > As we have done these two steps, I think we can go more closely for the profile extension. Is this the extension you're taking about? https://github.com/riscv/riscv-profiles/blob/main/profiles.adoc This looks like a good reason to keep the code. Let's see if anyone else has an opinion about it. We can do the improvements you mentioned above as a follow-up (this series was really about removing RISC_FEATURE_*) if we decide to keep it. Thanks, Daniel > > > Zhiwei > >> Given that allowing the dormant code to write MISA can cause tricky bugs >> to solve later on, and we don't have a particularly interesting case of >> writing MISA to support today, and we're already not violating the >> specification, let's erase all the body of write_misa() and turn it into >> an official no-op instead of an accidental one. We'll keep consistent >> with what we provide users today but with 50+ less lines to maintain. >> >> RISCV_FEATURE_MISA enum is erased in the process since there's no one >> else using it. >> >> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> >> Reviewed-by: Bin Meng <bmeng@tinylab.org> >> Reviewed-by: Andrew Jones <ajones@ventanamicro.com> >> --- >> target/riscv/cpu.h | 1 - >> target/riscv/csr.c | 55 ---------------------------------------------- >> 2 files changed, 56 deletions(-) >> >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >> index 7128438d8e..01803a020d 100644 >> --- a/target/riscv/cpu.h >> +++ b/target/riscv/cpu.h >> @@ -89,7 +89,6 @@ enum { >> RISCV_FEATURE_MMU, >> RISCV_FEATURE_PMP, >> RISCV_FEATURE_EPMP, >> - RISCV_FEATURE_MISA, >> RISCV_FEATURE_DEBUG >> }; >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c >> index 1b0a0c1693..f7862ff4a4 100644 >> --- a/target/riscv/csr.c >> +++ b/target/riscv/csr.c >> @@ -1329,61 +1329,6 @@ static RISCVException read_misa(CPURISCVState *env, int csrno, >> static RISCVException write_misa(CPURISCVState *env, int csrno, >> target_ulong val) >> { >> - if (!riscv_feature(env, RISCV_FEATURE_MISA)) { >> - /* drop write to misa */ >> - return RISCV_EXCP_NONE; >> - } >> - >> - /* 'I' or 'E' must be present */ >> - if (!(val & (RVI | RVE))) { >> - /* It is not, drop write to misa */ >> - return RISCV_EXCP_NONE; >> - } >> - >> - /* 'E' excludes all other extensions */ >> - if (val & RVE) { >> - /* when we support 'E' we can do "val = RVE;" however >> - * for now we just drop writes if 'E' is present. >> - */ >> - return RISCV_EXCP_NONE; >> - } >> - >> - /* >> - * misa.MXL writes are not supported by QEMU. >> - * Drop writes to those bits. >> - */ >> - >> - /* Mask extensions that are not supported by this hart */ >> - val &= env->misa_ext_mask; >> - >> - /* Mask extensions that are not supported by QEMU */ >> - val &= (RVI | RVE | RVM | RVA | RVF | RVD | RVC | RVS | RVU | RVV); >> - >> - /* 'D' depends on 'F', so clear 'D' if 'F' is not present */ >> - if ((val & RVD) && !(val & RVF)) { >> - val &= ~RVD; >> - } >> - >> - /* Suppress 'C' if next instruction is not aligned >> - * TODO: this should check next_pc >> - */ >> - if ((val & RVC) && (GETPC() & ~3) != 0) { >> - val &= ~RVC; >> - } >> - >> - /* If nothing changed, do nothing. */ >> - if (val == env->misa_ext) { >> - return RISCV_EXCP_NONE; >> - } >> - >> - if (!(val & RVF)) { >> - env->mstatus &= ~MSTATUS_FS; >> - } >> - >> - /* flush translation cache */ >> - tb_flush(env_cpu(env)); >> - env->misa_ext = val; >> - env->xl = riscv_cpu_mxl(env); >> return RISCV_EXCP_NONE; >> }
On Tue, Feb 21, 2023 at 12:49:11PM -0300, Daniel Henrique Barboza wrote: > Hey, > > On 2/16/23 22:42, LIU Zhiwei wrote: > > > > On 2023/2/17 5:55, Daniel Henrique Barboza wrote: > > > At this moment, and apparently since ever, we have no way of enabling > > > RISCV_FEATURE_MISA. This means that all the code from write_misa(), all > > > the nuts and bolts that handles how to write this CSR, has always been a > > > no-op as well because write_misa() will always exit earlier. > > > > > > This seems to be benign in the majority of cases. Booting an Ubuntu > > > 'virt' guest and logging all the calls to 'write_misa' shows that no > > > writes to MISA CSR was attempted. Writing MISA, i.e. enabling/disabling > > > RISC-V extensions after the machine is powered on, seems to be a niche > > > use. > > > > > > Before proceeding, let's recap what the spec says about MISA. It is a > > > CSR that is divided in 3 fields: > > > > > > - MXL, Machine XLEN, described as "may be writable"; > > > > > > - MXLEN, the XLEN in M-mode, which is given by the setting of MXL or a > > > fixed value if MISA is zero; > > > > > > - Extensions is defined as "a WARL field that can contain writable bits > > > where the implementation allows the supported ISA to be modified" > > > > > > Thus what we have today (write_misa() being a no-op) is already a valid > > > spec implementation. We're not obliged to have a particular set of MISA > > > writable bits, and at this moment we have none. > > > > Hi Daniel, > > > > I see there has been a discussion on this topic. And as no-op has no harmfulness for current implementation. > > However, I still think we should make misa writable as default, which is also a valid spec implementation. > > > > One reason is that may be we need to dynamic write access for some cpus in the future. The other is we should > > make QEMU a more useful implementation, not just a legal implementation. We have done in many aspects on this direction. > > > > I prefer your implementation before v4. It's not a complicated implementation. And I think the other extensions on QEMU currently > > can mostly be configurable already. > > I don't have a strong opinion in this matter to be honest. My problems with the > existing code are: > > - the code is untested. I cannot say that this was never tested, but I can say that > this has been mostly untested ever since introduced. Which is normal for a code that > is 'dormant'. > > - the code is dormant and most likely with bugs, but it's still maintained. For > example we have e91a7227 ("target/riscv: Split misa.mxl and misa.ext") that had > to make changes here. So we have the upkeep but no benefits. > > - we don't have an use case for it. Most OSes doesn't seem to care, and afaik no > applications seems to care either. > > > All this said, I think we can reach a consensus of keeping it if we can at least come > up with a way of testing it. > > > > > > Your work is a good step towards to unify the configuration and the check. I think two more steps we can go further. > > > > 1) Remove RVI/RVF and the similar macros, and add fields for them in the configuration struct. > > > > 2) Unify the check about configuration. write_misa and cpu_realize_fn can use the same check function. > > > > > > As we have done these two steps, I think we can go more closely for the profile extension. > > > Is this the extension you're taking about? > > https://github.com/riscv/riscv-profiles/blob/main/profiles.adoc > > > This looks like a good reason to keep the code. Let's see if anyone else has an opinion > about it. We can do the improvements you mentioned above as a follow-up (this series was > really about removing RISC_FEATURE_*) if we decide to keep it. If we decide to keep it and not guard it by default, then we should test and fix it now. Also, as we're already aware that it has insufficient sanity checks for extension dependencies, then we should fix our general extension dependency checking now too, in order to apply that to this. IOW, trying to keep this, without some guard on it, opens a can of worms. My vote is the same as it was before, merge this series and then revisit this function when someone has a use/test case for it. Nobody said this was never going to have a different implementation, just that the current implementation is known-buggy and there's no reason to expose it now. My only concern with the code deletion is that git-blame doesn't blame deleted code. I think we should add a comment describing the history which includes a git commit reference which can be used to see the latest implementation. Thanks, drew > > > > Thanks, > > > Daniel > > > > > > > Zhiwei > > > > > Given that allowing the dormant code to write MISA can cause tricky bugs > > > to solve later on, and we don't have a particularly interesting case of > > > writing MISA to support today, and we're already not violating the > > > specification, let's erase all the body of write_misa() and turn it into > > > an official no-op instead of an accidental one. We'll keep consistent > > > with what we provide users today but with 50+ less lines to maintain. > > > > > > RISCV_FEATURE_MISA enum is erased in the process since there's no one > > > else using it. > > > > > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > > > Reviewed-by: Bin Meng <bmeng@tinylab.org> > > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> > > > --- > > > target/riscv/cpu.h | 1 - > > > target/riscv/csr.c | 55 ---------------------------------------------- > > > 2 files changed, 56 deletions(-) > > > > > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > > > index 7128438d8e..01803a020d 100644 > > > --- a/target/riscv/cpu.h > > > +++ b/target/riscv/cpu.h > > > @@ -89,7 +89,6 @@ enum { > > > RISCV_FEATURE_MMU, > > > RISCV_FEATURE_PMP, > > > RISCV_FEATURE_EPMP, > > > - RISCV_FEATURE_MISA, > > > RISCV_FEATURE_DEBUG > > > }; > > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > > > index 1b0a0c1693..f7862ff4a4 100644 > > > --- a/target/riscv/csr.c > > > +++ b/target/riscv/csr.c > > > @@ -1329,61 +1329,6 @@ static RISCVException read_misa(CPURISCVState *env, int csrno, > > > static RISCVException write_misa(CPURISCVState *env, int csrno, > > > target_ulong val) > > > { > > > - if (!riscv_feature(env, RISCV_FEATURE_MISA)) { > > > - /* drop write to misa */ > > > - return RISCV_EXCP_NONE; > > > - } > > > - > > > - /* 'I' or 'E' must be present */ > > > - if (!(val & (RVI | RVE))) { > > > - /* It is not, drop write to misa */ > > > - return RISCV_EXCP_NONE; > > > - } > > > - > > > - /* 'E' excludes all other extensions */ > > > - if (val & RVE) { > > > - /* when we support 'E' we can do "val = RVE;" however > > > - * for now we just drop writes if 'E' is present. > > > - */ > > > - return RISCV_EXCP_NONE; > > > - } > > > - > > > - /* > > > - * misa.MXL writes are not supported by QEMU. > > > - * Drop writes to those bits. > > > - */ > > > - > > > - /* Mask extensions that are not supported by this hart */ > > > - val &= env->misa_ext_mask; > > > - > > > - /* Mask extensions that are not supported by QEMU */ > > > - val &= (RVI | RVE | RVM | RVA | RVF | RVD | RVC | RVS | RVU | RVV); > > > - > > > - /* 'D' depends on 'F', so clear 'D' if 'F' is not present */ > > > - if ((val & RVD) && !(val & RVF)) { > > > - val &= ~RVD; > > > - } > > > - > > > - /* Suppress 'C' if next instruction is not aligned > > > - * TODO: this should check next_pc > > > - */ > > > - if ((val & RVC) && (GETPC() & ~3) != 0) { > > > - val &= ~RVC; > > > - } > > > - > > > - /* If nothing changed, do nothing. */ > > > - if (val == env->misa_ext) { > > > - return RISCV_EXCP_NONE; > > > - } > > > - > > > - if (!(val & RVF)) { > > > - env->mstatus &= ~MSTATUS_FS; > > > - } > > > - > > > - /* flush translation cache */ > > > - tb_flush(env_cpu(env)); > > > - env->misa_ext = val; > > > - env->xl = riscv_cpu_mxl(env); > > > return RISCV_EXCP_NONE; > > > }
On 2/21/23 14:06, Andrew Jones wrote: > On Tue, Feb 21, 2023 at 12:49:11PM -0300, Daniel Henrique Barboza wrote: >> Hey, >> >> On 2/16/23 22:42, LIU Zhiwei wrote: >>> >>> On 2023/2/17 5:55, Daniel Henrique Barboza wrote: >>>> At this moment, and apparently since ever, we have no way of enabling >>>> RISCV_FEATURE_MISA. This means that all the code from write_misa(), all >>>> the nuts and bolts that handles how to write this CSR, has always been a >>>> no-op as well because write_misa() will always exit earlier. >>>> >>>> This seems to be benign in the majority of cases. Booting an Ubuntu >>>> 'virt' guest and logging all the calls to 'write_misa' shows that no >>>> writes to MISA CSR was attempted. Writing MISA, i.e. enabling/disabling >>>> RISC-V extensions after the machine is powered on, seems to be a niche >>>> use. >>>> >>>> Before proceeding, let's recap what the spec says about MISA. It is a >>>> CSR that is divided in 3 fields: >>>> >>>> - MXL, Machine XLEN, described as "may be writable"; >>>> >>>> - MXLEN, the XLEN in M-mode, which is given by the setting of MXL or a >>>> fixed value if MISA is zero; >>>> >>>> - Extensions is defined as "a WARL field that can contain writable bits >>>> where the implementation allows the supported ISA to be modified" >>>> >>>> Thus what we have today (write_misa() being a no-op) is already a valid >>>> spec implementation. We're not obliged to have a particular set of MISA >>>> writable bits, and at this moment we have none. >>> >>> Hi Daniel, >>> >>> I see there has been a discussion on this topic. And as no-op has no harmfulness for current implementation. >>> However, I still think we should make misa writable as default, which is also a valid spec implementation. >>> >>> One reason is that may be we need to dynamic write access for some cpus in the future. The other is we should >>> make QEMU a more useful implementation, not just a legal implementation. We have done in many aspects on this direction. >>> >>> I prefer your implementation before v4. It's not a complicated implementation. And I think the other extensions on QEMU currently >>> can mostly be configurable already. >> >> I don't have a strong opinion in this matter to be honest. My problems with the >> existing code are: >> >> - the code is untested. I cannot say that this was never tested, but I can say that >> this has been mostly untested ever since introduced. Which is normal for a code that >> is 'dormant'. >> >> - the code is dormant and most likely with bugs, but it's still maintained. For >> example we have e91a7227 ("target/riscv: Split misa.mxl and misa.ext") that had >> to make changes here. So we have the upkeep but no benefits. >> >> - we don't have an use case for it. Most OSes doesn't seem to care, and afaik no >> applications seems to care either. >> >> >> All this said, I think we can reach a consensus of keeping it if we can at least come >> up with a way of testing it. >> >> >>> >>> Your work is a good step towards to unify the configuration and the check. I think two more steps we can go further. >>> >>> 1) Remove RVI/RVF and the similar macros, and add fields for them in the configuration struct. >>> >>> 2) Unify the check about configuration. write_misa and cpu_realize_fn can use the same check function. >>> >>> >>> As we have done these two steps, I think we can go more closely for the profile extension. >> >> >> Is this the extension you're taking about? >> >> https://github.com/riscv/riscv-profiles/blob/main/profiles.adoc Zhiwei, I looked it up and at first I don't understand how writing MISA is related to this profile extension. Are you suggesting that the firmware can choose to run a specific profile and then the hardware must adapt to it on the fly? Because if not, then we can implement profiles by just passing them in the QEMU command line. >> >> >> This looks like a good reason to keep the code. Let's see if anyone else has an opinion >> about it. We can do the improvements you mentioned above as a follow-up (this series was >> really about removing RISC_FEATURE_*) if we decide to keep it. > > If we decide to keep it and not guard it by default, then we should test > and fix it now. Also, as we're already aware that it has insufficient > sanity checks for extension dependencies, then we should fix our general > extension dependency checking now too, in order to apply that to this. > IOW, trying to keep this, without some guard on it, opens a can of worms. > My vote is the same as it was before, merge this series and then revisit > this function when someone has a use/test case for it. Nobody said this > was never going to have a different implementation, just that the current > implementation is known-buggy and there's no reason to expose it now. It wouldn't be not guarded by default. In fact, in case we decide to go back to what we were doing a couple of versions ago, I would rename the 'misa-w' attribute to 'x-misa-w'. The 'x' would be an indication that this is really something experimental and expectations must be set accordingly if the user decides to enable it. In reality, what this 'x-misa-w' would do is to give us more time to stabilize the code inside write_misa(). Ideally we would get rid of it when the code is stable. Daniel > > My only concern with the code deletion is that git-blame doesn't blame > deleted code. I think we should add a comment describing the history > which includes a git commit reference which can be used to see the > latest implementation. > > Thanks, > drew > >> >> >> >> Thanks, >> >> >> Daniel >> >>> >>> >>> Zhiwei >>> >>>> Given that allowing the dormant code to write MISA can cause tricky bugs >>>> to solve later on, and we don't have a particularly interesting case of >>>> writing MISA to support today, and we're already not violating the >>>> specification, let's erase all the body of write_misa() and turn it into >>>> an official no-op instead of an accidental one. We'll keep consistent >>>> with what we provide users today but with 50+ less lines to maintain. >>>> >>>> RISCV_FEATURE_MISA enum is erased in the process since there's no one >>>> else using it. >>>> >>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> >>>> Reviewed-by: Bin Meng <bmeng@tinylab.org> >>>> Reviewed-by: Andrew Jones <ajones@ventanamicro.com> >>>> --- >>>> target/riscv/cpu.h | 1 - >>>> target/riscv/csr.c | 55 ---------------------------------------------- >>>> 2 files changed, 56 deletions(-) >>>> >>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >>>> index 7128438d8e..01803a020d 100644 >>>> --- a/target/riscv/cpu.h >>>> +++ b/target/riscv/cpu.h >>>> @@ -89,7 +89,6 @@ enum { >>>> RISCV_FEATURE_MMU, >>>> RISCV_FEATURE_PMP, >>>> RISCV_FEATURE_EPMP, >>>> - RISCV_FEATURE_MISA, >>>> RISCV_FEATURE_DEBUG >>>> }; >>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c >>>> index 1b0a0c1693..f7862ff4a4 100644 >>>> --- a/target/riscv/csr.c >>>> +++ b/target/riscv/csr.c >>>> @@ -1329,61 +1329,6 @@ static RISCVException read_misa(CPURISCVState *env, int csrno, >>>> static RISCVException write_misa(CPURISCVState *env, int csrno, >>>> target_ulong val) >>>> { >>>> - if (!riscv_feature(env, RISCV_FEATURE_MISA)) { >>>> - /* drop write to misa */ >>>> - return RISCV_EXCP_NONE; >>>> - } >>>> - >>>> - /* 'I' or 'E' must be present */ >>>> - if (!(val & (RVI | RVE))) { >>>> - /* It is not, drop write to misa */ >>>> - return RISCV_EXCP_NONE; >>>> - } >>>> - >>>> - /* 'E' excludes all other extensions */ >>>> - if (val & RVE) { >>>> - /* when we support 'E' we can do "val = RVE;" however >>>> - * for now we just drop writes if 'E' is present. >>>> - */ >>>> - return RISCV_EXCP_NONE; >>>> - } >>>> - >>>> - /* >>>> - * misa.MXL writes are not supported by QEMU. >>>> - * Drop writes to those bits. >>>> - */ >>>> - >>>> - /* Mask extensions that are not supported by this hart */ >>>> - val &= env->misa_ext_mask; >>>> - >>>> - /* Mask extensions that are not supported by QEMU */ >>>> - val &= (RVI | RVE | RVM | RVA | RVF | RVD | RVC | RVS | RVU | RVV); >>>> - >>>> - /* 'D' depends on 'F', so clear 'D' if 'F' is not present */ >>>> - if ((val & RVD) && !(val & RVF)) { >>>> - val &= ~RVD; >>>> - } >>>> - >>>> - /* Suppress 'C' if next instruction is not aligned >>>> - * TODO: this should check next_pc >>>> - */ >>>> - if ((val & RVC) && (GETPC() & ~3) != 0) { >>>> - val &= ~RVC; >>>> - } >>>> - >>>> - /* If nothing changed, do nothing. */ >>>> - if (val == env->misa_ext) { >>>> - return RISCV_EXCP_NONE; >>>> - } >>>> - >>>> - if (!(val & RVF)) { >>>> - env->mstatus &= ~MSTATUS_FS; >>>> - } >>>> - >>>> - /* flush translation cache */ >>>> - tb_flush(env_cpu(env)); >>>> - env->misa_ext = val; >>>> - env->xl = riscv_cpu_mxl(env); >>>> return RISCV_EXCP_NONE; >>>> }
On Tue, Feb 21, 2023 at 03:22:45PM -0300, Daniel Henrique Barboza wrote: > > > On 2/21/23 14:06, Andrew Jones wrote: > > On Tue, Feb 21, 2023 at 12:49:11PM -0300, Daniel Henrique Barboza wrote: > > > Hey, > > > > > > On 2/16/23 22:42, LIU Zhiwei wrote: > > > > > > > > On 2023/2/17 5:55, Daniel Henrique Barboza wrote: > > > > > At this moment, and apparently since ever, we have no way of enabling > > > > > RISCV_FEATURE_MISA. This means that all the code from write_misa(), all > > > > > the nuts and bolts that handles how to write this CSR, has always been a > > > > > no-op as well because write_misa() will always exit earlier. > > > > > > > > > > This seems to be benign in the majority of cases. Booting an Ubuntu > > > > > 'virt' guest and logging all the calls to 'write_misa' shows that no > > > > > writes to MISA CSR was attempted. Writing MISA, i.e. enabling/disabling > > > > > RISC-V extensions after the machine is powered on, seems to be a niche > > > > > use. > > > > > > > > > > Before proceeding, let's recap what the spec says about MISA. It is a > > > > > CSR that is divided in 3 fields: > > > > > > > > > > - MXL, Machine XLEN, described as "may be writable"; > > > > > > > > > > - MXLEN, the XLEN in M-mode, which is given by the setting of MXL or a > > > > > fixed value if MISA is zero; > > > > > > > > > > - Extensions is defined as "a WARL field that can contain writable bits > > > > > where the implementation allows the supported ISA to be modified" > > > > > > > > > > Thus what we have today (write_misa() being a no-op) is already a valid > > > > > spec implementation. We're not obliged to have a particular set of MISA > > > > > writable bits, and at this moment we have none. > > > > > > > > Hi Daniel, > > > > > > > > I see there has been a discussion on this topic. And as no-op has no harmfulness for current implementation. > > > > However, I still think we should make misa writable as default, which is also a valid spec implementation. > > > > > > > > One reason is that may be we need to dynamic write access for some cpus in the future. The other is we should > > > > make QEMU a more useful implementation, not just a legal implementation. We have done in many aspects on this direction. > > > > > > > > I prefer your implementation before v4. It's not a complicated implementation. And I think the other extensions on QEMU currently > > > > can mostly be configurable already. > > > > > > I don't have a strong opinion in this matter to be honest. My problems with the > > > existing code are: > > > > > > - the code is untested. I cannot say that this was never tested, but I can say that > > > this has been mostly untested ever since introduced. Which is normal for a code that > > > is 'dormant'. > > > > > > - the code is dormant and most likely with bugs, but it's still maintained. For > > > example we have e91a7227 ("target/riscv: Split misa.mxl and misa.ext") that had > > > to make changes here. So we have the upkeep but no benefits. > > > > > > - we don't have an use case for it. Most OSes doesn't seem to care, and afaik no > > > applications seems to care either. > > > > > > > > > All this said, I think we can reach a consensus of keeping it if we can at least come > > > up with a way of testing it. > > > > > > > > > > > > > > Your work is a good step towards to unify the configuration and the check. I think two more steps we can go further. > > > > > > > > 1) Remove RVI/RVF and the similar macros, and add fields for them in the configuration struct. > > > > > > > > 2) Unify the check about configuration. write_misa and cpu_realize_fn can use the same check function. > > > > > > > > > > > > As we have done these two steps, I think we can go more closely for the profile extension. > > > > > > > > > Is this the extension you're taking about? > > > > > > https://github.com/riscv/riscv-profiles/blob/main/profiles.adoc > > Zhiwei, I looked it up and at first I don't understand how writing MISA is > related to this profile extension. Are you suggesting that the firmware > can choose to run a specific profile and then the hardware must adapt to > it on the fly? Because if not, then we can implement profiles by just > passing them in the QEMU command line. > > > > > > > > > > This looks like a good reason to keep the code. Let's see if anyone else has an opinion > > > about it. We can do the improvements you mentioned above as a follow-up (this series was > > > really about removing RISC_FEATURE_*) if we decide to keep it. > > > > If we decide to keep it and not guard it by default, then we should test > > and fix it now. Also, as we're already aware that it has insufficient > > sanity checks for extension dependencies, then we should fix our general > > extension dependency checking now too, in order to apply that to this. > > IOW, trying to keep this, without some guard on it, opens a can of worms. > > My vote is the same as it was before, merge this series and then revisit > > this function when someone has a use/test case for it. Nobody said this > > was never going to have a different implementation, just that the current > > implementation is known-buggy and there's no reason to expose it now. > > > It wouldn't be not guarded by default. In fact, in case we decide to go back > to what we were doing a couple of versions ago, I would rename the 'misa-w' > attribute to 'x-misa-w'. > > The 'x' would be an indication that this is really something experimental and > expectations must be set accordingly if the user decides to enable it. In > reality, what this 'x-misa-w' would do is to give us more time to stabilize > the code inside write_misa(). Ideally we would get rid of it when the code > is stable. I'm good with bringing the code back under x-misa-w, which is off by default. Thanks, drew > > > > Daniel > > > > > My only concern with the code deletion is that git-blame doesn't blame > > deleted code. I think we should add a comment describing the history > > which includes a git commit reference which can be used to see the > > latest implementation. > > > > Thanks, > > drew > > > > > > > > > > > > > > Thanks, > > > > > > > > > Daniel > > > > > > > > > > > > > > > Zhiwei > > > > > > > > > Given that allowing the dormant code to write MISA can cause tricky bugs > > > > > to solve later on, and we don't have a particularly interesting case of > > > > > writing MISA to support today, and we're already not violating the > > > > > specification, let's erase all the body of write_misa() and turn it into > > > > > an official no-op instead of an accidental one. We'll keep consistent > > > > > with what we provide users today but with 50+ less lines to maintain. > > > > > > > > > > RISCV_FEATURE_MISA enum is erased in the process since there's no one > > > > > else using it. > > > > > > > > > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > > > > > Reviewed-by: Bin Meng <bmeng@tinylab.org> > > > > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> > > > > > --- > > > > > target/riscv/cpu.h | 1 - > > > > > target/riscv/csr.c | 55 ---------------------------------------------- > > > > > 2 files changed, 56 deletions(-) > > > > > > > > > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > > > > > index 7128438d8e..01803a020d 100644 > > > > > --- a/target/riscv/cpu.h > > > > > +++ b/target/riscv/cpu.h > > > > > @@ -89,7 +89,6 @@ enum { > > > > > RISCV_FEATURE_MMU, > > > > > RISCV_FEATURE_PMP, > > > > > RISCV_FEATURE_EPMP, > > > > > - RISCV_FEATURE_MISA, > > > > > RISCV_FEATURE_DEBUG > > > > > }; > > > > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > > > > > index 1b0a0c1693..f7862ff4a4 100644 > > > > > --- a/target/riscv/csr.c > > > > > +++ b/target/riscv/csr.c > > > > > @@ -1329,61 +1329,6 @@ static RISCVException read_misa(CPURISCVState *env, int csrno, > > > > > static RISCVException write_misa(CPURISCVState *env, int csrno, > > > > > target_ulong val) > > > > > { > > > > > - if (!riscv_feature(env, RISCV_FEATURE_MISA)) { > > > > > - /* drop write to misa */ > > > > > - return RISCV_EXCP_NONE; > > > > > - } > > > > > - > > > > > - /* 'I' or 'E' must be present */ > > > > > - if (!(val & (RVI | RVE))) { > > > > > - /* It is not, drop write to misa */ > > > > > - return RISCV_EXCP_NONE; > > > > > - } > > > > > - > > > > > - /* 'E' excludes all other extensions */ > > > > > - if (val & RVE) { > > > > > - /* when we support 'E' we can do "val = RVE;" however > > > > > - * for now we just drop writes if 'E' is present. > > > > > - */ > > > > > - return RISCV_EXCP_NONE; > > > > > - } > > > > > - > > > > > - /* > > > > > - * misa.MXL writes are not supported by QEMU. > > > > > - * Drop writes to those bits. > > > > > - */ > > > > > - > > > > > - /* Mask extensions that are not supported by this hart */ > > > > > - val &= env->misa_ext_mask; > > > > > - > > > > > - /* Mask extensions that are not supported by QEMU */ > > > > > - val &= (RVI | RVE | RVM | RVA | RVF | RVD | RVC | RVS | RVU | RVV); > > > > > - > > > > > - /* 'D' depends on 'F', so clear 'D' if 'F' is not present */ > > > > > - if ((val & RVD) && !(val & RVF)) { > > > > > - val &= ~RVD; > > > > > - } > > > > > - > > > > > - /* Suppress 'C' if next instruction is not aligned > > > > > - * TODO: this should check next_pc > > > > > - */ > > > > > - if ((val & RVC) && (GETPC() & ~3) != 0) { > > > > > - val &= ~RVC; > > > > > - } > > > > > - > > > > > - /* If nothing changed, do nothing. */ > > > > > - if (val == env->misa_ext) { > > > > > - return RISCV_EXCP_NONE; > > > > > - } > > > > > - > > > > > - if (!(val & RVF)) { > > > > > - env->mstatus &= ~MSTATUS_FS; > > > > > - } > > > > > - > > > > > - /* flush translation cache */ > > > > > - tb_flush(env_cpu(env)); > > > > > - env->misa_ext = val; > > > > > - env->xl = riscv_cpu_mxl(env); > > > > > return RISCV_EXCP_NONE; > > > > > }
On 2/21/23 15:28, Andrew Jones wrote: > On Tue, Feb 21, 2023 at 03:22:45PM -0300, Daniel Henrique Barboza wrote: >> >> >> On 2/21/23 14:06, Andrew Jones wrote: >>> On Tue, Feb 21, 2023 at 12:49:11PM -0300, Daniel Henrique Barboza wrote: >>>> Hey, >>>> >>>> On 2/16/23 22:42, LIU Zhiwei wrote: >>>>> >>>>> On 2023/2/17 5:55, Daniel Henrique Barboza wrote: >>>>>> At this moment, and apparently since ever, we have no way of enabling >>>>>> RISCV_FEATURE_MISA. This means that all the code from write_misa(), all >>>>>> the nuts and bolts that handles how to write this CSR, has always been a >>>>>> no-op as well because write_misa() will always exit earlier. >>>>>> >>>>>> This seems to be benign in the majority of cases. Booting an Ubuntu >>>>>> 'virt' guest and logging all the calls to 'write_misa' shows that no >>>>>> writes to MISA CSR was attempted. Writing MISA, i.e. enabling/disabling >>>>>> RISC-V extensions after the machine is powered on, seems to be a niche >>>>>> use. >>>>>> >>>>>> Before proceeding, let's recap what the spec says about MISA. It is a >>>>>> CSR that is divided in 3 fields: >>>>>> >>>>>> - MXL, Machine XLEN, described as "may be writable"; >>>>>> >>>>>> - MXLEN, the XLEN in M-mode, which is given by the setting of MXL or a >>>>>> fixed value if MISA is zero; >>>>>> >>>>>> - Extensions is defined as "a WARL field that can contain writable bits >>>>>> where the implementation allows the supported ISA to be modified" >>>>>> >>>>>> Thus what we have today (write_misa() being a no-op) is already a valid >>>>>> spec implementation. We're not obliged to have a particular set of MISA >>>>>> writable bits, and at this moment we have none. >>>>> >>>>> Hi Daniel, >>>>> >>>>> I see there has been a discussion on this topic. And as no-op has no harmfulness for current implementation. >>>>> However, I still think we should make misa writable as default, which is also a valid spec implementation. >>>>> >>>>> One reason is that may be we need to dynamic write access for some cpus in the future. The other is we should >>>>> make QEMU a more useful implementation, not just a legal implementation. We have done in many aspects on this direction. >>>>> >>>>> I prefer your implementation before v4. It's not a complicated implementation. And I think the other extensions on QEMU currently >>>>> can mostly be configurable already. >>>> >>>> I don't have a strong opinion in this matter to be honest. My problems with the >>>> existing code are: >>>> >>>> - the code is untested. I cannot say that this was never tested, but I can say that >>>> this has been mostly untested ever since introduced. Which is normal for a code that >>>> is 'dormant'. >>>> >>>> - the code is dormant and most likely with bugs, but it's still maintained. For >>>> example we have e91a7227 ("target/riscv: Split misa.mxl and misa.ext") that had >>>> to make changes here. So we have the upkeep but no benefits. >>>> >>>> - we don't have an use case for it. Most OSes doesn't seem to care, and afaik no >>>> applications seems to care either. >>>> >>>> >>>> All this said, I think we can reach a consensus of keeping it if we can at least come >>>> up with a way of testing it. >>>> >>>> >>>>> >>>>> Your work is a good step towards to unify the configuration and the check. I think two more steps we can go further. >>>>> >>>>> 1) Remove RVI/RVF and the similar macros, and add fields for them in the configuration struct. >>>>> >>>>> 2) Unify the check about configuration. write_misa and cpu_realize_fn can use the same check function. >>>>> >>>>> >>>>> As we have done these two steps, I think we can go more closely for the profile extension. >>>> >>>> >>>> Is this the extension you're taking about? >>>> >>>> https://github.com/riscv/riscv-profiles/blob/main/profiles.adoc >> >> Zhiwei, I looked it up and at first I don't understand how writing MISA is >> related to this profile extension. Are you suggesting that the firmware >> can choose to run a specific profile and then the hardware must adapt to >> it on the fly? Because if not, then we can implement profiles by just >> passing them in the QEMU command line. >> >>>> >>>> >>>> This looks like a good reason to keep the code. Let's see if anyone else has an opinion >>>> about it. We can do the improvements you mentioned above as a follow-up (this series was >>>> really about removing RISC_FEATURE_*) if we decide to keep it. >>> >>> If we decide to keep it and not guard it by default, then we should test >>> and fix it now. Also, as we're already aware that it has insufficient >>> sanity checks for extension dependencies, then we should fix our general >>> extension dependency checking now too, in order to apply that to this. >>> IOW, trying to keep this, without some guard on it, opens a can of worms. >>> My vote is the same as it was before, merge this series and then revisit >>> this function when someone has a use/test case for it. Nobody said this >>> was never going to have a different implementation, just that the current >>> implementation is known-buggy and there's no reason to expose it now. >> >> >> It wouldn't be not guarded by default. In fact, in case we decide to go back >> to what we were doing a couple of versions ago, I would rename the 'misa-w' >> attribute to 'x-misa-w'. >> >> The 'x' would be an indication that this is really something experimental and >> expectations must be set accordingly if the user decides to enable it. In >> reality, what this 'x-misa-w' would do is to give us more time to stabilize >> the code inside write_misa(). Ideally we would get rid of it when the code >> is stable. > > I'm good with bringing the code back under x-misa-w, which is off by > default. Ok! If no one else opposes I'll re-send v7 with write_misa() gated with x-misa-w. Daniel > > Thanks, > drew > >> >> >> >> Daniel >> >>> >>> My only concern with the code deletion is that git-blame doesn't blame >>> deleted code. I think we should add a comment describing the history >>> which includes a git commit reference which can be used to see the >>> latest implementation. >>> >>> Thanks, >>> drew >>> >>>> >>>> >>>> >>>> Thanks, >>>> >>>> >>>> Daniel >>>> >>>>> >>>>> >>>>> Zhiwei >>>>> >>>>>> Given that allowing the dormant code to write MISA can cause tricky bugs >>>>>> to solve later on, and we don't have a particularly interesting case of >>>>>> writing MISA to support today, and we're already not violating the >>>>>> specification, let's erase all the body of write_misa() and turn it into >>>>>> an official no-op instead of an accidental one. We'll keep consistent >>>>>> with what we provide users today but with 50+ less lines to maintain. >>>>>> >>>>>> RISCV_FEATURE_MISA enum is erased in the process since there's no one >>>>>> else using it. >>>>>> >>>>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> >>>>>> Reviewed-by: Bin Meng <bmeng@tinylab.org> >>>>>> Reviewed-by: Andrew Jones <ajones@ventanamicro.com> >>>>>> --- >>>>>> target/riscv/cpu.h | 1 - >>>>>> target/riscv/csr.c | 55 ---------------------------------------------- >>>>>> 2 files changed, 56 deletions(-) >>>>>> >>>>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >>>>>> index 7128438d8e..01803a020d 100644 >>>>>> --- a/target/riscv/cpu.h >>>>>> +++ b/target/riscv/cpu.h >>>>>> @@ -89,7 +89,6 @@ enum { >>>>>> RISCV_FEATURE_MMU, >>>>>> RISCV_FEATURE_PMP, >>>>>> RISCV_FEATURE_EPMP, >>>>>> - RISCV_FEATURE_MISA, >>>>>> RISCV_FEATURE_DEBUG >>>>>> }; >>>>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c >>>>>> index 1b0a0c1693..f7862ff4a4 100644 >>>>>> --- a/target/riscv/csr.c >>>>>> +++ b/target/riscv/csr.c >>>>>> @@ -1329,61 +1329,6 @@ static RISCVException read_misa(CPURISCVState *env, int csrno, >>>>>> static RISCVException write_misa(CPURISCVState *env, int csrno, >>>>>> target_ulong val) >>>>>> { >>>>>> - if (!riscv_feature(env, RISCV_FEATURE_MISA)) { >>>>>> - /* drop write to misa */ >>>>>> - return RISCV_EXCP_NONE; >>>>>> - } >>>>>> - >>>>>> - /* 'I' or 'E' must be present */ >>>>>> - if (!(val & (RVI | RVE))) { >>>>>> - /* It is not, drop write to misa */ >>>>>> - return RISCV_EXCP_NONE; >>>>>> - } >>>>>> - >>>>>> - /* 'E' excludes all other extensions */ >>>>>> - if (val & RVE) { >>>>>> - /* when we support 'E' we can do "val = RVE;" however >>>>>> - * for now we just drop writes if 'E' is present. >>>>>> - */ >>>>>> - return RISCV_EXCP_NONE; >>>>>> - } >>>>>> - >>>>>> - /* >>>>>> - * misa.MXL writes are not supported by QEMU. >>>>>> - * Drop writes to those bits. >>>>>> - */ >>>>>> - >>>>>> - /* Mask extensions that are not supported by this hart */ >>>>>> - val &= env->misa_ext_mask; >>>>>> - >>>>>> - /* Mask extensions that are not supported by QEMU */ >>>>>> - val &= (RVI | RVE | RVM | RVA | RVF | RVD | RVC | RVS | RVU | RVV); >>>>>> - >>>>>> - /* 'D' depends on 'F', so clear 'D' if 'F' is not present */ >>>>>> - if ((val & RVD) && !(val & RVF)) { >>>>>> - val &= ~RVD; >>>>>> - } >>>>>> - >>>>>> - /* Suppress 'C' if next instruction is not aligned >>>>>> - * TODO: this should check next_pc >>>>>> - */ >>>>>> - if ((val & RVC) && (GETPC() & ~3) != 0) { >>>>>> - val &= ~RVC; >>>>>> - } >>>>>> - >>>>>> - /* If nothing changed, do nothing. */ >>>>>> - if (val == env->misa_ext) { >>>>>> - return RISCV_EXCP_NONE; >>>>>> - } >>>>>> - >>>>>> - if (!(val & RVF)) { >>>>>> - env->mstatus &= ~MSTATUS_FS; >>>>>> - } >>>>>> - >>>>>> - /* flush translation cache */ >>>>>> - tb_flush(env_cpu(env)); >>>>>> - env->misa_ext = val; >>>>>> - env->xl = riscv_cpu_mxl(env); >>>>>> return RISCV_EXCP_NONE; >>>>>> }
On 2023/2/22 2:22, Daniel Henrique Barboza wrote: > > > On 2/21/23 14:06, Andrew Jones wrote: >> On Tue, Feb 21, 2023 at 12:49:11PM -0300, Daniel Henrique Barboza wrote: >>> Hey, >>> >>> On 2/16/23 22:42, LIU Zhiwei wrote: >>>> >>>> On 2023/2/17 5:55, Daniel Henrique Barboza wrote: >>>>> At this moment, and apparently since ever, we have no way of enabling >>>>> RISCV_FEATURE_MISA. This means that all the code from >>>>> write_misa(), all >>>>> the nuts and bolts that handles how to write this CSR, has always >>>>> been a >>>>> no-op as well because write_misa() will always exit earlier. >>>>> >>>>> This seems to be benign in the majority of cases. Booting an Ubuntu >>>>> 'virt' guest and logging all the calls to 'write_misa' shows that no >>>>> writes to MISA CSR was attempted. Writing MISA, i.e. >>>>> enabling/disabling >>>>> RISC-V extensions after the machine is powered on, seems to be a >>>>> niche >>>>> use. >>>>> >>>>> Before proceeding, let's recap what the spec says about MISA. It is a >>>>> CSR that is divided in 3 fields: >>>>> >>>>> - MXL, Machine XLEN, described as "may be writable"; >>>>> >>>>> - MXLEN, the XLEN in M-mode, which is given by the setting of MXL >>>>> or a >>>>> fixed value if MISA is zero; >>>>> >>>>> - Extensions is defined as "a WARL field that can contain writable >>>>> bits >>>>> where the implementation allows the supported ISA to be modified" >>>>> >>>>> Thus what we have today (write_misa() being a no-op) is already a >>>>> valid >>>>> spec implementation. We're not obliged to have a particular set of >>>>> MISA >>>>> writable bits, and at this moment we have none. >>>> >>>> Hi Daniel, >>>> >>>> I see there has been a discussion on this topic. And as no-op has >>>> no harmfulness for current implementation. >>>> However, I still think we should make misa writable as default, >>>> which is also a valid spec implementation. >>>> >>>> One reason is that may be we need to dynamic write access for some >>>> cpus in the future. The other is we should >>>> make QEMU a more useful implementation, not just a legal >>>> implementation. We have done in many aspects on this direction. >>>> >>>> I prefer your implementation before v4. It's not a complicated >>>> implementation. And I think the other extensions on QEMU currently >>>> can mostly be configurable already. >>> >>> I don't have a strong opinion in this matter to be honest. My >>> problems with the >>> existing code are: >>> >>> - the code is untested. I cannot say that this was never tested, but >>> I can say that >>> this has been mostly untested ever since introduced. Which is normal >>> for a code that >>> is 'dormant'. >>> >>> - the code is dormant and most likely with bugs, but it's still >>> maintained. For >>> example we have e91a7227 ("target/riscv: Split misa.mxl and >>> misa.ext") that had >>> to make changes here. So we have the upkeep but no benefits. >>> >>> - we don't have an use case for it. Most OSes doesn't seem to care, >>> and afaik no >>> applications seems to care either. >>> >>> >>> All this said, I think we can reach a consensus of keeping it if we >>> can at least come >>> up with a way of testing it. >>> >>> >>>> >>>> Your work is a good step towards to unify the configuration and the >>>> check. I think two more steps we can go further. >>>> >>>> 1) Remove RVI/RVF and the similar macros, and add fields for them >>>> in the configuration struct. >>>> >>>> 2) Unify the check about configuration. write_misa and >>>> cpu_realize_fn can use the same check function. >>>> >>>> >>>> As we have done these two steps, I think we can go more closely for >>>> the profile extension. >>> >>> >>> Is this the extension you're taking about? >>> >>> https://github.com/riscv/riscv-profiles/blob/main/profiles.adoc > > Zhiwei, I looked it up and at first I don't understand how writing > MISA is > related to this profile extension. Are you suggesting that the firmware > can choose to run a specific profile and then the hardware must adapt to > it on the fly? Because if not, then we can implement profiles by just > passing them in the QEMU command line. No, there is not a directly dependency relation between writing MISA and profile extension. I think we have too many places for configuring the extensions ( ISA or Non-ISA) and we don't have a unified check function. For example: 1) The cpu init function. 2) The -cpu property 3) wite_misa And if supported, the profile will be converted into the combination of extensions in cfg. It will add a new configuration point. Thus I think we should unify the check function before introduce the more configuration place. My initial idea is that we always use the cfg fields for configuration and misa is only a mapping of combination cfg fields. And the unified check function only works on cfg field. And of course, you can omit this comment for this patch set. Zhiwei > >>> >>> >>> This looks like a good reason to keep the code. Let's see if anyone >>> else has an opinion >>> about it. We can do the improvements you mentioned above as a >>> follow-up (this series was >>> really about removing RISC_FEATURE_*) if we decide to keep it. >> >> If we decide to keep it and not guard it by default, then we should test >> and fix it now. Also, as we're already aware that it has insufficient >> sanity checks for extension dependencies, then we should fix our general >> extension dependency checking now too, in order to apply that to this. >> IOW, trying to keep this, without some guard on it, opens a can of >> worms. >> My vote is the same as it was before, merge this series and then revisit >> this function when someone has a use/test case for it. Nobody said this >> was never going to have a different implementation, just that the >> current >> implementation is known-buggy and there's no reason to expose it now. > > > It wouldn't be not guarded by default. In fact, in case we decide to > go back > to what we were doing a couple of versions ago, I would rename the > 'misa-w' > attribute to 'x-misa-w'. > > The 'x' would be an indication that this is really something > experimental and > expectations must be set accordingly if the user decides to enable it. In > reality, what this 'x-misa-w' would do is to give us more time to > stabilize > the code inside write_misa(). Ideally we would get rid of it when the > code > is stable. > > > > Daniel > >> >> My only concern with the code deletion is that git-blame doesn't blame >> deleted code. I think we should add a comment describing the history >> which includes a git commit reference which can be used to see the >> latest implementation. >> >> Thanks, >> drew >> >>> >>> >>> >>> Thanks, >>> >>> >>> Daniel >>> >>>> >>>> >>>> Zhiwei >>>> >>>>> Given that allowing the dormant code to write MISA can cause >>>>> tricky bugs >>>>> to solve later on, and we don't have a particularly interesting >>>>> case of >>>>> writing MISA to support today, and we're already not violating the >>>>> specification, let's erase all the body of write_misa() and turn >>>>> it into >>>>> an official no-op instead of an accidental one. We'll keep consistent >>>>> with what we provide users today but with 50+ less lines to maintain. >>>>> >>>>> RISCV_FEATURE_MISA enum is erased in the process since there's no one >>>>> else using it. >>>>> >>>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> >>>>> Reviewed-by: Bin Meng <bmeng@tinylab.org> >>>>> Reviewed-by: Andrew Jones <ajones@ventanamicro.com> >>>>> --- >>>>> target/riscv/cpu.h | 1 - >>>>> target/riscv/csr.c | 55 >>>>> ---------------------------------------------- >>>>> 2 files changed, 56 deletions(-) >>>>> >>>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >>>>> index 7128438d8e..01803a020d 100644 >>>>> --- a/target/riscv/cpu.h >>>>> +++ b/target/riscv/cpu.h >>>>> @@ -89,7 +89,6 @@ enum { >>>>> RISCV_FEATURE_MMU, >>>>> RISCV_FEATURE_PMP, >>>>> RISCV_FEATURE_EPMP, >>>>> - RISCV_FEATURE_MISA, >>>>> RISCV_FEATURE_DEBUG >>>>> }; >>>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c >>>>> index 1b0a0c1693..f7862ff4a4 100644 >>>>> --- a/target/riscv/csr.c >>>>> +++ b/target/riscv/csr.c >>>>> @@ -1329,61 +1329,6 @@ static RISCVException >>>>> read_misa(CPURISCVState *env, int csrno, >>>>> static RISCVException write_misa(CPURISCVState *env, int csrno, >>>>> target_ulong val) >>>>> { >>>>> - if (!riscv_feature(env, RISCV_FEATURE_MISA)) { >>>>> - /* drop write to misa */ >>>>> - return RISCV_EXCP_NONE; >>>>> - } >>>>> - >>>>> - /* 'I' or 'E' must be present */ >>>>> - if (!(val & (RVI | RVE))) { >>>>> - /* It is not, drop write to misa */ >>>>> - return RISCV_EXCP_NONE; >>>>> - } >>>>> - >>>>> - /* 'E' excludes all other extensions */ >>>>> - if (val & RVE) { >>>>> - /* when we support 'E' we can do "val = RVE;" however >>>>> - * for now we just drop writes if 'E' is present. >>>>> - */ >>>>> - return RISCV_EXCP_NONE; >>>>> - } >>>>> - >>>>> - /* >>>>> - * misa.MXL writes are not supported by QEMU. >>>>> - * Drop writes to those bits. >>>>> - */ >>>>> - >>>>> - /* Mask extensions that are not supported by this hart */ >>>>> - val &= env->misa_ext_mask; >>>>> - >>>>> - /* Mask extensions that are not supported by QEMU */ >>>>> - val &= (RVI | RVE | RVM | RVA | RVF | RVD | RVC | RVS | RVU | >>>>> RVV); >>>>> - >>>>> - /* 'D' depends on 'F', so clear 'D' if 'F' is not present */ >>>>> - if ((val & RVD) && !(val & RVF)) { >>>>> - val &= ~RVD; >>>>> - } >>>>> - >>>>> - /* Suppress 'C' if next instruction is not aligned >>>>> - * TODO: this should check next_pc >>>>> - */ >>>>> - if ((val & RVC) && (GETPC() & ~3) != 0) { >>>>> - val &= ~RVC; >>>>> - } >>>>> - >>>>> - /* If nothing changed, do nothing. */ >>>>> - if (val == env->misa_ext) { >>>>> - return RISCV_EXCP_NONE; >>>>> - } >>>>> - >>>>> - if (!(val & RVF)) { >>>>> - env->mstatus &= ~MSTATUS_FS; >>>>> - } >>>>> - >>>>> - /* flush translation cache */ >>>>> - tb_flush(env_cpu(env)); >>>>> - env->misa_ext = val; >>>>> - env->xl = riscv_cpu_mxl(env); >>>>> return RISCV_EXCP_NONE; >>>>> }
On 2/22/23 06:21, LIU Zhiwei wrote: > > On 2023/2/22 2:22, Daniel Henrique Barboza wrote: >> >> >> On 2/21/23 14:06, Andrew Jones wrote: >>> On Tue, Feb 21, 2023 at 12:49:11PM -0300, Daniel Henrique Barboza wrote: >>>> Hey, >>>> >>>> On 2/16/23 22:42, LIU Zhiwei wrote: >>>>> >>>>> On 2023/2/17 5:55, Daniel Henrique Barboza wrote: >>>>>> At this moment, and apparently since ever, we have no way of enabling >>>>>> RISCV_FEATURE_MISA. This means that all the code from write_misa(), all >>>>>> the nuts and bolts that handles how to write this CSR, has always been a >>>>>> no-op as well because write_misa() will always exit earlier. >>>>>> >>>>>> This seems to be benign in the majority of cases. Booting an Ubuntu >>>>>> 'virt' guest and logging all the calls to 'write_misa' shows that no >>>>>> writes to MISA CSR was attempted. Writing MISA, i.e. enabling/disabling >>>>>> RISC-V extensions after the machine is powered on, seems to be a niche >>>>>> use. >>>>>> >>>>>> Before proceeding, let's recap what the spec says about MISA. It is a >>>>>> CSR that is divided in 3 fields: >>>>>> >>>>>> - MXL, Machine XLEN, described as "may be writable"; >>>>>> >>>>>> - MXLEN, the XLEN in M-mode, which is given by the setting of MXL or a >>>>>> fixed value if MISA is zero; >>>>>> >>>>>> - Extensions is defined as "a WARL field that can contain writable bits >>>>>> where the implementation allows the supported ISA to be modified" >>>>>> >>>>>> Thus what we have today (write_misa() being a no-op) is already a valid >>>>>> spec implementation. We're not obliged to have a particular set of MISA >>>>>> writable bits, and at this moment we have none. >>>>> >>>>> Hi Daniel, >>>>> >>>>> I see there has been a discussion on this topic. And as no-op has no harmfulness for current implementation. >>>>> However, I still think we should make misa writable as default, which is also a valid spec implementation. >>>>> >>>>> One reason is that may be we need to dynamic write access for some cpus in the future. The other is we should >>>>> make QEMU a more useful implementation, not just a legal implementation. We have done in many aspects on this direction. >>>>> >>>>> I prefer your implementation before v4. It's not a complicated implementation. And I think the other extensions on QEMU currently >>>>> can mostly be configurable already. >>>> >>>> I don't have a strong opinion in this matter to be honest. My problems with the >>>> existing code are: >>>> >>>> - the code is untested. I cannot say that this was never tested, but I can say that >>>> this has been mostly untested ever since introduced. Which is normal for a code that >>>> is 'dormant'. >>>> >>>> - the code is dormant and most likely with bugs, but it's still maintained. For >>>> example we have e91a7227 ("target/riscv: Split misa.mxl and misa.ext") that had >>>> to make changes here. So we have the upkeep but no benefits. >>>> >>>> - we don't have an use case for it. Most OSes doesn't seem to care, and afaik no >>>> applications seems to care either. >>>> >>>> >>>> All this said, I think we can reach a consensus of keeping it if we can at least come >>>> up with a way of testing it. >>>> >>>> >>>>> >>>>> Your work is a good step towards to unify the configuration and the check. I think two more steps we can go further. >>>>> >>>>> 1) Remove RVI/RVF and the similar macros, and add fields for them in the configuration struct. >>>>> >>>>> 2) Unify the check about configuration. write_misa and cpu_realize_fn can use the same check function. >>>>> >>>>> >>>>> As we have done these two steps, I think we can go more closely for the profile extension. >>>> >>>> >>>> Is this the extension you're taking about? >>>> >>>> https://github.com/riscv/riscv-profiles/blob/main/profiles.adoc >> >> Zhiwei, I looked it up and at first I don't understand how writing MISA is >> related to this profile extension. Are you suggesting that the firmware >> can choose to run a specific profile and then the hardware must adapt to >> it on the fly? Because if not, then we can implement profiles by just >> passing them in the QEMU command line. > > No, there is not a directly dependency relation between writing MISA and profile extension. > I think we have too many places for configuring the extensions ( ISA or Non-ISA) and we don't have a unified check function. > > For example: > > 1) The cpu init function. > > 2) The -cpu property > > 3) wite_misa > > And if supported, the profile will be converted into the combination of extensions in cfg. It will add a new configuration point. > Thus I think we should unify the check function before introduce the more configuration place. > > My initial idea is that we always use the cfg fields for configuration and misa is only a mapping of combination cfg fields. And the unified check function > only works on cfg field. > > And of course, you can omit this comment for this patch set. What I'm going to do then is to go back to the approach taken in v3, but gating the write_misa() implementation with a x-misa-w experimental attribute. The attribute will be default off. This will give us time to work on the cfg improvements you talked about while still having the write_misa() code around. I have some ideas about how we can have a centered configuration point like you suggested. The code freeze is March 7th, so no promises on having this fully done for this release cycle. A few cleanups here and there would be nice, so let's start small and see how that goes. Thanks, Daniel > > Zhiwei > >> >>>> >>>> >>>> This looks like a good reason to keep the code. Let's see if anyone else has an opinion >>>> about it. We can do the improvements you mentioned above as a follow-up (this series was >>>> really about removing RISC_FEATURE_*) if we decide to keep it. >>> >>> If we decide to keep it and not guard it by default, then we should test >>> and fix it now. Also, as we're already aware that it has insufficient >>> sanity checks for extension dependencies, then we should fix our general >>> extension dependency checking now too, in order to apply that to this. >>> IOW, trying to keep this, without some guard on it, opens a can of worms. >>> My vote is the same as it was before, merge this series and then revisit >>> this function when someone has a use/test case for it. Nobody said this >>> was never going to have a different implementation, just that the current >>> implementation is known-buggy and there's no reason to expose it now. >> >> >> It wouldn't be not guarded by default. In fact, in case we decide to go back >> to what we were doing a couple of versions ago, I would rename the 'misa-w' >> attribute to 'x-misa-w'. >> >> The 'x' would be an indication that this is really something experimental and >> expectations must be set accordingly if the user decides to enable it. In >> reality, what this 'x-misa-w' would do is to give us more time to stabilize >> the code inside write_misa(). Ideally we would get rid of it when the code >> is stable. >> >> >> >> Daniel >> >>> >>> My only concern with the code deletion is that git-blame doesn't blame >>> deleted code. I think we should add a comment describing the history >>> which includes a git commit reference which can be used to see the >>> latest implementation. >>> >>> Thanks, >>> drew >>> >>>> >>>> >>>> >>>> Thanks, >>>> >>>> >>>> Daniel >>>> >>>>> >>>>> >>>>> Zhiwei >>>>> >>>>>> Given that allowing the dormant code to write MISA can cause tricky bugs >>>>>> to solve later on, and we don't have a particularly interesting case of >>>>>> writing MISA to support today, and we're already not violating the >>>>>> specification, let's erase all the body of write_misa() and turn it into >>>>>> an official no-op instead of an accidental one. We'll keep consistent >>>>>> with what we provide users today but with 50+ less lines to maintain. >>>>>> >>>>>> RISCV_FEATURE_MISA enum is erased in the process since there's no one >>>>>> else using it. >>>>>> >>>>>> Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> >>>>>> Reviewed-by: Bin Meng <bmeng@tinylab.org> >>>>>> Reviewed-by: Andrew Jones <ajones@ventanamicro.com> >>>>>> --- >>>>>> target/riscv/cpu.h | 1 - >>>>>> target/riscv/csr.c | 55 ---------------------------------------------- >>>>>> 2 files changed, 56 deletions(-) >>>>>> >>>>>> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >>>>>> index 7128438d8e..01803a020d 100644 >>>>>> --- a/target/riscv/cpu.h >>>>>> +++ b/target/riscv/cpu.h >>>>>> @@ -89,7 +89,6 @@ enum { >>>>>> RISCV_FEATURE_MMU, >>>>>> RISCV_FEATURE_PMP, >>>>>> RISCV_FEATURE_EPMP, >>>>>> - RISCV_FEATURE_MISA, >>>>>> RISCV_FEATURE_DEBUG >>>>>> }; >>>>>> diff --git a/target/riscv/csr.c b/target/riscv/csr.c >>>>>> index 1b0a0c1693..f7862ff4a4 100644 >>>>>> --- a/target/riscv/csr.c >>>>>> +++ b/target/riscv/csr.c >>>>>> @@ -1329,61 +1329,6 @@ static RISCVException read_misa(CPURISCVState *env, int csrno, >>>>>> static RISCVException write_misa(CPURISCVState *env, int csrno, >>>>>> target_ulong val) >>>>>> { >>>>>> - if (!riscv_feature(env, RISCV_FEATURE_MISA)) { >>>>>> - /* drop write to misa */ >>>>>> - return RISCV_EXCP_NONE; >>>>>> - } >>>>>> - >>>>>> - /* 'I' or 'E' must be present */ >>>>>> - if (!(val & (RVI | RVE))) { >>>>>> - /* It is not, drop write to misa */ >>>>>> - return RISCV_EXCP_NONE; >>>>>> - } >>>>>> - >>>>>> - /* 'E' excludes all other extensions */ >>>>>> - if (val & RVE) { >>>>>> - /* when we support 'E' we can do "val = RVE;" however >>>>>> - * for now we just drop writes if 'E' is present. >>>>>> - */ >>>>>> - return RISCV_EXCP_NONE; >>>>>> - } >>>>>> - >>>>>> - /* >>>>>> - * misa.MXL writes are not supported by QEMU. >>>>>> - * Drop writes to those bits. >>>>>> - */ >>>>>> - >>>>>> - /* Mask extensions that are not supported by this hart */ >>>>>> - val &= env->misa_ext_mask; >>>>>> - >>>>>> - /* Mask extensions that are not supported by QEMU */ >>>>>> - val &= (RVI | RVE | RVM | RVA | RVF | RVD | RVC | RVS | RVU | RVV); >>>>>> - >>>>>> - /* 'D' depends on 'F', so clear 'D' if 'F' is not present */ >>>>>> - if ((val & RVD) && !(val & RVF)) { >>>>>> - val &= ~RVD; >>>>>> - } >>>>>> - >>>>>> - /* Suppress 'C' if next instruction is not aligned >>>>>> - * TODO: this should check next_pc >>>>>> - */ >>>>>> - if ((val & RVC) && (GETPC() & ~3) != 0) { >>>>>> - val &= ~RVC; >>>>>> - } >>>>>> - >>>>>> - /* If nothing changed, do nothing. */ >>>>>> - if (val == env->misa_ext) { >>>>>> - return RISCV_EXCP_NONE; >>>>>> - } >>>>>> - >>>>>> - if (!(val & RVF)) { >>>>>> - env->mstatus &= ~MSTATUS_FS; >>>>>> - } >>>>>> - >>>>>> - /* flush translation cache */ >>>>>> - tb_flush(env_cpu(env)); >>>>>> - env->misa_ext = val; >>>>>> - env->xl = riscv_cpu_mxl(env); >>>>>> return RISCV_EXCP_NONE; >>>>>> }
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index 7128438d8e..01803a020d 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -89,7 +89,6 @@ enum { RISCV_FEATURE_MMU, RISCV_FEATURE_PMP, RISCV_FEATURE_EPMP, - RISCV_FEATURE_MISA, RISCV_FEATURE_DEBUG }; diff --git a/target/riscv/csr.c b/target/riscv/csr.c index 1b0a0c1693..f7862ff4a4 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -1329,61 +1329,6 @@ static RISCVException read_misa(CPURISCVState *env, int csrno, static RISCVException write_misa(CPURISCVState *env, int csrno, target_ulong val) { - if (!riscv_feature(env, RISCV_FEATURE_MISA)) { - /* drop write to misa */ - return RISCV_EXCP_NONE; - } - - /* 'I' or 'E' must be present */ - if (!(val & (RVI | RVE))) { - /* It is not, drop write to misa */ - return RISCV_EXCP_NONE; - } - - /* 'E' excludes all other extensions */ - if (val & RVE) { - /* when we support 'E' we can do "val = RVE;" however - * for now we just drop writes if 'E' is present. - */ - return RISCV_EXCP_NONE; - } - - /* - * misa.MXL writes are not supported by QEMU. - * Drop writes to those bits. - */ - - /* Mask extensions that are not supported by this hart */ - val &= env->misa_ext_mask; - - /* Mask extensions that are not supported by QEMU */ - val &= (RVI | RVE | RVM | RVA | RVF | RVD | RVC | RVS | RVU | RVV); - - /* 'D' depends on 'F', so clear 'D' if 'F' is not present */ - if ((val & RVD) && !(val & RVF)) { - val &= ~RVD; - } - - /* Suppress 'C' if next instruction is not aligned - * TODO: this should check next_pc - */ - if ((val & RVC) && (GETPC() & ~3) != 0) { - val &= ~RVC; - } - - /* If nothing changed, do nothing. */ - if (val == env->misa_ext) { - return RISCV_EXCP_NONE; - } - - if (!(val & RVF)) { - env->mstatus &= ~MSTATUS_FS; - } - - /* flush translation cache */ - tb_flush(env_cpu(env)); - env->misa_ext = val; - env->xl = riscv_cpu_mxl(env); return RISCV_EXCP_NONE; }