[HW, RV_PLIC] Claim/Complete register behaviour on FPGA

[silvestrst]

Interrupts are firing in verilator but not FPGA. This issue has been first discovered by @alistair23 working on Tock. I have also ran into this issue when doing some proof-of-concept work, related to #1330 .

After examining the netlist, it seems that portions of RV_PLIC are optimised by the FPGA synthesis tool (rv_plic_target, as well as cc0 in rv_plic_reg_top). One of the hypothesis is that parts of PLIC have been optimised out by the synthesis tool, possibly as result of @msfschaffner PLIC rework.

We have set up a set of relevant UART and PLIC signals in the Integrated Logic Analyser, and triggered an UART TX watermark interrupt. Thus, we would expect to read a value of 33 from CC0 (the interrupt claim register). However, we observed the following results:

• The inputs to CC0 are always 0 (IP1 = 0x00000001, IE01 = 0x00000001, Priority = 0x00000003, THRESHOLD0 = 0x00000000, LE1 = 0x00000000, both settings for edge- and level-sensitivity did not make any difference)
• The output of CC0 (complete_id) with ILA shows 1 after a read by the core, however the value the core receives is 0. The expected value would be 33 for the UART TX watermark interrupt.

Thank you @GregAC for setting up ILA, and @vogelpi for going through RTL with me.

[silvestrst]

@msfschaffner @eunchan could you please look into this?

[GregAC]

So from looking at the ILA outputs what's happening is the gateway seems to set the ip and ia flops up correctly (i.e. bit 32 gets set when the TX watermark interrupt is triggered) but something goes wrong in the target presumably in the logic that determines the highest priority. The reg2hw.cc0.q signal reads as 'h1 on the ILA (well I'm actually probing complete_id[0] but it's assigned to reg2hw.cc0.q) but SW cannot read this value for whatever reason.

Again from the ILA output I don't think the irq_q flop is getting set in the target module (perhaps this causes CC0 to get masked to 0 somewhere?)

[silvestrst]

The SW proof-of-concept code that I have put together lives here

[eunchan]

Thanks for reporting the issue. When @alistair23 reported this, I overlooked the chance of optimized away the logic. Let me and @msfschaffner dig this issue. Thanks!

CC: @tjaychen

[eunchan]

CC: @phil-levis-google

[msfschaffner]

Thanks for reporting this issue. Might be a simulation/synthesis mismatch, yes. The fastest way to test your hypothesis would be to build two bitstreams (one with the old and one with the new plic target). I've just kicked these builds off and will test them with the uart test.

[msfschaffner]

Ok after running the uart test on both versions (i.e. old/new plic target) in simulation as well as on FPGA, I can confirm that we are likely seeing a simulation/synthesis mismatch here.

I could not root-cause the mismatch just yet (the synthesis reports do not offer any insight). Will continue digging tomorrow.

In the mean time you can use the hotfix
3828f60da05cacf54c9cac9352a6d4b66c6e49ea
which lives here:
https://github.com/msfschaffner/opentitan/tree/irq-proof-of-concept
I based this on top of @silvestrst branch, and it basically reverts the plic target to an older implementation.

Note that synthesis might take considerably longer since that implementation does not scale well to so many interrupt sources (63). Also, please do not yet merge this fix into the main repository if possible. I hope that we will find the actual bug soon such that we can use the new implementation.

[msfschaffner]

Btwy, I am using Vivado v2018.3. Has anyone tried with a different version than that yet?

[silvestrst]

@msfschaffner and @eunchan thank you for looking into this problem, and for the patch.

Btwy, I am using Vivado v2018.3. Has anyone tried with a different version than that yet?

I use the same version as you.

[silvestrst]

@msfschaffner I have tried your temporary patch, and can confirm that I can get TX watermark interrupt to fire on the FPGA. Thank you

[GregAC]

Tried with Vivado 2019.2, synthesis tool just crashes early, there's some messages from a memory allocater reporting large allocs. Looks like it runs out of memory and crashes.

However the reported allocs have a suspicious progress of sizes:

tcmalloc: large alloc 1073741824 bytes == 0x6d238000 @  0x7f3b8bff463f 0x7f3b494195ce
tcmalloc: large alloc 2147483648 bytes == 0xad238000 @  0x7f3b8bff463f 0x7f3b494195ce
tcmalloc: large alloc 4294967296 bytes == 0x12d238000 @  0x7f3b8bff463f 0x7f3b494195ce
tcmalloc: large alloc 8589934592 bytes == (nil) @  0x7f3b8bff463f 0x7f3b494195ce

So 1 GiB, 2GiB, 4GiB, 8GiB precisely.

[GregAC]

I tried a run with the priority logic commented out of rv_plic_target (the theory being Vivado is doing something wrong with it, in 2018.3 this is a synthesis bug in 2019.2 it causes the large memory allocations and a crash) but same result.

[msfschaffner]

Update: after closer netlist inspection, it looks like Vivado is dropping the whole target module for some reason in 2018.3. I can reproduce this behavior when synthesizing the rv_plic in isolation.

Moving to 2019.2 shows no improvement, neither.
I am inclined to file an issue with Xilinx...

[eunchan]

Thanks for the update Michael! In the meantime, we could add old (not efficient version) version of RV_PLIC to target and selectable between the BINTREE and MATRIX?

…

On Fri, Jan 17, 2020 at 11:38:23AM -0800, Michael Schaffner wrote: Update: after closer netlist inspection, it looks like Vivado is dropping the whole target module for some reason in 2018.3. I can reproduce this behavior when synthesizing the `rv_plic` in isolation. Moving to 2019.2 shows no improvement, neither. I am inclined to file an issue with Xilinx... -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #1355 (comment)

[msfschaffner]

I am still bisecting the target module and I think I have found something. If that does not lead anywhere within the next 1-2h I will do what you suggested.

[msfschaffner]

@GregAC, I think you ran into this (known) issue in 2019.2: https://www.xilinx.com/support/answers/73178.html

[msfschaffner]

Ok I found the problem, which is likely a bug in Vivado.

If you look at the new binary tree implementation of rv_plic_target, it has two nested generate loops within which the different nodes of the tree are connected with each other.

Vivado doesn't seem to like the ternary statements on these lines:

opentitan/hw/ip/rv_plic/rtl/rv_plic_target.sv

Lines 86 to 92 in de3b010

	assign sel = (!is_tree[c0] && is_tree[c1]) ? 1'b1 :
	(is_tree[c0] && is_tree[c1] && max_tree[c1] > max_tree[c0]) ? 1'b1 :
	1'b0;
	// forwarding muxes
	assign is_tree[pa] = (sel) ? is_tree[c1] : is_tree[c0];
	assign id_tree[pa] = (sel) ? id_tree[c1] : id_tree[c0];
	assign max_tree[pa] = (sel) ? max_tree[c1] : max_tree[c0];

If I rewrite this with bitwise ops mimicking a multiplexor, the module synthesizes without issues, and I can run the uart test with no problems.

I am going to push a patch for this and file a bug report to Xilinx.

[tjaychen]

Thanks Michael ! Do you want to send out a general notice? I have to imagine others will run into this also.

…

On Fri, Jan 17, 2020, 12:52 Michael Schaffner ***@***.***> wrote: Ok I found the problem, which is likely a bug in Vivado. If you look at the new binary tree implementation of rv_plic_target, it has two nested generate loops within which the different nodes of the tree are connected with each other. Vivado doesn't seem to like the ternary statements on these lines: https://github.com/lowRISC/opentitan/blob/de3b0108bbaa630a3a005595bdd57ea01f2af1e7/hw/ip/rv_plic/rtl/rv_plic_target.sv#L86-L92 If I rewrite this with bitwise ops mimicking a multiplexor, the module synthesizes without issues, and I can run the uart tests with no problems. I am going to make a patch for this and make a bug report to Xilinx. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1355?email_source=notifications&email_token=AAH2RSTTAI2CFPCMJ2B5UVDQ6ILCXA5CNFSM4KHXCWWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJI56KA#issuecomment-575790888>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAH2RSVEICQPENDR5ZYTT4TQ6ILCXANCNFSM4KHXCWWA> .

[msfschaffner]

Yes will do. The patch for this issue can be found here: #1364. PTAL

[sjgitty]

Great debugging and patience, @msfschaffner ! Let us know what Xilinx has to say.

[msfschaffner]

Thanks everyone for helping track down this problem.
I have filed an issue with Xilinx:
https://forums.xilinx.com/t5/Synthesis/Simulation-Synthesis-Mismatch-with-Vivado-2018-3/m-p/1065923#M33849
Let's see what they respond...

In the meanwhile, the hotfix for this problem has been merged.
I would be great if somebody could confirm that the problem is resolved now.

[silvestrst]

That's great, thank you @msfschaffner

[msfschaffner]

Quick update:

This is a tooling issue (a signal gets optimized in the wrong way), and a Xilinx internal issue has been filed to get it fixed. In the meanwhile, follow the guidance provided here:
https://forums.xilinx.com/t5/Synthesis/Simulation-Synthesis-Mismatch-with-Vivado-2018-3/m-p/1066319/highlight/true#M33859

[asb]

With #1364 merged to workaround this, and #1402 to track adding further testing, I think this issue can now be closed?

We could rewrite this issue to track the removal of the in-tree workaround but that seems messy/confusing. I propose:

• @msfschaffner creates a new tracking issue for removing the workaround
• We close this bug

[msfschaffner]

Thanks @asb, agreed. I've opened another issue for this.

[asb]

Thanks Michael, I like your optimism that this will be the only Vivado synthesis bug we encounter :)