Perf: Fix false store-to-load collisions across different physical pages

[anushkagupta200615-jpg]

Fixes #20.

Description

This PR fixes a performance regression in the store_buffer where loads were falsely stalled by in-flight stores.

Previously, the collision detector only checked the page offset (bits [11:0]) of the physical addresses. This meant that a store and a load on completely different physical pages would be falsely flagged as a collision if they happened to share the same page offset.

This fix wraps the offset checks in an initial if statement that first verifies the physical page numbers (PPN) match (data_rs1[PHY_VIRT_MAX_ADDR_SIZE-1:12]).

Changes Made

• store_buffer.sv: Added [PHY_VIRT_MAX_ADDR_SIZE-1:12] checks to ensure only accesses on the exact same physical page are evaluated for offset overlap.
• tb_store_buffer.sv: Added a new standalone SystemVerilog testbench that proves:
  1. False positives no longer occur (different physical pages, same offset -> no collision).
  2. True positives are still caught (same physical page, overlapping offset -> collision).

[Saksham05oct]

Does the collision detector always compare addresses that are guaranteed to be in the same, normalized address form? Concretely, can store entries ever be written into the buffer before address translation (or with guest vs host PPN encodings), or does the design guarantee enqueue happens only after translation and normalization? Also, is the page-offset width assumed fixed (4KB) everywhere this logic runs?
If those invariants hold, the change looks good, if not, there may be a corner case worth addressing (I can draft a small follow-up PR to make the assumption explicit and add a test). Could you confirm?

[Shivam-Shukla0]

Building on @Saksham05oct's question — I think the answer is concretely "no", and it may make this change unsafe rather than just cosmetic.

In load_store_queue.sv, st_buff_collision is consumed on paths that don't check whether the head entry has been translated:

• :289 — if (read_enable && is_next_load && !st_buff_collision && !io_address_space)
• :339 — the blocked_store_o path

whereas :308 and :318 do gate on control_table[head].translated. And a data_rs1 only holds a physical address once translated: :140-141 set translated = translate_enable together with data_rs1 = {dtlb_comm_i.resp.ppn, data_rs1[11:0]}.

So on those paths the load address feeding load_addr_i can still be virtual while store buffer entries hold physical addresses. Comparing [11:0] works in both cases because the page offset is translation-invariant, which may be exactly why the original comparison stops at bit 11. Comparing [PHY_VIRT_MAX_ADDR_SIZE-1:12] compares a VPN against a PPN in that situation, so a real collision on the same physical page would not be detected — the load would bypass an older store to the same address. That turns a conservative false-positive (a stall) into a missed dependency, which is a correctness issue rather than a performance one.

Two smaller things:

• PHY_VIRT_MAX_ADDR_SIZE is max(PHY_ADDR_SIZE, VIRT_ADDR_SIZE) (drac_pkg.sv:35), so it isn't really a PPN width — worth using the physical width explicitly if this direction is kept.
• tb_store_buffer.sv drives the module directly, so it can't observe the VA/PA distinction that comes from the LSQ. Reproducing this would need LSQ-level stimulus with an untranslated head load.

I might well be misreading the enqueue/dequeue ordering — if the design does guarantee the head is always translated before st_buff_collision is sampled, this concern goes away. Would be good to hear from the maintainers.