alexforencich / verilog-axi

Verilog AXI components for FPGA implementation
MIT License
1.44k stars 437 forks source link

AxiLiteMaster hangs with Verilator #48

Open catkira opened 1 year ago

catkira commented 1 year ago

I am using the latest Version of cocotbext-axi.

In my code I am using

AxiLiteMaster(AxiLiteBus.from_prefix(dut, "s_axi_if"), dut.clk_i, dut.reset_ni, reset_active_level = False)

This is working when I simulate it with iverilog but it hangs with Verilator.

alexforencich commented 1 year ago

You're most likely running in to https://github.com/verilator/verilator/issues/3919. I spent like a week trying to get to the bottom of that, and the root cause is simply the design of verilator itself. Without changes to make verilator behave a bit more like a regular HDL simulator, there isn't really a path to fix this.

catkira commented 1 year ago

ah yes, thats also my problem. Is there a workaround available or do I have to write a simple axil_read routine myself in python?

catkira commented 1 year ago

this is my workaround for now

    async def read_axil(self, addr):
        self.dut.s_axi_if_araddr.value = addr
        self.dut.s_axi_if_arvalid.value = 1
        self.dut.s_axi_if_rready.value = 1
        await RisingEdge(self.dut.clk_i)
        while self.dut.s_axi_if_rvalid.value == 0:
            await RisingEdge(self.dut.clk_i)
        self.dut.s_axi_if_arvalid.value = 0
        self.dut.s_axi_if_rready.value = 0
        data = self.dut.s_axi_if_rdata.value.integer
        return data
alexforencich commented 1 year ago

My workaround is quite simple: Verilator is not supported until all of the deeper verilator-cocotb integration issues are solved. I'm not going to try to patch around verilator bugs. So, until such time as verilator properly works with cocotb, I'm simply going to continue using icarus verilog.

catkira commented 1 year ago

Yes there was a problem with newer Verilator versions, where a vpi problem caused the simulation to hang. But this has been fixed with Verilator v5.006 and I can confirm that it runs flawlessly with cocotb. I have created this 5G PHY in HDL, where I do a top-level simulation with verilator. Doing it with iverilog is just not feasible, because it's around 100x slower than Verilator - Verilator is really a game changer here. For the individual core testbenches I am still using iverilog, because the executing speed does not matter so much there. I am about to add a AXI-DMA core to my 5G PHY. Being able to verify it using cocotbext-axi would be really helpful there.

alexforencich commented 1 year ago

Well, another option to consider is maybe take a crack at actually fixing the issue properly on the verilator end.

What it boils down to is that verilator's eval() method pushes values from top-level ports down the hierarchy unconditionally. Even if an internal signal is marked as public flat rw and can be written via VPI, as soon as eval() is run, the value gets blown away with the value from the top-level port. The ultimate solution is to change the verilated model so that it behaves similar to other HDL simulators, with values only being propagated when changes occur, at least for signals marked public flat rw.

I think a good place to start would be to create some representative tests in verilator that exhibit the problem, then perhaps the verilator folks can noodle over the best way to adjust the model so the tests pass. Then, hopefully verilator will actually be usable.

catkira commented 1 year ago

I am afraid thats a bit too deep into Verilator for my time budget and it is also not efficient to dig deep into it just to fix one thing. Did You talk to Wsnyder about it? I believe it's a piece of cake for him :P

alexforencich commented 1 year ago

From the verilator issue:

In summary, in general the VPI is intended to primarily allow VPI read to almost everything, VPI write only to storage for backdoor CSR loading and similar. Supporting VPI write to "random" signals is intentionally beyond the intent of Verilator's implemented VPI.

Sounds like he's more or less opposed to making this work in Verilator, tbh. So if we want to make it happen, we're at least going to have to put together some test cases that work in other simulators but fail in verilator.

catkira commented 1 year ago

hmm ok. Can I use cocotb.drivers.BusDriver for my axi testbenches or does this have the same problems with Verilator?

alexforencich commented 1 year ago

The problem is that as soon as something on the python side calls dir(), the whole simulator object cache gets filled with unusable simulator handles (at least, for that particular portion of the hierarchy) and all subsequent lookups will return effectively read-only handles. Currently, the thing that triggers this is the case-insensitive name matching in the Bus object in cocotb-bus. So anything that uses Bus will have this problem, including BusDriver. Disabling that will prevent dir() from being called during bus creation, but it could still be called elsewhere, and all of this only works if you're connecting to top-level ports. I put together a workaround that "short-circuits" the case-insensitive matching and tries a few variations before calling dir(), but this does not help in the case of optional signals, so I scrapped the idea.

The source of these "read-only" handles is how verilator internally assembles and iterates over the hierarchy. Basically, verilator adds a "fake" top-level called TOP, which drives the ports on the top-level module. If you do a lookup of a signal by name, verilator detects the top-level signals with a hacked-up test and returns references to the corresponding signals in the fake TOP instead of the ones in the actual module. But if you iterate over the signals in the module, you'll get the internal handles instead. These handles are then cached in the cocotb GPI layer, and all subsequent lookups of those signals return these internal handles, breaking the simulation rather badly. The internal handles are "read-only" because any changes are immediately overwritten by eval() - i.e. they are not actually read-only, as writing to them will trigger edge events and what not, but the values never actually get updated because they get clobbered immediately.

There are basically 4 ways to fix this. One of them is don't ever iterate over signals in cocotb. Which is an ugly workaround at best. Another is possibly to rework how the GPI cache works, perhaps iterate to get the name, then do a full lookup by name, and only cache that. Another is to fix the test in verilator that returns the "fake" top-level handles such that it also works while iterating over the top-level module, instead of only working for lookups by name. But, none of these fix the real underlying problem: the fact that these internal handles are "read-only" in the first place, and this is a much bigger problem because it means that cocotb cannot modify anything other than top-level signals when running with verilator. I do this in a bunch of different testbenches, so the only path that I am going to spend time and effort working on is the last one: a proper upstream fix in verilator so that all internal signals marked public flat rw are actually writable via VPI.

If you're interested enough in using verilator that you can put some cycles into at least putting together some test cases, I can probably help out a bit with that. If you just want a quick workaround, then specify case_insensitive=False when calling AxiLiteBus.from_prefix() and cross your fingers that dir() is not called anywhere else.

catkira commented 1 year ago

Thanks for the explaination, I understand the problem a bit better now. I cannot tell how big the required changes in Verilator would be to behave like the other simulators, maybe there are reasons we dont know yet, ie changing it would make Verilator slower. I think Verilator devs can give a statement if it can be done without sacrificing speed or code complexity. But since Wsnyder already gave the statement that he does not want to make these changes we have to live with it for now I guess.

I am also not deep inside cocotb, but for my understanding possibly to rework how the GPI cache works, perhaps iterate to get the name, then do a full lookup by name, and only cache that. sounds pretty good. Are there reasons why this solution cannot or should not be implemented in cocotb?

case_insensitive=False is working in for me, but I only tested a single Axi-lite read so far.

alexforencich commented 1 year ago

I suspect a proper fix would also introduce some performance penalty, but I think there is already some performance penalty with setting everything to public_flat_rw (as cocotb has to do so it can access internal signals via the VPI), and presumably this functionality would only be enabled for signals marked public_flat_rw (since if it's not public or if it's ro, then you can't write to it via the VPI). I suspect the devs would be open to changes if presented with a good justification, coupled with representative test cases that pass in other simulators but fail in verilator.

Modifying the cocotb GPI as a workaround for Verilator's broken VPI won't fully solve the problem, it will basically just make iterating over the top-level ports work correctly, it won't do anything about driving stuff deeper in the hierarchy. But potentially it might be worth considering simply as a hedge against buggy VPI implementations in other simulators.

Once you have good handles for all of the signals, then the rest of the simulation will run just fine. So if all of the signals that need to toggle are toggling, then you should be good to go.

marlonjames commented 10 months ago

https://github.com/verilator/verilator/issues/3919 is now fixed in Verilator master.

alexforencich commented 10 months ago

It is only fixed for accessing top-level ports. Which is good progress, but that issue should be reopened.