-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Litex2 #3674
Conversation
|
Do you need the crypto extensions? We restructured the B extension implementation, but crypto is not supported currently. |
Also, please refer to the |
My changes are on top of master, I've now rebased, changed the email in the commit and did a push -f |
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) => { |
There was a problem hiding this comment.
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
src/main/scala/system/Configs.scala
Outdated
@@ -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( |
There was a problem hiding this comment.
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?).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
9791a5e
to
8166b38
Compare
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
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