Skip to content
This repository has been archived by the owner on Aug 17, 2022. It is now read-only.

Add k-ext support with v1.0.0-rc #254

Conversation

pz9115
Copy link
Contributor

@pz9115 pz9115 commented Mar 30, 2021

PLCT had add k-ext support with crypto spec v0.90 with finish all regress testing in binutils part, please check it.

@Nelson1225
Copy link
Collaborator

Shouldn't these be sent to riscv-binutils-experiment branch?

@pz9115 pz9115 changed the base branch from riscv-binutils-2.36 to riscv-binutils-experiment March 30, 2021 07:44
@pz9115 pz9115 changed the title Add k-ext support with v0.93 Add k-ext support with v0.90 Mar 30, 2021
Copy link
Collaborator

@jim-wilson jim-wilson left a comment

Choose a reason for hiding this comment

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

Overall it looks OK. There are a just a few minor questions I have, and some apparent issues with changing the patch base which confused the patch a little.

bfd/elfxx-riscv.c Outdated Show resolved Hide resolved
bfd/elfxx-riscv.c Show resolved Hide resolved
bfd/elfxx-riscv.c Outdated Show resolved Hide resolved
gas/config/tc-riscv.c Outdated Show resolved Hide resolved
gas/config/tc-riscv.c Outdated Show resolved Hide resolved
include/opcode/riscv.h Outdated Show resolved Hide resolved
opcodes/riscv-dis.c Outdated Show resolved Hide resolved
opcodes/riscv-opc.c Outdated Show resolved Hide resolved
opcodes/riscv-opc.c Outdated Show resolved Hide resolved
opcodes/riscv-opc.c Outdated Show resolved Hide resolved
@jim-wilson
Copy link
Collaborator

In general, I would prefer that patches go upstream. Branches here in github.com/riscv/riscv-binutils-gdb are becoming a serious liability. But we are still in the process of moving stuff upstream (waiting for gcc-11 branch), and using github for patch reviews is probably more convenient than email for many people.
]

@jim-wilson
Copy link
Collaborator

Kito's review of the K gcc patch reminds me, I didn't see testcases. It is generally a good idea to add a gas testcase for new extensions, to verify that we can assemble all of the instructions, and then disassemble them correctly.

We will also need to make sure we didn't accidentally break gdb. We can't do that in riscv-gnu-toolchain because we use different release branches for binutils and gdb. But in the FSF development tree, binutils and gdb development sources are on the same master branch.

bfd/elfxx-riscv.c Outdated Show resolved Hide resolved
@pz9115 pz9115 force-pushed the riscv-binutils-2.36-k-ext branch 5 times, most recently from a69bb36 to 72ef429 Compare April 14, 2021 06:42
@pz9115 pz9115 closed this Apr 28, 2021
@pz9115 pz9115 reopened this Apr 28, 2021
Copy link
Collaborator

@Nelson1225 Nelson1225 left a comment

Choose a reason for hiding this comment

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

Generally looks good, except the k expansions, GNU coding standards and CSR issue. We don't need to care the k expansions here, we can fix them when sending the draft k stuff to the FSF users/riscv/binutils-integration-branch.

However, we should all pay attention to the GNU coding standards. You can refer to my previous three patches as follows,

  • RISC-V: Comments tidy and improvement.
  • RISC-V: Error and warning messages tidy.
  • RISC-V: Indent and GNU coding standards tidy, also aligned the code.

After fixing the GNU comments and indents, and the CSR problem, the patch looks good to me.

@@ -1760,6 +1761,66 @@ riscv_parse_add_implicit_subsets (riscv_parse_subset_t *rps)
riscv_parse_add_subset (rps, "zicsr",
RISCV_UNKNOWN_VERSION,
RISCV_UNKNOWN_VERSION, TRUE);
else if ((riscv_lookup_subset (rps->subset_list, "k", &subset)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose k, zkn and zks are similar to g, they used to expand into others. We won't output g to the final architecture string, so k, zkn and zks should be the same. I had done some improvement on FSF binutils about this, so it might be easier to complete over there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

However,my old implementation here did not consider so much, so it looks OK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, if it need change, I will update it.

@@ -143,6 +143,18 @@ static const struct riscv_ext_version ext_version_table[] =
{"zba", ISA_SPEC_CLASS_DRAFT, 0, 93},
{"zbc", ISA_SPEC_CLASS_DRAFT, 0, 93},

{"k", ISA_SPEC_CLASS_DRAFT, 0, 90},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above, if k, zkn and zks are expansions, then we don't need to add the default versions for them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I deleted the version for k,zkn,zks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry that may have misunderstood you. I meant this is OK for riscv-binutils-experiment, but could be improved in the FSF binutils. Therefore, we don't need to care about the k and p's expansions in the riscv-binutils-experiment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, let me delete the expansions with k,zkn,zks in the riscv-binutils-experiment..

gas/config/tc-riscv.c Outdated Show resolved Hide resolved
bfd/elfxx-riscv.c Outdated Show resolved Hide resolved
include/opcode/riscv-opc.h Outdated Show resolved Hide resolved
include/opcode/riscv-opc.h Outdated Show resolved Hide resolved
@Nelson1225
Copy link
Collaborator

FYI, you can press the "Resolve conversation" if you have fixed them. So that the PR will look a little cleaner, thanks.

Fix intend problems and update CSR testcases, delete the version for k, zkn, zks.
Copy link
Collaborator

@Nelson1225 Nelson1225 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@@ -710,6 +776,7 @@
#define CSR_MARCHID 0xf12
#define CSR_MIMPID 0xf13
#define CSR_MHARTID 0xf14
#define CSR_MENTROPY 0xf15
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe you forgot this one :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your metion, I had update the 'mentorpy' case.

@pz9115 pz9115 force-pushed the riscv-binutils-2.36-k-ext branch from 3b3c319 to 5792217 Compare June 9, 2021 09:06
Update the CRS testcases and remove the expand for 'k',‘zkn’,'zks'.
Update the testcases with csr define.
@pz9115 pz9115 changed the title Add k-ext support with v0.90 Add k-ext support with v0.92 Jun 22, 2021
Update to spec v0.93, change mentropy into sentropy, update some aes encoding with 'bs', change 'rcon' into 'rnum'.
@pz9115 pz9115 force-pushed the riscv-binutils-2.36-k-ext branch from adffaa1 to 5921fb6 Compare July 1, 2021 09:50
@pz9115 pz9115 changed the title Add k-ext support with v0.92 Add k-ext support with v0.93 Jul 29, 2021
@pz9115 pz9115 changed the title Add k-ext support with v0.93 Add k-ext support with v0.94 Aug 12, 2021
@pz9115 pz9115 closed this Aug 19, 2021
@pz9115 pz9115 deleted the riscv-binutils-2.36-k-ext branch August 19, 2021 09:09
@pz9115 pz9115 restored the riscv-binutils-2.36-k-ext branch August 23, 2021 09:39
@pz9115 pz9115 reopened this Aug 23, 2021
@pz9115 pz9115 changed the title Add k-ext support with v0.94 Add k-ext support with v1.0.0-rc Aug 23, 2021
@pz9115
Copy link
Contributor Author

pz9115 commented Aug 23, 2021

We had updated with the scalar crypto spec 1.0.0-rc, and merge it with the b-ext spec 1.0.0-rc contants zba_zbb_zbc_zbs for reusing of zbk[bcx]. Sorry for the miss closing of this PR due to branch rename.

Update with spec 1.0.0-rc4
@pz9115
Copy link
Contributor Author

pz9115 commented Jun 14, 2022

This work had been merged into the binutils upstream branch

https://sourceware.org/git/?p=binutils-gdb.git

close the PR.

@pz9115 pz9115 closed this Jun 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants