Re: [Patch 05/14] target/riscv: Fix relationship between V, Zve*, F and D

[Daniel Henrique Barboza]

On 2/14/23 10:40, weiwei wrote:
> On 2023/2/14 21:21, Daniel Henrique Barboza wrote:
> > On 2/14/23 05:38, Weiwei Li wrote:
> > > Add dependence chain:
> > > *  V => Zve64d => Zve64f => Zve32f => F
> > > *  V => Zve64d => D
> > >
> > > Signed-off-by: Weiwei Li <liweiwei@iscas.ac.cn>
> > > Signed-off-by: Junqiang Wang <wangjunqiang@iscas.ac.cn>
> > > ---
> > >   target/riscv/cpu.c | 21 ++++++++++++++++++---
> > >   1 file changed, 18 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > index 9a89bea2a3..4797ef9c42 100644
> > > --- a/target/riscv/cpu.c
> > > +++ b/target/riscv/cpu.c
> > > @@ -743,12 +743,27 @@ static void riscv_cpu_validate_set_extensions(RISCVCPU 
> > > *cpu, Error **errp)
> > >           return;
> > >       }
> > >   -    if (cpu->cfg.ext_v && !cpu->cfg.ext_d) {
> > > -        error_setg(errp, "V extension requires D extension");
> > > +    /* The V vector extension depends on the Zve64d extension */
> > > +    if (cpu->cfg.ext_v) {
> > > +        cpu->cfg.ext_zve64d = true;
> > > +    }
> > > +
> > > +    /* The Zve64d extension depends on the Zve64f extension */
> > > +    if (cpu->cfg.ext_zve64d) {
> > > +        cpu->cfg.ext_zve64f = true;
> > > +    }
> > > +
> > > +    /* The Zve64f extension depends on the Zve32f extension */
> > > +    if (cpu->cfg.ext_zve64f) {
> > > +        cpu->cfg.ext_zve32f = true;
> > > +    }
> > > +
> > > +    if (cpu->cfg.ext_zve64d && !cpu->cfg.ext_d) {
> > > +        error_setg(errp, "Zve64d extensions require D extension");
> > >           return;
> >
> > I'll be honest and confess that I wrote a short essay about the problems I have
> > with this code. I gave up because in the end it's all stuff that we've been 
> > doing
> > for a long time in riscv_cpu_validate_set_extensions(). I'll see if I can work 
> > in
> > a redesign of that function and in how we're setting extensions automatically
> > without checking user input and so on.
> >
> > For now I'll say that this error message seems weird because Zve64d was set to 
> > true
> > without user input. So this ends up happening:
> >
> > $ ./qemu-system-riscv64 -M virt -cpu rv64,v=true,d=false
> > qemu-system-riscv64: Zve64d extensions require D extension
> >
> > It's weird because the user didn't enabled Zve64d but the error message is 
> > complaining
> > about it. Given that the root cause is that ext_v was set, and then we've set 
> > other
> > extensions under the hood, a saner error message in this case would be "V 
> > extension
> > requires D extension".
> >
> >
> > Thanks,
> >
> >
> > Daniel
>
> Thanks for your comments.
>
> V extension depends on Zve64d(which is actually parts of V). So Zve64d will be 
> enabled when V is enabled.
>
> And in fact, only the instructions in the Zve64d part of V require D extension.
>
> To make it more readable, maybe it can be change to :
>
> "Zve64d (or V) extension requires D extension"


Yes, that looks better to me. Thanks,


Daniel

> Regards,
>
> Weiwei Li
>
> > >       }
> > >   -    if ((cpu->cfg.ext_zve32f || cpu->cfg.ext_zve64f) && !cpu->cfg.ext_f) {
> > > +    if (cpu->cfg.ext_zve32f && !cpu->cfg.ext_f) {
> > >           error_setg(errp, "Zve32f/Zve64f extensions require F extension");
> > >           return;
> > >       }