[Libre-soc-dev] [OpenPOWER-HDL-Cores] microwatt dcache potential bug (overlap r0 and r1)

[Paul Mackerras]

On Fri, Jan 14, 2022 at 11:25:02PM +0000, Luke Kenneth Casson Leighton wrote:
> On Fri, Jan 14, 2022 at 10:26 PM lkcl <luke.leighton at gmail.com> wrote:
> 
> > >The VHDL certainly is designed to be able to handle a store coming in
> > >while r1.full = 0 and the state machine is in RELOAD_WAIT_ACK state.
> >
> > hmm then i need to look at that closely.
> 
> arrrgh, i think i've got it.
> 
> everything's "good"... the RELOAD_WAIT_ACKs completed, set
> r1.state == IDLE, no problem.
> 
> this here tries to set r1.full = 1, again, looks like it's fine:
> 
>                     -- Note that r1.full = 1 implies req_op = OP_NONE
>                     if req_op = OP_LOAD_MISS or req_op = OP_LOAD_NC or
>                         req_op = OP_STORE_MISS or req_op = OP_STORE_HIT then
>                         r1.req <= req;
>                         r1.full <= '1';
>                     end if;
> 
> but... butbutbut... down below, bear in mind r1.state is currently
> RELOAD_WAIT_ACK, and bear in mind ld_stbs_done=true, this
> code *also* activates:
> 
>                         if req.valid = '1' and req.same_tag = '1' and
>                             ((r1.dcbz = '1' and req.dcbz = '1') or
>                              (r1.dcbz = '0' and req.op = OP_LOAD_MISS)) and
>                             r1.store_row = get_row(req.real_addr) then
>                             r1.full <= '0';
> 
> and it *overwrites* r1.full back to zero.

Ummm, it shouldn't, because the store should have req.op =
OP_STORE_HIT or OP_STORE_MISS, not OP_LOAD_MISS, so the condition of
the if should be false.

I see this at line 1561 of dcache.py:

		(~r1.dcbz & (r1.req.op == Op.OP_LOAD_MISS))) &

Notice you have r1.req.op there whereas the VHDL has req.op.  I think
that's your bug.  (Similarly line 1560 has r1.req.dcbz not req.dcbz,
and line 1559 has r1.req.same_tag not req.same_tag.)

Paul.