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

Litex2 #3674

Merged
merged 1 commit into from
Aug 22, 2024
Merged

Litex2 #3674

merged 1 commit into from
Aug 22, 2024

Conversation

leviathanch
Copy link
Contributor

Adding support for the Crypto features back in and adding the configurations from Litex so that I can generate the platforms for pythondata_cpu_rocket

Copy link

linux-foundation-easycla bot commented Aug 19, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: leviathanch / name: David Lanzendörfer (445b5da)

@jerryz123
Copy link
Contributor

Do you need the crypto extensions? We restructured the B extension implementation, but crypto is not supported currently.

@jerryz123
Copy link
Contributor

Also, please refer to the dev branch for the latest implementations

@leviathanch
Copy link
Contributor Author

My changes are on top of master, I've now rebased, changed the email in the commit and did a push -f

@jerryz123
Copy link
Contributor

Can you confirm if the crypto B extensions are necessary for your use case?

They were removed from rocket because of the uncertain implications those implementations had on QoR and the intrusive nature of the original implementation. If we were to re-add support for those extensions, we would pursue a different approach.

Note that we have reimplemented the base B extensions (Zba/Zbb/Zbs), which should be sufficient for the most common use cases.

@@ -632,3 +664,29 @@ class WithCloneRocketTiles(n: Int = 1, cloneHart: Int = 0, overrideIdOffset: Opt
}
})

class WithMemoryDataBits(dataBits: Int) extends Config((site, here, up) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

To preserve naming consistency, this should be WithNBitMemoryBus

@@ -91,3 +94,210 @@ class BaseFPGAConfig extends Config(new BaseConfig ++ new WithCoherentBusTopolog
class DefaultFPGAConfig extends Config(new WithNSmallCores(1) ++ new BaseFPGAConfig)

class CloneTileConfig extends Config(new WithCloneRocketTiles(7) ++ new WithNBigCores(1) ++ new WithCoherentBusTopology ++ new BaseConfig)

class BaseLitexConfig extends Config(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move the Litex-configs to a new file under system (LitexConfigs.scala?).

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, I will move the Litex configurations into a separate file.
The problem with the crypto extension is that several projects which use Litex and in turn Rocket Chip have been developed for using the old crypto implementation.

Maybe we can do something like a legacy flag for what crypto extension to use or so?

Also, BTW, now that I fixed yosys and it parses through the System Verilog code produced by rocket chip, it seems to do something very nasty and declares variables (automatic logic) within always blocks

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you point me to the downstream projects which rely on crypto extensions? I want to see exactly which ones of the many extensions are used.
I'm happy to do a bit of work adding back in some of the crypto support, but I'd like to know where to start.

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, I've now decided to get rid of the legacy crypto B extension and using your proper implementation, so much has changed in Rocket Chip that pythondata_cpu_rocket will need a major rework anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry about the churn. We should have taken more care before merging the original crypto+B implementation.

Ultimately what motivated me to revert the entire crypto support was the intrusive change to decode that the crypto+B implementation mandated. Attempting to implement a flag that switches between a B-enabled ALU and the normal ALU made the system quite a bit messier, and caused headaches for downstream projects which reference the rocket ALU or decode.

Any future extensions to rocket, including if/when we add back the crypto support, must be done in a way to have minimal QoR impact on base components, and must not potentially introduce bugs into base features or instructions.

The good news is, now that we have the crypto FUs in the commit history, it would be only a small lift to re-integrate them in a less intrusive way (although the lack of readily available tests for many crypto instructions still makes me nervous about doing so).

build.sc Outdated Show resolved Hide resolved
@leviathanch leviathanch force-pushed the litex2 branch 2 times, most recently from 9791a5e to 8166b38 Compare August 21, 2024 23:33
There is a naming conflict of the ALU module which prevents a
successful synthesis with Yosys. This patch fixes this conflict.

In addition, this patch introduces the configurations expected
by Litex when generating an SoC

This patch also adds a generator for System Verilog which works with Yosys
Copy link
Contributor

@jerryz123 jerryz123 left a comment

Choose a reason for hiding this comment

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

Thanks!

@jerryz123 jerryz123 merged commit 50744b3 into chipsalliance:master Aug 22, 2024
28 checks passed
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.

2 participants