Add overlap check between MMIO passthrough regions and MMIO intercept handler regions

[Solicey]

#327

[Solicey]

It seems that CI server is currently unavailable. @dallasxy

[dallasxy]

It seems that CI server is currently unavailable. @dallasxy

server has already rescan the requests

[li041]

Before this call, the MSI-X path removes the old handler at old_vaddr and updates the BAR/virtual address state. With the new overlap checks, mmio_region_register can now return an error; propagating it here leaves the old handler removed while no new handler is installed. Subsequent accesses to the old BAR then become unhandled and the device state is inconsistent. Please validate the new range before removing the old handler, or restore the old handler and BAR state on failure. The bridge MSI-X paths have the same ordering.

[Solicey]

The MSI-X update path now preflights the new range with mmio_region_register_would_fail before removing the old handler at old_vaddr. If the new table region would overlap an existing handler or passthrough region, it returns EINVAL up front and the old handler stays installed. Applied to all four MSI-X sites (endpoint/bridge, direct/normal).

[li041]

This path deletes the old passthrough mapping before calling try_insert_passthrough_region_quiet. If the new BAR overlaps an MMIO handler, the helper returns an error and ? exits without restoring the old mapping, even though the BAR and hardware state were already updated. The guest then loses the old BAR mapping. Please make this update transactional by preflighting the new region or rolling back the deletion on error. The ROM and bridge BAR paths need the same treatment.

[Solicey]

The BAR and ROM paths now preflight the new region with is_mmio_handler_overlap before deleting the old passthrough mapping. A would-be overlap is rejected before any mutation, so the old mapping is never lost. Applied to the four BAR and two ROM sites.

[li041]

viommu_init stores a global instance before this fallible MMIO registration. If the new overlap check rejects IOMMU_SYS_BASE, zone_create returns here without calling viommu_remove. Since the zone has not been published yet, a retry for the same ID sees the stale VIOMMU_ARR entry and skips initialization. Please clean up the vIOMMU instance on registration failure, or register the handler before committing the global instance.

[Solicey]

zone_create now calls viommu_mmio_handler_register before viommu_init. If registration is rejected, viommu_init never runs, so no global VIOMMU_ARR entry is committed and there is no stale instance to clean up.

[li041]

Overall, this PR improves error reporting by making MMIO and passthrough registration fallible and detecting overlapping regions. However, the newly propagated errors are not handled transactionally: several paths mutate state before an operation that can now fail, such as removing old MMIO handlers, deleting old stage-2 mappings, or creating the global vIOMMU instance. When the operation fails, the previous state is not restored, which can leave stale or incomplete mappings and leaked global state. Please add preflight validation or explicit rollback/cleanup for these dynamic update paths, together with tests that verify state remains unchanged after rejected registrations.

[li041]

Why these memory regions are removed? Can you give an abstract when creates a PR? It makes review difficult without any useful introduction.

[li041]

@Solicey

[Solicey]

These three regions (0xf4000000 / 0xf2000000 / 0xf0000000) are the DWC PCIe config spaces. They are already the cfg_base of the matching ROOT_DWC_ATU_CONFIG entries and are exposed through the DWC vPCI MMIO intercept handlers (virtual_pci_mmio_init_delay), so the MEM_TYPE_IO passthrough declarations were redundant. With the new overlap check, registering those handlers over the passthrough regions failed and broke the target at boot; removing the passthrough regions lets the config-space handlers register cleanly.

[li041]

With the new overlap check, registering those handlers over the passthrough regions failed and broke the target at boot;

What does broke mean here? If it does mean panic, then the modification isn't robust enough.

[li041]

These three regions (0xf4000000 / 0xf2000000 / 0xf0000000) are the DWC PCIe config spaces. They are already the cfg_base of the matching ROOT_DWC_ATU_CONFIG entries and are exposed through the DWC vPCI MMIO intercept handlers (virtual_pci_mmio_init_delay), so the MEM_TYPE_IO passthrough declarations were redundant. With the new overlap check, registering those handlers over the passthrough regions failed and broke the target at boot; removing the passthrough regions lets the config-space handlers register cleanly.

So why these changes are dropped? Is your implemetion really useful?

[li041]

The commit history currently contains a large number of commits that consist solely of comment/annotation changes. Could we please clean this up by using git rebase -i? This makes the history noisy and harder to follow when reviewing changes or bisecting issues. This would greatly improve readability and make future code archaeology much easier.

[li041]

Thanks for the effort on this. I've gone through the changes and identified several critical issues that appear to stem from the AI-assisted modifications. These issues suggest that the generated code did not undergo sufficient manual verification before submission. I'd kindly request that future PRs include a thorough self-review of AI-generated diffs prior to assigning reviewers.

[li041]

This preflight check runs after the BAR state has already been updated: config_value, the BAR virtual/value caches, MSI-X metadata, and, in the direct/root path, the hardware BAR register have all been modified above. If this check rejects the new range, the old stage-2 mapping or MMIO handler remains active while the device now points to the new address, leaving the BAR state inconsistent and making subsequent accesses fail. Please perform the validation before mutating BAR or hardware state, or roll back all changes on failure. The ROM and bridge BAR paths have the same ordering issue.

[li041]

The duplicate-start path bypasses the new overlap checks and returns success after updating only the handler and argument. If the same start address is registered with a different size, the code merely logs the mismatch and keeps the old region.size, so the new range is neither validated against passthrough regions nor represented correctly by find_mmio_region. Please revalidate duplicate registrations and either update the stored size consistently or reject size mismatches.

[li041]

Moving viommu_init after MMIO registration fixes the registration-failure case, but the global vIOMMU instance is still created before the remaining fallible steps in zone_create, such as arch_zone_pre_configuration, IOMMU setup, and post-configuration. If any of those steps fails, VIOMMU_ARR retains an instance for a zone that was never published; a retry with the same zone ID then skips initialization and reuses stale state. Please defer the global initialization until zone creation succeeds or add cleanup for every subsequent failure path.

[li041]

The current interface has duplicated overlap checks: mmio_region_register() validates the range internally, while several PCI BAR paths perform their own preflight checks before manually removing the old handler or stage-2 mapping. This duplication is error-prone, and the caller-controlled check -> remove -> register sequence is also the reason rollback is difficult: any failure between these steps can leave BAR, MMIO, and stage-2 state inconsistent.

A workable design would be to keep validation inside ZoneInner and add atomic replacement APIs such as replace_mmio_handler(old_region, new_region) and replace_passthrough_region(old_region, new_region). These APIs should validate the new range while excluding the old region, perform the update as one operation, and restore the old mapping if the commit fails. Callers would then no longer need to duplicate overlap checks or manage rollback manually. mmio_region_register() should remain as the final invariant guard, and duplicate registrations with a different size should return an error.

[Solicey]

The previous attempt rejected the overlap and made the registration path return an error, which required touching many call sites and a large refactor. This version only logs a warning, so the change is much smaller. It still catches the overlap at both register-time and BAR/ROM remap-time; the only difference is the user now has to resolve the conflict themselves.

[li041]

Thanks for clarifying the scope reduction. I understand that logging a warning avoids touching many call sites, and the diagnostic itself is useful.

This version only logs a warning, so the change is much smaller.

However, with warning-only behavior, the conflicting MMIO and passthrough regions are still accepted, so the system can continue with an ambiguous or invalid mapping; the actual failure is merely deferred to a later access path.

This means the PR no longer enforces the invariant that motivated the change.

Since resolving this correctly appears to require atomic replacement, rollback, and a broader call-site refactor, would it be more appropriate to track that work in a dedicated issue and delay this pr until we have a dedicated design?

Otherwise, please document why continuing after an overlap is safe and add tests covering the resulting behavior.

[Solicey]

Thanks. You're right — with warning-only this no longer enforces the invariant, and there's definitely room to improve it. The proper fix (atomic replacement + rollback) does need a broader refactor. I'd suggest tracking that as a follow-up issue and solving it properly there.

To be clear about what this PR is and isn't: the warning is only meant as a debugging aid. It does not mean an overlap is safe — users still has to make sure the regions don't conflict, and the warning just points them at the problem so they can decide how to fix it themselves.

[li041]

I'd suggest tracking that as a follow-up issue and solving it properly there.

So are there any issues to track this?

To be clear about what this PR is and isn't: the warning is only meant as a debugging aid. It does not mean an overlap is safe — users still has to make sure the regions don't conflict, and the warning just points them at the problem so they can decide how to fix it themselves.

So we should at least document them inline, otherwise it's wrong-prone.

Actually, my suggestion is below

would it be more appropriate to track that work in a dedicated issue and delay this pr until we have a dedicated design?

[Solicey]

Thanks, agreed. Warning-only doesn't enforce the invariant, and the proper fix does need atomic replacement + rollback, which is a broader refactor.

I've opened a follow-up issue to track that work (#385), and added TODO comments as a remark.

[li041]

LGTM! Thanks for your contribution. Looking forward to your later efforts!