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

Cleanup main processor controller #132

Merged
merged 13 commits into from Jul 11, 2019

Conversation

@vogelpi
Copy link
Contributor

commented Jul 10, 2019

This PR contains a series commits to cleanup the main processor controller inside ibex_controller.sv and solve related issues.

The main changes are:

  • Interrupt and debug requests no longer interrupt/abort multicycle instructions such as loads, stores, jumps, branches etc. Instead such instructions are first finished and if they cause an exception, the core jumps into the handler before serving the debug or interrupt requests. This fixes issues #108 and #121 .
  • Exceptions are prioritized according to the spec inside the controller FSM.
  • Obsolete signals between ID stage, IF stage and controller have been removed which simplifies the design.
  • The interplay of IF stage and controller has been streamlined.
@vogelpi

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2019

@towoe : I would be happy if you could have a look at the changes that have an impact on the RVFI. For example, the signal rvfi_valid_int in ibex_core.sv depends on id_valid/id_out_valid. I am pretty sure that this signal was not properly generated previously inside the ibex_controller.sv. It was also high when the core faced an illegal instruction exception. I don't think the output of the ID stage should be considered valid in this case.

@towoe
Copy link
Contributor

left a comment

Hi @vogelpi, constructing the valid signal for RVFI was not so straightforward. I did not want to change the core but needed a way to basically check when an instruction was valid and at which point it was retired. It might be that id_valid on its own was not correct, but in combination with the other signals the faulty behavior was masked out. I guess RVFI can be improved a lot / even rewrite now with the improvements made to the core. I did a quick check with the tracer and the signals look good to me.

rtl/ibex_controller.sv Outdated Show resolved Hide resolved
rtl/ibex_controller.sv Outdated Show resolved Hide resolved
rtl/ibex_controller.sv Outdated Show resolved Hide resolved
@imphil
Copy link
Collaborator

left a comment

Thanks Pirmin for this set of changes! I've left a couple comments inline.

Side note: I know it's not always easy to split things into different PRs when doing cleanups, but please try. PRs are much easier to review for me if they are smaller and have a single goal. (This one took me ~ 2.5 hours. It was fine today, but on other days it would have been hard to find such a long time consecutive time slot.)

rtl/ibex_controller.sv Outdated Show resolved Hide resolved
rtl/ibex_controller.sv Outdated Show resolved Hide resolved
rtl/ibex_controller.sv Outdated Show resolved Hide resolved
rtl/ibex_controller.sv Outdated Show resolved Hide resolved
rtl/ibex_controller.sv Outdated Show resolved Hide resolved
rtl/ibex_id_stage.sv Outdated Show resolved Hide resolved
rtl/ibex_id_stage.sv Outdated Show resolved Hide resolved
halt_id = 1'b1;
// stall IF stage to not starve debug and interrupt requests, these just
// need to wait until after the current (multicycle) instruction
if ((debug_req_i || irq) && stall && !debug_mode_q) begin

This comment has been minimized.

Copy link
@imphil

imphil Jul 11, 2019

Collaborator

You want to complete an instruction here if you're asked to

  • jump into debug mode: (debug_req_i && !debug_mode_q), or
  • get an interrupt request irq

so we get

if (stall && ((debug_req_i && !debug_mode_q) || irq))

This comment has been minimized.

Copy link
@vogelpi

vogelpi Jul 11, 2019

Author Contributor

No, according to the spec. Interrupts must be ignored in debug mode. See Debug Spec v0.13.2 p.39.

This comment has been minimized.

Copy link
@imphil

imphil Jul 11, 2019

Collaborator

OK, now I get it.

For now or for a later cleanup:

We have two things here were !debug_mode_q is used:

  • jump into debug mode: (debug_req_i && !debug_mode_q)
  • handle interrupts only in non-debug mode (irq & !debug_mode_q)

How about this:

assign enter_debug_mode = debug_req_i & ~debug_mode_q;

// extend earlier code
assign irq =irq_req_ctrl_i & m_IE_i & ~debug_mode_q;

and then use enter_debug_mode and irq without the additional !debug_mode_q guards.

This would also ensure that interrupts are really ignored in debug mode. (Looking at the FIRST_FETCH code, this might not always be the case currently.)

This comment has been minimized.

Copy link
@vogelpi

vogelpi Jul 11, 2019

Author Contributor

That makes sense! I will include it in the next PR adding CLINT.

rtl/ibex_controller.sv Show resolved Hide resolved
rtl/ibex_controller.sv Show resolved Hide resolved
@vogelpi

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

Hi @towoe ,
thanks a lot for checking the RVFI but also for reviewing the rest of the code!
Best regards,
Pirmin

@vogelpi vogelpi force-pushed the vogelpi:vogelpi-dev branch 2 times, most recently from 405ca7a to 2b7a0df Jul 11, 2019

@vogelpi

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

Thanks a lot for your feedback @imphil , it is very much appreciated.

I implemented most of the changes you suggested. The ones I did not implement are related to the fact that in debug mode, interrupts are not active as pointed out in the conversations.

vogelpi added some commits Jul 4, 2019

Replace `pipe_flush` signal by `wfi_insn`
This signal is triggered iff a `wfi` instruction is decoded.
Fix control for interrupts, debug request and multicycle instr
This commit makes sure that interrupts and debug requests do not
interrupt currently running multicycle instructions. Priority is
given 1) to the currently running instruction, possible exceptions
caused by this instruction, 2) debug requests, 3) interrupt
requests.

Previously, currently running instructions were aborted upon
incoming debug and interrupt requests, which could corrupt the
processor state and lead to exceptions being ignored.

This commit resolves #108 and #121.
Fix handling of single stepping
Single stepping should execute exactly one more instruction, not abort
any running (multicycle) instructions, and only enter debug mode after
the current instruction is finished. In case the current instruction
leads to an exception, the exception registers must be set accordingly,
but the core must jump into debug mode instead of the exception
handler.

Previous to this commit, single stepping would immediately jump into
debug mode and ignore any exceptions.
Do not set PC when setting exceptions registers in single stepping
In the next state, the PC is set to jump into debug anyway.
Rework register file write enable mux
The register file shall not be written upon illegal CSR operations
and when the core is idle (waiting for new instructions). Otherwise
the write enable signal from the WB FSM/decoder is used.
Cleanup interplay of IF and ID stage (controller)
Once the core is executing, the IF stage now only gets two signals
from the controller: One to clear the instruction valid output and one
to prevent the IF stage from pushing new instructions.

The former `if_valid_o` of the IF stage has been renamed to clarify
that this is not a valid bit but just the write enable for the
pipeline register.

@vogelpi vogelpi force-pushed the vogelpi:vogelpi-dev branch from 2b7a0df to b0d91ce Jul 11, 2019

vogelpi added some commits Jul 10, 2019

Use decoder regfile write enable for non-LSU/mult/div ops
Previsouly, the regfile write enable output of the decoder was always
fed through the WB FSM which is unnecessary except for stores and
mult/div operations.

@vogelpi vogelpi force-pushed the vogelpi:vogelpi-dev branch from b0d91ce to 8f9a5a7 Jul 11, 2019

@vogelpi vogelpi requested a review from imphil Jul 11, 2019

@imphil

imphil approved these changes Jul 11, 2019

@vogelpi vogelpi merged commit c1bceb2 into lowRISC:master Jul 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.