Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Renesas RH850 architecture support #1918

Open
wants to merge 267 commits into
base: dev
Choose a base branch
from

Conversation

virtualabs
Copy link

Hello,

This PR adds support for the Renesas RH850 architecture and has been tested against different firmware files designed for the Renesas F1KM-S4 development board.

This implementation supports all instructions for the V850e3v2 CPU used in the Renesas RH850 architecture.

@wtdcode
Copy link
Member

wtdcode commented Jan 9, 2024

Hello, thanks for your contribution!

But first of all, may I confirm that the target code under target/rh850 is port-ed from elsewhere or written by hand? I can't find it from upstream qemu here: https://github.com/qemu/qemu/tree/master/target

@virtualabs
Copy link
Author

The target code under target/rh850 is based on the partial implementation of Marko Klocic's forked qemu repo (https://github.com/markok314/qemu) which had some bugs and missing instructions. We fixed most of the bugs and added support of the missing instructions, and tested it against some RH850 firmware files we had. This code is part of a research we did and presented at the 2023 Qualcomm Product Security Summit (https://qcbizdev.my.salesforce-sites.com/QCTConference/GenericSitePage?eventname=SecuritySummit&page=Summit+Schedule), but it took us some time to polish the code and do some more testing on our target.

So it has not been taken from upstream qemu and is basically a rework/improvement of a previous implementation made by Marko Klopcic (https://github.com/markok314).

@wtdcode
Copy link
Member

wtdcode commented Jan 10, 2024

I checked your slides and really amazing work!

I will give a review hopefully before the end of this week. I need to grab some docs about rh850 firstly.

Btw, is there any showcase using this port I can play with?

@wtdcode
Copy link
Member

wtdcode commented Jan 10, 2024

btw, you will need to fix CI firstly ;)

I just reran all failed jobs.

@virtualabs
Copy link
Author

Yes I saw all that failed jobs, I thought it failed on my forked repo because of a badly configured CI but it seems I was wrong. I will work on it ASAP and update the PR once all checks pass and look for some piece of code I can share to test the implementation.

@virtualabs
Copy link
Author

It looks like I'm spamming the CI with my modifications :( ... Solved one issue (missing target-config.h file), but there is still some errors with MSVC. I should have created another branch to test my modifications, sorry.

@wtdcode
Copy link
Member

wtdcode commented Jan 10, 2024

It looks like I'm spamming the CI with my modifications :( ... Solved one issue (missing target-config.h file), but there is still some errors with MSVC. I should have created another branch to test my modifications, sorry.

No worry. I will suggest you doing this in your fork repo which is easier. From my personal experience, our CI should work anywhere except release stages which require tokens stored in our repo.

@wtdcode
Copy link
Member

wtdcode commented Jan 10, 2024

btw, I just found your target branch is not correct. You also need to rebase to dev branch afterward.

@virtualabs
Copy link
Author

Okay, I managed to fix all the remaining issues and it builds nicely in my forked repository (all checks passed). I've updated this PR to reflect these changes, but still need to rebase to the dev branch in order to get a clean PR.

@virtualabs virtualabs changed the base branch from master to dev January 11, 2024 10:00
@bet4it
Copy link
Contributor

bet4it commented Jan 15, 2024

Could you add Rust bindings?

@virtualabs
Copy link
Author

virtualabs commented Jan 15, 2024

To be honest, I'm not a git guru and having our forked unicorn repo based on unicorn's master branch rather than dev is a bit puzzling especially to push new commits while testing everything on my side to check everything is fine. If you have any tip or advice to sync the forked repo with the correct branch while keeping this PR OK i'll take it.

@bet4it I thought Rust bindings were automatically generated but in fact they don't seem so. Will do it once my working repository is synced with the dev branch.

@wtdcode
Copy link
Member

wtdcode commented Jan 23, 2024

Looks like you rebase your work against the master branch? This is causing chaos in the commit history.

Regarding the git problem, my suggestion is:

  1. Get a clean branch based on the master branch. I found rh850-arch-fix-ci in your fork repo but I'm not sure if it's intended.
  2. Merge it with our current dev branch.
  3. If there are conflicts, I can have a look.

Sorry for slow response.

@virtualabs
Copy link
Author

Got some help from a colleague to solve this one, hope now the commit history will be clean enough.

Copy link
Contributor

@bet4it bet4it left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forget to add mode and arch ID in Rust bindings

@virtualabs
Copy link
Author

Added the missing Rust bindings (arch and mode), but still messed up with my branch commits.

@shaqed
Copy link

shaqed commented Feb 20, 2024

Hey, first of all let me please thank you for this amazing work!
Not sure if this is the correct place to put this. But I found a weird behaviour with the SETF instruction.

According to the manual: If a specified condition is met, 0 should be written to the destination register. Else, write 1 to the destination register.

It seems that when the condition is met, 0 is not written. But instead random data is written to the destination register each time I ran it.

Here's some code from the Python binding (maybe that's the problem?)

from unicorn import *
from unicorn.rh850_const import *

uc = Uc(UC_ARCH_RH850, 0)

CODE_AREA = 0x000F_0000
CODE_AREA_SIZE = 0x1000

uc.mem_map(CODE_AREA, CODE_AREA_SIZE)

# cmp  r1,r2
# setf nz, r28
uc.mem_write(CODE_AREA, b"\xe1\x11\xea\xe7\x00\x00")


print("Testing when R1==R2")
uc.reg_write(UC_RH850_REG_PC, CODE_AREA)
uc.reg_write(UC_RH850_REG_R1, 1)
uc.reg_write(UC_RH850_REG_R2, 1)
uc.reg_write(UC_RH850_REG_R28, 0x12345678)

print("R28: {:08X}".format(uc.reg_read(UC_RH850_REG_R28)))
uc.emu_start(CODE_AREA, CODE_AREA+6)
print("R28: {:08X}".format(uc.reg_read(UC_RH850_REG_R28)))

You can see each run yields different values in the destination register R28

$ python rh850_setf_bug.py 
Testing when R1==R2
R28: 12345678
R28: 61736C30
$
$ python rh850_setf_bug.py 
Testing when R1==R2
R28: 12345678
R28: 5A79AC30
$
$ python rh850_setf_bug.py 
Testing when R1==R2
R28: 12345678
R28: BBB9AC30

However, if the R1 does not match R2, 1 is correctly written to R28.

print("Testing when R1!=R2")
uc.reg_write(UC_RH850_REG_PC, CODE_AREA)
uc.reg_write(UC_RH850_REG_R1, 1)
uc.reg_write(UC_RH850_REG_R2, 2)
uc.reg_write(UC_RH850_REG_R28, 0x12345678)
$ python rh850_setf_bug.py 
Testing when R1!=R2
R28: 12345678
R28: 00000001
$
$ python rh850_setf_bug.py 
Testing when R1!=R2
R28: 12345678
R28: 00000001
$
$ python rh850_setf_bug.py 
Testing when R1!=R2
R28: 12345678
R28: 00000001

I could not detect anything weird in the translate.c code which handles this instruction so maybe you can help me figure out what's wrong:

case OPC_RH850_SETF_cccc_reg2:{

	TCGv operand_local = tcg_temp_local_new_i32(tcg_ctx);
	int_cond = extract32(ctx->opcode,0,4);
	TCGv condResult = condition_satisfied(tcg_ctx, int_cond);
	cont = gen_new_label(tcg_ctx);

	tcg_gen_movi_i32(tcg_ctx, operand_local, 0x00000000);
	tcg_gen_brcondi_i32(tcg_ctx, TCG_COND_NE, condResult, 0x1, cont);
	  tcg_gen_movi_i32(tcg_ctx, operand_local, 0x00000001);

	gen_set_label(tcg_ctx, cont);

	gen_set_gpr(tcg_ctx, rs2, operand_local);

	tcg_temp_free(tcg_ctx, condResult);
	tcg_temp_free(tcg_ctx, operand_local);
}

Please forgive me if this comments sections is not the appropriate place to point this out, I couldn't post a new issue in the original Quarkslab repo.

@virtualabs
Copy link
Author

I made some experiments with the TCG code corresponding to the SETF instruction, I changed the code a bit to force the destination to be set to 0 when the conditional test fails and it produces the expected behavior. I used the following code:

TCGv operand_local = tcg_temp_local_new_i32(tcg_ctx);
int_cond = extract32(ctx->opcode,0,4);
TCGv condResult = condition_satisfied(tcg_ctx, int_cond);
cont = gen_new_label(tcg_ctx);
end = gen_new_label(tcg_ctx);

//tcg_gen_movi_i32(tcg_ctx, operand_local, 0x00000000);
tcg_gen_brcondi_i32(tcg_ctx, TCG_COND_NE, condResult, 0x1, cont); /* Jump to cont if not equal */
tcg_gen_movi_i32(tcg_ctx, operand_local, 0x00000001);
tcg_gen_br(tcg_ctx, end); /* jump to end */

/* cont: */
gen_set_label(tcg_ctx, cont);
tcg_gen_movi_i32(tcg_ctx, operand_local, 0x00000000);

/* end: */
gen_set_label(tcg_ctx, end);
gen_set_gpr(tcg_ctx, rs2, operand_local);

tcg_temp_free(tcg_ctx, condResult);
tcg_temp_free(tcg_ctx, operand_local);

My guess is that the IR code setting the operand_local to zero is not executed for some reason (when placed as the first instruction of the IR block) and you end up with a random value in the destination register when the condition is not met. It could be related with the TCG IR optimizations, perhaps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet