Re: [PATCH 0/2] target/riscv: Separate implicitly-enabled and explicitly-enabled extensions

[Daniel Henrique Barboza]

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(-)