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

[Paul Mackerras]

On Fri, Jan 14, 2022 at 02:07:23AM +0000, Luke Kenneth Casson Leighton wrote:
> On Thu, Jan 13, 2022 at 10:49 PM Paul Mackerras <paulus at ozlabs.org> wrote:
> 
> > I guess the first question is, what git commit in the microwatt repo
> > are you working from?
> 
> good point! currently f636bb7c3999d
> 
> >  I would suggest you use dcache.vhdl from
> > current master, because with the pipelined loadstore1.vhdl, microwatt
> > can and does do back-to-back loads and stores successfully.
> 
> something i'll need to write a unit test for.
> 
> > If you can't use dcache.vhdl from the head of the master branch, then
> > have a look at commit f636bb7c3999 in particular and also
> > f812832ad78d, 1a9834c506a8 and 0b23a5e76000.
> 
> ah one of the big(ish) ones i've avoided was the 4-way selection
> from wb bus, 2 forwarding and the cache. have to work up to it.
> 
> > > i suspect that the occasional LD corruption is down to r1.full
> > > being set to zero, a batch of ACKs still being expected, but
> > > a *new* r0 LD operation comes in, gets transferred to r1 *whilst
> > > there are still ACKs pending from the previous LD*.
> >
> > The new ld would get into r1.req but shouldn't affect r1.wb until all
> > the acks for the previous operation have come in.
> 
> a trace i'm currently looking at, STORE_WAIT_ACK ends early
> due to "stbs_done and acks=1", which sets r1.state = IDLE
> and also sets r1.dec_acks = 1.
> 
>             if wishbone_in.ack = '1' then
>                         if stbs_done and acks = 1 then
>                             r1.state <= IDLE;
>                             r1.wb.cyc <= '0';
>                             r1.wb.stb <= '0';
>                         end if;
>                         r1.dec_acks <= '1';
> 
> HOWEVER... on the next cycle (remember that
> r1.dec_acks is set to 1...)
> 
> * the case r1.state=IDLE kicks in (because we just requested that,
>    line 1382)
> * line 1431, OP_STORE_MISS kicks us back to STORE_WAIT_ACK
>   in the next cycle
> 
> BUT...
> 
> r1.dec_acks was, during that time, set to ZERO (line 1312).
> 
> thus, the number of acks gets out of sync by one, because of
> this missed (tiny) window.

The value of r1.acks_pending is only relevant in STORE_WAIT_ACK state,
and it is set correctly (to 1) on entry to that state from IDLE state,
which is the only way we get to that state.

> now, the question is: does this actually matter?  i can see some of
> the state information in r1 being overwritten (r1.acks_pending=1,
> r1.state= STORE_WAIT_ACK/RELOAD_WAIT_ACK etc), which
> in theory should be ok.
> 
> it is however quite confusing, when looking at signal traces,
> to find that r1.req.op stays in OP_STORE_MISS, dec_acks remains
> at 1, acks_pending remains at 1.

Fair enough; and it is possible that decoupling the update of
r1.acks_pending from r1.state might result in simpler logic.
However, I don't think we have identified any bug yet.  What exactly
is the misbehaviour that you are seeing?

Paul.