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

[liweiwei]

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