feat(bar): serve donor register BAR at its real PCI index (#613)#630
Merged
Conversation
The controller serves config-space BAR0 == the donor's primary register BAR, but bar_config exposed only the largest-MMIO metadata and templates read a never-set 'primary_bar' key (silently defaulting to 0). Emit served_bar_index (donor PCI index), primary_bar (list position), served_is_64bit and aperture from the served BAR so the IP aperture, cfgspace and controller agree.
Donor devices whose primary register BAR is not index 0 (e.g. NICs with
BAR0=IO, BAR2=MMIO) were served on the wrong BAR index, lowering fidelity.
Now the donor's primary MMIO BAR is served at its real PCI index N:
- IP override emits CONFIG.Bar{N}_* (size via Scale/Size, type/64-bit/prefetch)
and disables every other BAR so the host enumerates exactly one window.
- BAR controller wires the device impl to wr_bar[N]/rd_req_bar[N] via an
index-aware loop; the 64-bit partner slot N+1 is forced to 'none' (fixes a
latent double-drive for a 64-bit BAR0); loopaddr kept at slot 1 when free.
- cfgspace COE renders the served BAR at slot N with the 64-bit upper dword at
N+1; fixes the pre-existing hardcoded slot-2=IO bug.
- Overlay keys the learned-model lookup by served_bar_index.
7-series only; single served register BAR. 64-bit at BAR5 rejected (upper half
would be the Expansion ROM). Per PG054 the hard IP routes BAR hits by physical
index, so this matches what enumeration actually reads. Needs Vivado/lspci
hardware validation before merge.
Contributor
There was a problem hiding this comment.
Pull request overview
Improves device-clone fidelity (issue #613) by serving the donor's primary register BAR at its real PCI index instead of hard-coding BAR0. The IP override, BAR controller template, cfgspace COE, and learned-model overlay are all updated to agree on a single served_bar_index (donor index N), disabling all other BAR slots so the host enumerates exactly one window matching the donor.
Changes:
- Introduce
served_bar_index/served_is_64bit/aperture_sizeinbar_config, replacing the overloadedprimary_barsemantics, and extract them in the donor-IP override extractor. - Emit
CONFIG.Bar{N}_*for the served BAR (with size via Scale/Size, 64-bit/prefetch flags) and disable the other 5 standard BARs; reject 64-bit at index 5. - Rewrite the BAR controller SV template and cfgspace COE to serve / render the device impl at the donor's real index N (with 64-bit partner forced to
none, fixing the latent 64-bit BAR0 double-drive and the hardcoded slot-2-IO bug).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/device_clone/pcileech_context.py | Builds new served_bar_index / served_is_64bit / primary_bar (as list position) keys in bar_config. |
| src/vivado_handling/pcie_ip_donor_override.py | Adds aperture fields to DonorPCIeIPConfig, emits CONFIG.Bar{N}_* and disables the other BARs, with validation. |
| src/templating/sv_overlay_generator.py | Keys learned-model lookup by served_bar_index (fallback to legacy keys). |
| src/templates/sv/pcileech_tlps128_bar_controller.sv.j2 | Index-aware loop wires the device impl to slot N; partner slot forced to none for 64-bit. |
| src/templates/sv/pcileech_cfgspace.coe.j2 | Renders served BAR at donor slot N with 64-bit upper dword at N+1; removes hardcoded IO/positional layout. |
| tests/test_pcileech_context.py | New test that served metadata uses donor index N with primary_bar as list position. |
| tests/test_pcie_ip_donor_override_bar.py | New tests for Scale/Size encoding, index-N emit, 64-bit partner disable, rejections. |
| tests/test_cfgspace_bar_index.py | New tests that COE places the BAR at slot N and 64-bit upper dword at slot N+1. |
| tests/test_bar_controller_index_serving.py | New tests for index-N device gating, partner slot rules, single-driver invariants. |
| tests/test_bar_implementation_generation.py | Updates assertions to match the new "served register window" template header. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+7
to
+9
| The served BAR is always presented as config-space BAR0 (the only window the | ||
| PCILeech BAR controller services via ``i_bar0``), so v1 mirrors a single BAR. | ||
| """ |
Comment on lines
+20
to
+22
| - A5 scope: only the single served BAR is mirrored, presented as config-space | ||
| BAR0 (the one window the PCILeech controller answers). Multi-BAR layout | ||
| fidelity and donors whose served window isn't BAR0 are out of scope. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Addresses Copilot review on #630 — the A5 module scope note and the test docstring still described the served BAR as config-space BAR0; gap C serves it at the donor's real index N.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Improves device-clone fidelity for issue #613 by serving the donor's primary
register BAR at its real PCI BAR index, instead of always assuming BAR0.
Donors whose register window isn't BAR0 (common for NICs — e.g. Realtek
RTL8168 with BAR0=IO, BAR2=MMIO) were previously served on the wrong BAR index,
so the host enumerated a Xilinx-default BAR rather than the donor's. Per PG054
the 7-series hard IP routes BAR hits by physical PCI index, so the host sees what
the IP
Bar{N}config advertises — this PR makes that index match the donor.Commits
never-set
primary_barkey (which silently defaulted to 0) with explicitserved_bar_index(donor index),primary_bar(list position),served_is_64bit, and a served-BARaperture_size. This makes the IPaperture, cfgspace shadow, and controller all agree on one served BAR.
CONFIG.Bar{N}_*(size via Scale/Size, type/64-bit/prefetch) and disables every other BAR so exactly one window enumerates
(no phantom/hanging BARs).
wr_bar[N]/rd_req_bar[N]via anindex-aware loop; the 64-bit partner slot
N+1is forced tonone(fixes a latent double-drive for a 64-bit BAR0); loopaddr kept at slot 1
when free; byte-equivalent wiring when N=0.
N+1; also fixes the pre-existing hardcodedslot 2 = IObug.served_bar_index.Scope / limits
and IO-primary handling are deferred follow-ups.
Testing
serving, cfgspace index rendering) plus updated context/impl tests.
pre-existing, unrelated
donor-template/ CLI-help e2e tests.lspci -vvon anon-BAR0 donor (e.g. RTL8168) should confirm the donor BAR geometry enumerates
and BAR reads are serviced before merge — this can't be verified in CI.
Closes #613 (pending hardware validation).