| Message ID | 20230215185726.691759-3-dbarboza@ventanamicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | enable write_misa() and RISCV_FEATURE_* cleanups | expand |
On Thu, Feb 16, 2023 at 2:58 AM Daniel Henrique Barboza <dbarboza@ventanamicro.com> 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 properly 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. > > Regardless, the spec says that MISA is a WARL read-write CSR, and gating > the writes in the register doesn't make sense. OS and applications > should be wary of the consequences when writing it, but the write itself > must always be allowed. > > Remove the RISCV_FEATURE_MISA verification at the start of write_misa(), > removing RISCV_FEATURE_MISA altogether since there will be no more > callers of this enum. > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > --- > target/riscv/cpu.h | 1 - > target/riscv/csr.c | 5 ----- > 2 files changed, 6 deletions(-) > Reviewed-by: Bin Meng <bmeng@tinylab.org>
On 2023/2/16 02:57, 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 properly 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. > > Regardless, the spec says that MISA is a WARL read-write CSR, and gating > the writes in the register doesn't make sense. OS and applications > should be wary of the consequences when writing it, but the write itself > must always be allowed. > > Remove the RISCV_FEATURE_MISA verification at the start of write_misa(), > removing RISCV_FEATURE_MISA altogether since there will be no more > callers of this enum. > > Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> > --- > target/riscv/cpu.h | 1 - > target/riscv/csr.c | 5 ----- > 2 files changed, 6 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 e149b453da..5bd4cdbef5 100644 > --- a/target/riscv/csr.c > +++ b/target/riscv/csr.c > @@ -1329,11 +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 have a question here: If we directly remove this here without other limits, the bugs introduced by following code will be exposed to users, such as: - V may be still enabled when misa.D is cleared. - Zfh/Zfhmin may be still enabled when misa.D is cleared - Misa.U may be cleared when Misa.S is still set. ... Should we fix these bugs before this patch? Or fix them in following patchset? If we choose the latter, I think it's better to add a limitation(such as writable_mask) to the changable fields of misa here. Regards, Weiwei Li > /* 'I' or 'E' must be present */ > if (!(val & (RVI | RVE))) { > /* It is not, drop write to misa */
On Wed, Feb 15, 2023 at 03:57:18PM -0300, 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 properly 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. > > Regardless, the spec says that MISA is a WARL read-write CSR, and gating > the writes in the register doesn't make sense. OS and applications > should be wary of the consequences when writing it, but the write itself > must always be allowed. The write is already allowed, i.e. no exception is raised when writing it. The spec only says that the fields may/can be writable. So we can correctly implement the spec with just write_misa() { return RISCV_EXCP_NONE; } as it has effectively been implemented to this point. Based on Weiwei Li's pointing out of known bugs, and the fact that this function has likely never been tested, then maybe we should just implement it as above for now. Once a better solution to extension sanity checking exists and a use (or at least test) case arises, then the function could be expanded with some actually writable bits. (Also, I think that when/if we do the expansion, then something like the misa_w config proposed in the previous version of this series may still be needed in order to allow opting-in/out of the behavior change.) Thanks, drew
On Thu, Feb 16, 2023 at 5:29 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > On Wed, Feb 15, 2023 at 03:57:18PM -0300, 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 properly 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. > > > > Regardless, the spec says that MISA is a WARL read-write CSR, and gating > > the writes in the register doesn't make sense. OS and applications > > should be wary of the consequences when writing it, but the write itself > > must always be allowed. > > The write is already allowed, i.e. no exception is raised when writing it. > The spec only says that the fields may/can be writable. So we can > correctly implement the spec with just > > write_misa() > { > return RISCV_EXCP_NONE; > } Agree. Such change is still spec compliant without worrying about the bugs. > > as it has effectively been implemented to this point. > > Based on Weiwei Li's pointing out of known bugs, and the fact that > this function has likely never been tested, then maybe we should just > implement it as above for now. Once a better solution to extension > sanity checking exists and a use (or at least test) case arises, then > the function could be expanded with some actually writable bits. (Also, > I think that when/if we do the expansion, then something like the misa_w > config proposed in the previous version of this series may still be > needed in order to allow opting-in/out of the behavior change.) In QEMU RISC-V we have some examples of implementing optional spec features without exposing a config parameter. Do we need to add config parameters for those cases too? If yes, I am afraid the parameters will grow a lot. Regards, Bin
On Thu, Feb 16, 2023 at 05:33:55PM +0800, Bin Meng wrote: > On Thu, Feb 16, 2023 at 5:29 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > > > On Wed, Feb 15, 2023 at 03:57:18PM -0300, 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 properly 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. > > > > > > Regardless, the spec says that MISA is a WARL read-write CSR, and gating > > > the writes in the register doesn't make sense. OS and applications > > > should be wary of the consequences when writing it, but the write itself > > > must always be allowed. > > > > The write is already allowed, i.e. no exception is raised when writing it. > > The spec only says that the fields may/can be writable. So we can > > correctly implement the spec with just > > > > write_misa() > > { > > return RISCV_EXCP_NONE; > > } > > Agree. Such change is still spec compliant without worrying about the bugs. > > > > > as it has effectively been implemented to this point. > > > > Based on Weiwei Li's pointing out of known bugs, and the fact that > > this function has likely never been tested, then maybe we should just > > implement it as above for now. Once a better solution to extension > > sanity checking exists and a use (or at least test) case arises, then > > the function could be expanded with some actually writable bits. (Also, > > I think that when/if we do the expansion, then something like the misa_w > > config proposed in the previous version of this series may still be > > needed in order to allow opting-in/out of the behavior change.) > > In QEMU RISC-V we have some examples of implementing optional spec > features without exposing a config parameter. Do we need to add config > parameters for those cases too? If yes, I am afraid the parameters > will grow a lot. > I agree, particularly for RISC-V, the options grow quickly. How about this for a policy? 1) When adding an optional, on-by-default CPU feature, which applies to all currently existing CPU types, then just add the feature without a config. 2) When, later, a CPU type or use case needs to disable an optional CPU feature, which doesn't have a config, then the config is added at that time. While that policy seems reasonable (to me), I have a feeling the "applies to all currently existing CPU types" requirement won't be easily satisfied, so we'll end up mostly always adding configs anyway. We can always change RISCVCPUConfig.ext_* to a bitmap if we feel like the CPU is getting too bloated. Thanks, drew
On 2/16/23 06:29, Andrew Jones wrote: > On Wed, Feb 15, 2023 at 03:57:18PM -0300, 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 properly 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. >> >> Regardless, the spec says that MISA is a WARL read-write CSR, and gating >> the writes in the register doesn't make sense. OS and applications >> should be wary of the consequences when writing it, but the write itself >> must always be allowed. > > The write is already allowed, i.e. no exception is raised when writing it. > The spec only says that the fields may/can be writable. So we can > correctly implement the spec with just > > write_misa() > { > return RISCV_EXCP_NONE; > } > > as it has effectively been implemented to this point. I'm fine with that. We can always use git history to see the removed code and reintroduce it back. Thanks, Daniel > > Based on Weiwei Li's pointing out of known bugs, and the fact that > this function has likely never been tested, then maybe we should just > implement it as above for now. Once a better solution to extension > sanity checking exists and a use (or at least test) case arises, then > the function could be expanded with some actually writable bits. (Also, > I think that when/if we do the expansion, then something like the misa_w > config proposed in the previous version of this series may still be > needed in order to allow opting-in/out of the behavior change.) > > Thanks, > drew
On Thu, Feb 16, 2023 at 6:08 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > On Thu, Feb 16, 2023 at 05:33:55PM +0800, Bin Meng wrote: > > On Thu, Feb 16, 2023 at 5:29 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > > > > > On Wed, Feb 15, 2023 at 03:57:18PM -0300, 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 properly 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. > > > > > > > > Regardless, the spec says that MISA is a WARL read-write CSR, and gating > > > > the writes in the register doesn't make sense. OS and applications > > > > should be wary of the consequences when writing it, but the write itself > > > > must always be allowed. > > > > > > The write is already allowed, i.e. no exception is raised when writing it. > > > The spec only says that the fields may/can be writable. So we can > > > correctly implement the spec with just > > > > > > write_misa() > > > { > > > return RISCV_EXCP_NONE; > > > } > > > > Agree. Such change is still spec compliant without worrying about the bugs. > > > > > > > > as it has effectively been implemented to this point. > > > > > > Based on Weiwei Li's pointing out of known bugs, and the fact that > > > this function has likely never been tested, then maybe we should just > > > implement it as above for now. Once a better solution to extension > > > sanity checking exists and a use (or at least test) case arises, then > > > the function could be expanded with some actually writable bits. (Also, > > > I think that when/if we do the expansion, then something like the misa_w > > > config proposed in the previous version of this series may still be > > > needed in order to allow opting-in/out of the behavior change.) > > > > In QEMU RISC-V we have some examples of implementing optional spec > > features without exposing a config parameter. Do we need to add config > > parameters for those cases too? If yes, I am afraid the parameters > > will grow a lot. > > > > I agree, particularly for RISC-V, the options grow quickly. How about this > for a policy? > > 1) When adding an optional, on-by-default CPU feature, which applies to > all currently existing CPU types, then just add the feature without a > config. > > 2) When, later, a CPU type or use case needs to disable an optional > CPU feature, which doesn't have a config, then the config is added > at that time. This policy sounds good to me. > While that policy seems reasonable (to me), I have a feeling the "applies > to all currently existing CPU types" requirement won't be easily > satisfied, so we'll end up mostly always adding configs anyway. Probably this does not apply to machines that have fixed configuration RISC-V processor. But let's see what happens. > We can always change RISCVCPUConfig.ext_* to a bitmap if we feel like the > CPU is getting too bloated. Regards, Bin
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 e149b453da..5bd4cdbef5 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -1329,11 +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 */
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 properly 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. Regardless, the spec says that MISA is a WARL read-write CSR, and gating the writes in the register doesn't make sense. OS and applications should be wary of the consequences when writing it, but the write itself must always be allowed. Remove the RISCV_FEATURE_MISA verification at the start of write_misa(), removing RISCV_FEATURE_MISA altogether since there will be no more callers of this enum. Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com> --- target/riscv/cpu.h | 1 - target/riscv/csr.c | 5 ----- 2 files changed, 6 deletions(-)