On 2026/07/09 23:36, Philippe Mathieu-Daudé wrote:
On 9/7/26 07:07, Akihiko Odaki wrote:
On 2026/07/09 7:13, Philippe Mathieu-Daudé wrote:
On 3/11/25 11:10, Philippe Mathieu-Daudé wrote:
From: Richard Henderson <[email protected]>
Signed-off-by: Richard Henderson <[email protected]>
Tested-by: Philippe Mathieu-Daudé <[email protected]>
Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
Signed-off-by: Philippe Mathieu-Daudé <[email protected]>
---
include/system/hvf_int.h | 3 +++
accel/hvf/hvf-all.c | 25 +++++++++++++++++++++++++
accel/hvf/trace-events | 1 +
3 files changed, 29 insertions(+)
diff --git a/include/system/hvf_int.h b/include/system/hvf_int.h
index 3d2be4092ef..5a57691885f 100644
--- a/include/system/hvf_int.h
+++ b/include/system/hvf_int.h
@@ -89,6 +89,9 @@ int hvf_arch_get_registers(CPUState *);
/* Must be called by the owning thread */
void hvf_arch_update_guest_debug(CPUState *cpu);
+void hvf_protect_clean_range(hwaddr addr, size_t size);
+void hvf_unprotect_dirty_range(hwaddr addr, size_t size);
+
struct hvf_sw_breakpoint {
vaddr pc;
vaddr saved_insn;
diff --git a/accel/hvf/hvf-all.c b/accel/hvf/hvf-all.c
index 0a4b498e836..e13abddbd9c 100644
--- a/accel/hvf/hvf-all.c
+++ b/accel/hvf/hvf-all.c
@@ -58,6 +58,31 @@ void assert_hvf_ok_impl(hv_return_t ret, const
char *file, unsigned int line,
abort();
}
+static void do_hv_vm_protect(hwaddr start, size_t size,
+ hv_memory_flags_t flags)
+{
+ hv_return_t ret;
+
+ trace_hvf_vm_protect(start, size, flags,
+ flags & HV_MEMORY_READ ? 'R' : '-',
+ flags & HV_MEMORY_WRITE ? 'W' : '-',
+ flags & HV_MEMORY_EXEC ? 'X' : '-');
+
+ ret = hv_vm_protect(start, size, flags);
+ assert_hvf_ok(ret);
Maybe this assertion is a bit aggressive toward users, since while
we know the host limits, we don't know what the guest wants to do.
I got:
qemu-system-aarch64: Error: ret = HV_ERROR (0xfae94001, at ../../
accel/ hvf/hvf-all.c:77)
Running with '-trace hvf_vm_map':
hvf_vm_map paddr:0x0000000010000000 size:0x00020000 vaddr:0x104514000
flags:0x07/RWX
hvf_vm_map paddr:0x0000000000000000 size:0x00020000 vaddr:0x104890000
flags:0x07/RWX
hvf_vm_map paddr:0x0000000400000000 size:0x80000000 vaddr:0x380000000
flags:0x07/RWX
hvf_vm_map paddr:0x0000000030bc0000 size:0x00040000 vaddr:0x1048b4000
flags:0x07/RWX
hvf_vm_map paddr:0x0000000050bc0000 size:0x00040000 vaddr:0x1048f8000
flags:0x07/RWX
hvf_vm_protect paddr:0x0000000430000000 size:0x00004000 flags:0x05/R-X
hvf_vm_protect paddr:0x0000000030000000 size:0x00004000 flags:0x05/R-X
qemu-system-aarch64: Error: ret = HV_ERROR (0xfae94001, at ../../
accel/ hvf/hvf-all.c:77)
Should we propagate the error to upper layer?
Are there cases we can safely ignore?
I don't think this should be propagated to the guest. In this case the
failing hv_vm_protect() is for a GPA that was not mapped into HVF in
the first place.
The memory listener currently calls hvf_log_start() for every dirty-
log section, but hvf_set_phys_mem() intentionally skips some sections,
for example writable non-RAM regions where we want every access to
trap. KVM handles the equivalent case by looking up the matching slot
and returning if there is none.
So I think we should only ignore/skip protect for ranges that we know
do not have an HVF mapping. We should not ignore arbitrary
hv_vm_protect() failures for mapped RAM, because then dirty logging
can miss writes or keep faulting forever.
A fix probably needs factoring the same mappable section test used by
hvf_set_phys_mem() and using it in log_start, log_stop and log_clear.
Indeed, I am having a log_clear -> DIRTY_MEMORY_CODE call on a ROMD
device. In commit d1f4ba9d94c ("accel/hvf: Simplify hvf_set_phys_mem")
we removed this check:
bool writable = !area->readonly && !area->rom_device;
...
- if (area->readonly ||
- (!memory_region_is_ram(area) && memory_region_is_romd(area))) {
- flags = HV_MEMORY_READ | HV_MEMORY_EXEC;
- } else {
- flags = HV_MEMORY_READ | HV_MEMORY_WRITE | HV_MEMORY_EXEC;
- }
+ flags = HV_MEMORY_READ | HV_MEMORY_EXEC | (writable ?
HV_MEMORY_WRITE : 0);
DIRTY_MEMORY_CODE is TCG-only, so it does not enable logging under HVF.
physical_memory_test_and_clear_dirty() may still be called for that
bitmap, but memory_region_clear_dirty_bitmap() calls log_clear()
whenever any client is logging the region.
The other clients are DIRTY_MEMORY_VGA and DIRTY_MEMORY_MIGRATION.
DIRTY_MEMORY_VGA is enabled per region by memory_region_set_log(), which
is not used for ROMD regions. Under HVF, DIRTY_MEMORY_MIGRATION can be
enabled by RAM migration or calc-dirty-rate in dirty-bitmap mode. If
neither was active, the region was probably not in ROMD mode.
The change in commit d1f4ba9d94c ("accel/hvf: Simplify
hvf_set_phys_mem") only simplified permissions for mapped sections; it
did not remove hvf_set_phys_mem()'s mapping checks.
Maybe we should use back one of this helpers instead of accessing
MemoryRegion internal fields?
1473 static inline bool memory_region_is_romd(const MemoryRegion *mr)
1474 {
1475 return mr->rom_device && mr->romd_mode;
1476 }
1716 static inline bool memory_region_is_rom(const MemoryRegion *mr)
1717 {
1718 return mr->ram && mr->readonly;
1719 }
2697 static inline bool memory_access_is_direct(const MemoryRegion *mr,
2698 bool is_write,
MemTxAttrs attrs)
2699 {
2700 if (!memory_region_supports_direct_access(mr)) {
2701 return false;
2702 }
2703 /* Debug access can write to ROM. */
2704 if (is_write && !attrs.debug) {
2705 return !mr->readonly && !mr->rom_device;
2706 }
2707 return true;
2708 }
For the dirty-log callbacks, none of these helpers alone captures the
complete mapping condition, including alignment. I think we should
factor the mappability check out of hvf_set_phys_mem() and reuse it in
hvf_log_start(), hvf_log_stop(), and hvf_log_clear().
Regards,
Akihiko Odaki
