[PATCH 0/2] riscv: riscv,isa fixes + check script

Daniel Henrique Barboza posted 2 patches 1 week, 1 day ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20251106112044.162617-1-dbarboza@ventanamicro.com
Maintainers: John Snow <jsnow@redhat.com>, Cleber Rosa <crosa@redhat.com>, Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Weiwei Li <liwei1518@gmail.com>, Daniel Henrique Barboza <dbarboza@ventanamicro.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>
scripts/riscv-isaedata-check.py | 164 ++++++++++++++++++++++++++++++++
target/riscv/cpu.c              |  17 ++--
2 files changed, 174 insertions(+), 7 deletions(-)
create mode 100755 scripts/riscv-isaedata-check.py
[PATCH 0/2] riscv: riscv,isa fixes + check script
Posted by Daniel Henrique Barboza 1 week, 1 day ago
Hi,

We have an array called isa_edata_arr[] in target/riscv/cpu.c which
needs to be always kept in the RISC-V specification riscv,isa order.
Easier said that done: as more and more extensions are added we're
failing to keep up with the array ordering in the review process.

I have considered changing how we're retrieving riscv,isa to not rely on
the array ordering (in fact I have code that does that). We would sort
the enabled extensions using riscv,is ordering during init time, before
writing it in the DT, ignoring the current isa_edata_arr ordering. When
all was said and done that sounded a bit extreme and I think there's
other stuff we can try first.

Patch 2 in this series is a python script I put together to, hopefully
help us identify if we're messing up the isa_edata_arr ordering and fix
it before pushing changes upstream. I used it to validate the changes I
did in patch 1 (the usual 'fix isa_edata_arr ordering' patch) and I can
tell that the script picked up *way* more ordering errors that I was
able to identify on my own. 

I didn't made any CI related changes w.r.t this new script but it is
something to consider - if changes in target/riscv/cpu.c are made, run
this script to validate the array again.

Patches based on alistair/riscv-to-apply.next.


Daniel Henrique Barboza (2):
  target/riscv/cpu.c: isa_edata_arr[] ordering fixes
  scripts: RISC-V python script to check isa_edata_arr[]

 scripts/riscv-isaedata-check.py | 164 ++++++++++++++++++++++++++++++++
 target/riscv/cpu.c              |  17 ++--
 2 files changed, 174 insertions(+), 7 deletions(-)
 create mode 100755 scripts/riscv-isaedata-check.py

-- 
2.51.1
Re: [PATCH 0/2] riscv: riscv,isa fixes + check script
Posted by Conor Dooley 1 week ago
On Thu, Nov 06, 2025 at 08:20:42AM -0300, Daniel Henrique Barboza wrote:
> Hi,
> 
> We have an array called isa_edata_arr[] in target/riscv/cpu.c which
> needs to be always kept in the RISC-V specification riscv,isa order.
> Easier said that done: as more and more extensions are added we're
> failing to keep up with the array ordering in the review process.
> 
> I have considered changing how we're retrieving riscv,isa to not rely on
> the array ordering (in fact I have code that does that). We would sort
> the enabled extensions using riscv,is ordering during init time, before
> writing it in the DT, ignoring the current isa_edata_arr ordering. When
> all was said and done that sounded a bit extreme and I think there's
> other stuff we can try first.

FWIW, I have yet to actually see a parser for it in a "real" application
that relied on the ordering. It probably makes a parser more complicated
to write than one where the ordering is ignored. The only time I can really
see ordering mattering is if something has a very simple bit of code and
is looking for "rv##ima" or similar, and using a string comparison
function.
Either way, my point basically is that you shouldn't have to go to any
extreme effort to make sure it is perfect, particularly when it comes to
the multiletter stuff as, at least for devicetree, the binding has never
enforced ordering for multiletter extensions. I know ACPI cites spec
order (and spec definitions, so GL there lol), and there could be relevant
for some ACPI only code where the devicetree parser is not being reused.
Re: [PATCH 0/2] riscv: riscv,isa fixes + check script
Posted by Daniel Henrique Barboza 4 days, 1 hour ago

On 11/6/25 12:48 PM, Conor Dooley wrote:
> On Thu, Nov 06, 2025 at 08:20:42AM -0300, Daniel Henrique Barboza wrote:
>> Hi,
>>
>> We have an array called isa_edata_arr[] in target/riscv/cpu.c which
>> needs to be always kept in the RISC-V specification riscv,isa order.
>> Easier said that done: as more and more extensions are added we're
>> failing to keep up with the array ordering in the review process.
>>
>> I have considered changing how we're retrieving riscv,isa to not rely on
>> the array ordering (in fact I have code that does that). We would sort
>> the enabled extensions using riscv,is ordering during init time, before
>> writing it in the DT, ignoring the current isa_edata_arr ordering. When
>> all was said and done that sounded a bit extreme and I think there's
>> other stuff we can try first.
> 
> FWIW, I have yet to actually see a parser for it in a "real" application
> that relied on the ordering. It probably makes a parser more complicated
> to write than one where the ordering is ignored. The only time I can really
> see ordering mattering is if something has a very simple bit of code and
> is looking for "rv##ima" or similar, and using a string comparison
> function.
> Either way, my point basically is that you shouldn't have to go to any
> extreme effort to make sure it is perfect, particularly when it comes to
> the multiletter stuff as, at least for devicetree, the binding has never
> enforced ordering for multiletter extensions. I know ACPI cites spec
> order (and spec definitions, so GL there lol), and there could be relevant
> for some ACPI only code where the devicetree parser is not being reused.

Thanks for the insight. Yeah, I haven't seen software caring that much about
the riscv,isa ordering either. In case it exists it would be broken in QEMU
given that we're using the wrong order :D


Thanks,

Daniel