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

Arm coproc registers read/write #889

Closed
wants to merge 2 commits into from

Conversation

jbcayrou
Copy link
Contributor

Add ARM feature to read / write coprocessor registers, see #652 for details.

Work :

  • Coproc registers are automatically generated from ARM XML documentation
  • Use Qemu ARMCPRegInfo objects with read/write handlers
  • const_generator.sh was modified to use intermediary header file with macro expanded (to insert cpregs)

@jbcayrou
Copy link
Contributor Author

Continuous integration fails, I will check that

@jbcayrou
Copy link
Contributor Author

It seems good now, let me know if you want I rebase all commits into one.

@mothran
Copy link
Contributor

mothran commented Nov 15, 2017

Whats the status of this @aquynh? This pull solves my original issue ticket quite well and if this is merged I can move forward with releasing code that depends on it. Thanks!

@aquynh
Copy link
Member

aquynh commented Nov 17, 2017

i am taking another look now, but this can get merged soon.

@jbcayrou
Copy link
Contributor Author

jbcayrou commented Feb 5, 2018

Hi @aquynh,
Did you have time to check this PR ?
Let me know if you have remarks or suggestions.

@RomainQuidet
Copy link

Hi anyone still working on this PR ?

@RomainQuidet
Copy link

@jbcayrou seems there are some conflicts now.

@jbcayrou
Copy link
Contributor Author

jbcayrou commented Mar 9, 2019

Hi Romain,
Sorry I did not work on this PR since my last comment.
I can try to fix conflicts but I am a little busy at the moment.
Did you try to build it on my branch ? Do you have feedbacks, if all work for your use case ?

@RomainQuidet
Copy link

RomainQuidet commented Mar 10, 2019 via email

@jbcayrou
Copy link
Contributor Author

Hum if you have an illegal instruction error with your bytecode I don't think this patch will resolve it. The patch goal is to set/get a coproc register without writing assembly, with uc_reg_read/uc_reg_write. Maybe you have to configure the CPU before calling MSR/MRS

You do not need to generate .h files

@RomainQuidet
Copy link

@jbcayrou I don't know yet, I'm learning this framework.
I've got UC_ERR_INSN_INVALID on this line:
mcr p15, #0x0, r1, c9, c1, #0x1

@RomainQuidet
Copy link

seems that the bug is deeper in qemu armv5 handling. Thanks anyway !

@derrekr
Copy link

derrekr commented Jan 11, 2020

FWIW, there is a bug in the arm_reg_read/write functions, where the CP regs are read uncondionally: jbcayrou@1f9f484#diff-b07e20947cbe5aac5dadd747a74fc6b3R222
This should have been inside the default case of the switch statement. At the moment it causes a nasty performance degradation when these functions are called frequently during emulation. ARM64 might be affected as well, I haven't checked.

@wtdcode
Copy link
Member

wtdcode commented Oct 3, 2021

Closed due to inactivity.

Special note: I need @jbcayrou your help to make this PR merged into Unicorn2, especially your scripts to generates all theses registers.

@wtdcode wtdcode closed this Oct 3, 2021
@wtdcode
Copy link
Member

wtdcode commented Oct 3, 2021

Re-open this and link to #1217

@wtdcode wtdcode reopened this Oct 3, 2021
This was referenced Oct 3, 2021
@jbcayrou
Copy link
Contributor Author

jbcayrou commented Oct 4, 2021

Hi all !

Sorry for the inactivity but it did not work on this patch since several years now.

Special note: I need @jbcayrou your help to make this PR merged into Unicorn2, especially your scripts to generates all theses registers.

I will rework the patch to make it mergeable.

  • The derrekr's comment is right : the CP registers should be fetched in a default case and this PR needs some performance improvement. Another improvement could be to replace the list (ARM/ARM64)_CP_REGS_INFO with a hash map.

  • Regarding the issue Why not support cr8? #1330, X86 specials registers should be accessed with qemu x86 API and this patch only manages ARM coproc registers.

@wtdcode wtdcode added this to the Unicorn2 Official Release milestone Oct 5, 2021
@jbcayrou jbcayrou changed the base branch from master to dev October 12, 2021 06:00
Each unicorn/*.h is preprocessed in bindings/tmp/*.h to expand C macro,
then const_generator.py extracts enum from these files.

unicorn.h was adapted by using enum for some defines (because they disapear
in expanded headers)

With preprocessed headers, all <target>.h files are include in tmp/unicorn.h
so unicorn_const.* files contains enum of all targets.
<target>_const.* are kept for compatibility.
@jbcayrou
Copy link
Contributor Author

Hi !

I rework all the previous patch and I changed the way the coproc register are generated.

  • The former patch generated registers with the ARM documentation and many registers were produced. However QEMU implements just some of them and most of registers written by the python script were not supported by QEMU. It was not convenient for users to not know what really work.
    This new version is more pragmatic and the generation is done by getting all cpregs registered in a running CPU instance. In this way we can be sure QEMU implements them. The C util arm_list_coprocregs.c iterates on registred cpregs to produces a list which is parsed by the python script gen_arm_cpregs.py

  • To improve the performance of the first patch version, the values of enumeration entries UC_ARM_REGS_XXXX match the internal qemu attributes "regid" (and a mask was added to make the distinction with others unicorn enum values) thus read/write access to cpregs are direct.
    I removed some cpregs which were already implemented with a direct access to env.cp15 and now supported in this patch.

Note :

This patch implements the read/write cpreg using read_raw_cp_reg/write_raw_cp_reg. Theses functions try to call the setter/getter registered with a given cpreg and can perform some processing on the value as add a mask or do some qemu updates (for instance tlb_flush(CPU(cpu)) in the case of TTBR).
If this change produces issues we can simply change read_raw_cp_reg by raw_read to keep the same behavior than before this patch.

PS : If needed, I moved the old patch version https://github.com/jbcayrou/unicorn/tree/arm_cpregs_access_deprecated

@wtdcode
Copy link
Member

wtdcode commented Oct 12, 2021 via email

@jbcayrou
Copy link
Contributor Author

jbcayrou commented Oct 12, 2021

But it looks like you are failing CI?

Oops sorry :/ I will check why.

target_link_libraries(arm64_list_coprocregs unicorn-common)
target_compile_options(arm64_list_coprocregs PRIVATE
-DNEED_CPU_H
-include aarch64.h
Copy link
Member

Choose a reason for hiding this comment

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

You are failing CI because MSVC doesn't recognize this options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ho yes I forgot to manage MSVC case

target_link_libraries(arm_list_coprocregs unicorn-common)
target_compile_options(arm_list_coprocregs PRIVATE
-DNEED_CPU_H
-include arm.h
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@@ -67,26 +67,33 @@ typedef size_t uc_hook;
#endif

// Unicorn API version
#define UC_API_MAJOR 2
#define UC_API_MINOR 0
typedef enum {
Copy link
Member

Choose a reason for hiding this comment

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

Why are you adding typedef-ed enums?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To expand the UC_ARM_CPREG_LIST macro, I needed to change the Makefile of binding folder to generate the header file with macro expanded. However the command $(CC) -E -C $< -o $@ will replace the #define XXX by the real value and it is problematic for the bindings/const_generator.py script.
Using a enum is a workaround to this issue (I did found how to create enum { UC_API_MAJOR = UC_API_MAJOR} to keep the defines.

If you have an idea how to manage better this issue I am interested in.

Copy link
Member

@wtdcode wtdcode Oct 12, 2021

Choose a reason for hiding this comment

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

Why "$(CC) -E -C $&lt; -o $@" is needed? Sorry I'm lost in your black magic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, the GCC compilation command just preprocesses the C header to integrate the UC_ARM_CPREG_LIST content in a same file. (I also forgot the MSVC case)

gcc --h
[...]
-E                       Preprocess only; do not compile, assemble or link.

I wrote this part several years ago, I will review it again to see how I can avoid the typedef workaround.

ARMCPU *cpu = ARM_CPU(s);

// printf("Reg;cp;crn;crm;opc0;opc1;opc2;regid\n");
g_hash_table_foreach(cpu->cp_regs, cp_reg_test, cpu);
Copy link
Member

Choose a reason for hiding this comment

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

This approach is rather not straightforward and can be problematic.

  1. Considering Unicorn supports multiple CPU models (I would implement more possible models in the near future), the co-processor you retrieve in this approach may be incomplete.
  2. A better way is to parse register_cp_regs_for_features function in qemu/target/arm/helper.c semi-manually. In other words, just copy all these definitions:
static const ARMCPRegInfo actlr2_hactlr2_reginfo[] = {
    { .name = "ACTLR2", .state = ARM_CP_STATE_AA32,
      .cp = 15, .opc1 = 0, .crn = 1, .crm = 0, .opc2 = 3,
      .access = PL1_RW, .accessfn = access_tacr,
      .type = ARM_CP_CONST, .resetvalue = 0 },
    { .name = "HACTLR2", .state = ARM_CP_STATE_AA32,
      .cp = 15, .opc1 = 4, .crn = 1, .crm = 0, .opc2 = 3,
      .access = PL2_RW, .type = ARM_CP_CONST,
      .resetvalue = 0 },
    REGINFO_SENTINEL
};

to this file and generate all coproc registers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding the different CPU models I thought it was better to generate at runtime the list of all cpregs for each CPU models. The advantage of a dynamic listing is that we are sure of what is implemented. Static analysis seems more painful as the cpregs available depend on the features enabled and that depends on many things. More over some registers has wildcard value that we have to manage.
A semi-manually way involves to gathers manually all ARMCPRegInfo list regarding the CPU models supported by unicorn.

It is for these reasons I found the dynamic approach more exhaustive and easy to maintain, arm_list_coprocregs.c can be modify to generate the cpregs list of all ARM CPU models.

If you prefer to parse the C file qemu/target/arm/helper.c I think I could not have time to work the modifications that it involves.

Copy link
Member

@wtdcode wtdcode Oct 12, 2021

Choose a reason for hiding this comment

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

Yes, I had a look again just now and the work is also more than I expected. Sorry I didn't check it thoroughly before.

For the dynamic approach, looks like we have to add the API like uc_option in #117

@wtdcode wtdcode mentioned this pull request Oct 29, 2021
@wtdcode
Copy link
Member

wtdcode commented Feb 11, 2022

I implemented co processor registers read/write in 3e6665d and 8bc1489 inspired from your work.

Thanks for your contributions!

@wtdcode wtdcode closed this Feb 11, 2022
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.

6 participants