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

Fix PowerPC:BE:64 LQ instruction to support signed offsets #6827

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

v-p-b
Copy link

@v-p-b v-p-b commented Aug 19, 2024

Based on the Power ISA manual sign extended DQ<<4 is added to RA to get source EA.

I'm working on adding support for AS/400 programs to Ghidra. While my code is available in a dedicated repository, I'd like to see at least the Processor module improvements upstreamed. This PR is my first attempt to contribute code to Ghidra, so this is just a minimal change, and I'd appreciate any further feedback on how to improve the rest of the Processor module and maybe even getting the Loader/Analyzer exts to the project!

This PR can be tested with the following instruction (also present in the DEREF2.pgm shared here):

e1 1f ff b1 lq r8,-50(r31),0x01

This instruction doesn't decompile with the existing PowerPC:BE:64:A2-32addr language variant (which includes ppc_isa.sinc that defines LQ), but it correctly decompiles with this fix.

Based on the Power ISA manual sign extended DQ<<4 is added to RA to
get source EA.
@GhidorahRex
Copy link
Collaborator

This looks like a good fix. I would drop BITS_0_3 entirely. I'm not sure why your example binary has it set to something other than 0, but the Power ISA manual specifically says that the reserved bits are ignored. I had to double-check this since I was sure this was a bug.

I would also change the line setting ea to:
ea:$(REGISTER_SIZE) = dqPlusRaOrZeroAddress; since the sub-constructor already contains the calculation.

I also think if we're making this fix, we should fix the STQ instruction at the same time, so we should update that too. It has the same issue and the fix should be almost identical (except dsPlusRaOrZeroAddress instead of dq...)

@v-p-b
Copy link
Author

v-p-b commented Nov 18, 2024

Thanks for looking into this!

BITS_0_3 includes an AS/400 (IBM i) specific mask used by (at least) the ILE C compiler for safety checks:

https://github.com/silentsignal/BelowMI/blob/6791b2ff116d3c54060a53e6bf6fb80632a7bd10/BelowMI.md?plain=1#L443

I developed the Processor module to get insight about such vendor-specific details, so I'd prefer this information to be included upstream as well. Please let me know if/how this can work for you!

In the meantime I'll test your proposed changes and will add the code for STQ in this PR as well.

@GhidorahRex
Copy link
Collaborator

I think it might be worth creating a separate AS variant for memory tagging instruction support that sets/unsets XER[43], since that behavior in LQ and STQ is specific to this processor. and also supports the other instructions - SETTAG, TXER, SEL*, LTPTR, SCV.

@v-p-b
Copy link
Author

v-p-b commented Nov 24, 2024

I pushed the requested changes. I created some very basic test files that are disassembled as expected:

lq_test.bin:

           00000000 e1 1f ff b1     lq         r8,-0x50(r31)
           00000004 e1 0f ff b1     lq         r8,-0x50(r15)
           00000008 e1 1f 7f b1     lq         r8,0x7fb0(r31)

stq_test.bin:



           00000000 f8 c7 ff f2     stq        r6,-0x10(r7)
           00000004 f8 c7 80 02     stq        r6,-0x8000(r7)
           00000008 f8 c7 7f f2     stq        r6,0x7ff0(r7)

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

Successfully merging this pull request may close these issues.

3 participants