| Message ID | 20230410033526.31708-1-liweiwei@iscas.ac.cn (mailing list archive) |
|---|---|
| Headers | show |
| Series | target/riscv: Separate implicitly-enabled and explicitly-enabled extensions | expand |
Hi, On 4/10/23 00:35, Weiwei Li wrote: > The patch tries to separate the multi-letter extensions that may implicitly-enabled by misa.EXT from the explicitly-enabled cases, so that the misa.EXT can truely disabled by write_misa(). > With this separation, the implicitly-enabled zve64d/f and zve32f extensions will no work if we clear misa.V. And clear misa.V will have no effect on the explicitly-enalbed zve64d/f and zve32f extensions. For this particular case of write_misa() I'm not sure if we need all that. If we want to grant user choice on write_misa(), let's say that the user wants to enable/disable RVV, we can enable/disable all RVV related Z-extensions by hand. It's just a matter of writing enable/disable code that write_misa() would use. In the end, write_misa() is also an user choice. If write_misa() wants to disable RVV, this means that the user wants to disable RVV, so it doesn't matter whether the user enabled zve32f on the command line or not - we disable zve32f as well. Same thing for RVC and its related Z-extensions. The reason why I didn't do this particular code for RVC and RVV is because we have pending work in the ML that I would like to get it merged first. And there's a few caveats we need to decide what to do, e.g. what if the user disables F but V is enabled? Do we refuse write_misa()? Do we disable RVV? All this said, patch 1 is still a good addition to make it easier to distinguish the Z-extensions we're enabling due to MISA bits. I believe we should use set_implicit_extensions_from_ext() in the future for all similar situations. Thanks, Daniel > > Weiwei Li (2): > target/riscv: Add set_implicit_extensions_from_ext() function > target/riscv: Add ext_z*_enabled for implicitly enabled extensions > > target/riscv/cpu.c | 73 +++++++++++++++---------- > target/riscv/cpu.h | 8 +++ > target/riscv/cpu_helper.c | 2 +- > target/riscv/csr.c | 2 +- > target/riscv/insn_trans/trans_rvd.c.inc | 2 +- > target/riscv/insn_trans/trans_rvf.c.inc | 2 +- > target/riscv/insn_trans/trans_rvi.c.inc | 5 +- > target/riscv/insn_trans/trans_rvv.c.inc | 16 +++--- > target/riscv/translate.c | 4 +- > 9 files changed, 68 insertions(+), 46 deletions(-) >
On 2023/4/10 21:48, Daniel Henrique Barboza wrote: > Hi, > > On 4/10/23 00:35, Weiwei Li wrote: >> The patch tries to separate the multi-letter extensions that may >> implicitly-enabled by misa.EXT from the explicitly-enabled cases, so >> that the misa.EXT can truely disabled by write_misa(). >> With this separation, the implicitly-enabled zve64d/f and zve32f >> extensions will no work if we clear misa.V. And clear misa.V will >> have no effect on the explicitly-enalbed zve64d/f and zve32f extensions. > > For this particular case of write_misa() I'm not sure if we need all > that. If we want > to grant user choice on write_misa(), let's say that the user wants to > enable/disable > RVV, we can enable/disable all RVV related Z-extensions by hand. It's > just a matter > of writing enable/disable code that write_misa() would use. > > In the end, write_misa() is also an user choice. If write_misa() wants > to disable RVV, > this means that the user wants to disable RVV, so it doesn't matter > whether the user > enabled zve32f on the command line or not - we disable zve32f as well. > Same thing for > RVC and its related Z-extensions. > Yeah. It's also a choice. It's a question whether we take C_Zca and C as the same user choice. If we consider them as different, then this patch works. And this patch can bypass the priv version problem. > The reason why I didn't do this particular code for RVC and RVV is > because we have > pending work in the ML that I would like to get it merged first. And > there's a few > caveats we need to decide what to do, e.g. what if the user disables F > but V is > enabled? Do we refuse write_misa()? Do we disable RVV? > In section 3.1.1 of privilege spec: "If an ISA feature x depends on an ISA feature y, then attempting to enable feature x but disable feature y results in both features being disabled." Even though there is also another description in the following content of the same section: "An implementation may impose additional constraints on the collective setting of two or more misa fields, in which case they function collectively as a single WARL field. An attempt to write an unsupported combination causes those bits to be set to some supported combination." I think the former description is more explicit. Regards, Weiwei Li > > All this said, patch 1 is still a good addition to make it easier to > distinguish > the Z-extensions we're enabling due to MISA bits. I believe we should use > set_implicit_extensions_from_ext() in the future for all similar > situations. > > > > Thanks, > > Daniel > > > >> >> Weiwei Li (2): >> target/riscv: Add set_implicit_extensions_from_ext() function >> target/riscv: Add ext_z*_enabled for implicitly enabled extensions >> >> target/riscv/cpu.c | 73 +++++++++++++++---------- >> target/riscv/cpu.h | 8 +++ >> target/riscv/cpu_helper.c | 2 +- >> target/riscv/csr.c | 2 +- >> target/riscv/insn_trans/trans_rvd.c.inc | 2 +- >> target/riscv/insn_trans/trans_rvf.c.inc | 2 +- >> target/riscv/insn_trans/trans_rvi.c.inc | 5 +- >> target/riscv/insn_trans/trans_rvv.c.inc | 16 +++--- >> target/riscv/translate.c | 4 +- >> 9 files changed, 68 insertions(+), 46 deletions(-) >>
On 4/10/23 11:20, liweiwei wrote: > > On 2023/4/10 21:48, Daniel Henrique Barboza wrote: >> Hi, >> >> On 4/10/23 00:35, Weiwei Li wrote: >>> The patch tries to separate the multi-letter extensions that may implicitly-enabled by misa.EXT from the explicitly-enabled cases, so that the misa.EXT can truely disabled by write_misa(). >>> With this separation, the implicitly-enabled zve64d/f and zve32f extensions will no work if we clear misa.V. And clear misa.V will have no effect on the explicitly-enalbed zve64d/f and zve32f extensions. >> >> For this particular case of write_misa() I'm not sure if we need all that. If we want >> to grant user choice on write_misa(), let's say that the user wants to enable/disable >> RVV, we can enable/disable all RVV related Z-extensions by hand. It's just a matter >> of writing enable/disable code that write_misa() would use. >> >> In the end, write_misa() is also an user choice. If write_misa() wants to disable RVV, >> this means that the user wants to disable RVV, so it doesn't matter whether the user >> enabled zve32f on the command line or not - we disable zve32f as well. Same thing for >> RVC and its related Z-extensions. >> > Yeah. It's also a choice. It's a question whether we take C_Zca and C as the same user choice. > > If we consider them as different, then this patch works. And this patch can bypass the priv version problem. > >> The reason why I didn't do this particular code for RVC and RVV is because we have >> pending work in the ML that I would like to get it merged first. And there's a few >> caveats we need to decide what to do, e.g. what if the user disables F but V is >> enabled? Do we refuse write_misa()? Do we disable RVV? >> > In section 3.1.1 of privilege spec: > > "If an ISA feature x depends on an ISA feature y, then attempting to enable feature x but disable > > feature y results in both features being disabled." > > Even though there is also another description in the following content of the same section: > > "An implementation may impose additional constraints on the collective setting of two or more misa > fields, in which case they function collectively as a single WARL field. An attempt to write an > unsupported combination causes those bits to be set to some supported combination." > > I think the former description is more explicit. Yeah. Disabling a MISA bit should disable all dependencies of the bit and there's not much to discuss about it. As far as the current write_misa() impl in the mailing list goes, we're refusing to disable the bit (e.g. the validation would fail if RVF is disabled while keeping all its dependencies, write_misa() becomes a no-op). If we decide to give users this kind of control I believe we should disregard all user choice during boot and enable/disable extensions as required. Thanks, Daniel > > Regards, > > Weiwei Li > >> >> All this said, patch 1 is still a good addition to make it easier to distinguish >> the Z-extensions we're enabling due to MISA bits. I believe we should use >> set_implicit_extensions_from_ext() in the future for all similar situations. >> >> >> >> Thanks, >> >> Daniel >> >> >> >>> >>> Weiwei Li (2): >>> target/riscv: Add set_implicit_extensions_from_ext() function >>> target/riscv: Add ext_z*_enabled for implicitly enabled extensions >>> >>> target/riscv/cpu.c | 73 +++++++++++++++---------- >>> target/riscv/cpu.h | 8 +++ >>> target/riscv/cpu_helper.c | 2 +- >>> target/riscv/csr.c | 2 +- >>> target/riscv/insn_trans/trans_rvd.c.inc | 2 +- >>> target/riscv/insn_trans/trans_rvf.c.inc | 2 +- >>> target/riscv/insn_trans/trans_rvi.c.inc | 5 +- >>> target/riscv/insn_trans/trans_rvv.c.inc | 16 +++--- >>> target/riscv/translate.c | 4 +- >>> 9 files changed, 68 insertions(+), 46 deletions(-) >>> >
On 2023/4/10 21:48, Daniel Henrique Barboza wrote: > Hi, > > On 4/10/23 00:35, Weiwei Li wrote: >> The patch tries to separate the multi-letter extensions that may >> implicitly-enabled by misa.EXT from the explicitly-enabled cases, so >> that the misa.EXT can truely disabled by write_misa(). >> With this separation, the implicitly-enabled zve64d/f and zve32f >> extensions will no work if we clear misa.V. And clear misa.V will >> have no effect on the explicitly-enalbed zve64d/f and zve32f extensions. > > For this particular case of write_misa() I'm not sure if we need all > that. If we want > to grant user choice on write_misa(), let's say that the user wants to > enable/disable > RVV, we can enable/disable all RVV related Z-extensions by hand. It's > just a matter > of writing enable/disable code that write_misa() would use. > > In the end, write_misa() is also an user choice. If write_misa() wants > to disable RVV, > this means that the user wants to disable RVV, so it doesn't matter > whether the user > enabled zve32f on the command line or not - we disable zve32f as well. > Same thing for > RVC and its related Z-extensions. I just find we should also separate the cases here. Zcmp/Zcmt require Zca. If we disable Zca when misa.C is cleared, whether we need disable Zcmp/Zcmt? If yes, then we need another new parameter(to indicate they are diabled by clearing misa.C ) to decide whether we should re-enable them when misa.C is set. If not, we should make clearing misa.C failed in this case. Regards, Weiwei Li > > The reason why I didn't do this particular code for RVC and RVV is > because we have > pending work in the ML that I would like to get it merged first. And > there's a few > caveats we need to decide what to do, e.g. what if the user disables F > but V is > enabled? Do we refuse write_misa()? Do we disable RVV? > > > All this said, patch 1 is still a good addition to make it easier to > distinguish > the Z-extensions we're enabling due to MISA bits. I believe we should use > set_implicit_extensions_from_ext() in the future for all similar > situations. > > > > Thanks, > > Daniel > > > >> >> Weiwei Li (2): >> target/riscv: Add set_implicit_extensions_from_ext() function >> target/riscv: Add ext_z*_enabled for implicitly enabled extensions >> >> target/riscv/cpu.c | 73 +++++++++++++++---------- >> target/riscv/cpu.h | 8 +++ >> target/riscv/cpu_helper.c | 2 +- >> target/riscv/csr.c | 2 +- >> target/riscv/insn_trans/trans_rvd.c.inc | 2 +- >> target/riscv/insn_trans/trans_rvf.c.inc | 2 +- >> target/riscv/insn_trans/trans_rvi.c.inc | 5 +- >> target/riscv/insn_trans/trans_rvv.c.inc | 16 +++--- >> target/riscv/translate.c | 4 +- >> 9 files changed, 68 insertions(+), 46 deletions(-) >>
On 2023/4/11 02:35, Daniel Henrique Barboza wrote: > > > On 4/10/23 11:20, liweiwei wrote: >> >> On 2023/4/10 21:48, Daniel Henrique Barboza wrote: >>> Hi, >>> >>> On 4/10/23 00:35, Weiwei Li wrote: >>>> The patch tries to separate the multi-letter extensions that may >>>> implicitly-enabled by misa.EXT from the explicitly-enabled cases, >>>> so that the misa.EXT can truely disabled by write_misa(). >>>> With this separation, the implicitly-enabled zve64d/f and zve32f >>>> extensions will no work if we clear misa.V. And clear misa.V will >>>> have no effect on the explicitly-enalbed zve64d/f and zve32f >>>> extensions. >>> >>> For this particular case of write_misa() I'm not sure if we need all >>> that. If we want >>> to grant user choice on write_misa(), let's say that the user wants >>> to enable/disable >>> RVV, we can enable/disable all RVV related Z-extensions by hand. >>> It's just a matter >>> of writing enable/disable code that write_misa() would use. >>> >>> In the end, write_misa() is also an user choice. If write_misa() >>> wants to disable RVV, >>> this means that the user wants to disable RVV, so it doesn't matter >>> whether the user >>> enabled zve32f on the command line or not - we disable zve32f as >>> well. Same thing for >>> RVC and its related Z-extensions. >>> >> Yeah. It's also a choice. It's a question whether we take C_Zca and C >> as the same user choice. >> >> If we consider them as different, then this patch works. And this >> patch can bypass the priv version problem. >> >>> The reason why I didn't do this particular code for RVC and RVV is >>> because we have >>> pending work in the ML that I would like to get it merged first. And >>> there's a few >>> caveats we need to decide what to do, e.g. what if the user disables >>> F but V is >>> enabled? Do we refuse write_misa()? Do we disable RVV? >>> >> In section 3.1.1 of privilege spec: >> >> "If an ISA feature x depends on an ISA feature y, then attempting to >> enable feature x but disable >> >> feature y results in both features being disabled." >> >> Even though there is also another description in the following >> content of the same section: >> >> "An implementation may impose additional constraints on the >> collective setting of two or more misa >> fields, in which case they function collectively as a single WARL >> field. An attempt to write an >> unsupported combination causes those bits to be set to some supported >> combination." >> >> I think the former description is more explicit. > > Yeah. Disabling a MISA bit should disable all dependencies of the bit > and there's > not much to discuss about it. > > As far as the current write_misa() impl in the mailing list goes, > we're refusing to > disable the bit (e.g. the validation would fail if RVF is disabled > while keeping all > its dependencies, write_misa() becomes a no-op). > > If we decide to give users this kind of control I believe we should > disregard all user > choice during boot and enable/disable extensions as required. Sorry, I don't get your idea here. Why should we disregard all user choice during boot? Regards, Weiwei Li > > > > Thanks, > > Daniel > >> >> Regards, >> >> Weiwei Li >> >>> >>> All this said, patch 1 is still a good addition to make it easier to >>> distinguish >>> the Z-extensions we're enabling due to MISA bits. I believe we >>> should use >>> set_implicit_extensions_from_ext() in the future for all similar >>> situations. >>> >>> >>> >>> Thanks, >>> >>> Daniel >>> >>> >>> >>>> >>>> Weiwei Li (2): >>>> target/riscv: Add set_implicit_extensions_from_ext() function >>>> target/riscv: Add ext_z*_enabled for implicitly enabled extensions >>>> >>>> target/riscv/cpu.c | 73 >>>> +++++++++++++++---------- >>>> target/riscv/cpu.h | 8 +++ >>>> target/riscv/cpu_helper.c | 2 +- >>>> target/riscv/csr.c | 2 +- >>>> target/riscv/insn_trans/trans_rvd.c.inc | 2 +- >>>> target/riscv/insn_trans/trans_rvf.c.inc | 2 +- >>>> target/riscv/insn_trans/trans_rvi.c.inc | 5 +- >>>> target/riscv/insn_trans/trans_rvv.c.inc | 16 +++--- >>>> target/riscv/translate.c | 4 +- >>>> 9 files changed, 68 insertions(+), 46 deletions(-) >>>> >>
On 4/10/23 21:15, liweiwei wrote: > > On 2023/4/10 21:48, Daniel Henrique Barboza wrote: >> Hi, >> >> On 4/10/23 00:35, Weiwei Li wrote: >>> The patch tries to separate the multi-letter extensions that may implicitly-enabled by misa.EXT from the explicitly-enabled cases, so that the misa.EXT can truely disabled by write_misa(). >>> With this separation, the implicitly-enabled zve64d/f and zve32f extensions will no work if we clear misa.V. And clear misa.V will have no effect on the explicitly-enalbed zve64d/f and zve32f extensions. >> >> For this particular case of write_misa() I'm not sure if we need all that. If we want >> to grant user choice on write_misa(), let's say that the user wants to enable/disable >> RVV, we can enable/disable all RVV related Z-extensions by hand. It's just a matter >> of writing enable/disable code that write_misa() would use. >> >> In the end, write_misa() is also an user choice. If write_misa() wants to disable RVV, >> this means that the user wants to disable RVV, so it doesn't matter whether the user >> enabled zve32f on the command line or not - we disable zve32f as well. Same thing for >> RVC and its related Z-extensions. > > I just find we should also separate the cases here. Zcmp/Zcmt require Zca. > > If we disable Zca when misa.C is cleared, whether we need disable Zcmp/Zcmt? IMO write_misa() shouldn't be able to disable Z-extensions that it can't turn it back on. E.g. RVV disabling zve64d/f is ok because, if we re-enable RVV, they'll be re-enabled back. > > If yes, then we need another new parameter(to indicate they are diabled by > > clearing misa.C ) to decide whether we should re-enable them when misa.C is set. > > If not, we should make clearing misa.C failed in this case. I'd rather fail the operation. write_misa() should always make reversible operations. If a certain CSR write would put us in a state that we can't go back on, we should fail. For now I think I'll go back to that cleanup I made, rebase it, get one of Weiwei patches that fixes the sifive breakage I talked about in the other thread, and see if we can get that more simple version of write_misa() merged. We can continue these discussions on top of that code. Thanks, Daniel > > Regards, > > Weiwei Li > >> >> The reason why I didn't do this particular code for RVC and RVV is because we have >> pending work in the ML that I would like to get it merged first. And there's a few >> caveats we need to decide what to do, e.g. what if the user disables F but V is >> enabled? Do we refuse write_misa()? Do we disable RVV? >> >> >> All this said, patch 1 is still a good addition to make it easier to distinguish >> the Z-extensions we're enabling due to MISA bits. I believe we should use >> set_implicit_extensions_from_ext() in the future for all similar situations. >> >> >> >> Thanks, >> >> Daniel >> >> >> >>> >>> Weiwei Li (2): >>> target/riscv: Add set_implicit_extensions_from_ext() function >>> target/riscv: Add ext_z*_enabled for implicitly enabled extensions >>> >>> target/riscv/cpu.c | 73 +++++++++++++++---------- >>> target/riscv/cpu.h | 8 +++ >>> target/riscv/cpu_helper.c | 2 +- >>> target/riscv/csr.c | 2 +- >>> target/riscv/insn_trans/trans_rvd.c.inc | 2 +- >>> target/riscv/insn_trans/trans_rvf.c.inc | 2 +- >>> target/riscv/insn_trans/trans_rvi.c.inc | 5 +- >>> target/riscv/insn_trans/trans_rvv.c.inc | 16 +++--- >>> target/riscv/translate.c | 4 +- >>> 9 files changed, 68 insertions(+), 46 deletions(-) >>> >