[PATCH v2 0/6] VFIO: cpr-transfer fixes

[PATCH v2 0/6] VFIO: cpr-transfer fixes

[Zhenzhong Duan]

Hi,

Patch1: fixed an error restore path when virtio-mem is configured.
Patch2: fixed assert failure on error restore path, this issue happens
no matter if virtio-mem is configured.
Some trick is played to trigger the error path,
see https://github.com/yiliu1765/qemu/commit/494d19e7f7242dbc47d7f236937cde0c396a4a7c

Patch3-4: issue only happens with two or more VFIO devices, no issue
if only one VFIO device.

Patch5: fix a bug that impact "query-balloon" execution

Patch6: SIGSEGV if I send "query-balloon" to source qmp monitor,
I'm not quite sure if it's deserved to be fixed, as guest has been
migrated to destination, it's not a big issue for source qemu to
SIGSEGV?

Thanks
Zhenzhong

Changelog:
v2:
- minor polishment to commit log (Steve)
- keep kvm_state so "query-balloon" could work after CPR-transfer (Markus)
- add a fix which is found during "query-balloon" execution
- rebased to master

Zhenzhong Duan (6):
  vfio/container: Remap only populated parts in a section
  vfio/cpr-legacy: drop an erroneous assert
  vfio/iommufd: Set cpr.ioas_id on source side for CPR transfer
  vfio/iommufd: Restore vbasedev's reference to hwpt after CPR transfer
  accel/kvm: Fix an erroneous check on coalesced_mmio_ring
  accel/kvm: Fix SIGSEGV when execute "query-balloon" after CPR transfer

 include/hw/vfio/vfio-cpr.h |  2 +-
 accel/kvm/kvm-all.c        | 14 ++++++--------
 hw/vfio/cpr-legacy.c       | 22 +++++++++++++++-------
 hw/vfio/iommufd.c          |  8 ++++----
 hw/vfio/listener.c         |  4 ++--
 5 files changed, 28 insertions(+), 22 deletions(-)

-- 
2.47.1

[PATCH v2 1/6] vfio/container: Remap only populated parts in a section

[Zhenzhong Duan]

If there are multiple containers and unmap-all fails for some of them, we
need to remap vaddr for the other containers for which unmap-all succeeded.
When ram discard is enabled, we should only remap populated parts in a
section instead of the whole section.

Fixes: eba1f657cbb1 ("vfio/container: recover from unmap-all-vaddr failure")
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Steven Sistare <steven.sistare@oracle.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 include/hw/vfio/vfio-cpr.h |  2 +-
 hw/vfio/cpr-legacy.c       | 20 +++++++++++++++-----
 hw/vfio/listener.c         |  4 ++--
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/include/hw/vfio/vfio-cpr.h b/include/hw/vfio/vfio-cpr.h
index 26ee0c4fe1..56fa226f35 100644
--- a/include/hw/vfio/vfio-cpr.h
+++ b/include/hw/vfio/vfio-cpr.h
@@ -68,7 +68,7 @@ bool vfio_cpr_container_match(struct VFIOLegacyContainer *container,
 void vfio_cpr_giommu_remap(struct VFIOContainer *bcontainer,
                            MemoryRegionSection *section);
 
-bool vfio_cpr_ram_discard_register_listener(
+bool vfio_cpr_ram_discard_replay_populated(
     struct VFIOContainer *bcontainer, MemoryRegionSection *section);
 
 void vfio_cpr_save_vector_fd(struct VFIOPCIDevice *vdev, const char *name,
diff --git a/hw/vfio/cpr-legacy.c b/hw/vfio/cpr-legacy.c
index bbf7a0d35f..9a37e8c604 100644
--- a/hw/vfio/cpr-legacy.c
+++ b/hw/vfio/cpr-legacy.c
@@ -227,22 +227,32 @@ void vfio_cpr_giommu_remap(VFIOContainer *bcontainer,
     memory_region_iommu_replay(giommu->iommu_mr, &giommu->n);
 }
 
+static int vfio_cpr_rdm_remap(MemoryRegionSection *section, void *opaque)
+{
+    RamDiscardListener *rdl = opaque;
+
+    return rdl->notify_populate(rdl, section);
+}
+
 /*
  * In old QEMU, VFIO_DMA_UNMAP_FLAG_VADDR may fail on some mapping after
  * succeeding for others, so the latter have lost their vaddr.  Call this
- * to restore vaddr for a section with a RamDiscardManager.
+ * to restore vaddr for populated parts in a section with a RamDiscardManager.
  *
- * The ram discard listener already exists.  Call its populate function
+ * The ram discard listener already exists.  Call its replay_populated function
  * directly, which calls vfio_legacy_cpr_dma_map.
  */
-bool vfio_cpr_ram_discard_register_listener(VFIOContainer *bcontainer,
-                                            MemoryRegionSection *section)
+bool vfio_cpr_ram_discard_replay_populated(VFIOContainer *bcontainer,
+                                           MemoryRegionSection *section)
 {
+    RamDiscardManager *rdm = memory_region_get_ram_discard_manager(section->mr);
     VFIORamDiscardListener *vrdl =
         vfio_find_ram_discard_listener(bcontainer, section);
 
     g_assert(vrdl);
-    return vrdl->listener.notify_populate(&vrdl->listener, section) == 0;
+    return ram_discard_manager_replay_populated(rdm, section,
+                                                vfio_cpr_rdm_remap,
+                                                &vrdl->listener) == 0;
 }
 
 int vfio_cpr_group_get_device_fd(int d, const char *name)
diff --git a/hw/vfio/listener.c b/hw/vfio/listener.c
index 3b6f17f0c3..04c64874fa 100644
--- a/hw/vfio/listener.c
+++ b/hw/vfio/listener.c
@@ -577,8 +577,8 @@ void vfio_container_region_add(VFIOContainer *bcontainer,
             if (!vfio_ram_discard_register_listener(bcontainer, section, &err)) {
                 goto fail;
             }
-        } else if (!vfio_cpr_ram_discard_register_listener(bcontainer,
-                                                           section)) {
+        } else if (!vfio_cpr_ram_discard_replay_populated(bcontainer,
+                                                          section)) {
             error_setg(&err,
                        "vfio_cpr_ram_discard_register_listener for %s failed",
                        memory_region_name(section->mr));
-- 
2.47.1

[PATCH v2 2/6] vfio/cpr-legacy: drop an erroneous assert

[Zhenzhong Duan]

vfio_legacy_cpr_dma_map() is not only used in post_load on destination
but also error recovery path on source side. Assert it for destination
is wrong.

Fixes: 7e9f21411302 ("vfio/container: restore DMA vaddr")
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Steve Sistare <steven.sistare@oracle.com>
---
 hw/vfio/cpr-legacy.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/vfio/cpr-legacy.c b/hw/vfio/cpr-legacy.c
index 9a37e8c604..c2b81f4a4b 100644
--- a/hw/vfio/cpr-legacy.c
+++ b/hw/vfio/cpr-legacy.c
@@ -52,8 +52,6 @@ static int vfio_legacy_cpr_dma_map(const VFIOContainer *bcontainer,
         .size = size,
     };
 
-    g_assert(cpr_is_incoming());
-
     if (ioctl(container->fd, VFIO_IOMMU_MAP_DMA, &map)) {
         return -errno;
     }
-- 
2.47.1

[PATCH v2 3/6] vfio/iommufd: Set cpr.ioas_id on source side for CPR transfer

[Zhenzhong Duan]

On source side, if there are more than one VFIO devices and they
attach to same container, only the first device sets cpr.ioas_id,
the others are bypassed. We should set it for each device, or
else only first device works.

Fixes: 4296ee07455e ("vfio/iommufd: reconstruct device")
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Steve Sistare <steven.sistare@oracle.com>
---
 hw/vfio/iommufd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index f0ffe23591..13c5c49d5e 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -602,7 +602,6 @@ skip_ioas_alloc:
     container->be = vbasedev->iommufd;
     container->ioas_id = ioas_id;
     QLIST_INIT(&container->hwpt_list);
-    vbasedev->cpr.ioas_id = ioas_id;
 
     bcontainer = VFIO_IOMMU(container);
     vfio_address_space_insert(space, bcontainer);
@@ -636,6 +635,8 @@ skip_ioas_alloc:
     bcontainer->initialized = true;
 
 found_container:
+    vbasedev->cpr.ioas_id = container->ioas_id;
+
     ret = ioctl(devfd, VFIO_DEVICE_GET_INFO, &dev_info);
     if (ret) {
         error_setg_errno(errp, errno, "error getting device info");
-- 
2.47.1

[PATCH v2 4/6] vfio/iommufd: Restore vbasedev's reference to hwpt after CPR transfer

[Zhenzhong Duan]

After CPR transfer, if there are more than one VFIO devices, device is
not added to hwpt->device_list and its reference to hwpt isn't restored
on destination. We still need to call iommufd_cdev_attach_container() to
restore it after a matching container is found, or else SIGSEV triggers.

Fixes: 4296ee07455e ("vfio/iommufd: reconstruct device")
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Steve Sistare <steven.sistare@oracle.com>
---
 hw/vfio/iommufd.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 13c5c49d5e..c6ca5db009 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -560,10 +560,9 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
             continue;
         }
 
-        if (!cpr_is_incoming()) {
+        if (!cpr_is_incoming() ||
+            (vbasedev->cpr.ioas_id == container->ioas_id)) {
             res = iommufd_cdev_attach_container(vbasedev, container, &err);
-        } else if (vbasedev->cpr.ioas_id == container->ioas_id) {
-            res = true;
         } else {
             continue;
         }
-- 
2.47.1

[PATCH v2 5/6] accel/kvm: Fix an erroneous check on coalesced_mmio_ring

[Zhenzhong Duan]

According to KVM uAPI, coalesced mmio page is KVM_COALESCED_MMIO_PAGE_OFFSET
offset from kvm_run pages. For x86 it's 2 pages offset, for arm it's 1 page
offset currently. We shouldn't presume it's hardcoded 1 page or else
coalesced_mmio_ring will not be cleared in do_kvm_destroy_vcpu() in x86.

Fixes: 7ed0919119b0 ("migration: close kvm after cpr")
Cc: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 accel/kvm/kvm-all.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 9060599cd7..23fd491441 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -523,7 +523,8 @@ static int do_kvm_destroy_vcpu(CPUState *cpu)
     }
 
     /* If I am the CPU that created coalesced_mmio_ring, then discard it */
-    if (s->coalesced_mmio_ring == (void *)cpu->kvm_run + PAGE_SIZE) {
+    if (s->coalesced_mmio_ring ==
+           (void *)cpu->kvm_run + s->coalesced_mmio * PAGE_SIZE) {
         s->coalesced_mmio_ring = NULL;
     }
 
-- 
2.47.1

[PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute "query-balloon" after CPR transfer

[Zhenzhong Duan]

After CPR transfer, source QEMU closes kvm fd and sets kvm_state to NULL,
"query-balloon" will check kvm_state->sync_mmu and trigger NULL pointer
reference.

We don't need to NULL kvm_state as all states in kvm_state aren't released
actually. Just closing kvm fd is enough so we could still query states
through "query_*" qmp command.

Opportunistically drop an unnecessary check in kvm_close().

Fixes: 7ed0919119b0 ("migration: close kvm after cpr")
Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 accel/kvm/kvm-all.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 23fd491441..b4c717290d 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -639,13 +639,10 @@ void kvm_close(void)
         cpu->kvm_vcpu_stats_fd = -1;
     }
 
-    if (kvm_state && kvm_state->fd != -1) {
-        close(kvm_state->vmfd);
-        kvm_state->vmfd = -1;
-        close(kvm_state->fd);
-        kvm_state->fd = -1;
-    }
-    kvm_state = NULL;
+    close(kvm_state->vmfd);
+    kvm_state->vmfd = -1;
+    close(kvm_state->fd);
+    kvm_state->fd = -1;
 }
 
 /*
-- 
2.47.1

Re: [PATCH v2 5/6] accel/kvm: Fix an erroneous check on coalesced_mmio_ring

[Steven Sistare]

On 9/28/2025 4:54 AM, Zhenzhong Duan wrote:
> According to KVM uAPI, coalesced mmio page is KVM_COALESCED_MMIO_PAGE_OFFSET
> offset from kvm_run pages. For x86 it's 2 pages offset, for arm it's 1 page
> offset currently. We shouldn't presume it's hardcoded 1 page or else
> coalesced_mmio_ring will not be cleared in do_kvm_destroy_vcpu() in x86.
> 
> Fixes: 7ed0919119b0 ("migration: close kvm after cpr")
> Cc: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

Reviewed-by: Steve Sistare <steven.sistare@oracle.com>

> ---
>   accel/kvm/kvm-all.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 9060599cd7..23fd491441 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -523,7 +523,8 @@ static int do_kvm_destroy_vcpu(CPUState *cpu)
>       }
>   
>       /* If I am the CPU that created coalesced_mmio_ring, then discard it */
> -    if (s->coalesced_mmio_ring == (void *)cpu->kvm_run + PAGE_SIZE) {
> +    if (s->coalesced_mmio_ring ==
> +           (void *)cpu->kvm_run + s->coalesced_mmio * PAGE_SIZE) {
>           s->coalesced_mmio_ring = NULL;
>       }
>   

Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute "query-balloon" after CPR transfer

[Steven Sistare]

On 9/28/2025 4:54 AM, Zhenzhong Duan wrote:
> After CPR transfer, source QEMU closes kvm fd and sets kvm_state to NULL,
> "query-balloon" will check kvm_state->sync_mmu and trigger NULL pointer
> reference.
> 
> We don't need to NULL kvm_state as all states in kvm_state aren't released
> actually. Just closing kvm fd is enough so we could still query states
> through "query_*" qmp command.

IMO this does not make sense.  Much of the state in kvm_state was derived
from ioctl's on the descriptors, and closing them invalidates it.  Asking
historical questions about what used to be makes no sense.

Clearing kvm_state and setting kvm_allowed=false would be a safer fix.

- Steve

> Opportunistically drop an unnecessary check in kvm_close().
> 
> Fixes: 7ed0919119b0 ("migration: close kvm after cpr")
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   accel/kvm/kvm-all.c | 11 ++++-------
>   1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 23fd491441..b4c717290d 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -639,13 +639,10 @@ void kvm_close(void)
>           cpu->kvm_vcpu_stats_fd = -1;
>       }
>   
> -    if (kvm_state && kvm_state->fd != -1) {
> -        close(kvm_state->vmfd);
> -        kvm_state->vmfd = -1;
> -        close(kvm_state->fd);
> -        kvm_state->fd = -1;
> -    }
> -    kvm_state = NULL;
> +    close(kvm_state->vmfd);
> +    kvm_state->vmfd = -1;
> +    close(kvm_state->fd);
> +    kvm_state->fd = -1;
>   }
>   
>   /*

RE: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute "query-balloon" after CPR transfer

[Duan, Zhenzhong]

>-----Original Message-----
>From: Steven Sistare <steven.sistare@oracle.com>
>Subject: Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute
>"query-balloon" after CPR transfer
>
>On 9/28/2025 4:54 AM, Zhenzhong Duan wrote:
>> After CPR transfer, source QEMU closes kvm fd and sets kvm_state to
>NULL,
>> "query-balloon" will check kvm_state->sync_mmu and trigger NULL pointer
>> reference.
>>
>> We don't need to NULL kvm_state as all states in kvm_state aren't released
>> actually. Just closing kvm fd is enough so we could still query states
>> through "query_*" qmp command.
>
>IMO this does not make sense.  Much of the state in kvm_state was derived
>from ioctl's on the descriptors, and closing them invalidates it.  Asking
>historical questions about what used to be makes no sense.

You also have your valid point.

>
>Clearing kvm_state and setting kvm_allowed=false would be a safer fix.

But clearing kvm_state will make some qmp commands which dereferencing
kvm_state trigger SIGSEGV. We can expect failure on those qmp, but SIGSEGV
is worse.

E.g., {"execute": "query-cpu-definitions"}, below error print with v2 but SIGSEGV with v1:

KVM get MSR (index=0x10a) feature failed, Bad file descriptor


I also see inconsistence on "query-balloon" result with v1 patch, before cpr-transfer:

{"error": {"class": "DeviceNotActive", "desc": "No balloon device has been activated"}}

after transfer:

{"error": {"class": "KVMMissingCap", "desc": "Using KVM without synchronous MMU, balloon unavailable"}}

It's confusing there is no synchronous MMU support but we do have it.

Thanks
Zhenzhong

Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute "query-balloon" after CPR transfer

[Steven Sistare]

On 9/30/2025 2:00 AM, Duan, Zhenzhong wrote:
>> -----Original Message-----
>> From: Steven Sistare <steven.sistare@oracle.com>
>> Subject: Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute
>> "query-balloon" after CPR transfer
>>
>> On 9/28/2025 4:54 AM, Zhenzhong Duan wrote:
>>> After CPR transfer, source QEMU closes kvm fd and sets kvm_state to
>> NULL,
>>> "query-balloon" will check kvm_state->sync_mmu and trigger NULL pointer
>>> reference.
>>>
>>> We don't need to NULL kvm_state as all states in kvm_state aren't released
>>> actually. Just closing kvm fd is enough so we could still query states
>>> through "query_*" qmp command.
>>
>> IMO this does not make sense.  Much of the state in kvm_state was derived
>>from ioctl's on the descriptors, and closing them invalidates it.  Asking
>> historical questions about what used to be makes no sense.
> 
> You also have your valid point.
> 
>>
>> Clearing kvm_state and setting kvm_allowed=false would be a safer fix.

Setting kvm_allowed=false causes kvm_enabled() to return false which should
prevent kvm_state from being dereferenced anywhere:

     #define kvm_enabled()           (kvm_allowed)

   Eg for the balloon:

     static bool have_balloon(Error **errp)
     {
         if (kvm_enabled() && !kvm_has_sync_mmu()) {

- Steve

> But clearing kvm_state will make some qmp commands which dereferencing
> kvm_state trigger SIGSEGV. We can expect failure on those qmp, but SIGSEGV
> is worse.
> 
> E.g., {"execute": "query-cpu-definitions"}, below error print with v2 but SIGSEGV with v1:
> 
> KVM get MSR (index=0x10a) feature failed, Bad file descriptor
> 
> 
> I also see inconsistence on "query-balloon" result with v1 patch, before cpr-transfer:
> 
> {"error": {"class": "DeviceNotActive", "desc": "No balloon device has been activated"}}
> 
> after transfer:
> 
> {"error": {"class": "KVMMissingCap", "desc": "Using KVM without synchronous MMU, balloon unavailable"}}
> 
> It's confusing there is no synchronous MMU support but we do have it.
> 
> Thanks
> Zhenzhong

Re: [PATCH v2 0/6] VFIO: cpr-transfer fixes

[Cédric Le Goater]

On 9/28/25 10:54, Zhenzhong Duan wrote:
> Hi,
> 
> Patch1: fixed an error restore path when virtio-mem is configured.
> Patch2: fixed assert failure on error restore path, this issue happens
> no matter if virtio-mem is configured.
> Some trick is played to trigger the error path,
> see https://github.com/yiliu1765/qemu/commit/494d19e7f7242dbc47d7f236937cde0c396a4a7c
> 
> Patch3-4: issue only happens with two or more VFIO devices, no issue
> if only one VFIO device.
> 
> Patch5: fix a bug that impact "query-balloon" execution
> 
> Patch6: SIGSEGV if I send "query-balloon" to source qmp monitor,
> I'm not quite sure if it's deserved to be fixed, as guest has been
> migrated to destination, it's not a big issue for source qemu to
> SIGSEGV?
> 
> Thanks
> Zhenzhong
> 
> Changelog:
> v2:
> - minor polishment to commit log (Steve)
> - keep kvm_state so "query-balloon" could work after CPR-transfer (Markus)
> - add a fix which is found during "query-balloon" execution
> - rebased to master
> 
> Zhenzhong Duan (6):
>    vfio/container: Remap only populated parts in a section
>    vfio/cpr-legacy: drop an erroneous assert
>    vfio/iommufd: Set cpr.ioas_id on source side for CPR transfer
>    vfio/iommufd: Restore vbasedev's reference to hwpt after CPR transfer
>    accel/kvm: Fix an erroneous check on coalesced_mmio_ring
>    accel/kvm: Fix SIGSEGV when execute "query-balloon" after CPR transfer
> 
>   include/hw/vfio/vfio-cpr.h |  2 +-
>   accel/kvm/kvm-all.c        | 14 ++++++--------
>   hw/vfio/cpr-legacy.c       | 22 +++++++++++++++-------
>   hw/vfio/iommufd.c          |  8 ++++----
>   hw/vfio/listener.c         |  4 ++--
>   5 files changed, 28 insertions(+), 22 deletions(-)
> 


Applied patches 1-5 to vfio-next.

Thanks,

C.

RE: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute "query-balloon" after CPR transfer

[Duan, Zhenzhong]

>-----Original Message-----
>From: Steven Sistare <steven.sistare@oracle.com>
>Subject: Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute
>"query-balloon" after CPR transfer
>
>On 9/30/2025 2:00 AM, Duan, Zhenzhong wrote:
>>> -----Original Message-----
>>> From: Steven Sistare <steven.sistare@oracle.com>
>>> Subject: Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute
>>> "query-balloon" after CPR transfer
>>>
>>> On 9/28/2025 4:54 AM, Zhenzhong Duan wrote:
>>>> After CPR transfer, source QEMU closes kvm fd and sets kvm_state to
>>> NULL,
>>>> "query-balloon" will check kvm_state->sync_mmu and trigger NULL
>pointer
>>>> reference.
>>>>
>>>> We don't need to NULL kvm_state as all states in kvm_state aren't
>released
>>>> actually. Just closing kvm fd is enough so we could still query states
>>>> through "query_*" qmp command.
>>>
>>> IMO this does not make sense.  Much of the state in kvm_state was
>derived
>>>from ioctl's on the descriptors, and closing them invalidates it.  Asking
>>> historical questions about what used to be makes no sense.
>>
>> You also have your valid point.
>>
>>>
>>> Clearing kvm_state and setting kvm_allowed=false would be a safer fix.
>
>Setting kvm_allowed=false causes kvm_enabled() to return false which should
>prevent kvm_state from being dereferenced anywhere:
>
>     #define kvm_enabled()           (kvm_allowed)
>
>   Eg for the balloon:
>
>     static bool have_balloon(Error **errp)
>     {
>         if (kvm_enabled() && !kvm_has_sync_mmu()) {

OK, will do, clearing kvm_allowed is a good idea.

Currently there are two qmp commands "query-balloon" and "query-cpu-definitions"
causing SIGSEGV after CPR-transfer, clearing kvm_allowed helps for both.
But clearing both kvm_allowed and kvm_state cause SIGSEGV on "query-cpu-definitions".

I'll send a patch to clearing only kvm_allowed if you are fine with it.

Thanks
Zhenzhong

Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute "query-balloon" after CPR transfer

[Steven Sistare]

On 10/8/2025 6:22 AM, Duan, Zhenzhong wrote:
> 
> 
>> -----Original Message-----
>> From: Steven Sistare <steven.sistare@oracle.com>
>> Subject: Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute
>> "query-balloon" after CPR transfer
>>
>> On 9/30/2025 2:00 AM, Duan, Zhenzhong wrote:
>>>> -----Original Message-----
>>>> From: Steven Sistare <steven.sistare@oracle.com>
>>>> Subject: Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute
>>>> "query-balloon" after CPR transfer
>>>>
>>>> On 9/28/2025 4:54 AM, Zhenzhong Duan wrote:
>>>>> After CPR transfer, source QEMU closes kvm fd and sets kvm_state to
>>>> NULL,
>>>>> "query-balloon" will check kvm_state->sync_mmu and trigger NULL
>> pointer
>>>>> reference.
>>>>>
>>>>> We don't need to NULL kvm_state as all states in kvm_state aren't
>> released
>>>>> actually. Just closing kvm fd is enough so we could still query states
>>>>> through "query_*" qmp command.
>>>>
>>>> IMO this does not make sense.  Much of the state in kvm_state was
>> derived
>>> >from ioctl's on the descriptors, and closing them invalidates it.  Asking
>>>> historical questions about what used to be makes no sense.
>>>
>>> You also have your valid point.
>>>
>>>>
>>>> Clearing kvm_state and setting kvm_allowed=false would be a safer fix.
>>
>> Setting kvm_allowed=false causes kvm_enabled() to return false which should
>> prevent kvm_state from being dereferenced anywhere:
>>
>>      #define kvm_enabled()           (kvm_allowed)
>>
>>    Eg for the balloon:
>>
>>      static bool have_balloon(Error **errp)
>>      {
>>          if (kvm_enabled() && !kvm_has_sync_mmu()) {
> 
> OK, will do, clearing kvm_allowed is a good idea.
> 
> Currently there are two qmp commands "query-balloon" and "query-cpu-definitions"
> causing SIGSEGV after CPR-transfer, clearing kvm_allowed helps for both.
> But clearing both kvm_allowed and kvm_state cause SIGSEGV on "query-cpu-definitions".
> 
> I'll send a patch to clearing only kvm_allowed if you are fine with it.

I still don't love the idea.  kvm_state is no longer valid.

It sounds like "query-cpu-definitions" is missing a check for kvm_enabled().

- Steve

Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute "query-balloon" after CPR transfer

[Daniel P. Berrangé]

On Thu, Oct 09, 2025 at 10:11:17AM -0400, Steven Sistare wrote:
> On 10/8/2025 6:22 AM, Duan, Zhenzhong wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Steven Sistare <steven.sistare@oracle.com>
> > > Subject: Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute
> > > "query-balloon" after CPR transfer
> > > 
> > > On 9/30/2025 2:00 AM, Duan, Zhenzhong wrote:
> > > > > -----Original Message-----
> > > > > From: Steven Sistare <steven.sistare@oracle.com>
> > > > > Subject: Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute
> > > > > "query-balloon" after CPR transfer
> > > > > 
> > > > > On 9/28/2025 4:54 AM, Zhenzhong Duan wrote:
> > > > > > After CPR transfer, source QEMU closes kvm fd and sets kvm_state to
> > > > > NULL,
> > > > > > "query-balloon" will check kvm_state->sync_mmu and trigger NULL
> > > pointer
> > > > > > reference.
> > > > > > 
> > > > > > We don't need to NULL kvm_state as all states in kvm_state aren't
> > > released
> > > > > > actually. Just closing kvm fd is enough so we could still query states
> > > > > > through "query_*" qmp command.
> > > > > 
> > > > > IMO this does not make sense.  Much of the state in kvm_state was
> > > derived
> > > > >from ioctl's on the descriptors, and closing them invalidates it.  Asking
> > > > > historical questions about what used to be makes no sense.
> > > > 
> > > > You also have your valid point.
> > > > 
> > > > > 
> > > > > Clearing kvm_state and setting kvm_allowed=false would be a safer fix.
> > > 
> > > Setting kvm_allowed=false causes kvm_enabled() to return false which should
> > > prevent kvm_state from being dereferenced anywhere:
> > > 
> > >      #define kvm_enabled()           (kvm_allowed)
> > > 
> > >    Eg for the balloon:
> > > 
> > >      static bool have_balloon(Error **errp)
> > >      {
> > >          if (kvm_enabled() && !kvm_has_sync_mmu()) {
> > 
> > OK, will do, clearing kvm_allowed is a good idea.
> > 
> > Currently there are two qmp commands "query-balloon" and "query-cpu-definitions"
> > causing SIGSEGV after CPR-transfer, clearing kvm_allowed helps for both.
> > But clearing both kvm_allowed and kvm_state cause SIGSEGV on "query-cpu-definitions".
> > 
> > I'll send a patch to clearing only kvm_allowed if you are fine with it.
> 
> I still don't love the idea.  kvm_state is no longer valid.
> 
> It sounds like "query-cpu-definitions" is missing a check for kvm_enabled().

This patch  / bug feels like it is side-stepping a general conceptual
question.  After cpr-transfer is complete what QMP commands are still
valid to call in general ? This thread mentions two which have caused
bugs, but beyond that it feels like a large subset of QMP comamnds
are conceptually invalid to use.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute "query-balloon" after CPR transfer

[Steven Sistare]

On 10/9/2025 10:19 AM, Daniel P. Berrangé wrote:
> On Thu, Oct 09, 2025 at 10:11:17AM -0400, Steven Sistare wrote:
>> On 10/8/2025 6:22 AM, Duan, Zhenzhong wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Steven Sistare <steven.sistare@oracle.com>
>>>> Subject: Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute
>>>> "query-balloon" after CPR transfer
>>>>
>>>> On 9/30/2025 2:00 AM, Duan, Zhenzhong wrote:
>>>>>> -----Original Message-----
>>>>>> From: Steven Sistare <steven.sistare@oracle.com>
>>>>>> Subject: Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute
>>>>>> "query-balloon" after CPR transfer
>>>>>>
>>>>>> On 9/28/2025 4:54 AM, Zhenzhong Duan wrote:
>>>>>>> After CPR transfer, source QEMU closes kvm fd and sets kvm_state to
>>>>>> NULL,
>>>>>>> "query-balloon" will check kvm_state->sync_mmu and trigger NULL
>>>> pointer
>>>>>>> reference.
>>>>>>>
>>>>>>> We don't need to NULL kvm_state as all states in kvm_state aren't
>>>> released
>>>>>>> actually. Just closing kvm fd is enough so we could still query states
>>>>>>> through "query_*" qmp command.
>>>>>>
>>>>>> IMO this does not make sense.  Much of the state in kvm_state was
>>>> derived
>>>>> >from ioctl's on the descriptors, and closing them invalidates it.  Asking
>>>>>> historical questions about what used to be makes no sense.
>>>>>
>>>>> You also have your valid point.
>>>>>
>>>>>>
>>>>>> Clearing kvm_state and setting kvm_allowed=false would be a safer fix.
>>>>
>>>> Setting kvm_allowed=false causes kvm_enabled() to return false which should
>>>> prevent kvm_state from being dereferenced anywhere:
>>>>
>>>>       #define kvm_enabled()           (kvm_allowed)
>>>>
>>>>     Eg for the balloon:
>>>>
>>>>       static bool have_balloon(Error **errp)
>>>>       {
>>>>           if (kvm_enabled() && !kvm_has_sync_mmu()) {
>>>
>>> OK, will do, clearing kvm_allowed is a good idea.
>>>
>>> Currently there are two qmp commands "query-balloon" and "query-cpu-definitions"
>>> causing SIGSEGV after CPR-transfer, clearing kvm_allowed helps for both.
>>> But clearing both kvm_allowed and kvm_state cause SIGSEGV on "query-cpu-definitions".
>>>
>>> I'll send a patch to clearing only kvm_allowed if you are fine with it.
>>
>> I still don't love the idea.  kvm_state is no longer valid.
>>
>> It sounds like "query-cpu-definitions" is missing a check for kvm_enabled().
> 
> This patch  / bug feels like it is side-stepping a general conceptual
> question.  After cpr-transfer is complete what QMP commands are still
> valid to call in general ? This thread mentions two which have caused
> bugs, but beyond that it feels like a large subset of QMP comamnds
> are conceptually invalid to use.

Agreed. I don't see why these commands should be issued to the dead instance.
How about migration status, quit, and nothing else?
Half serious.

- Steve

Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute "query-balloon" after CPR transfer

[Daniel P. Berrangé]

On Thu, Oct 09, 2025 at 10:32:34AM -0400, Steven Sistare wrote:
> On 10/9/2025 10:19 AM, Daniel P. Berrangé wrote:
> > On Thu, Oct 09, 2025 at 10:11:17AM -0400, Steven Sistare wrote:
> > > On 10/8/2025 6:22 AM, Duan, Zhenzhong wrote:
> > > > 
> > > > 
> > > > > -----Original Message-----
> > > > > From: Steven Sistare <steven.sistare@oracle.com>
> > > > > Subject: Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute
> > > > > "query-balloon" after CPR transfer
> > > > > 
> > > > > On 9/30/2025 2:00 AM, Duan, Zhenzhong wrote:
> > > > > > > -----Original Message-----
> > > > > > > From: Steven Sistare <steven.sistare@oracle.com>
> > > > > > > Subject: Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute
> > > > > > > "query-balloon" after CPR transfer
> > > > > > > 
> > > > > > > On 9/28/2025 4:54 AM, Zhenzhong Duan wrote:
> > > > > > > > After CPR transfer, source QEMU closes kvm fd and sets kvm_state to
> > > > > > > NULL,
> > > > > > > > "query-balloon" will check kvm_state->sync_mmu and trigger NULL
> > > > > pointer
> > > > > > > > reference.
> > > > > > > > 
> > > > > > > > We don't need to NULL kvm_state as all states in kvm_state aren't
> > > > > released
> > > > > > > > actually. Just closing kvm fd is enough so we could still query states
> > > > > > > > through "query_*" qmp command.
> > > > > > > 
> > > > > > > IMO this does not make sense.  Much of the state in kvm_state was
> > > > > derived
> > > > > > >from ioctl's on the descriptors, and closing them invalidates it.  Asking
> > > > > > > historical questions about what used to be makes no sense.
> > > > > > 
> > > > > > You also have your valid point.
> > > > > > 
> > > > > > > 
> > > > > > > Clearing kvm_state and setting kvm_allowed=false would be a safer fix.
> > > > > 
> > > > > Setting kvm_allowed=false causes kvm_enabled() to return false which should
> > > > > prevent kvm_state from being dereferenced anywhere:
> > > > > 
> > > > >       #define kvm_enabled()           (kvm_allowed)
> > > > > 
> > > > >     Eg for the balloon:
> > > > > 
> > > > >       static bool have_balloon(Error **errp)
> > > > >       {
> > > > >           if (kvm_enabled() && !kvm_has_sync_mmu()) {
> > > > 
> > > > OK, will do, clearing kvm_allowed is a good idea.
> > > > 
> > > > Currently there are two qmp commands "query-balloon" and "query-cpu-definitions"
> > > > causing SIGSEGV after CPR-transfer, clearing kvm_allowed helps for both.
> > > > But clearing both kvm_allowed and kvm_state cause SIGSEGV on "query-cpu-definitions".
> > > > 
> > > > I'll send a patch to clearing only kvm_allowed if you are fine with it.
> > > 
> > > I still don't love the idea.  kvm_state is no longer valid.
> > > 
> > > It sounds like "query-cpu-definitions" is missing a check for kvm_enabled().
> > 
> > This patch  / bug feels like it is side-stepping a general conceptual
> > question.  After cpr-transfer is complete what QMP commands are still
> > valid to call in general ? This thread mentions two which have caused
> > bugs, but beyond that it feels like a large subset of QMP comamnds
> > are conceptually invalid to use.
> 
> Agreed. I don't see why these commands should be issued to the dead instance.
> How about migration status, quit, and nothing else?
> Half serious.

I was hoping you'd suggest such a short list :-)

Essentially a case of calling qmp_for_each_command(), and in the callback
run qmp_disable_command() for everything not in the allow-list.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

RE: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute "query-balloon" after CPR transfer

[Duan, Zhenzhong]

>-----Original Message-----
>From: Steven Sistare <steven.sistare@oracle.com>
>Subject: Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute
>"query-balloon" after CPR transfer
>
>On 10/8/2025 6:22 AM, Duan, Zhenzhong wrote:
>>
>>
>>> -----Original Message-----
>>> From: Steven Sistare <steven.sistare@oracle.com>
>>> Subject: Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute
>>> "query-balloon" after CPR transfer
>>>
>>> On 9/30/2025 2:00 AM, Duan, Zhenzhong wrote:
>>>>> -----Original Message-----
>>>>> From: Steven Sistare <steven.sistare@oracle.com>
>>>>> Subject: Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute
>>>>> "query-balloon" after CPR transfer
>>>>>
>>>>> On 9/28/2025 4:54 AM, Zhenzhong Duan wrote:
>>>>>> After CPR transfer, source QEMU closes kvm fd and sets kvm_state to
>>>>> NULL,
>>>>>> "query-balloon" will check kvm_state->sync_mmu and trigger NULL
>>> pointer
>>>>>> reference.
>>>>>>
>>>>>> We don't need to NULL kvm_state as all states in kvm_state aren't
>>> released
>>>>>> actually. Just closing kvm fd is enough so we could still query states
>>>>>> through "query_*" qmp command.
>>>>>
>>>>> IMO this does not make sense.  Much of the state in kvm_state was
>>> derived
>>>> >from ioctl's on the descriptors, and closing them invalidates it.  Asking
>>>>> historical questions about what used to be makes no sense.
>>>>
>>>> You also have your valid point.
>>>>
>>>>>
>>>>> Clearing kvm_state and setting kvm_allowed=false would be a safer fix.
>>>
>>> Setting kvm_allowed=false causes kvm_enabled() to return false which
>should
>>> prevent kvm_state from being dereferenced anywhere:
>>>
>>>      #define kvm_enabled()           (kvm_allowed)
>>>
>>>    Eg for the balloon:
>>>
>>>      static bool have_balloon(Error **errp)
>>>      {
>>>          if (kvm_enabled() && !kvm_has_sync_mmu()) {
>>
>> OK, will do, clearing kvm_allowed is a good idea.
>>
>> Currently there are two qmp commands "query-balloon" and
>"query-cpu-definitions"
>> causing SIGSEGV after CPR-transfer, clearing kvm_allowed helps for both.
>> But clearing both kvm_allowed and kvm_state cause SIGSEGV on
>"query-cpu-definitions".
>>
>> I'll send a patch to clearing only kvm_allowed if you are fine with it.
>
>I still don't love the idea.  kvm_state is no longer valid.

OK, what about another idea to not call vfio_cpr_kvm_close_notifier() for cpr-transfer, like below:

--- a/hw/vfio/cpr.c
+++ b/hw/vfio/cpr.c
@@ -197,7 +197,7 @@ void vfio_cpr_add_kvm_notifier(void)
     if (!kvm_close_notifier.notify) {
         migration_add_notifier_modes(&kvm_close_notifier,
                                      vfio_cpr_kvm_close_notifier,
-                                     MIG_MODE_CPR_TRANSFER, MIG_MODE_CPR_EXEC,
+                                     MIG_MODE_CPR_EXEC,
                                      -1);
     }
 }

The close_kvm_after_cpr is only for cpr-exec, with this change, we are in same situation as live migration and can run all qmp commands same as after live migration.

>
>It sounds like "query-cpu-definitions" is missing a check for kvm_enabled().

Yes.

Thanks
Zhenzhong

RE: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute "query-balloon" after CPR transfer

[Duan, Zhenzhong]

>-----Original Message-----
>From: Steven Sistare <steven.sistare@oracle.com>
>Subject: Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute
>"query-balloon" after CPR transfer
>
>On 10/9/2025 10:19 AM, Daniel P. Berrangé wrote:
>> On Thu, Oct 09, 2025 at 10:11:17AM -0400, Steven Sistare wrote:
>>> On 10/8/2025 6:22 AM, Duan, Zhenzhong wrote:
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Steven Sistare <steven.sistare@oracle.com>
>>>>> Subject: Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute
>>>>> "query-balloon" after CPR transfer
>>>>>
>>>>> On 9/30/2025 2:00 AM, Duan, Zhenzhong wrote:
>>>>>>> -----Original Message-----
>>>>>>> From: Steven Sistare <steven.sistare@oracle.com>
>>>>>>> Subject: Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute
>>>>>>> "query-balloon" after CPR transfer
>>>>>>>
>>>>>>> On 9/28/2025 4:54 AM, Zhenzhong Duan wrote:
>>>>>>>> After CPR transfer, source QEMU closes kvm fd and sets kvm_state
>to
>>>>>>> NULL,
>>>>>>>> "query-balloon" will check kvm_state->sync_mmu and trigger NULL
>>>>> pointer
>>>>>>>> reference.
>>>>>>>>
>>>>>>>> We don't need to NULL kvm_state as all states in kvm_state aren't
>>>>> released
>>>>>>>> actually. Just closing kvm fd is enough so we could still query states
>>>>>>>> through "query_*" qmp command.
>>>>>>>
>>>>>>> IMO this does not make sense.  Much of the state in kvm_state was
>>>>> derived
>>>>>> >from ioctl's on the descriptors, and closing them invalidates it.
>Asking
>>>>>>> historical questions about what used to be makes no sense.
>>>>>>
>>>>>> You also have your valid point.
>>>>>>
>>>>>>>
>>>>>>> Clearing kvm_state and setting kvm_allowed=false would be a safer
>fix.
>>>>>
>>>>> Setting kvm_allowed=false causes kvm_enabled() to return false which
>should
>>>>> prevent kvm_state from being dereferenced anywhere:
>>>>>
>>>>>       #define kvm_enabled()           (kvm_allowed)
>>>>>
>>>>>     Eg for the balloon:
>>>>>
>>>>>       static bool have_balloon(Error **errp)
>>>>>       {
>>>>>           if (kvm_enabled() && !kvm_has_sync_mmu()) {
>>>>
>>>> OK, will do, clearing kvm_allowed is a good idea.
>>>>
>>>> Currently there are two qmp commands "query-balloon" and
>"query-cpu-definitions"
>>>> causing SIGSEGV after CPR-transfer, clearing kvm_allowed helps for both.
>>>> But clearing both kvm_allowed and kvm_state cause SIGSEGV on
>"query-cpu-definitions".
>>>>
>>>> I'll send a patch to clearing only kvm_allowed if you are fine with it.
>>>
>>> I still don't love the idea.  kvm_state is no longer valid.
>>>
>>> It sounds like "query-cpu-definitions" is missing a check for kvm_enabled().
>>
>> This patch  / bug feels like it is side-stepping a general conceptual
>> question.  After cpr-transfer is complete what QMP commands are still
>> valid to call in general ? This thread mentions two which have caused
>> bugs, but beyond that it feels like a large subset of QMP comamnds
>> are conceptually invalid to use.
>
>Agreed. I don't see why these commands should be issued to the dead
>instance.

Uh, yes, that's why I hesitate if it's deserved to fix it.

>How about migration status, quit, and nothing else?
>Half serious.

We only have issues with "query-balloon" and "query-cpu-definitions", all other "query-*" qmp commands work fine.

{"execute": "query-migrate"}
{"timestamp": {"seconds": 1760067694, "microseconds": 815419}, "event": "STOP"}
{"return": {"status": "completed", "setup-time": 3, "downtime": 14, "total-time": 67, "ram": {"total": 0, "postcopy-requests": 0, "dirty-sync-count": 2, "multifd-bytes": 0, "pages-per-second": 0, "downtime-bytes": 0, "page-size": 4096, "remaining": 0, "postcopy-bytes": 0, "mbps": 58.152999999999999, "transferred": 465224, "dirty-sync-missed-zero-copy": 0, "precopy-bytes": 0, "duplicate": 0, "dirty-pages-rate": 0, "normal-bytes": 0, "normal": 0}}}

{"execute": "quit"}
{"timestamp": {"seconds": 1760067734, "microseconds": 599830}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
{"return": {}}
Connection closed by foreign host.Connection closed by foreign host.

Thanks
Zhenzhong

RE: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute "query-balloon" after CPR transfer

[Duan, Zhenzhong]

>-----Original Message-----
>From: Daniel P. Berrangé <berrange@redhat.com>
>Subject: Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute
>"query-balloon" after CPR transfer
>
>On Thu, Oct 09, 2025 at 10:32:34AM -0400, Steven Sistare wrote:
>> On 10/9/2025 10:19 AM, Daniel P. Berrangé wrote:
>> > On Thu, Oct 09, 2025 at 10:11:17AM -0400, Steven Sistare wrote:
>> > > On 10/8/2025 6:22 AM, Duan, Zhenzhong wrote:
>> > > >
>> > > >
>> > > > > -----Original Message-----
>> > > > > From: Steven Sistare <steven.sistare@oracle.com>
>> > > > > Subject: Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute
>> > > > > "query-balloon" after CPR transfer
>> > > > >
>> > > > > On 9/30/2025 2:00 AM, Duan, Zhenzhong wrote:
>> > > > > > > -----Original Message-----
>> > > > > > > From: Steven Sistare <steven.sistare@oracle.com>
>> > > > > > > Subject: Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when
>execute
>> > > > > > > "query-balloon" after CPR transfer
>> > > > > > >
>> > > > > > > On 9/28/2025 4:54 AM, Zhenzhong Duan wrote:
>> > > > > > > > After CPR transfer, source QEMU closes kvm fd and sets
>kvm_state to
>> > > > > > > NULL,
>> > > > > > > > "query-balloon" will check kvm_state->sync_mmu and trigger
>NULL
>> > > > > pointer
>> > > > > > > > reference.
>> > > > > > > >
>> > > > > > > > We don't need to NULL kvm_state as all states in kvm_state
>aren't
>> > > > > released
>> > > > > > > > actually. Just closing kvm fd is enough so we could still query
>states
>> > > > > > > > through "query_*" qmp command.
>> > > > > > >
>> > > > > > > IMO this does not make sense.  Much of the state in kvm_state
>was
>> > > > > derived
>> > > > > > >from ioctl's on the descriptors, and closing them invalidates it.
>Asking
>> > > > > > > historical questions about what used to be makes no sense.
>> > > > > >
>> > > > > > You also have your valid point.
>> > > > > >
>> > > > > > >
>> > > > > > > Clearing kvm_state and setting kvm_allowed=false would be a
>safer fix.
>> > > > >
>> > > > > Setting kvm_allowed=false causes kvm_enabled() to return false
>which should
>> > > > > prevent kvm_state from being dereferenced anywhere:
>> > > > >
>> > > > >       #define kvm_enabled()           (kvm_allowed)
>> > > > >
>> > > > >     Eg for the balloon:
>> > > > >
>> > > > >       static bool have_balloon(Error **errp)
>> > > > >       {
>> > > > >           if (kvm_enabled() && !kvm_has_sync_mmu()) {
>> > > >
>> > > > OK, will do, clearing kvm_allowed is a good idea.
>> > > >
>> > > > Currently there are two qmp commands "query-balloon" and
>"query-cpu-definitions"
>> > > > causing SIGSEGV after CPR-transfer, clearing kvm_allowed helps for
>both.
>> > > > But clearing both kvm_allowed and kvm_state cause SIGSEGV on
>"query-cpu-definitions".
>> > > >
>> > > > I'll send a patch to clearing only kvm_allowed if you are fine with it.
>> > >
>> > > I still don't love the idea.  kvm_state is no longer valid.
>> > >
>> > > It sounds like "query-cpu-definitions" is missing a check for
>kvm_enabled().
>> >
>> > This patch  / bug feels like it is side-stepping a general conceptual
>> > question.  After cpr-transfer is complete what QMP commands are still
>> > valid to call in general ? This thread mentions two which have caused
>> > bugs, but beyond that it feels like a large subset of QMP comamnds
>> > are conceptually invalid to use.
>>
>> Agreed. I don't see why these commands should be issued to the dead
>instance.
>> How about migration status, quit, and nothing else?
>> Half serious.
>
>I was hoping you'd suggest such a short list :-)
>
>Essentially a case of calling qmp_for_each_command(), and in the callback
>run qmp_disable_command() for everything not in the allow-list.

Thanks for your suggestion, I agree this is a perfect scheme, but is also heavy on this corner case😊
Just curious if we need to do same for live migration. E.g., send qmp command on source qemu after live migration.

Thanks
Zhenzhong

Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute "query-balloon" after CPR transfer

[Daniel P. Berrangé]

On Fri, Oct 10, 2025 at 03:54:42AM +0000, Duan, Zhenzhong wrote:
> 
> 
> >-----Original Message-----
> >From: Daniel P. Berrangé <berrange@redhat.com>
> >Subject: Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute
> >"query-balloon" after CPR transfer
> >
> >On Thu, Oct 09, 2025 at 10:32:34AM -0400, Steven Sistare wrote:
> >> On 10/9/2025 10:19 AM, Daniel P. Berrangé wrote:
> >> > On Thu, Oct 09, 2025 at 10:11:17AM -0400, Steven Sistare wrote:
> >> > > On 10/8/2025 6:22 AM, Duan, Zhenzhong wrote:
> >> > > >
> >> > > >
> >> > > > > -----Original Message-----
> >> > > > > From: Steven Sistare <steven.sistare@oracle.com>
> >> > > > > Subject: Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when execute
> >> > > > > "query-balloon" after CPR transfer
> >> > > > >
> >> > > > > On 9/30/2025 2:00 AM, Duan, Zhenzhong wrote:
> >> > > > > > > -----Original Message-----
> >> > > > > > > From: Steven Sistare <steven.sistare@oracle.com>
> >> > > > > > > Subject: Re: [PATCH v2 6/6] accel/kvm: Fix SIGSEGV when
> >execute
> >> > > > > > > "query-balloon" after CPR transfer
> >> > > > > > >
> >> > > > > > > On 9/28/2025 4:54 AM, Zhenzhong Duan wrote:
> >> > > > > > > > After CPR transfer, source QEMU closes kvm fd and sets
> >kvm_state to
> >> > > > > > > NULL,
> >> > > > > > > > "query-balloon" will check kvm_state->sync_mmu and trigger
> >NULL
> >> > > > > pointer
> >> > > > > > > > reference.
> >> > > > > > > >
> >> > > > > > > > We don't need to NULL kvm_state as all states in kvm_state
> >aren't
> >> > > > > released
> >> > > > > > > > actually. Just closing kvm fd is enough so we could still query
> >states
> >> > > > > > > > through "query_*" qmp command.
> >> > > > > > >
> >> > > > > > > IMO this does not make sense.  Much of the state in kvm_state
> >was
> >> > > > > derived
> >> > > > > > >from ioctl's on the descriptors, and closing them invalidates it.
> >Asking
> >> > > > > > > historical questions about what used to be makes no sense.
> >> > > > > >
> >> > > > > > You also have your valid point.
> >> > > > > >
> >> > > > > > >
> >> > > > > > > Clearing kvm_state and setting kvm_allowed=false would be a
> >safer fix.
> >> > > > >
> >> > > > > Setting kvm_allowed=false causes kvm_enabled() to return false
> >which should
> >> > > > > prevent kvm_state from being dereferenced anywhere:
> >> > > > >
> >> > > > >       #define kvm_enabled()           (kvm_allowed)
> >> > > > >
> >> > > > >     Eg for the balloon:
> >> > > > >
> >> > > > >       static bool have_balloon(Error **errp)
> >> > > > >       {
> >> > > > >           if (kvm_enabled() && !kvm_has_sync_mmu()) {
> >> > > >
> >> > > > OK, will do, clearing kvm_allowed is a good idea.
> >> > > >
> >> > > > Currently there are two qmp commands "query-balloon" and
> >"query-cpu-definitions"
> >> > > > causing SIGSEGV after CPR-transfer, clearing kvm_allowed helps for
> >both.
> >> > > > But clearing both kvm_allowed and kvm_state cause SIGSEGV on
> >"query-cpu-definitions".
> >> > > >
> >> > > > I'll send a patch to clearing only kvm_allowed if you are fine with it.
> >> > >
> >> > > I still don't love the idea.  kvm_state is no longer valid.
> >> > >
> >> > > It sounds like "query-cpu-definitions" is missing a check for
> >kvm_enabled().
> >> >
> >> > This patch  / bug feels like it is side-stepping a general conceptual
> >> > question.  After cpr-transfer is complete what QMP commands are still
> >> > valid to call in general ? This thread mentions two which have caused
> >> > bugs, but beyond that it feels like a large subset of QMP comamnds
> >> > are conceptually invalid to use.
> >>
> >> Agreed. I don't see why these commands should be issued to the dead
> >instance.
> >> How about migration status, quit, and nothing else?
> >> Half serious.
> >
> >I was hoping you'd suggest such a short list :-)
> >
> >Essentially a case of calling qmp_for_each_command(), and in the callback
> >run qmp_disable_command() for everything not in the allow-list.
> 
> Thanks for your suggestion, I agree this is a perfect scheme, but is also heavy on this corner case😊
> Just curious if we need to do same for live migration. E.g., send qmp command on source qemu after live migration.

At the end of a normal migration, there is no functional reason to prevent
use of any QMP command.  Indeed we need to have the ability to carry on
using the orignal source QEMU, in case the final steps on migration on
the target fail and we need to restart the source.

The cpr-transfer is unusual in the restrictions it has after completion.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|