On 8/10/26 07:52, Akihiko Odaki wrote:
On 2026/10/08 7:05, Philippe Mathieu-Daudé wrote:
On 7/10/26 21:50, Philippe Mathieu-Daudé wrote:
On 2026-10-07 16:50, Akihiko Odaki wrote:
On 2026/10/07 18:30, Philippe Mathieu-Daudé wrote:
From: Mark Burton <[email protected]>
Dirty-page logging handles a RAM write fault by marking the page
dirty,
restoring write permission, and retrying the guest instruction.
The handler also retried when the resolved RAM region had no dirty-log
client. This path neither restores permission nor advances the PC, so
the instruction can repeatedly exit to QEMU.
Retry only after marking the page dirty and restoring write
permission.
Otherwise, use the existing emulation path to complete the access and
advance the guest PC.
I can identify three cases of RAM write faults. The existing code
handles all three, but this patch breaks the third:
1. The RAM is read-only. assert(!mr->readonly) trips in this case.
2. Dirty logging is enabled. The vCPU marks the page dirty, restores
write permission, and retries the write.
3. Dirty logging is enabled when the fault occurs, but another thread
disables it before the vCPU acquires the BQL. The vCPU then
observes
a zero dirty-log mask. The write still needs to be retried because
write permission has already been restored. The existing code does
this correctly, but this patch falls through to emulation instead.
This patch is useful only if there is another case that the existing
code does not handle correctly. Please describe that case and preserve
the retry behavior in case 3.
Thanks for the deep review.
The reporter said:
We reproduced is a level-2 translation fault, DFSC 0x06, on a
dynamic DMI RAM leaf that has no corresponding HVF mapping.
address_space_translate() resolves the leaf as RAM, but the dirty-log
mask is zero, no page is marked or unprotected, and the unconditional
retry makes no progress.
That would be another case:
4. FlatView says RAM, but HVF has no mapping (nothing has changed
to make retry succeed).
So to prevent retrying a known translation miss that cannot make any
progress checking DFSC could be sufficient:
(also reproduced DFSC 0x07, but now irrelevant).
Checking DFSC specifically for a translation fault feels arbitrary. The
broader lesson from case 3 is that the faulting condition can be
resolved concurrently before acquiring the BQL. That same race window
applies to case 4. Instead of matching on DFSC, we should re-check
whether the faulting condition persists after acquiring the BQL.
I don't have direct contact with the reporter, so guessed from the
"dynamic DMI RAM" report which misguided me (no clue what is DMI btw).
Filtering out s1ptw also seems extraneous. If the page-table memory
requires dirty logging, we should handle dirty-page tracking for it
rather than skipping it with an additional guard.
Done (IIUC) as a separate patch.
However I had to do this change in hvf_set_phys_mem():
-- >8 --
diff --git a/accel/hvf/hvf-all.c b/accel/hvf/hvf-all.c
index 8bd9154a50b..7b34a5f586d 100644
--- a/accel/hvf/hvf-all.c
+++ b/accel/hvf/hvf-all.c
@@ -113,8 +113,14 @@ static void hvf_set_phys_mem(MemoryRegionSection
*section, bool add)
if (!QEMU_IS_ALIGNED(size, page_size) ||
!QEMU_IS_ALIGNED(gpa, page_size)) {
- /* Not page aligned, so we can not map as RAM */
- add = false;
+ return;
}
hvf_vm_unmap paddr:0x0000000010001000 size:0x00001000
qemu-system-aarch64: Error: ret = HV_BAD_ARGUMENT (0xfae94003,
at ../../ accel/hvf/hvf-all.c:123)
Abort trap: 6
The change to hvf_set_phys_mem() fixes error handling for misaligned
memory mappings and is worth submitting as a separate patch.
Yep, done.
This also implies case 4 isn't fully understood yet. Ideally, whenever
FlatView resolves a region as RAM, HVF should have a mapping for it. A
host-page-misaligned region is one scenario where HVF cannot create a
mapping, but that previously triggered an assert (HV_BAD_ARGUMENT),
ruling it out as the reporter's original root cause.
Please investigate why case 4 occurs and whether HVF can map the RAM
directly. If emulation is required, we should check for the specific
condition that requires it rather than treating a translation fault
alone as sufficient.
Please investigate why case 4 occurs in practice and consider adding a
missing mapping rather than relying on emulation. If emulation is
genuinely required, we should check for that specific condition rather
than assuming it for a translation fault.
Per the report QEMU v10.2.0 works and v11.0.0 doesn't. From there it was
easy to test up to the 6d0a5a44560..58bc47fd1c6 range. Testing commits
d1f4ba9d94c and 7d7654a643f was painful due to the amount of changes but
in the end bisection pointed at commit 58bc47fd1c6.
I followed your "Instead of matching on DFSC, we should re-check whether
the faulting condition persists after acquiring the BQL" suggestion,
since DFSC is only an indirect indication of a missing HVF mapping.
We are already with the BQL but now we assert it. Add added mapping
tracking in hvf_set_phys_mem() using an IntervalTree so can check that
back in the dabort handler before emulating. I am not checking vs
FlatView because I don't understand yet if this is necessary. Also I
need to implement the begin/commit MemoryListener handlers so the
hvf_set_phys_mem() is protected, but I might leave that as TODO and
post a v2. Testing is hard.
Thanks for your review and guidance :)
Phil.
