[v1] disas/riscv: Add vendor extension support

[PATCH 6/9] target/riscv/cpu: Share RISCVCPUConfig with disassembler

Posted by Christoph Muellner 2 years, 8 months ago

From: Christoph Müllner <christoph.muellner@vrull.eu>

The disassembler needs the available extensions in order
to properly decode instructions in case of overlapping
encodings (e.g. for vendor extensions).

Let's use the field 'disassemble_info::private_data' to store
our RISCVCPUConfig pointer.

Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
---
 target/riscv/cpu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 5b7818dbd1..6f0cd9a0bb 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -819,6 +819,9 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
 {
     RISCVCPU *cpu = RISCV_CPU(s);
     CPURISCVState *env = &cpu->env;
+    RISCVCPUConfig *cfg = &cpu->cfg;
+
+    info->private_data = cfg;
 
     switch (env->xl) {
     case MXL_RV32:
-- 
2.40.1

Re: [PATCH 6/9] target/riscv/cpu: Share RISCVCPUConfig with disassembler

Posted by LIU Zhiwei 2 years, 8 months ago

On 2023/5/30 21:18, Christoph Muellner wrote:
> From: Christoph Müllner <christoph.muellner@vrull.eu>
>
> The disassembler needs the available extensions in order
> to properly decode instructions in case of overlapping
> encodings (e.g. for vendor extensions).
>
> Let's use the field 'disassemble_info::private_data' to store
> our RISCVCPUConfig pointer.
>
> Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
> ---
>   target/riscv/cpu.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 5b7818dbd1..6f0cd9a0bb 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -819,6 +819,9 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
>   {
>       RISCVCPU *cpu = RISCV_CPU(s);
>       CPURISCVState *env = &cpu->env;
> +    RISCVCPUConfig *cfg = &cpu->cfg;
> +
> +    info->private_data = cfg;

I don't know if this field will be overridden by the binutils. Can we 
extend the struct disassemble_info, and add some fields like supporting 
for Capstone?

Zhiwei

>   
>       switch (env->xl) {
>       case MXL_RV32:

Re: [PATCH 6/9] target/riscv/cpu: Share RISCVCPUConfig with disassembler

Posted by Christoph Müllner 2 years, 8 months ago

On Mon, Jun 12, 2023 at 8:25 AM LIU Zhiwei <baxiantai@gmail.com> wrote:
>
>
> On 2023/5/30 21:18, Christoph Muellner wrote:
> > From: Christoph Müllner <christoph.muellner@vrull.eu>
> >
> > The disassembler needs the available extensions in order
> > to properly decode instructions in case of overlapping
> > encodings (e.g. for vendor extensions).
> >
> > Let's use the field 'disassemble_info::private_data' to store
> > our RISCVCPUConfig pointer.
> >
> > Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
> > ---
> >   target/riscv/cpu.c | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index 5b7818dbd1..6f0cd9a0bb 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -819,6 +819,9 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
> >   {
> >       RISCVCPU *cpu = RISCV_CPU(s);
> >       CPURISCVState *env = &cpu->env;
> > +    RISCVCPUConfig *cfg = &cpu->cfg;
> > +
> > +    info->private_data = cfg;
>
> I don't know if this field will be overridden by the binutils. Can we
> extend the struct disassemble_info, and add some fields like supporting
> for Capstone?


Initially I wanted to add a new field, but then I noticed that the field
'disassemble_info::private_data' is used for a similar purpose by
disas/cris.c, disas/m68k.c, and dias/xtensa.c.
So I decided to not add yet another field, which only serves one architecture.

But if that's the preferred way, then I can change.

Thanks
Christoph

>
> Zhiwei
>
> >
> >       switch (env->xl) {
> >       case MXL_RV32:

Re: [PATCH 6/9] target/riscv/cpu: Share RISCVCPUConfig with disassembler

Posted by LIU Zhiwei 2 years, 8 months ago

On 2023/6/12 17:47, Christoph Müllner wrote:
> On Mon, Jun 12, 2023 at 8:25 AM LIU Zhiwei <baxiantai@gmail.com> wrote:
>>
>> On 2023/5/30 21:18, Christoph Muellner wrote:
>>> From: Christoph Müllner <christoph.muellner@vrull.eu>
>>>
>>> The disassembler needs the available extensions in order
>>> to properly decode instructions in case of overlapping
>>> encodings (e.g. for vendor extensions).
>>>
>>> Let's use the field 'disassemble_info::private_data' to store
>>> our RISCVCPUConfig pointer.
>>>
>>> Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
>>> ---
>>>    target/riscv/cpu.c | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>>> index 5b7818dbd1..6f0cd9a0bb 100644
>>> --- a/target/riscv/cpu.c
>>> +++ b/target/riscv/cpu.c
>>> @@ -819,6 +819,9 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
>>>    {
>>>        RISCVCPU *cpu = RISCV_CPU(s);
>>>        CPURISCVState *env = &cpu->env;
>>> +    RISCVCPUConfig *cfg = &cpu->cfg;
>>> +
>>> +    info->private_data = cfg;
>> I don't know if this field will be overridden by the binutils. Can we
>> extend the struct disassemble_info, and add some fields like supporting
>> for Capstone?
>
> Initially I wanted to add a new field, but then I noticed that the field
> 'disassemble_info::private_data' is used for a similar purpose by
> disas/cris.c, disas/m68k.c, and dias/xtensa.c.
> So I decided to not add yet another field, which only serves one architecture.
I think you can CC these arch maintainers to see if it need some 
specially process before using the private_data.
>
> But if that's the preferred way, then I can change.

I prefer this way, but not insist on  if it really works using the 
private_data.

Zhiwei

>
> Thanks
> Christoph
>
>> Zhiwei
>>
>>>        switch (env->xl) {
>>>        case MXL_RV32:

Re: [PATCH 6/9] target/riscv/cpu: Share RISCVCPUConfig with disassembler

Posted by Christoph Müllner 2 years, 8 months ago

On Mon, Jun 12, 2023 at 12:01 PM LIU Zhiwei <baxiantai@gmail.com> wrote:
>
>
> On 2023/6/12 17:47, Christoph Müllner wrote:
> > On Mon, Jun 12, 2023 at 8:25 AM LIU Zhiwei <baxiantai@gmail.com> wrote:
> >>
> >> On 2023/5/30 21:18, Christoph Muellner wrote:
> >>> From: Christoph Müllner <christoph.muellner@vrull.eu>
> >>>
> >>> The disassembler needs the available extensions in order
> >>> to properly decode instructions in case of overlapping
> >>> encodings (e.g. for vendor extensions).
> >>>
> >>> Let's use the field 'disassemble_info::private_data' to store
> >>> our RISCVCPUConfig pointer.
> >>>
> >>> Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
> >>> ---
> >>>    target/riscv/cpu.c | 3 +++
> >>>    1 file changed, 3 insertions(+)
> >>>
> >>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> >>> index 5b7818dbd1..6f0cd9a0bb 100644
> >>> --- a/target/riscv/cpu.c
> >>> +++ b/target/riscv/cpu.c
> >>> @@ -819,6 +819,9 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
> >>>    {
> >>>        RISCVCPU *cpu = RISCV_CPU(s);
> >>>        CPURISCVState *env = &cpu->env;
> >>> +    RISCVCPUConfig *cfg = &cpu->cfg;
> >>> +
> >>> +    info->private_data = cfg;
> >> I don't know if this field will be overridden by the binutils. Can we
> >> extend the struct disassemble_info, and add some fields like supporting
> >> for Capstone?
> >
> > Initially I wanted to add a new field, but then I noticed that the field
> > 'disassemble_info::private_data' is used for a similar purpose by
> > disas/cris.c, disas/m68k.c, and dias/xtensa.c.
> > So I decided to not add yet another field, which only serves one architecture.
> I think you can CC these arch maintainers to see if it need some
> specially process before using the private_data.
> >
> > But if that's the preferred way, then I can change.
>
> I prefer this way, but not insist on  if it really works using the
> private_data.

This topic is already resolved by using the field 'info->target_info'.
So I dropped this patch anyway.

BR
Christoph

>
> Zhiwei
>
> >
> > Thanks
> > Christoph
> >
> >> Zhiwei
> >>
> >>>        switch (env->xl) {
> >>>        case MXL_RV32:

Re: [PATCH 6/9] target/riscv/cpu: Share RISCVCPUConfig with disassembler

Posted by LIU Zhiwei 2 years, 8 months ago

On 2023/6/12 18:04, Christoph Müllner wrote:
> On Mon, Jun 12, 2023 at 12:01 PM LIU Zhiwei <baxiantai@gmail.com> wrote:
>>
>> On 2023/6/12 17:47, Christoph Müllner wrote:
>>> On Mon, Jun 12, 2023 at 8:25 AM LIU Zhiwei <baxiantai@gmail.com> wrote:
>>>> On 2023/5/30 21:18, Christoph Muellner wrote:
>>>>> From: Christoph Müllner <christoph.muellner@vrull.eu>
>>>>>
>>>>> The disassembler needs the available extensions in order
>>>>> to properly decode instructions in case of overlapping
>>>>> encodings (e.g. for vendor extensions).
>>>>>
>>>>> Let's use the field 'disassemble_info::private_data' to store
>>>>> our RISCVCPUConfig pointer.
>>>>>
>>>>> Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
>>>>> ---
>>>>>     target/riscv/cpu.c | 3 +++
>>>>>     1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>>>>> index 5b7818dbd1..6f0cd9a0bb 100644
>>>>> --- a/target/riscv/cpu.c
>>>>> +++ b/target/riscv/cpu.c
>>>>> @@ -819,6 +819,9 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
>>>>>     {
>>>>>         RISCVCPU *cpu = RISCV_CPU(s);
>>>>>         CPURISCVState *env = &cpu->env;
>>>>> +    RISCVCPUConfig *cfg = &cpu->cfg;
>>>>> +
>>>>> +    info->private_data = cfg;
>>>> I don't know if this field will be overridden by the binutils. Can we
>>>> extend the struct disassemble_info, and add some fields like supporting
>>>> for Capstone?
>>> Initially I wanted to add a new field, but then I noticed that the field
>>> 'disassemble_info::private_data' is used for a similar purpose by
>>> disas/cris.c, disas/m68k.c, and dias/xtensa.c.
>>> So I decided to not add yet another field, which only serves one architecture.
>> I think you can CC these arch maintainers to see if it need some
>> specially process before using the private_data.
>>> But if that's the preferred way, then I can change.
>> I prefer this way, but not insist on  if it really works using the
>> private_data.
> This topic is already resolved by using the field 'info->target_info'.
> So I dropped this patch anyway.

OK. I remembered I also used this field to pass the ISA information in 
the multi-path disassemble patch set.

Zhiwei

>
> BR
> Christoph
>
>> Zhiwei
>>
>>> Thanks
>>> Christoph
>>>
>>>> Zhiwei
>>>>
>>>>>         switch (env->xl) {
>>>>>         case MXL_RV32:

Re: [PATCH 6/9] target/riscv/cpu: Share RISCVCPUConfig with disassembler

Posted by Alistair Francis 2 years, 8 months ago

On Tue, May 30, 2023 at 11:20 PM Christoph Muellner
<christoph.muellner@vrull.eu> wrote:
>
> From: Christoph Müllner <christoph.muellner@vrull.eu>
>
> The disassembler needs the available extensions in order
> to properly decode instructions in case of overlapping
> encodings (e.g. for vendor extensions).
>
> Let's use the field 'disassemble_info::private_data' to store
> our RISCVCPUConfig pointer.
>
> Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>

Acked-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  target/riscv/cpu.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index 5b7818dbd1..6f0cd9a0bb 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -819,6 +819,9 @@ static void riscv_cpu_disas_set_info(CPUState *s, disassemble_info *info)
>  {
>      RISCVCPU *cpu = RISCV_CPU(s);
>      CPURISCVState *env = &cpu->env;
> +    RISCVCPUConfig *cfg = &cpu->cfg;
> +
> +    info->private_data = cfg;
>
>      switch (env->xl) {
>      case MXL_RV32:
> --
> 2.40.1
>
>