[PATCH V8 0/8] Add architecture agnostic code to support vCPU Hotplug

[PATCH V8 0/8] Add architecture agnostic code to support vCPU Hotplug

[Salil Mehta via]

Virtual CPU hotplug support is being added across various architectures[1][3].
This series adds various code bits common across all architectures:

1. vCPU creation and Parking code refactor [Patch 1]
2. Update ACPI GED framework to support vCPU Hotplug [Patch 2,3]
3. ACPI CPUs AML code change [Patch 4,5]
4. Helper functions to support unrealization of CPU objects [Patch 6,7]
5. Docs [Patch 8]


Repository:

[*] https://github.com/salil-mehta/qemu.git virt-cpuhp-armv8/rfc-v2.common.v8


Revision History:

Patch-set V7 -> V8

1. Rebased and Fixed the conflicts

Patch-set  V6 -> V7
1. Addressed Alex Bennée's comments
   - Updated the docs
2. Addressed Igor Mammedov's comments
   - Merged patches [Patch V6 3/9] & [Patch V6 7/9] with [Patch V6 4/9]
   - Updated commit-log of [Patch V6 1/9] and [Patch V6 5/9]     
3. Added Shaoqin Huang's Reviewed-by tags for whole series.
Link: https://lore.kernel.org/qemu-devel/20231013105129.25648-1-salil.mehta@huawei.com/

Patch-set  V5 -> V6
1. Addressed Gavin Shan's comments
   - Fixed the assert() ranges of address spaces
   - Rebased the patch-set to latest changes in the qemu.git
   - Added Reviewed-by tags for patches {8,9}
2. Addressed Jonathan Cameron's comments
   - Updated commit-log for [Patch V5 1/9] with mention of trace events
   - Added Reviewed-by tags for patches {1,5}
3. Added Tested-by tags from Xianglai Li
4. Fixed checkpatch.pl error "Qemu -> QEMU" in [Patch V5 1/9] 
Link: https://lore.kernel.org/qemu-devel/20231011194355.15628-1-salil.mehta@huawei.com/

Patch-set  V4 -> V5
1. Addressed Gavin Shan's comments
   - Fixed the trace events print string for kvm_{create,get,park,destroy}_vcpu
   - Added Reviewed-by tag for patch {1}
2. Added Shaoqin Huang's Reviewed-by tags for Patches {2,3}
3. Added Tested-by Tag from Vishnu Pajjuri to the patch-set
4. Dropped the ARM specific [Patch V4 10/10]
Link: https://lore.kernel.org/qemu-devel/20231009203601.17584-1-salil.mehta@huawei.com/

Patch-set  V3 -> V4
1. Addressed David Hilderbrand's comments
   - Fixed the wrong doc comment of kvm_park_vcpu API prototype
   - Added Reviewed-by tags for patches {2,4}
Link: https://lore.kernel.org/qemu-devel/20231009112812.10612-1-salil.mehta@huawei.com/

Patch-set  V2 -> V3
1. Addressed Jonathan Cameron's comments
   - Fixed 'vcpu-id' type wrongly changed from 'unsigned long' to 'integer'
   - Removed unnecessary use of variable 'vcpu_id' in kvm_park_vcpu
   - Updated [Patch V2 3/10] commit-log with details of ACPI_CPU_SCAN_METHOD macro
   - Updated [Patch V2 5/10] commit-log with details of conditional event handler method
   - Added Reviewed-by tags for patches {2,3,4,6,7}
2. Addressed Gavin Shan's comments
   - Remove unnecessary use of variable 'vcpu_id' in kvm_par_vcpu
   - Fixed return value in kvm_get_vcpu from -1 to -ENOENT
   - Reset the value of 'gdb_num_g_regs' in gdb_unregister_coprocessor_all
   - Fixed the kvm_{create,park}_vcpu prototypes docs
   - Added Reviewed-by tags for patches {2,3,4,5,6,7,9,10}
3. Addressed one earlier missed comment by Alex Bennée in RFC V1
   - Added traces instead of DPRINTF in the newly added and some existing functions
Link: https://lore.kernel.org/qemu-devel/20230930001933.2660-1-salil.mehta@huawei.com/

Patch-set V1 -> V2
1. Addressed Alex Bennée's comments
   - Refactored the kvm_create_vcpu logic to get rid of goto
   - Added the docs for kvm_{create,park}_vcpu prototypes
   - Splitted the gdbstub and AddressSpace destruction change into separate patches
   - Added Reviewed-by tags for patches {2,10}
Link: https://lore.kernel.org/qemu-devel/20230929124304.13672-1-salil.mehta@huawei.com/

References:

[1] https://lore.kernel.org/qemu-devel/20230926100436.28284-1-salil.mehta@huawei.com/
[2] https://lore.kernel.org/all/20230913163823.7880-1-james.morse@arm.com/
[3] https://lore.kernel.org/qemu-devel/cover.1695697701.git.lixianglai@loongson.cn/



Salil Mehta (8):
  accel/kvm: Extract common KVM vCPU {creation,parking} code
  hw/acpi: Move CPU ctrl-dev MMIO region len macro to common header file
  hw/acpi: Update ACPI GED framework to support vCPU Hotplug
  hw/acpi: Update GED _EVT method AML with CPU scan
  hw/acpi: Update CPUs AML with cpu-(ctrl)dev change
  physmem: Add helper function to destroy CPU AddressSpace
  gdbstub: Add helper function to unregister GDB register space
  docs/specs/acpi_hw_reduced_hotplug: Add the CPU Hotplug Event Bit

 accel/kvm/kvm-all.c                    | 64 ++++++++++++++++++++------
 accel/kvm/trace-events                 |  5 +-
 docs/specs/acpi_hw_reduced_hotplug.rst |  3 +-
 gdbstub/gdbstub.c                      | 12 +++++
 hw/acpi/acpi-cpu-hotplug-stub.c        |  6 +++
 hw/acpi/cpu.c                          | 27 +++++++----
 hw/acpi/generic_event_device.c         | 21 +++++++++
 hw/i386/acpi-build.c                   |  3 +-
 include/exec/cpu-common.h              |  8 ++++
 include/exec/gdbstub.h                 |  6 +++
 include/hw/acpi/cpu.h                  |  5 +-
 include/hw/acpi/cpu_hotplug.h          |  4 ++
 include/hw/acpi/generic_event_device.h |  4 ++
 include/hw/core/cpu.h                  |  1 +
 include/sysemu/kvm.h                   | 16 +++++++
 system/physmem.c                       | 29 ++++++++++++
 16 files changed, 185 insertions(+), 29 deletions(-)

-- 
2.34.1

[PATCH V8 1/8] accel/kvm: Extract common KVM vCPU {creation, parking} code

[Salil Mehta via]

KVM vCPU creation is done once during the vCPU realization when Qemu vCPU thread
is spawned. This is common to all the architectures as of now.

Hot-unplug of vCPU results in destruction of the vCPU object in QOM but the
corresponding KVM vCPU object in the Host KVM is not destroyed as KVM doesn't
support vCPU removal. Therefore, its representative KVM vCPU object/context in
Qemu is parked.

Refactor architecture common logic so that some APIs could be reused by vCPU
Hotplug code of some architectures likes ARM, Loongson etc. Update new/old APIs
with trace events instead of DPRINTF. No functional change is intended here.

Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
Reviewed-by: Gavin Shan <gshan@redhat.com>
Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Tested-by: Xianglai Li <lixianglai@loongson.cn>
Tested-by: Miguel Luis <miguel.luis@oracle.com>
Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
---
 accel/kvm/kvm-all.c    | 64 ++++++++++++++++++++++++++++++++----------
 accel/kvm/trace-events |  5 +++-
 include/sysemu/kvm.h   | 16 +++++++++++
 3 files changed, 69 insertions(+), 16 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index a8cecd040e..3bc3207bda 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -126,6 +126,7 @@ static QemuMutex kml_slots_lock;
 #define kvm_slots_unlock()  qemu_mutex_unlock(&kml_slots_lock)
 
 static void kvm_slot_init_dirty_bitmap(KVMSlot *mem);
+static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id);
 
 static inline void kvm_resample_fd_remove(int gsi)
 {
@@ -314,14 +315,53 @@ err:
     return ret;
 }
 
+void kvm_park_vcpu(CPUState *cpu)
+{
+    struct KVMParkedVcpu *vcpu;
+
+    trace_kvm_park_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
+
+    vcpu = g_malloc0(sizeof(*vcpu));
+    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
+    vcpu->kvm_fd = cpu->kvm_fd;
+    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
+}
+
+int kvm_create_vcpu(CPUState *cpu)
+{
+    unsigned long vcpu_id = kvm_arch_vcpu_id(cpu);
+    KVMState *s = kvm_state;
+    int kvm_fd;
+
+    trace_kvm_create_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
+
+    /* check if the KVM vCPU already exist but is parked */
+    kvm_fd = kvm_get_vcpu(s, vcpu_id);
+    if (kvm_fd < 0) {
+        /* vCPU not parked: create a new KVM vCPU */
+        kvm_fd = kvm_vm_ioctl(s, KVM_CREATE_VCPU, vcpu_id);
+        if (kvm_fd < 0) {
+            error_report("KVM_CREATE_VCPU IOCTL failed for vCPU %lu", vcpu_id);
+            return kvm_fd;
+        }
+    }
+
+    cpu->kvm_fd = kvm_fd;
+    cpu->kvm_state = s;
+    cpu->vcpu_dirty = true;
+    cpu->dirty_pages = 0;
+    cpu->throttle_us_per_full = 0;
+
+    return 0;
+}
+
 static int do_kvm_destroy_vcpu(CPUState *cpu)
 {
     KVMState *s = kvm_state;
     long mmap_size;
-    struct KVMParkedVcpu *vcpu = NULL;
     int ret = 0;
 
-    trace_kvm_destroy_vcpu();
+    trace_kvm_destroy_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
 
     ret = kvm_arch_destroy_vcpu(cpu);
     if (ret < 0) {
@@ -347,10 +387,7 @@ static int do_kvm_destroy_vcpu(CPUState *cpu)
         }
     }
 
-    vcpu = g_malloc0(sizeof(*vcpu));
-    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
-    vcpu->kvm_fd = cpu->kvm_fd;
-    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
+    kvm_park_vcpu(cpu);
 err:
     return ret;
 }
@@ -371,6 +408,8 @@ static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
         if (cpu->vcpu_id == vcpu_id) {
             int kvm_fd;
 
+            trace_kvm_get_vcpu(vcpu_id);
+
             QLIST_REMOVE(cpu, node);
             kvm_fd = cpu->kvm_fd;
             g_free(cpu);
@@ -378,7 +417,7 @@ static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
         }
     }
 
-    return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
+    return -ENOENT;
 }
 
 int kvm_init_vcpu(CPUState *cpu, Error **errp)
@@ -389,19 +428,14 @@ int kvm_init_vcpu(CPUState *cpu, Error **errp)
 
     trace_kvm_init_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
 
-    ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu));
+    ret = kvm_create_vcpu(cpu);
     if (ret < 0) {
-        error_setg_errno(errp, -ret, "kvm_init_vcpu: kvm_get_vcpu failed (%lu)",
+        error_setg_errno(errp, -ret,
+                         "kvm_init_vcpu: kvm_create_vcpu failed (%lu)",
                          kvm_arch_vcpu_id(cpu));
         goto err;
     }
 
-    cpu->kvm_fd = ret;
-    cpu->kvm_state = s;
-    cpu->vcpu_dirty = true;
-    cpu->dirty_pages = 0;
-    cpu->throttle_us_per_full = 0;
-
     mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
     if (mmap_size < 0) {
         ret = mmap_size;
diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events
index a25902597b..5558cff0dc 100644
--- a/accel/kvm/trace-events
+++ b/accel/kvm/trace-events
@@ -9,6 +9,10 @@ kvm_device_ioctl(int fd, int type, void *arg) "dev fd %d, type 0x%x, arg %p"
 kvm_failed_reg_get(uint64_t id, const char *msg) "Warning: Unable to retrieve ONEREG %" PRIu64 " from KVM: %s"
 kvm_failed_reg_set(uint64_t id, const char *msg) "Warning: Unable to set ONEREG %" PRIu64 " to KVM: %s"
 kvm_init_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
+kvm_create_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
+kvm_get_vcpu(unsigned long arch_cpu_id) "id: %lu"
+kvm_destroy_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
+kvm_park_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
 kvm_irqchip_commit_routes(void) ""
 kvm_irqchip_add_msi_route(char *name, int vector, int virq) "dev %s vector %d virq %d"
 kvm_irqchip_update_msi_route(int virq) "Updating MSI route virq=%d"
@@ -25,7 +29,6 @@ kvm_dirty_ring_reaper(const char *s) "%s"
 kvm_dirty_ring_reap(uint64_t count, int64_t t) "reaped %"PRIu64" pages (took %"PRIi64" us)"
 kvm_dirty_ring_reaper_kick(const char *reason) "%s"
 kvm_dirty_ring_flush(int finished) "%d"
-kvm_destroy_vcpu(void) ""
 kvm_failed_get_vcpu_mmap_size(void) ""
 kvm_cpu_exec(void) ""
 kvm_interrupt_exit_request(void) ""
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index fad9a7e8ff..2ed928aa71 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -435,6 +435,22 @@ void kvm_set_sigmask_len(KVMState *s, unsigned int sigmask_len);
 int kvm_physical_memory_addr_from_host(KVMState *s, void *ram_addr,
                                        hwaddr *phys_addr);
 
+/**
+ * kvm_create_vcpu - Gets a parked KVM vCPU or creates a KVM vCPU
+ * @cpu: QOM CPUState object for which KVM vCPU has to be fetched/created.
+ *
+ * @returns: 0 when success, errno (<0) when failed.
+ */
+int kvm_create_vcpu(CPUState *cpu);
+
+/**
+ * kvm_park_vcpu - Park QEMU KVM vCPU context
+ * @cpu: QOM CPUState object for which QEMU KVM vCPU context has to be parked.
+ *
+ * @returns: none
+ */
+void kvm_park_vcpu(CPUState *cpu);
+
 #endif /* NEED_CPU_H */
 
 void kvm_cpu_synchronize_state(CPUState *cpu);
-- 
2.34.1

[PATCH V8 2/8] hw/acpi: Move CPU ctrl-dev MMIO region len macro to common header file

[Salil Mehta via]

CPU ctrl-dev MMIO region length could be used in ACPI GED and various other
architecture specific places. Move ACPI_CPU_HOTPLUG_REG_LEN macro to more
appropriate common header file.

Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
Tested-by: Xianglai Li <lixianglai@loongson.cn>
Tested-by: Miguel Luis <miguel.luis@oracle.com>
---
 hw/acpi/cpu.c                 | 2 +-
 include/hw/acpi/cpu_hotplug.h | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index 2d81c1e790..69aaa563db 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -1,13 +1,13 @@
 #include "qemu/osdep.h"
 #include "migration/vmstate.h"
 #include "hw/acpi/cpu.h"
+#include "hw/acpi/cpu_hotplug.h"
 #include "hw/core/cpu.h"
 #include "qapi/error.h"
 #include "qapi/qapi-events-acpi.h"
 #include "trace.h"
 #include "sysemu/numa.h"
 
-#define ACPI_CPU_HOTPLUG_REG_LEN 12
 #define ACPI_CPU_SELECTOR_OFFSET_WR 0
 #define ACPI_CPU_FLAGS_OFFSET_RW 4
 #define ACPI_CPU_CMD_OFFSET_WR 5
diff --git a/include/hw/acpi/cpu_hotplug.h b/include/hw/acpi/cpu_hotplug.h
index 3b932abbbb..48b291e45e 100644
--- a/include/hw/acpi/cpu_hotplug.h
+++ b/include/hw/acpi/cpu_hotplug.h
@@ -19,6 +19,8 @@
 #include "hw/hotplug.h"
 #include "hw/acpi/cpu.h"
 
+#define ACPI_CPU_HOTPLUG_REG_LEN 12
+
 typedef struct AcpiCpuHotplug {
     Object *device;
     MemoryRegion io;
-- 
2.34.1

[PATCH V8 3/8] hw/acpi: Update ACPI GED framework to support vCPU Hotplug

[Salil Mehta via]

ACPI GED (as described in the ACPI 6.4 spec) uses an interrupt listed in the
_CRS object of GED to intimate OSPM about an event. Later then demultiplexes the
notified event by evaluating ACPI _EVT method to know the type of event. Use
ACPI GED to also notify the guest kernel about any CPU hot(un)plug events.

ACPI CPU hotplug related initialization should only happen if ACPI_CPU_HOTPLUG
support has been enabled for particular architecture. Add cpu_hotplug_hw_init()
stub to avoid compilation break.

Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
Tested-by: Xianglai Li <lixianglai@loongson.cn>
Tested-by: Miguel Luis <miguel.luis@oracle.com>
---
 hw/acpi/acpi-cpu-hotplug-stub.c        |  6 ++++++
 hw/acpi/generic_event_device.c         | 17 +++++++++++++++++
 include/hw/acpi/generic_event_device.h |  4 ++++
 3 files changed, 27 insertions(+)

diff --git a/hw/acpi/acpi-cpu-hotplug-stub.c b/hw/acpi/acpi-cpu-hotplug-stub.c
index 3fc4b14c26..c6c61bb9cd 100644
--- a/hw/acpi/acpi-cpu-hotplug-stub.c
+++ b/hw/acpi/acpi-cpu-hotplug-stub.c
@@ -19,6 +19,12 @@ void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner,
     return;
 }
 
+void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
+                         CPUHotplugState *state, hwaddr base_addr)
+{
+    return;
+}
+
 void acpi_cpu_ospm_status(CPUHotplugState *cpu_st, ACPIOSTInfoList ***list)
 {
     return;
diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index 2d6e91b124..35a71505a5 100644
--- a/hw/acpi/generic_event_device.c
+++ b/hw/acpi/generic_event_device.c
@@ -12,6 +12,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "hw/acpi/acpi.h"
+#include "hw/acpi/cpu.h"
 #include "hw/acpi/generic_event_device.h"
 #include "hw/irq.h"
 #include "hw/mem/pc-dimm.h"
@@ -25,6 +26,7 @@ static const uint32_t ged_supported_events[] = {
     ACPI_GED_MEM_HOTPLUG_EVT,
     ACPI_GED_PWR_DOWN_EVT,
     ACPI_GED_NVDIMM_HOTPLUG_EVT,
+    ACPI_GED_CPU_HOTPLUG_EVT,
 };
 
 /*
@@ -234,6 +236,8 @@ static void acpi_ged_device_plug_cb(HotplugHandler *hotplug_dev,
         } else {
             acpi_memory_plug_cb(hotplug_dev, &s->memhp_state, dev, errp);
         }
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+        acpi_cpu_plug_cb(hotplug_dev, &s->cpuhp_state, dev, errp);
     } else {
         error_setg(errp, "virt: device plug request for unsupported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
@@ -248,6 +252,8 @@ static void acpi_ged_unplug_request_cb(HotplugHandler *hotplug_dev,
     if ((object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
                        !(object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)))) {
         acpi_memory_unplug_request_cb(hotplug_dev, &s->memhp_state, dev, errp);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+        acpi_cpu_unplug_request_cb(hotplug_dev, &s->cpuhp_state, dev, errp);
     } else {
         error_setg(errp, "acpi: device unplug request for unsupported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
@@ -261,6 +267,8 @@ static void acpi_ged_unplug_cb(HotplugHandler *hotplug_dev,
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
         acpi_memory_unplug_cb(&s->memhp_state, dev, errp);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+        acpi_cpu_unplug_cb(&s->cpuhp_state, dev, errp);
     } else {
         error_setg(errp, "acpi: device unplug for unsupported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
@@ -272,6 +280,7 @@ static void acpi_ged_ospm_status(AcpiDeviceIf *adev, ACPIOSTInfoList ***list)
     AcpiGedState *s = ACPI_GED(adev);
 
     acpi_memory_ospm_status(&s->memhp_state, list);
+    acpi_cpu_ospm_status(&s->cpuhp_state, list);
 }
 
 static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
@@ -286,6 +295,8 @@ static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
         sel = ACPI_GED_PWR_DOWN_EVT;
     } else if (ev & ACPI_NVDIMM_HOTPLUG_STATUS) {
         sel = ACPI_GED_NVDIMM_HOTPLUG_EVT;
+    } else if (ev & ACPI_CPU_HOTPLUG_STATUS) {
+        sel = ACPI_GED_CPU_HOTPLUG_EVT;
     } else {
         /* Unknown event. Return without generating interrupt. */
         warn_report("GED: Unsupported event %d. No irq injected", ev);
@@ -400,6 +411,12 @@ static void acpi_ged_initfn(Object *obj)
     memory_region_init_io(&ged_st->regs, obj, &ged_regs_ops, ged_st,
                           TYPE_ACPI_GED "-regs", ACPI_GED_REG_COUNT);
     sysbus_init_mmio(sbd, &ged_st->regs);
+
+    memory_region_init(&s->container_cpuhp, OBJECT(dev), "cpuhp container",
+                       ACPI_CPU_HOTPLUG_REG_LEN);
+    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->container_cpuhp);
+    cpu_hotplug_hw_init(&s->container_cpuhp, OBJECT(dev),
+                        &s->cpuhp_state, 0);
 }
 
 static void acpi_ged_class_init(ObjectClass *class, void *data)
diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
index ba84ce0214..90fc41cbb8 100644
--- a/include/hw/acpi/generic_event_device.h
+++ b/include/hw/acpi/generic_event_device.h
@@ -60,6 +60,7 @@
 #define HW_ACPI_GENERIC_EVENT_DEVICE_H
 
 #include "hw/sysbus.h"
+#include "hw/acpi/cpu_hotplug.h"
 #include "hw/acpi/memory_hotplug.h"
 #include "hw/acpi/ghes.h"
 #include "qom/object.h"
@@ -95,6 +96,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
 #define ACPI_GED_MEM_HOTPLUG_EVT   0x1
 #define ACPI_GED_PWR_DOWN_EVT      0x2
 #define ACPI_GED_NVDIMM_HOTPLUG_EVT 0x4
+#define ACPI_GED_CPU_HOTPLUG_EVT    0x8
 
 typedef struct GEDState {
     MemoryRegion evt;
@@ -106,6 +108,8 @@ struct AcpiGedState {
     SysBusDevice parent_obj;
     MemHotplugState memhp_state;
     MemoryRegion container_memhp;
+    CPUHotplugState cpuhp_state;
+    MemoryRegion container_cpuhp;
     GEDState ged_state;
     uint32_t ged_event_bitmap;
     qemu_irq irq;
-- 
2.34.1

[PATCH V8 4/8] hw/acpi: Update GED _EVT method AML with CPU scan

[Salil Mehta via]

OSPM evaluates _EVT method to map the event. The CPU hotplug event eventually
results in start of the CPU scan. Scan figures out the CPU and the kind of
event(plug/unplug) and notifies it back to the guest. Update the GED AML _EVT
method with the call to \\_SB.CPUS.CSCN

Also, macro CPU_SCAN_METHOD might be referred in other places like during GED
intialization so it makes sense to have its definition placed in some common
header file like cpu_hotplug.h. But doing this can cause compilation break
because of the conflicting macro definitions present in cpu.c and cpu_hotplug.c
and because both these files get compiled due to historic reasons of x86 world
i.e. decision to use legacy(GPE.2)/modern(GED) CPU hotplug interface happens
during runtime [1]. To mitigate above, for now, declare a new common macro
ACPI_CPU_SCAN_METHOD for CPU scan method instead.
(This needs a separate discussion later on for clean-up)

Reference:
[1] https://lore.kernel.org/qemu-devel/1463496205-251412-24-git-send-email-imammedo@redhat.com/

Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Gavin Shan <gshan@redhat.com>
Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
Tested-by: Xianglai Li <lixianglai@loongson.cn>
Tested-by: Miguel Luis <miguel.luis@oracle.com>
Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
---
 hw/acpi/cpu.c                  | 2 +-
 hw/acpi/generic_event_device.c | 4 ++++
 include/hw/acpi/cpu_hotplug.h  | 2 ++
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index 69aaa563db..bde5a42a0b 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -323,7 +323,7 @@ const VMStateDescription vmstate_cpu_hotplug = {
 #define CPUHP_RES_DEVICE  "PRES"
 #define CPU_LOCK          "CPLK"
 #define CPU_STS_METHOD    "CSTA"
-#define CPU_SCAN_METHOD   "CSCN"
+#define CPU_SCAN_METHOD   ACPI_CPU_SCAN_METHOD
 #define CPU_NOTIFY_METHOD "CTFY"
 #define CPU_EJECT_METHOD  "CEJ0"
 #define CPU_OST_METHOD    "COST"
diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index 35a71505a5..58c7882555 100644
--- a/hw/acpi/generic_event_device.c
+++ b/hw/acpi/generic_event_device.c
@@ -109,6 +109,10 @@ void build_ged_aml(Aml *table, const char *name, HotplugHandler *hotplug_dev,
                 aml_append(if_ctx, aml_call0(MEMORY_DEVICES_CONTAINER "."
                                              MEMORY_SLOT_SCAN_METHOD));
                 break;
+            case ACPI_GED_CPU_HOTPLUG_EVT:
+                aml_append(if_ctx, aml_call0(ACPI_CPU_CONTAINER "."
+                                             ACPI_CPU_SCAN_METHOD));
+                break;
             case ACPI_GED_PWR_DOWN_EVT:
                 aml_append(if_ctx,
                            aml_notify(aml_name(ACPI_POWER_BUTTON_DEVICE),
diff --git a/include/hw/acpi/cpu_hotplug.h b/include/hw/acpi/cpu_hotplug.h
index 48b291e45e..ef631750b4 100644
--- a/include/hw/acpi/cpu_hotplug.h
+++ b/include/hw/acpi/cpu_hotplug.h
@@ -20,6 +20,8 @@
 #include "hw/acpi/cpu.h"
 
 #define ACPI_CPU_HOTPLUG_REG_LEN 12
+#define ACPI_CPU_SCAN_METHOD "CSCN"
+#define ACPI_CPU_CONTAINER "\\_SB.CPUS"
 
 typedef struct AcpiCpuHotplug {
     Object *device;
-- 
2.34.1

[PATCH V8 5/8] hw/acpi: Update CPUs AML with cpu-(ctrl)dev change

[Salil Mehta via]

CPUs Control device(\\_SB.PCI0) register interface for the x86 arch is IO port
based and existing CPUs AML code assumes _CRS objects would evaluate to a system
resource which describes IO Port address. But on ARM arch CPUs control
device(\\_SB.PRES) register interface is memory-mapped hence _CRS object should
evaluate to system resource which describes memory-mapped base address. Update
build CPUs AML function to accept both IO/MEMORY region spaces and accordingly
update the _CRS object.

On x86, CPU Hotplug uses Generic ACPI GPE Block Bit 2 (GPE.2) event handler to
notify OSPM about any CPU hot(un)plug events. Latest CPU Hotplug is based on
ACPI Generic Event Device framework and uses ACPI GED device for the same. Not
all architectures support GPE based CPU Hotplug event handler. Hence, make AML
for GPE.2 event handler conditional.

Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
Reviewed-by: Gavin Shan <gshan@redhat.com>
Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Tested-by: Xianglai Li <lixianglai@loongson.cn>
Tested-by: Miguel Luis <miguel.luis@oracle.com>
Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
---
 hw/acpi/cpu.c         | 23 ++++++++++++++++-------
 hw/i386/acpi-build.c  |  3 ++-
 include/hw/acpi/cpu.h |  5 +++--
 3 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index bde5a42a0b..b5fb2075d0 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -339,9 +339,10 @@ const VMStateDescription vmstate_cpu_hotplug = {
 #define CPU_FW_EJECT_EVENT "CEJF"
 
 void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
-                    build_madt_cpu_fn build_madt_cpu, hwaddr io_base,
+                    build_madt_cpu_fn build_madt_cpu, hwaddr base_addr,
                     const char *res_root,
-                    const char *event_handler_method)
+                    const char *event_handler_method,
+                    AmlRegionSpace rs)
 {
     Aml *ifctx;
     Aml *field;
@@ -366,13 +367,19 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
         aml_append(cpu_ctrl_dev, aml_mutex(CPU_LOCK, 0));
 
         crs = aml_resource_template();
-        aml_append(crs, aml_io(AML_DECODE16, io_base, io_base, 1,
+        if (rs == AML_SYSTEM_IO) {
+            aml_append(crs, aml_io(AML_DECODE16, base_addr, base_addr, 1,
                                ACPI_CPU_HOTPLUG_REG_LEN));
+        } else {
+            aml_append(crs, aml_memory32_fixed(base_addr,
+                               ACPI_CPU_HOTPLUG_REG_LEN, AML_READ_WRITE));
+        }
+
         aml_append(cpu_ctrl_dev, aml_name_decl("_CRS", crs));
 
         /* declare CPU hotplug MMIO region with related access fields */
         aml_append(cpu_ctrl_dev,
-            aml_operation_region("PRST", AML_SYSTEM_IO, aml_int(io_base),
+            aml_operation_region("PRST", rs, aml_int(base_addr),
                                  ACPI_CPU_HOTPLUG_REG_LEN));
 
         field = aml_field("PRST", AML_BYTE_ACC, AML_NOLOCK,
@@ -696,9 +703,11 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
     aml_append(sb_scope, cpus_dev);
     aml_append(table, sb_scope);
 
-    method = aml_method(event_handler_method, 0, AML_NOTSERIALIZED);
-    aml_append(method, aml_call0("\\_SB.CPUS." CPU_SCAN_METHOD));
-    aml_append(table, method);
+    if (event_handler_method) {
+        method = aml_method(event_handler_method, 0, AML_NOTSERIALIZED);
+        aml_append(method, aml_call0("\\_SB.CPUS." CPU_SCAN_METHOD));
+        aml_append(table, method);
+    }
 
     g_free(cphp_res_path);
 }
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 15242b9096..f0cdfaf9aa 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1536,7 +1536,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
             .fw_unplugs_cpu = pm->smi_on_cpu_unplug,
         };
         build_cpus_aml(dsdt, machine, opts, pc_madt_cpu_entry,
-                       pm->cpu_hp_io_base, "\\_SB.PCI0", "\\_GPE._E02");
+                       pm->cpu_hp_io_base, "\\_SB.PCI0", "\\_GPE._E02",
+                       AML_SYSTEM_IO);
     }
 
     if (pcms->memhp_io_base && nr_mem) {
diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
index e6e1a9ef59..48cded697c 100644
--- a/include/hw/acpi/cpu.h
+++ b/include/hw/acpi/cpu.h
@@ -61,9 +61,10 @@ typedef void (*build_madt_cpu_fn)(int uid, const CPUArchIdList *apic_ids,
                                   GArray *entry, bool force_enabled);
 
 void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
-                    build_madt_cpu_fn build_madt_cpu, hwaddr io_base,
+                    build_madt_cpu_fn build_madt_cpu, hwaddr base_addr,
                     const char *res_root,
-                    const char *event_handler_method);
+                    const char *event_handler_method,
+                    AmlRegionSpace rs);
 
 void acpi_cpu_ospm_status(CPUHotplugState *cpu_st, ACPIOSTInfoList ***list);
 
-- 
2.34.1

[PATCH V8 6/8] physmem: Add helper function to destroy CPU AddressSpace

[Salil Mehta via]

Virtual CPU Hot-unplug leads to unrealization of a CPU object. This also
involves destruction of the CPU AddressSpace. Add common function to help
destroy the CPU AddressSpace.

Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
Reviewed-by: Gavin Shan <gshan@redhat.com>
Tested-by: Xianglai Li <lixianglai@loongson.cn>
Tested-by: Miguel Luis <miguel.luis@oracle.com>
Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
---
 include/exec/cpu-common.h |  8 ++++++++
 include/hw/core/cpu.h     |  1 +
 system/physmem.c          | 29 +++++++++++++++++++++++++++++
 3 files changed, 38 insertions(+)

diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index 6346df17ce..a427d80340 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -123,6 +123,14 @@ size_t qemu_ram_pagesize_largest(void);
  */
 void cpu_address_space_init(CPUState *cpu, int asidx,
                             const char *prefix, MemoryRegion *mr);
+/**
+ * cpu_address_space_destroy:
+ * @cpu: CPU for which address space needs to be destroyed
+ * @asidx: integer index of this address space
+ *
+ * Note that with KVM only one address space is supported.
+ */
+void cpu_address_space_destroy(CPUState *cpu, int asidx);
 
 void cpu_physical_memory_rw(hwaddr addr, void *buf,
                             hwaddr len, bool is_write);
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index ec14f74ce5..e975b8085f 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -493,6 +493,7 @@ struct CPUState {
     QSIMPLEQ_HEAD(, qemu_work_item) work_list;
 
     CPUAddressSpace *cpu_ases;
+    int cpu_ases_count;
     int num_ases;
     AddressSpace *as;
     MemoryRegion *memory;
diff --git a/system/physmem.c b/system/physmem.c
index 6e9ed97597..61b32ac4f2 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -761,6 +761,7 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
 
     if (!cpu->cpu_ases) {
         cpu->cpu_ases = g_new0(CPUAddressSpace, cpu->num_ases);
+        cpu->cpu_ases_count = cpu->num_ases;
     }
 
     newas = &cpu->cpu_ases[asidx];
@@ -774,6 +775,34 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
     }
 }
 
+void cpu_address_space_destroy(CPUState *cpu, int asidx)
+{
+    CPUAddressSpace *cpuas;
+
+    assert(cpu->cpu_ases);
+    assert(asidx >= 0 && asidx < cpu->num_ases);
+    /* KVM cannot currently support multiple address spaces. */
+    assert(asidx == 0 || !kvm_enabled());
+
+    cpuas = &cpu->cpu_ases[asidx];
+    if (tcg_enabled()) {
+        memory_listener_unregister(&cpuas->tcg_as_listener);
+    }
+
+    address_space_destroy(cpuas->as);
+    g_free_rcu(cpuas->as, rcu);
+
+    if (asidx == 0) {
+        /* reset the convenience alias for address space 0 */
+        cpu->as = NULL;
+    }
+
+    if (--cpu->cpu_ases_count == 0) {
+        g_free(cpu->cpu_ases);
+        cpu->cpu_ases = NULL;
+    }
+}
+
 AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx)
 {
     /* Return the AddressSpace corresponding to the specified index */
-- 
2.34.1

[PATCH V8 7/8] gdbstub: Add helper function to unregister GDB register space

[Salil Mehta via]

Add common function to help unregister the GDB register space. This shall be
done in context to the CPU unrealization.

Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
Reviewed-by: Gavin Shan <gshan@redhat.com>
Tested-by: Xianglai Li <lixianglai@loongson.cn>
Tested-by: Miguel Luis <miguel.luis@oracle.com>
Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
---
 gdbstub/gdbstub.c      | 12 ++++++++++++
 include/exec/gdbstub.h |  6 ++++++
 2 files changed, 18 insertions(+)

diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 17efcae0d0..a8449dc309 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -615,6 +615,18 @@ void gdb_register_coprocessor(CPUState *cpu,
     }
 }
 
+void gdb_unregister_coprocessor_all(CPUState *cpu)
+{
+    /*
+     * Safe to nuke everything. GDBRegisterState::xml is static const char so
+     * it won't be freed
+     */
+    g_array_free(cpu->gdb_regs, true);
+
+    cpu->gdb_regs = NULL;
+    cpu->gdb_num_g_regs = 0;
+}
+
 static void gdb_process_breakpoint_remove_all(GDBProcess *p)
 {
     CPUState *cpu = gdb_get_first_cpu_in_process(p);
diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index eb14b91139..249d4d4bc8 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -49,6 +49,12 @@ void gdb_register_coprocessor(CPUState *cpu,
                               gdb_get_reg_cb get_reg, gdb_set_reg_cb set_reg,
                               const GDBFeature *feature, int g_pos);
 
+/**
+ * gdb_unregister_coprocessor_all() - unregisters supplemental set of registers
+ * @cpu - the CPU associated with registers
+ */
+void gdb_unregister_coprocessor_all(CPUState *cpu);
+
 /**
  * gdbserver_start: start the gdb server
  * @port_or_device: connection spec for gdb
-- 
2.34.1

[PATCH V8 8/8] docs/specs/acpi_hw_reduced_hotplug: Add the CPU Hotplug Event Bit

[Salil Mehta via]

GED interface is used by many hotplug events like memory hotplug, NVDIMM hotplug
and non-hotplug events like system power down event. Each of these can be
selected using a bit in the 32 bit GED IO interface. A bit has been reserved for
the CPU hotplug event.

Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
Reviewed-by: Gavin Shan <gshan@redhat.com>
---
 docs/specs/acpi_hw_reduced_hotplug.rst | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/docs/specs/acpi_hw_reduced_hotplug.rst b/docs/specs/acpi_hw_reduced_hotplug.rst
index 0bd3f9399f..3acd6fcd8b 100644
--- a/docs/specs/acpi_hw_reduced_hotplug.rst
+++ b/docs/specs/acpi_hw_reduced_hotplug.rst
@@ -64,7 +64,8 @@ GED IO interface (4 byte access)
        0: Memory hotplug event
        1: System power down event
        2: NVDIMM hotplug event
-    3-31: Reserved
+       3: CPU hotplug event
+    4-31: Reserved
 
 **write_access:**
 
-- 
2.34.1

Re: [PATCH V8 0/8] Add architecture agnostic code to support vCPU Hotplug

[Michael S. Tsirkin]

On Tue, Mar 12, 2024 at 01:59:52AM +0000, Salil Mehta wrote:
> Virtual CPU hotplug support is being added across various architectures[1][3].
> This series adds various code bits common across all architectures:
> 
> 1. vCPU creation and Parking code refactor [Patch 1]
> 2. Update ACPI GED framework to support vCPU Hotplug [Patch 2,3]
> 3. ACPI CPUs AML code change [Patch 4,5]
> 4. Helper functions to support unrealization of CPU objects [Patch 6,7]
> 5. Docs [Patch 8]


I replied separately: patch 1 needs ack from KVM maintainers.

I suspect I could pick some bits I have no way to know
whether they were tested separately.
In any way looks like something best left to next release ...

> 
> Repository:
> 
> [*] https://github.com/salil-mehta/qemu.git virt-cpuhp-armv8/rfc-v2.common.v8
> 
> 
> Revision History:
> 
> Patch-set V7 -> V8
> 
> 1. Rebased and Fixed the conflicts
> 
> Patch-set  V6 -> V7
> 1. Addressed Alex Bennée's comments
>    - Updated the docs
> 2. Addressed Igor Mammedov's comments
>    - Merged patches [Patch V6 3/9] & [Patch V6 7/9] with [Patch V6 4/9]
>    - Updated commit-log of [Patch V6 1/9] and [Patch V6 5/9]     
> 3. Added Shaoqin Huang's Reviewed-by tags for whole series.
> Link: https://lore.kernel.org/qemu-devel/20231013105129.25648-1-salil.mehta@huawei.com/
> 
> Patch-set  V5 -> V6
> 1. Addressed Gavin Shan's comments
>    - Fixed the assert() ranges of address spaces
>    - Rebased the patch-set to latest changes in the qemu.git
>    - Added Reviewed-by tags for patches {8,9}
> 2. Addressed Jonathan Cameron's comments
>    - Updated commit-log for [Patch V5 1/9] with mention of trace events
>    - Added Reviewed-by tags for patches {1,5}
> 3. Added Tested-by tags from Xianglai Li
> 4. Fixed checkpatch.pl error "Qemu -> QEMU" in [Patch V5 1/9] 
> Link: https://lore.kernel.org/qemu-devel/20231011194355.15628-1-salil.mehta@huawei.com/
> 
> Patch-set  V4 -> V5
> 1. Addressed Gavin Shan's comments
>    - Fixed the trace events print string for kvm_{create,get,park,destroy}_vcpu
>    - Added Reviewed-by tag for patch {1}
> 2. Added Shaoqin Huang's Reviewed-by tags for Patches {2,3}
> 3. Added Tested-by Tag from Vishnu Pajjuri to the patch-set
> 4. Dropped the ARM specific [Patch V4 10/10]
> Link: https://lore.kernel.org/qemu-devel/20231009203601.17584-1-salil.mehta@huawei.com/
> 
> Patch-set  V3 -> V4
> 1. Addressed David Hilderbrand's comments
>    - Fixed the wrong doc comment of kvm_park_vcpu API prototype
>    - Added Reviewed-by tags for patches {2,4}
> Link: https://lore.kernel.org/qemu-devel/20231009112812.10612-1-salil.mehta@huawei.com/
> 
> Patch-set  V2 -> V3
> 1. Addressed Jonathan Cameron's comments
>    - Fixed 'vcpu-id' type wrongly changed from 'unsigned long' to 'integer'
>    - Removed unnecessary use of variable 'vcpu_id' in kvm_park_vcpu
>    - Updated [Patch V2 3/10] commit-log with details of ACPI_CPU_SCAN_METHOD macro
>    - Updated [Patch V2 5/10] commit-log with details of conditional event handler method
>    - Added Reviewed-by tags for patches {2,3,4,6,7}
> 2. Addressed Gavin Shan's comments
>    - Remove unnecessary use of variable 'vcpu_id' in kvm_par_vcpu
>    - Fixed return value in kvm_get_vcpu from -1 to -ENOENT
>    - Reset the value of 'gdb_num_g_regs' in gdb_unregister_coprocessor_all
>    - Fixed the kvm_{create,park}_vcpu prototypes docs
>    - Added Reviewed-by tags for patches {2,3,4,5,6,7,9,10}
> 3. Addressed one earlier missed comment by Alex Bennée in RFC V1
>    - Added traces instead of DPRINTF in the newly added and some existing functions
> Link: https://lore.kernel.org/qemu-devel/20230930001933.2660-1-salil.mehta@huawei.com/
> 
> Patch-set V1 -> V2
> 1. Addressed Alex Bennée's comments
>    - Refactored the kvm_create_vcpu logic to get rid of goto
>    - Added the docs for kvm_{create,park}_vcpu prototypes
>    - Splitted the gdbstub and AddressSpace destruction change into separate patches
>    - Added Reviewed-by tags for patches {2,10}
> Link: https://lore.kernel.org/qemu-devel/20230929124304.13672-1-salil.mehta@huawei.com/
> 
> References:
> 
> [1] https://lore.kernel.org/qemu-devel/20230926100436.28284-1-salil.mehta@huawei.com/
> [2] https://lore.kernel.org/all/20230913163823.7880-1-james.morse@arm.com/
> [3] https://lore.kernel.org/qemu-devel/cover.1695697701.git.lixianglai@loongson.cn/
> 
> 
> 
> Salil Mehta (8):
>   accel/kvm: Extract common KVM vCPU {creation,parking} code
>   hw/acpi: Move CPU ctrl-dev MMIO region len macro to common header file
>   hw/acpi: Update ACPI GED framework to support vCPU Hotplug
>   hw/acpi: Update GED _EVT method AML with CPU scan
>   hw/acpi: Update CPUs AML with cpu-(ctrl)dev change
>   physmem: Add helper function to destroy CPU AddressSpace
>   gdbstub: Add helper function to unregister GDB register space
>   docs/specs/acpi_hw_reduced_hotplug: Add the CPU Hotplug Event Bit
> 
>  accel/kvm/kvm-all.c                    | 64 ++++++++++++++++++++------
>  accel/kvm/trace-events                 |  5 +-
>  docs/specs/acpi_hw_reduced_hotplug.rst |  3 +-
>  gdbstub/gdbstub.c                      | 12 +++++
>  hw/acpi/acpi-cpu-hotplug-stub.c        |  6 +++
>  hw/acpi/cpu.c                          | 27 +++++++----
>  hw/acpi/generic_event_device.c         | 21 +++++++++
>  hw/i386/acpi-build.c                   |  3 +-
>  include/exec/cpu-common.h              |  8 ++++
>  include/exec/gdbstub.h                 |  6 +++
>  include/hw/acpi/cpu.h                  |  5 +-
>  include/hw/acpi/cpu_hotplug.h          |  4 ++
>  include/hw/acpi/generic_event_device.h |  4 ++
>  include/hw/core/cpu.h                  |  1 +
>  include/sysemu/kvm.h                   | 16 +++++++
>  system/physmem.c                       | 29 ++++++++++++
>  16 files changed, 185 insertions(+), 29 deletions(-)
> 
> -- 
> 2.34.1

Re: [PATCH V8 3/8] hw/acpi: Update ACPI GED framework to support vCPU Hotplug

[Zhao Liu]

Hi Salil,

It seems my comment [1] in v7 was missed, but I still hit the same
issue. Pls let me paste the previous comment here again.

[1]: https://lore.kernel.org/qemu-devel/ZXCqp32ggIFvUweu@intel.com/

[snip]

> @@ -400,6 +411,12 @@ static void acpi_ged_initfn(Object *obj)
>      memory_region_init_io(&ged_st->regs, obj, &ged_regs_ops, ged_st,
>                            TYPE_ACPI_GED "-regs", ACPI_GED_REG_COUNT);
>      sysbus_init_mmio(sbd, &ged_st->regs);
> +
> +    memory_region_init(&s->container_cpuhp, OBJECT(dev), "cpuhp container",
> +                       ACPI_CPU_HOTPLUG_REG_LEN);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->container_cpuhp);
> +    cpu_hotplug_hw_init(&s->container_cpuhp, OBJECT(dev),
> +                        &s->cpuhp_state, 0);
>  }
>

I find this cpu_hotplug_hw_init() can still cause qtest errors (for v8)
on x86 platforms as you mentioned in v6:
https://lore.kernel.org/qemu-devel/15e70616-6abb-63a4-17d0-820f4a254607@opnsrc.net/T/#m108f102b2fe92b7dd7218f2f942f7b233a9d6af3

IIUC, microvm machine has its own 'possible_cpus_arch_ids' and that is
inherited from its parent x86 machine.

The above error is because device-introspect-test sets the none-machine:

# starting QEMU: exec ./qemu-system-i386 -qtest unix:/tmp/qtest-3094820.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-3094820.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -nodefaults -machine none -accel qtest

So what about just checking mc->possible_cpu_arch_ids instead of an
assert in cpu_hotplug_hw_init()?

diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index 4b24a2500361..303f1f1f57bc 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -221,7 +221,10 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
     const CPUArchIdList *id_list;
     int i;

-    assert(mc->possible_cpu_arch_ids);
+    if (!mc->possible_cpu_arch_ids) {
+        return;
+    }
+
     id_list = mc->possible_cpu_arch_ids(machine);
     state->dev_count = id_list->len;
     state->devs = g_new0(typeof(*state->devs), state->dev_count);

This check seems to be acceptable in the general code path? Not all machines
have possible_cpu_arch_ids, after all.

Thanks,
Zhao

答复: [PATCH V8 6/8] physmem: Add helper function to destroy CPU AddressSpace

[zhukeqian via]

Hi Salil,

[...]

+void cpu_address_space_destroy(CPUState *cpu, int asidx) {
+    CPUAddressSpace *cpuas;
+
+    assert(cpu->cpu_ases);
+    assert(asidx >= 0 && asidx < cpu->num_ases);
+    /* KVM cannot currently support multiple address spaces. */
+    assert(asidx == 0 || !kvm_enabled());
+
+    cpuas = &cpu->cpu_ases[asidx];
+    if (tcg_enabled()) {
+        memory_listener_unregister(&cpuas->tcg_as_listener);
+    }
+
+    address_space_destroy(cpuas->as);
+    g_free_rcu(cpuas->as, rcu);

In address_space_destroy(), it calls call_rcu1() on cpuas->as which will set do_address_space_destroy() as the rcu func.
And g_free_rcu() also calls call_rcu1() on cpuas->as which will overwrite the rcu func as g_free().

Then I think the g_free() may be called twice in rcu thread, please verify that.

The source code of call_rcu1:

void call_rcu1(struct rcu_head *node, void (*func)(struct rcu_head *node))
{
    node->func = func;
    enqueue(node);
    qatomic_inc(&rcu_call_count);
    qemu_event_set(&rcu_call_ready_event);
}

Thanks,
Keqian

+
+    if (asidx == 0) {
+        /* reset the convenience alias for address space 0 */
+        cpu->as = NULL;
+    }
+
+    if (--cpu->cpu_ases_count == 0) {
+        g_free(cpu->cpu_ases);
+        cpu->cpu_ases = NULL;
+    }
+}
+
 AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx)  {
     /* Return the AddressSpace corresponding to the specified index */
--
2.34.1

Re: [PATCH V8 1/8] accel/kvm: Extract common KVM vCPU {creation, parking} code

[Harsh Prateek Bora]

+ Vaibhav, Shiva

Hi Salil,

I came across your patch while trying to solve a related problem on 
spapr. One query below ..

On 3/12/24 07:29, Salil Mehta via wrote:
> KVM vCPU creation is done once during the vCPU realization when Qemu vCPU thread
> is spawned. This is common to all the architectures as of now.
> 
> Hot-unplug of vCPU results in destruction of the vCPU object in QOM but the
> corresponding KVM vCPU object in the Host KVM is not destroyed as KVM doesn't
> support vCPU removal. Therefore, its representative KVM vCPU object/context in
> Qemu is parked.
> 
> Refactor architecture common logic so that some APIs could be reused by vCPU
> Hotplug code of some architectures likes ARM, Loongson etc. Update new/old APIs
> with trace events instead of DPRINTF. No functional change is intended here.
> 
> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> Reviewed-by: Gavin Shan <gshan@redhat.com>
> Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Tested-by: Xianglai Li <lixianglai@loongson.cn>
> Tested-by: Miguel Luis <miguel.luis@oracle.com>
> Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
> ---
>   accel/kvm/kvm-all.c    | 64 ++++++++++++++++++++++++++++++++----------
>   accel/kvm/trace-events |  5 +++-
>   include/sysemu/kvm.h   | 16 +++++++++++
>   3 files changed, 69 insertions(+), 16 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index a8cecd040e..3bc3207bda 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -126,6 +126,7 @@ static QemuMutex kml_slots_lock;
>   #define kvm_slots_unlock()  qemu_mutex_unlock(&kml_slots_lock)
>   
>   static void kvm_slot_init_dirty_bitmap(KVMSlot *mem);
> +static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id);
>   
>   static inline void kvm_resample_fd_remove(int gsi)
>   {
> @@ -314,14 +315,53 @@ err:
>       return ret;
>   }
>   
> +void kvm_park_vcpu(CPUState *cpu)
> +{
> +    struct KVMParkedVcpu *vcpu;
> +
> +    trace_kvm_park_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
> +
> +    vcpu = g_malloc0(sizeof(*vcpu));
> +    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
> +    vcpu->kvm_fd = cpu->kvm_fd;
> +    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
> +}
> +
> +int kvm_create_vcpu(CPUState *cpu)
> +{
> +    unsigned long vcpu_id = kvm_arch_vcpu_id(cpu);
> +    KVMState *s = kvm_state;
> +    int kvm_fd;
> +
> +    trace_kvm_create_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
> +
> +    /* check if the KVM vCPU already exist but is parked */
> +    kvm_fd = kvm_get_vcpu(s, vcpu_id);
> +    if (kvm_fd < 0) {
> +        /* vCPU not parked: create a new KVM vCPU */
> +        kvm_fd = kvm_vm_ioctl(s, KVM_CREATE_VCPU, vcpu_id);
> +        if (kvm_fd < 0) {
> +            error_report("KVM_CREATE_VCPU IOCTL failed for vCPU %lu", vcpu_id);
> +            return kvm_fd;
> +        }
> +    }
> +
> +    cpu->kvm_fd = kvm_fd;
> +    cpu->kvm_state = s;
> +    cpu->vcpu_dirty = true;
> +    cpu->dirty_pages = 0;
> +    cpu->throttle_us_per_full = 0;
> +
> +    return 0;
> +}
> +
>   static int do_kvm_destroy_vcpu(CPUState *cpu)
>   {
>       KVMState *s = kvm_state;
>       long mmap_size;
> -    struct KVMParkedVcpu *vcpu = NULL;
>       int ret = 0;
>   
> -    trace_kvm_destroy_vcpu();
> +    trace_kvm_destroy_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>   
>       ret = kvm_arch_destroy_vcpu(cpu);
>       if (ret < 0) {
> @@ -347,10 +387,7 @@ static int do_kvm_destroy_vcpu(CPUState *cpu)
>           }
>       }
>   
> -    vcpu = g_malloc0(sizeof(*vcpu));
> -    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
> -    vcpu->kvm_fd = cpu->kvm_fd;
> -    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
> +    kvm_park_vcpu(cpu);
>   err:
>       return ret;
>   }
> @@ -371,6 +408,8 @@ static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
>           if (cpu->vcpu_id == vcpu_id) {
>               int kvm_fd;
>   
> +            trace_kvm_get_vcpu(vcpu_id);
> +
>               QLIST_REMOVE(cpu, node);
>               kvm_fd = cpu->kvm_fd;
>               g_free(cpu);
> @@ -378,7 +417,7 @@ static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
>           }
>       }
>   
> -    return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
> +    return -ENOENT;
>   }
>   
>   int kvm_init_vcpu(CPUState *cpu, Error **errp)
> @@ -389,19 +428,14 @@ int kvm_init_vcpu(CPUState *cpu, Error **errp)
>   
>       trace_kvm_init_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>   
> -    ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu));
> +    ret = kvm_create_vcpu(cpu);
>       if (ret < 0) {
> -        error_setg_errno(errp, -ret, "kvm_init_vcpu: kvm_get_vcpu failed (%lu)",
> +        error_setg_errno(errp, -ret,
> +                         "kvm_init_vcpu: kvm_create_vcpu failed (%lu)",
>                            kvm_arch_vcpu_id(cpu));

If a vcpu hotplug fails due to failure with kvm_create_vcpu ioctl,
current behaviour would be to bring down the guest as errp is
&error_fatal. Any thoughts on how do we ensure that a failure with
kvm_create_vcpu ioctl for hotplugged cpus (only) doesnt bring down the
guest and fail gracefully (by reporting error to user on monitor?)?

regards,
Harsh
>           goto err;
>       }
>   
> -    cpu->kvm_fd = ret;
> -    cpu->kvm_state = s;
> -    cpu->vcpu_dirty = true;
> -    cpu->dirty_pages = 0;
> -    cpu->throttle_us_per_full = 0;
> -
>       mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
>       if (mmap_size < 0) {
>           ret = mmap_size;
> diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events
> index a25902597b..5558cff0dc 100644
> --- a/accel/kvm/trace-events
> +++ b/accel/kvm/trace-events
> @@ -9,6 +9,10 @@ kvm_device_ioctl(int fd, int type, void *arg) "dev fd %d, type 0x%x, arg %p"
>   kvm_failed_reg_get(uint64_t id, const char *msg) "Warning: Unable to retrieve ONEREG %" PRIu64 " from KVM: %s"
>   kvm_failed_reg_set(uint64_t id, const char *msg) "Warning: Unable to set ONEREG %" PRIu64 " to KVM: %s"
>   kvm_init_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
> +kvm_create_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
> +kvm_get_vcpu(unsigned long arch_cpu_id) "id: %lu"
> +kvm_destroy_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
> +kvm_park_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
>   kvm_irqchip_commit_routes(void) ""
>   kvm_irqchip_add_msi_route(char *name, int vector, int virq) "dev %s vector %d virq %d"
>   kvm_irqchip_update_msi_route(int virq) "Updating MSI route virq=%d"
> @@ -25,7 +29,6 @@ kvm_dirty_ring_reaper(const char *s) "%s"
>   kvm_dirty_ring_reap(uint64_t count, int64_t t) "reaped %"PRIu64" pages (took %"PRIi64" us)"
>   kvm_dirty_ring_reaper_kick(const char *reason) "%s"
>   kvm_dirty_ring_flush(int finished) "%d"
> -kvm_destroy_vcpu(void) ""
>   kvm_failed_get_vcpu_mmap_size(void) ""
>   kvm_cpu_exec(void) ""
>   kvm_interrupt_exit_request(void) ""
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index fad9a7e8ff..2ed928aa71 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -435,6 +435,22 @@ void kvm_set_sigmask_len(KVMState *s, unsigned int sigmask_len);
>   int kvm_physical_memory_addr_from_host(KVMState *s, void *ram_addr,
>                                          hwaddr *phys_addr);
>   
> +/**
> + * kvm_create_vcpu - Gets a parked KVM vCPU or creates a KVM vCPU
> + * @cpu: QOM CPUState object for which KVM vCPU has to be fetched/created.
> + *
> + * @returns: 0 when success, errno (<0) when failed.
> + */
> +int kvm_create_vcpu(CPUState *cpu);
> +
> +/**
> + * kvm_park_vcpu - Park QEMU KVM vCPU context
> + * @cpu: QOM CPUState object for which QEMU KVM vCPU context has to be parked.
> + *
> + * @returns: none
> + */
> +void kvm_park_vcpu(CPUState *cpu);
> +
>   #endif /* NEED_CPU_H */
>   
>   void kvm_cpu_synchronize_state(CPUState *cpu);

Re: [PATCH V8 1/8] accel/kvm: Extract common KVM vCPU {creation,parking} code

[Vishnu Pajjuri]

[-- Attachment #1: Type: text/plain, Size: 7900 bytes --]

Hi Salil,

On 12-03-2024 07:29, Salil Mehta wrote:
> KVM vCPU creation is done once during the vCPU realization when Qemu vCPU thread
> is spawned. This is common to all the architectures as of now.
>
> Hot-unplug of vCPU results in destruction of the vCPU object in QOM but the
> corresponding KVM vCPU object in the Host KVM is not destroyed as KVM doesn't
> support vCPU removal. Therefore, its representative KVM vCPU object/context in
> Qemu is parked.
>
> Refactor architecture common logic so that some APIs could be reused by vCPU
> Hotplug code of some architectures likes ARM, Loongson etc. Update new/old APIs
> with trace events instead of DPRINTF. No functional change is intended here.
>
> Signed-off-by: Salil Mehta<salil.mehta@huawei.com>
> Reviewed-by: Gavin Shan<gshan@redhat.com>
> Tested-by: Vishnu Pajjuri<vishnu@os.amperecomputing.com>
> Reviewed-by: Jonathan Cameron<Jonathan.Cameron@huawei.com>
> Tested-by: Xianglai Li<lixianglai@loongson.cn>
> Tested-by: Miguel Luis<miguel.luis@oracle.com>
> Reviewed-by: Shaoqin Huang<shahuang@redhat.com>
> ---
>   accel/kvm/kvm-all.c    | 64 ++++++++++++++++++++++++++++++++----------
>   accel/kvm/trace-events |  5 +++-
>   include/sysemu/kvm.h   | 16 +++++++++++
>   3 files changed, 69 insertions(+), 16 deletions(-)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index a8cecd040e..3bc3207bda 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -126,6 +126,7 @@ static QemuMutex kml_slots_lock;
>   #define kvm_slots_unlock()  qemu_mutex_unlock(&kml_slots_lock)
>   
>   static void kvm_slot_init_dirty_bitmap(KVMSlot *mem);
> +static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id);
>   
>   static inline void kvm_resample_fd_remove(int gsi)
>   {
> @@ -314,14 +315,53 @@ err:
>       return ret;
>   }
>   
> +void kvm_park_vcpu(CPUState *cpu)
> +{
> +    struct KVMParkedVcpu *vcpu;
> +
> +    trace_kvm_park_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
It's good if we add kvm_fd to trace.
It will be useful to cross verify kvm_get_vcpu()'s kvm_fd with parked vcpu.
> +
> +    vcpu = g_malloc0(sizeof(*vcpu));
> +    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
> +    vcpu->kvm_fd = cpu->kvm_fd;
> +    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
> +}
> +
> +int kvm_create_vcpu(CPUState *cpu)
> +{
> +    unsigned long vcpu_id = kvm_arch_vcpu_id(cpu);
> +    KVMState *s = kvm_state;
> +    int kvm_fd;
> +
> +    trace_kvm_create_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
vcpu_id can be used instead of kvm_arch_vcpu_id(cpu).
> +
> +    /* check if the KVM vCPU already exist but is parked */
> +    kvm_fd = kvm_get_vcpu(s, vcpu_id);
> +    if (kvm_fd < 0) {
> +        /* vCPU not parked: create a new KVM vCPU */
> +        kvm_fd = kvm_vm_ioctl(s, KVM_CREATE_VCPU, vcpu_id);
> +        if (kvm_fd < 0) {
> +            error_report("KVM_CREATE_VCPU IOCTL failed for vCPU %lu", vcpu_id);
> +            return kvm_fd;
> +        }
> +    }
> +
> +    cpu->kvm_fd = kvm_fd;
> +    cpu->kvm_state = s;
> +    cpu->vcpu_dirty = true;
> +    cpu->dirty_pages = 0;
> +    cpu->throttle_us_per_full = 0;
> +
> +    return 0;
> +}
> +
>   static int do_kvm_destroy_vcpu(CPUState *cpu)
>   {
>       KVMState *s = kvm_state;
>       long mmap_size;
> -    struct KVMParkedVcpu *vcpu = NULL;
>       int ret = 0;
>   
> -    trace_kvm_destroy_vcpu();
> +    trace_kvm_destroy_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>   
>       ret = kvm_arch_destroy_vcpu(cpu);
>       if (ret < 0) {
> @@ -347,10 +387,7 @@ static int do_kvm_destroy_vcpu(CPUState *cpu)
>           }
>       }
>   
> -    vcpu = g_malloc0(sizeof(*vcpu));
> -    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
> -    vcpu->kvm_fd = cpu->kvm_fd;
> -    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
> +    kvm_park_vcpu(cpu);
>   err:
>       return ret;
>   }
> @@ -371,6 +408,8 @@ static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
>           if (cpu->vcpu_id == vcpu_id) {
>               int kvm_fd;
>   
> +            trace_kvm_get_vcpu(vcpu_id);
It's good if we add kvm_fd to trace.
It will be useful to cross verify kvm_get_vcpu's kvm_fd with parked vcpu.
> +
>               QLIST_REMOVE(cpu, node);
>               kvm_fd = cpu->kvm_fd;
>               g_free(cpu);
> @@ -378,7 +417,7 @@ static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
>           }
>       }
>   
> -    return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
> +    return -ENOENT;
>   }
>   
>   int kvm_init_vcpu(CPUState *cpu, Error **errp)
> @@ -389,19 +428,14 @@ int kvm_init_vcpu(CPUState *cpu, Error **errp)
>   
>       trace_kvm_init_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>   
> -    ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu));
> +    ret = kvm_create_vcpu(cpu);
>       if (ret < 0) {
> -        error_setg_errno(errp, -ret, "kvm_init_vcpu: kvm_get_vcpu failed (%lu)",
> +        error_setg_errno(errp, -ret,
> +                         "kvm_init_vcpu: kvm_create_vcpu failed (%lu)",
>                            kvm_arch_vcpu_id(cpu));
>           goto err;
>       }
>   
> -    cpu->kvm_fd = ret;
> -    cpu->kvm_state = s;
> -    cpu->vcpu_dirty = true;
> -    cpu->dirty_pages = 0;
> -    cpu->throttle_us_per_full = 0;
> -
>       mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
>       if (mmap_size < 0) {
>           ret = mmap_size;
> diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events
> index a25902597b..5558cff0dc 100644
> --- a/accel/kvm/trace-events
> +++ b/accel/kvm/trace-events
> @@ -9,6 +9,10 @@ kvm_device_ioctl(int fd, int type, void *arg) "dev fd %d, type 0x%x, arg %p"
>   kvm_failed_reg_get(uint64_t id, const char *msg) "Warning: Unable to retrieve ONEREG %" PRIu64 " from KVM: %s"
>   kvm_failed_reg_set(uint64_t id, const char *msg) "Warning: Unable to set ONEREG %" PRIu64 " to KVM: %s"
>   kvm_init_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
> +kvm_create_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
> +kvm_get_vcpu(unsigned long arch_cpu_id) "id: %lu"
> +kvm_destroy_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
> +kvm_park_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
>   kvm_irqchip_commit_routes(void) ""
>   kvm_irqchip_add_msi_route(char *name, int vector, int virq) "dev %s vector %d virq %d"
>   kvm_irqchip_update_msi_route(int virq) "Updating MSI route virq=%d"
> @@ -25,7 +29,6 @@ kvm_dirty_ring_reaper(const char *s) "%s"
>   kvm_dirty_ring_reap(uint64_t count, int64_t t) "reaped %"PRIu64" pages (took %"PRIi64" us)"
>   kvm_dirty_ring_reaper_kick(const char *reason) "%s"
>   kvm_dirty_ring_flush(int finished) "%d"
> -kvm_destroy_vcpu(void) ""
>   kvm_failed_get_vcpu_mmap_size(void) ""
>   kvm_cpu_exec(void) ""
>   kvm_interrupt_exit_request(void) ""
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index fad9a7e8ff..2ed928aa71 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -435,6 +435,22 @@ void kvm_set_sigmask_len(KVMState *s, unsigned int sigmask_len);
>   int kvm_physical_memory_addr_from_host(KVMState *s, void *ram_addr,
>                                          hwaddr *phys_addr);
>   
> +/**
> + * kvm_create_vcpu - Gets a parked KVM vCPU or creates a KVM vCPU
> + * @cpu: QOM CPUState object for which KVM vCPU has to be fetched/created.
> + *
> + * @returns: 0 when success, errno (<0) when failed.
> + */
> +int kvm_create_vcpu(CPUState *cpu);
> +
> +/**
> + * kvm_park_vcpu - Park QEMU KVM vCPU context
> + * @cpu: QOM CPUState object for which QEMU KVM vCPU context has to be parked.
> + *
> + * @returns: none
> + */
> +void kvm_park_vcpu(CPUState *cpu);
> +
>   #endif /* NEED_CPU_H */
>   
>   void kvm_cpu_synchronize_state(CPUState *cpu);

Otherwise, Looks good to me.  Feel free to add
Reviewed-by: "Vishnu Pajjuri" <vishnu@os.amperecomputing.com>

_Thanks_,

-Vishnu

[-- Attachment #2: Type: text/html, Size: 9553 bytes --]

Re: [PATCH V8 3/8] hw/acpi: Update ACPI GED framework to support vCPU Hotplug

[Vishnu Pajjuri]

[-- Attachment #1: Type: text/plain, Size: 6925 bytes --]

Hi Salil,

On 12-03-2024 07:29, Salil Mehta wrote:
> ACPI GED (as described in the ACPI 6.4 spec) uses an interrupt listed in the
> _CRS object of GED to intimate OSPM about an event. Later then demultiplexes the
> notified event by evaluating ACPI _EVT method to know the type of event. Use
> ACPI GED to also notify the guest kernel about any CPU hot(un)plug events.
>
> ACPI CPU hotplug related initialization should only happen if ACPI_CPU_HOTPLUG
> support has been enabled for particular architecture. Add cpu_hotplug_hw_init()
> stub to avoid compilation break.
>
> Co-developed-by: Keqian Zhu<zhukeqian1@huawei.com>
> Signed-off-by: Keqian Zhu<zhukeqian1@huawei.com>
> Signed-off-by: Salil Mehta<salil.mehta@huawei.com>
> Reviewed-by: Jonathan Cameron<Jonathan.Cameron@huawei.com>
> Reviewed-by: Gavin Shan<gshan@redhat.com>
> Reviewed-by: David Hildenbrand<david@redhat.com>
> Reviewed-by: Shaoqin Huang<shahuang@redhat.com>
> Tested-by: Vishnu Pajjuri<vishnu@os.amperecomputing.com>
> Tested-by: Xianglai Li<lixianglai@loongson.cn>
> Tested-by: Miguel Luis<miguel.luis@oracle.com>
> ---
>   hw/acpi/acpi-cpu-hotplug-stub.c        |  6 ++++++
>   hw/acpi/generic_event_device.c         | 17 +++++++++++++++++
>   include/hw/acpi/generic_event_device.h |  4 ++++
>   3 files changed, 27 insertions(+)
>
> diff --git a/hw/acpi/acpi-cpu-hotplug-stub.c b/hw/acpi/acpi-cpu-hotplug-stub.c
> index 3fc4b14c26..c6c61bb9cd 100644
> --- a/hw/acpi/acpi-cpu-hotplug-stub.c
> +++ b/hw/acpi/acpi-cpu-hotplug-stub.c
> @@ -19,6 +19,12 @@ void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner,
>       return;
>   }
>   
> +void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
> +                         CPUHotplugState *state, hwaddr base_addr)
> +{
> +    return;
> +}
> +
>   void acpi_cpu_ospm_status(CPUHotplugState *cpu_st, ACPIOSTInfoList ***list)
>   {
>       return;
> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> index 2d6e91b124..35a71505a5 100644
> --- a/hw/acpi/generic_event_device.c
> +++ b/hw/acpi/generic_event_device.c
> @@ -12,6 +12,7 @@
>   #include "qemu/osdep.h"
>   #include "qapi/error.h"
>   #include "hw/acpi/acpi.h"
> +#include "hw/acpi/cpu.h"
>   #include "hw/acpi/generic_event_device.h"
>   #include "hw/irq.h"
>   #include "hw/mem/pc-dimm.h"
> @@ -25,6 +26,7 @@ static const uint32_t ged_supported_events[] = {
>       ACPI_GED_MEM_HOTPLUG_EVT,
>       ACPI_GED_PWR_DOWN_EVT,
>       ACPI_GED_NVDIMM_HOTPLUG_EVT,
> +    ACPI_GED_CPU_HOTPLUG_EVT,
>   };
>   
>   /*
> @@ -234,6 +236,8 @@ static void acpi_ged_device_plug_cb(HotplugHandler *hotplug_dev,
>           } else {
>               acpi_memory_plug_cb(hotplug_dev, &s->memhp_state, dev, errp);
>           }
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> +        acpi_cpu_plug_cb(hotplug_dev, &s->cpuhp_state, dev, errp);
>       } else {
>           error_setg(errp, "virt: device plug request for unsupported device"
>                      " type: %s", object_get_typename(OBJECT(dev)));
> @@ -248,6 +252,8 @@ static void acpi_ged_unplug_request_cb(HotplugHandler *hotplug_dev,
>       if ((object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
>                          !(object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)))) {
>           acpi_memory_unplug_request_cb(hotplug_dev, &s->memhp_state, dev, errp);
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> +        acpi_cpu_unplug_request_cb(hotplug_dev, &s->cpuhp_state, dev, errp);
>       } else {
>           error_setg(errp, "acpi: device unplug request for unsupported device"
>                      " type: %s", object_get_typename(OBJECT(dev)));
> @@ -261,6 +267,8 @@ static void acpi_ged_unplug_cb(HotplugHandler *hotplug_dev,
>   
>       if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>           acpi_memory_unplug_cb(&s->memhp_state, dev, errp);
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> +        acpi_cpu_unplug_cb(&s->cpuhp_state, dev, errp);
>       } else {
>           error_setg(errp, "acpi: device unplug for unsupported device"
>                      " type: %s", object_get_typename(OBJECT(dev)));
> @@ -272,6 +280,7 @@ static void acpi_ged_ospm_status(AcpiDeviceIf *adev, ACPIOSTInfoList ***list)
>       AcpiGedState *s = ACPI_GED(adev);
>   
>       acpi_memory_ospm_status(&s->memhp_state, list);
> +    acpi_cpu_ospm_status(&s->cpuhp_state, list);
>   }
>   
>   static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
> @@ -286,6 +295,8 @@ static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
>           sel = ACPI_GED_PWR_DOWN_EVT;
>       } else if (ev & ACPI_NVDIMM_HOTPLUG_STATUS) {
>           sel = ACPI_GED_NVDIMM_HOTPLUG_EVT;
> +    } else if (ev & ACPI_CPU_HOTPLUG_STATUS) {
> +        sel = ACPI_GED_CPU_HOTPLUG_EVT;
>       } else {
>           /* Unknown event. Return without generating interrupt. */
>           warn_report("GED: Unsupported event %d. No irq injected", ev);
> @@ -400,6 +411,12 @@ static void acpi_ged_initfn(Object *obj)
>       memory_region_init_io(&ged_st->regs, obj, &ged_regs_ops, ged_st,
>                             TYPE_ACPI_GED "-regs", ACPI_GED_REG_COUNT);
>       sysbus_init_mmio(sbd, &ged_st->regs);
> +
> +    memory_region_init(&s->container_cpuhp, OBJECT(dev), "cpuhp container",
> +                       ACPI_CPU_HOTPLUG_REG_LEN);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->container_cpuhp);
same sbd can be used here instead of SYS_BUS_DEVICE(dev).
> +    cpu_hotplug_hw_init(&s->container_cpuhp, OBJECT(dev),
> +                        &s->cpuhp_state, 0);
>   }
>   
>   static void acpi_ged_class_init(ObjectClass *class, void *data)
> diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
> index ba84ce0214..90fc41cbb8 100644
> --- a/include/hw/acpi/generic_event_device.h
> +++ b/include/hw/acpi/generic_event_device.h
> @@ -60,6 +60,7 @@
>   #define HW_ACPI_GENERIC_EVENT_DEVICE_H
>   
>   #include "hw/sysbus.h"
> +#include "hw/acpi/cpu_hotplug.h"
>   #include "hw/acpi/memory_hotplug.h"
>   #include "hw/acpi/ghes.h"
>   #include "qom/object.h"
> @@ -95,6 +96,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
>   #define ACPI_GED_MEM_HOTPLUG_EVT   0x1
>   #define ACPI_GED_PWR_DOWN_EVT      0x2
>   #define ACPI_GED_NVDIMM_HOTPLUG_EVT 0x4
> +#define ACPI_GED_CPU_HOTPLUG_EVT    0x8
>   
>   typedef struct GEDState {
>       MemoryRegion evt;
> @@ -106,6 +108,8 @@ struct AcpiGedState {
>       SysBusDevice parent_obj;
>       MemHotplugState memhp_state;
>       MemoryRegion container_memhp;
> +    CPUHotplugState cpuhp_state;
> +    MemoryRegion container_cpuhp;
>       GEDState ged_state;
>       uint32_t ged_event_bitmap;
>       qemu_irq irq;

Otherwise, Looks good to me.  Feel free to add
Reviewed-by: "Vishnu Pajjuri" <vishnu@os.amperecomputing.com>

_Regards_,

-Vishnu

[-- Attachment #2: Type: text/html, Size: 8523 bytes --]

Re: [PATCH V8 7/8] gdbstub: Add helper function to unregister GDB register space

[Vishnu Pajjuri]

[-- Attachment #1: Type: text/plain, Size: 2160 bytes --]

Hi Salil,

On 12-03-2024 07:29, Salil Mehta wrote:
> Add common function to help unregister the GDB register space. This shall be
> done in context to the CPU unrealization.
>
> Signed-off-by: Salil Mehta<salil.mehta@huawei.com>
> Tested-by: Vishnu Pajjuri<vishnu@os.amperecomputing.com>
> Reviewed-by: Gavin Shan<gshan@redhat.com>
> Tested-by: Xianglai Li<lixianglai@loongson.cn>
> Tested-by: Miguel Luis<miguel.luis@oracle.com>
> Reviewed-by: Shaoqin Huang<shahuang@redhat.com>
> ---
>   gdbstub/gdbstub.c      | 12 ++++++++++++
>   include/exec/gdbstub.h |  6 ++++++
>   2 files changed, 18 insertions(+)
>
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index 17efcae0d0..a8449dc309 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -615,6 +615,18 @@ void gdb_register_coprocessor(CPUState *cpu,
>       }
>   }
>   
> +void gdb_unregister_coprocessor_all(CPUState *cpu)
> +{
> +    /*
> +     * Safe to nuke everything. GDBRegisterState::xml is static const char so
> +     * it won't be freed
> +     */
> +    g_array_free(cpu->gdb_regs, true);
> +
> +    cpu->gdb_regs = NULL;
> +    cpu->gdb_num_g_regs = 0;
Likewise, you may need to set gdb_num_regs to zero as well.
> +}
> +
>   static void gdb_process_breakpoint_remove_all(GDBProcess *p)
>   {
>       CPUState *cpu = gdb_get_first_cpu_in_process(p);
> diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
> index eb14b91139..249d4d4bc8 100644
> --- a/include/exec/gdbstub.h
> +++ b/include/exec/gdbstub.h
> @@ -49,6 +49,12 @@ void gdb_register_coprocessor(CPUState *cpu,
>                                 gdb_get_reg_cb get_reg, gdb_set_reg_cb set_reg,
>                                 const GDBFeature *feature, int g_pos);
>   
> +/**
> + * gdb_unregister_coprocessor_all() - unregisters supplemental set of registers
> + * @cpu - the CPU associated with registers
> + */
> +void gdb_unregister_coprocessor_all(CPUState *cpu);
> +
>   /**
>    * gdbserver_start: start the gdb server
>    * @port_or_device: connection spec for gdb

Otherwise, Looks good to me.  Feel free to add
Reviewed-by: "Vishnu Pajjuri" <vishnu@os.amperecomputing.com>

_Regards_,

-Vishnu

[-- Attachment #2: Type: text/html, Size: 3366 bytes --]

Re: [PATCH V8 1/8] accel/kvm: Extract common KVM vCPU {creation, parking} code

[Harsh Prateek Bora]

+ Nick

Hi Salil,
I have posted a patch [1] for ppc which based on this refactoring patch.
I see there were some comments from Vishnu on this patch.
Are we expecting any further updates on this patch before merge?

Thanks
Harsh

[1] 
https://lore.kernel.org/qemu-devel/a0f9b2fc-4c8a-4c37-bc36-26bbaa627fec@linux.ibm.com/T/#u

On 3/22/24 13:45, Harsh Prateek Bora wrote:
> + Vaibhav, Shiva
> 
> Hi Salil,
> 
> I came across your patch while trying to solve a related problem on 
> spapr. One query below ..
> 
> On 3/12/24 07:29, Salil Mehta via wrote:
>> KVM vCPU creation is done once during the vCPU realization when Qemu 
>> vCPU thread
>> is spawned. This is common to all the architectures as of now.
>>
>> Hot-unplug of vCPU results in destruction of the vCPU object in QOM 
>> but the
>> corresponding KVM vCPU object in the Host KVM is not destroyed as KVM 
>> doesn't
>> support vCPU removal. Therefore, its representative KVM vCPU 
>> object/context in
>> Qemu is parked.
>>
>> Refactor architecture common logic so that some APIs could be reused 
>> by vCPU
>> Hotplug code of some architectures likes ARM, Loongson etc. Update 
>> new/old APIs
>> with trace events instead of DPRINTF. No functional change is intended 
>> here.
>>
>> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
>> Reviewed-by: Gavin Shan <gshan@redhat.com>
>> Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Tested-by: Xianglai Li <lixianglai@loongson.cn>
>> Tested-by: Miguel Luis <miguel.luis@oracle.com>
>> Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
>> ---
>>   accel/kvm/kvm-all.c    | 64 ++++++++++++++++++++++++++++++++----------
>>   accel/kvm/trace-events |  5 +++-
>>   include/sysemu/kvm.h   | 16 +++++++++++
>>   3 files changed, 69 insertions(+), 16 deletions(-)
>>
>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>> index a8cecd040e..3bc3207bda 100644
>> --- a/accel/kvm/kvm-all.c
>> +++ b/accel/kvm/kvm-all.c
>> @@ -126,6 +126,7 @@ static QemuMutex kml_slots_lock;
>>   #define kvm_slots_unlock()  qemu_mutex_unlock(&kml_slots_lock)
>>   static void kvm_slot_init_dirty_bitmap(KVMSlot *mem);
>> +static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id);
>>   static inline void kvm_resample_fd_remove(int gsi)
>>   {
>> @@ -314,14 +315,53 @@ err:
>>       return ret;
>>   }
>> +void kvm_park_vcpu(CPUState *cpu)
>> +{
>> +    struct KVMParkedVcpu *vcpu;
>> +
>> +    trace_kvm_park_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>> +
>> +    vcpu = g_malloc0(sizeof(*vcpu));
>> +    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
>> +    vcpu->kvm_fd = cpu->kvm_fd;
>> +    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
>> +}
>> +
>> +int kvm_create_vcpu(CPUState *cpu)
>> +{
>> +    unsigned long vcpu_id = kvm_arch_vcpu_id(cpu);
>> +    KVMState *s = kvm_state;
>> +    int kvm_fd;
>> +
>> +    trace_kvm_create_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>> +
>> +    /* check if the KVM vCPU already exist but is parked */
>> +    kvm_fd = kvm_get_vcpu(s, vcpu_id);
>> +    if (kvm_fd < 0) {
>> +        /* vCPU not parked: create a new KVM vCPU */
>> +        kvm_fd = kvm_vm_ioctl(s, KVM_CREATE_VCPU, vcpu_id);
>> +        if (kvm_fd < 0) {
>> +            error_report("KVM_CREATE_VCPU IOCTL failed for vCPU %lu", 
>> vcpu_id);
>> +            return kvm_fd;
>> +        }
>> +    }
>> +
>> +    cpu->kvm_fd = kvm_fd;
>> +    cpu->kvm_state = s;
>> +    cpu->vcpu_dirty = true;
>> +    cpu->dirty_pages = 0;
>> +    cpu->throttle_us_per_full = 0;
>> +
>> +    return 0;
>> +}
>> +
>>   static int do_kvm_destroy_vcpu(CPUState *cpu)
>>   {
>>       KVMState *s = kvm_state;
>>       long mmap_size;
>> -    struct KVMParkedVcpu *vcpu = NULL;
>>       int ret = 0;
>> -    trace_kvm_destroy_vcpu();
>> +    trace_kvm_destroy_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>>       ret = kvm_arch_destroy_vcpu(cpu);
>>       if (ret < 0) {
>> @@ -347,10 +387,7 @@ static int do_kvm_destroy_vcpu(CPUState *cpu)
>>           }
>>       }
>> -    vcpu = g_malloc0(sizeof(*vcpu));
>> -    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
>> -    vcpu->kvm_fd = cpu->kvm_fd;
>> -    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
>> +    kvm_park_vcpu(cpu);
>>   err:
>>       return ret;
>>   }
>> @@ -371,6 +408,8 @@ static int kvm_get_vcpu(KVMState *s, unsigned long 
>> vcpu_id)
>>           if (cpu->vcpu_id == vcpu_id) {
>>               int kvm_fd;
>> +            trace_kvm_get_vcpu(vcpu_id);
>> +
>>               QLIST_REMOVE(cpu, node);
>>               kvm_fd = cpu->kvm_fd;
>>               g_free(cpu);
>> @@ -378,7 +417,7 @@ static int kvm_get_vcpu(KVMState *s, unsigned long 
>> vcpu_id)
>>           }
>>       }
>> -    return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
>> +    return -ENOENT;
>>   }
>>   int kvm_init_vcpu(CPUState *cpu, Error **errp)
>> @@ -389,19 +428,14 @@ int kvm_init_vcpu(CPUState *cpu, Error **errp)
>>       trace_kvm_init_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>> -    ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu));
>> +    ret = kvm_create_vcpu(cpu);
>>       if (ret < 0) {
>> -        error_setg_errno(errp, -ret, "kvm_init_vcpu: kvm_get_vcpu 
>> failed (%lu)",
>> +        error_setg_errno(errp, -ret,
>> +                         "kvm_init_vcpu: kvm_create_vcpu failed (%lu)",
>>                            kvm_arch_vcpu_id(cpu));
> 
> If a vcpu hotplug fails due to failure with kvm_create_vcpu ioctl,
> current behaviour would be to bring down the guest as errp is
> &error_fatal. Any thoughts on how do we ensure that a failure with
> kvm_create_vcpu ioctl for hotplugged cpus (only) doesnt bring down the
> guest and fail gracefully (by reporting error to user on monitor?)?
> 
> regards,
> Harsh
>>           goto err;
>>       }
>> -    cpu->kvm_fd = ret;
>> -    cpu->kvm_state = s;
>> -    cpu->vcpu_dirty = true;
>> -    cpu->dirty_pages = 0;
>> -    cpu->throttle_us_per_full = 0;
>> -
>>       mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
>>       if (mmap_size < 0) {
>>           ret = mmap_size;
>> diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events
>> index a25902597b..5558cff0dc 100644
>> --- a/accel/kvm/trace-events
>> +++ b/accel/kvm/trace-events
>> @@ -9,6 +9,10 @@ kvm_device_ioctl(int fd, int type, void *arg) "dev fd 
>> %d, type 0x%x, arg %p"
>>   kvm_failed_reg_get(uint64_t id, const char *msg) "Warning: Unable to 
>> retrieve ONEREG %" PRIu64 " from KVM: %s"
>>   kvm_failed_reg_set(uint64_t id, const char *msg) "Warning: Unable to 
>> set ONEREG %" PRIu64 " to KVM: %s"
>>   kvm_init_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d 
>> id: %lu"
>> +kvm_create_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d 
>> id: %lu"
>> +kvm_get_vcpu(unsigned long arch_cpu_id) "id: %lu"
>> +kvm_destroy_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d 
>> id: %lu"
>> +kvm_park_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d 
>> id: %lu"
>>   kvm_irqchip_commit_routes(void) ""
>>   kvm_irqchip_add_msi_route(char *name, int vector, int virq) "dev %s 
>> vector %d virq %d"
>>   kvm_irqchip_update_msi_route(int virq) "Updating MSI route virq=%d"
>> @@ -25,7 +29,6 @@ kvm_dirty_ring_reaper(const char *s) "%s"
>>   kvm_dirty_ring_reap(uint64_t count, int64_t t) "reaped %"PRIu64" 
>> pages (took %"PRIi64" us)"
>>   kvm_dirty_ring_reaper_kick(const char *reason) "%s"
>>   kvm_dirty_ring_flush(int finished) "%d"
>> -kvm_destroy_vcpu(void) ""
>>   kvm_failed_get_vcpu_mmap_size(void) ""
>>   kvm_cpu_exec(void) ""
>>   kvm_interrupt_exit_request(void) ""
>> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
>> index fad9a7e8ff..2ed928aa71 100644
>> --- a/include/sysemu/kvm.h
>> +++ b/include/sysemu/kvm.h
>> @@ -435,6 +435,22 @@ void kvm_set_sigmask_len(KVMState *s, unsigned 
>> int sigmask_len);
>>   int kvm_physical_memory_addr_from_host(KVMState *s, void *ram_addr,
>>                                          hwaddr *phys_addr);
>> +/**
>> + * kvm_create_vcpu - Gets a parked KVM vCPU or creates a KVM vCPU
>> + * @cpu: QOM CPUState object for which KVM vCPU has to be 
>> fetched/created.
>> + *
>> + * @returns: 0 when success, errno (<0) when failed.
>> + */
>> +int kvm_create_vcpu(CPUState *cpu);
>> +
>> +/**
>> + * kvm_park_vcpu - Park QEMU KVM vCPU context
>> + * @cpu: QOM CPUState object for which QEMU KVM vCPU context has to 
>> be parked.
>> + *
>> + * @returns: none
>> + */
>> +void kvm_park_vcpu(CPUState *cpu);
>> +
>>   #endif /* NEED_CPU_H */
>>   void kvm_cpu_synchronize_state(CPUState *cpu);

Re: [PATCH V8 1/8] accel/kvm: Extract common KVM vCPU {creation,parking} code

[Philippe Mathieu-Daudé]

Hi Salil,

On 12/3/24 02:59, Salil Mehta wrote:
> KVM vCPU creation is done once during the vCPU realization when Qemu vCPU thread
> is spawned. This is common to all the architectures as of now.
> 
> Hot-unplug of vCPU results in destruction of the vCPU object in QOM but the
> corresponding KVM vCPU object in the Host KVM is not destroyed as KVM doesn't
> support vCPU removal. Therefore, its representative KVM vCPU object/context in
> Qemu is parked.
> 
> Refactor architecture common logic so that some APIs could be reused by vCPU
> Hotplug code of some architectures likes ARM, Loongson etc. Update new/old APIs
> with trace events instead of DPRINTF. No functional change is intended here.
> 
> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> Reviewed-by: Gavin Shan <gshan@redhat.com>
> Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Tested-by: Xianglai Li <lixianglai@loongson.cn>
> Tested-by: Miguel Luis <miguel.luis@oracle.com>
> Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
> ---
>   accel/kvm/kvm-all.c    | 64 ++++++++++++++++++++++++++++++++----------
>   accel/kvm/trace-events |  5 +++-
>   include/sysemu/kvm.h   | 16 +++++++++++
>   3 files changed, 69 insertions(+), 16 deletions(-)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index a8cecd040e..3bc3207bda 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -126,6 +126,7 @@ static QemuMutex kml_slots_lock;
>   #define kvm_slots_unlock()  qemu_mutex_unlock(&kml_slots_lock)
>   
>   static void kvm_slot_init_dirty_bitmap(KVMSlot *mem);
> +static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id);
>   
>   static inline void kvm_resample_fd_remove(int gsi)
>   {
> @@ -314,14 +315,53 @@ err:
>       return ret;
>   }
>   
> +void kvm_park_vcpu(CPUState *cpu)
> +{
> +    struct KVMParkedVcpu *vcpu;
> +
> +    trace_kvm_park_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
> +
> +    vcpu = g_malloc0(sizeof(*vcpu));
> +    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
> +    vcpu->kvm_fd = cpu->kvm_fd;
> +    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
> +}
> +
> +int kvm_create_vcpu(CPUState *cpu)
> +{
> +    unsigned long vcpu_id = kvm_arch_vcpu_id(cpu);
> +    KVMState *s = kvm_state;
> +    int kvm_fd;
> +
> +    trace_kvm_create_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
> +
> +    /* check if the KVM vCPU already exist but is parked */
> +    kvm_fd = kvm_get_vcpu(s, vcpu_id);
> +    if (kvm_fd < 0) {
> +        /* vCPU not parked: create a new KVM vCPU */
> +        kvm_fd = kvm_vm_ioctl(s, KVM_CREATE_VCPU, vcpu_id);
> +        if (kvm_fd < 0) {
> +            error_report("KVM_CREATE_VCPU IOCTL failed for vCPU %lu", vcpu_id);
> +            return kvm_fd;
> +        }
> +    }
> +
> +    cpu->kvm_fd = kvm_fd;
> +    cpu->kvm_state = s;
> +    cpu->vcpu_dirty = true;
> +    cpu->dirty_pages = 0;
> +    cpu->throttle_us_per_full = 0;
> +
> +    return 0;
> +}

This seems generic enough to be implemented for all accelerators.

See AccelOpsClass in include/sysemu/accel-ops.h.

That said, can be done later on top.

RE: [PATCH V8 1/8] accel/kvm: Extract common KVM vCPU {creation,parking} code

[Salil Mehta via]

Hi Philippe,

>  From: Philippe Mathieu-Daudé <philmd@linaro.org>
>  Sent: Friday, May 3, 2024 10:40 AM
>  Subject: Re: [PATCH V8 1/8] accel/kvm: Extract common KVM vCPU
>  {creation,parking} code
>  
>  Hi Salil,
>  
>  On 12/3/24 02:59, Salil Mehta wrote:
>  > KVM vCPU creation is done once during the vCPU realization when Qemu
>  > vCPU thread is spawned. This is common to all the architectures as of now.
>  >
>  > Hot-unplug of vCPU results in destruction of the vCPU object in QOM
>  > but the corresponding KVM vCPU object in the Host KVM is not destroyed
>  > as KVM doesn't support vCPU removal. Therefore, its representative KVM
>  > vCPU object/context in Qemu is parked.
>  >
>  > Refactor architecture common logic so that some APIs could be reused
>  > by vCPU Hotplug code of some architectures likes ARM, Loongson etc.
>  > Update new/old APIs with trace events instead of DPRINTF. No functional
>  change is intended here.
>  >
>  > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
>  > Reviewed-by: Gavin Shan <gshan@redhat.com>
>  > Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
>  > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>  > Tested-by: Xianglai Li <lixianglai@loongson.cn>
>  > Tested-by: Miguel Luis <miguel.luis@oracle.com>
>  > Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
>  > ---
>  >   accel/kvm/kvm-all.c    | 64 ++++++++++++++++++++++++++++++++------
>  ----
>  >   accel/kvm/trace-events |  5 +++-
>  >   include/sysemu/kvm.h   | 16 +++++++++++
>  >   3 files changed, 69 insertions(+), 16 deletions(-)
>  >
>  > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index
>  > a8cecd040e..3bc3207bda 100644
>  > --- a/accel/kvm/kvm-all.c
>  > +++ b/accel/kvm/kvm-all.c
>  > @@ -126,6 +126,7 @@ static QemuMutex kml_slots_lock;
>  >   #define kvm_slots_unlock()  qemu_mutex_unlock(&kml_slots_lock)
>  >
>  >   static void kvm_slot_init_dirty_bitmap(KVMSlot *mem);
>  > +static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id);
>  >
>  >   static inline void kvm_resample_fd_remove(int gsi)
>  >   {
>  > @@ -314,14 +315,53 @@ err:
>  >       return ret;
>  >   }
>  >
>  > +void kvm_park_vcpu(CPUState *cpu)
>  > +{
>  > +    struct KVMParkedVcpu *vcpu;
>  > +
>  > +    trace_kvm_park_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>  > +
>  > +    vcpu = g_malloc0(sizeof(*vcpu));
>  > +    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
>  > +    vcpu->kvm_fd = cpu->kvm_fd;
>  > +    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node); }
>  > +
>  > +int kvm_create_vcpu(CPUState *cpu)
>  > +{
>  > +    unsigned long vcpu_id = kvm_arch_vcpu_id(cpu);
>  > +    KVMState *s = kvm_state;
>  > +    int kvm_fd;
>  > +
>  > +    trace_kvm_create_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>  > +
>  > +    /* check if the KVM vCPU already exist but is parked */
>  > +    kvm_fd = kvm_get_vcpu(s, vcpu_id);
>  > +    if (kvm_fd < 0) {
>  > +        /* vCPU not parked: create a new KVM vCPU */
>  > +        kvm_fd = kvm_vm_ioctl(s, KVM_CREATE_VCPU, vcpu_id);
>  > +        if (kvm_fd < 0) {
>  > +            error_report("KVM_CREATE_VCPU IOCTL failed for vCPU %lu",  vcpu_id);
>  > +            return kvm_fd;
>  > +        }
>  > +    }
>  > +
>  > +    cpu->kvm_fd = kvm_fd;
>  > +    cpu->kvm_state = s;
>  > +    cpu->vcpu_dirty = true;
>  > +    cpu->dirty_pages = 0;
>  > +    cpu->throttle_us_per_full = 0;
>  > +
>  > +    return 0;
>  > +}
>  
>  This seems generic enough to be implemented for all accelerators.
>  
>  See AccelOpsClass in include/sysemu/accel-ops.h.
>  
>  That said, can be done later on top.

Let me understand correctly. Are you suggesting to implement above even for
HVF, TCG, QTEST etc?

Thanks
Salil.

RE: [PATCH V8 1/8] accel/kvm: Extract common KVM vCPU {creation,parking} code

[Salil Mehta via]

Hi Vishnu,

>  From: Vishnu Pajjuri <vishnu@amperemail.onmicrosoft.com> 
>  Sent: Thursday, April 4, 2024 3:00 PM
>  Subject: Re: [PATCH V8 1/8] accel/kvm: Extract common KVM vCPU {creation,parking} code
>  
>  Hi Salil,
>>  On 12-03-2024 07:29, Salil Mehta wrote:
>>  KVM vCPU creation is done once during the vCPU realization when Qemu vCPU thread
>>  is spawned. This is common to all the architectures as of now.
>>  
>>  Hot-unplug of vCPU results in destruction of the vCPU object in QOM but the
>>  corresponding KVM vCPU object in the Host KVM is not destroyed as KVM doesn't
>>  support vCPU removal. Therefore, its representative KVM vCPU object/context in
>>  Qemu is parked.
>>  
>>  Refactor architecture common logic so that some APIs could be reused by vCPU
>>  Hotplug code of some architectures likes ARM, Loongson etc. Update new/old APIs
>>  with trace events instead of DPRINTF. No functional change is intended here.
>>  
>>  Signed-off-by: Salil Mehta mailto:salil.mehta@huawei.com
>>  Reviewed-by: Gavin Shan mailto:gshan@redhat.com
>>  Tested-by: Vishnu Pajjuri mailto:vishnu@os.amperecomputing.com
>>  Reviewed-by: Jonathan Cameron mailto:Jonathan.Cameron@huawei.com
>>  Tested-by: Xianglai Li mailto:lixianglai@loongson.cn
>>  Tested-by: Miguel Luis mailto:miguel.luis@oracle.com
>>  Reviewed-by: Shaoqin Huang mailto:shahuang@redhat.com
>>  ---
>>   accel/kvm/kvm-all.c    | 64 ++++++++++++++++++++++++++++++++----------
>>   accel/kvm/trace-events |  5 +++-
>>   include/sysemu/kvm.h   | 16 +++++++++++
>>   3 files changed, 69 insertions(+), 16 deletions(-)
>>  
>>  diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>>  index a8cecd040e..3bc3207bda 100644
>>  --- a/accel/kvm/kvm-all.c
>>  +++ b/accel/kvm/kvm-all.c
>>  @@ -126,6 +126,7 @@ static QemuMutex kml_slots_lock;
>>   #define kvm_slots_unlock()  qemu_mutex_unlock(&kml_slots_lock)
>>   
>>   static void kvm_slot_init_dirty_bitmap(KVMSlot *mem);
>>  +static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id);
>>   
>>   static inline void kvm_resample_fd_remove(int gsi)
>>   {
>>  @@ -314,14 +315,53 @@ err:
>>  return ret;
>>   }
>>   
>>  +void kvm_park_vcpu(CPUState *cpu)
>>  +{
>>  +    struct KVMParkedVcpu *vcpu;
>>  +
>>  +    trace_kvm_park_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>  It's good if we add kvm_fd to trace.
>  It will be useful to cross verify kvm_get_vcpu()'s kvm_fd with parked vcpu.


Agreed. But this is currently called in context to create and destroy vCPU
where the trace already exists with the info you are seeking. Having
trace here might duplicate the info and end up increasing the noise.

Let me know if you think otherwise or have something else to add.

Thanks

 
>>  +
>>  +    vcpu = g_malloc0(sizeof(*vcpu));
>>  +    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
>>  +    vcpu->kvm_fd = cpu->kvm_fd;
>>  +    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
>>  +}
>>  +
>>  +int kvm_create_vcpu(CPUState *cpu)
>>  +{
>>  +    unsigned long vcpu_id = kvm_arch_vcpu_id(cpu);
>>  +    KVMState *s = kvm_state;
>>  +    int kvm_fd;
>>  +
>>  +    trace_kvm_create_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>  vcpu_id can be used instead of kvm_arch_vcpu_id(cpu).


KVM arch VCPU Id ensures that ID being traced is meaningful for that
architecture. The way CPU ID gets calculated in on different architectures
could be different. Hence, its value might be quite different.

  
>>  +
>>  +    /* check if the KVM vCPU already exist but is parked */
>>  +    kvm_fd = kvm_get_vcpu(s, vcpu_id);
>>  +    if (kvm_fd < 0) {
>>  +>   /* vCPU not parked: create a new KVM vCPU */
>>  +>   kvm_fd = kvm_vm_ioctl(s, KVM_CREATE_VCPU, vcpu_id);
>>  +>   if (kvm_fd < 0) {
>>  +>       error_report("KVM_CREATE_VCPU IOCTL failed for vCPU %lu", vcpu_id);
>>  +>       return kvm_fd;
>>  +>   }
>>  +    }
>>  +
>>  +    cpu->kvm_fd = kvm_fd;
>>  +    cpu->kvm_state = s;
>>  +    cpu->vcpu_dirty = true;
>>  +    cpu->dirty_pages = 0;
>>  +    cpu->throttle_us_per_full = 0;
>>  +
>>  +    return 0;
>>  +}
>>  +
>>   static int do_kvm_destroy_vcpu(CPUState *cpu)
>>   {
>>       KVMState *s = kvm_state;
>>       long mmap_size;
>>  -    struct KVMParkedVcpu *vcpu = NULL;
>>       int ret = 0;
>>   
>>  -    trace_kvm_destroy_vcpu();
>>  +    trace_kvm_destroy_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>>   
>>       ret = kvm_arch_destroy_vcpu(cpu);
>>       if (ret < 0) {
>>  @@ -347,10 +387,7 @@ static int do_kvm_destroy_vcpu(CPUState *cpu)
>>  >    }
>>       }
>>   
>>  -    vcpu = g_malloc0(sizeof(*vcpu));
>>  -    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
>>  -    vcpu->kvm_fd = cpu->kvm_fd;
>>  -    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
>>  +    kvm_park_vcpu(cpu);
>>   err:
>>       return ret;
>>   }
>>  @@ -371,6 +408,8 @@ static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
>>  >    if (cpu->vcpu_id == vcpu_id) {
>>  >        int kvm_fd;
>>   
>>  +>       trace_kvm_get_vcpu(vcpu_id);
>  It's good if we add kvm_fd to trace.
>  It will be useful to cross verify kvm_get_vcpu's kvm_fd with parked vcpu.


I can but I'm wondering why you've raised this? Perhaps, I'm not aware of the
interface you are using to configure the VMs and how traces across diferent
VMs get reflected. Please help in my understanding.


>>  +
>>  >        QLIST_REMOVE(cpu, node);
>>  >        kvm_fd = cpu->kvm_fd;
>>  >        g_free(cpu);
>>  @@ -378,7 +417,7 @@ static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
>>  >    }
>>       }
>>   
>>  -    return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
>>  +    return -ENOENT;
>>   }
>>   
>>   int kvm_init_vcpu(CPUState *cpu, Error **errp)
>>  @@ -389,19 +428,14 @@ int kvm_init_vcpu(CPUState *cpu, Error **errp)
>>   
>>       trace_kvm_init_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>>   
>>  -    ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu));
>>  +    ret = kvm_create_vcpu(cpu);
>>       if (ret < 0) {
>>  -   error_setg_errno(errp, -ret, "kvm_init_vcpu: kvm_get_vcpu failed (%lu)",
>>  +   error_setg_errno(errp, -ret,
>>  +          "kvm_init_vcpu: kvm_create_vcpu failed (%lu)",
>             kvm_arch_vcpu_id(cpu));
>>      goto err;
>>       }
>>   
>>  -    cpu->kvm_fd = ret;
>>  -    cpu->kvm_state = s;
>>  -    cpu->vcpu_dirty = true;
>>  -    cpu->dirty_pages = 0;
>>  -    cpu->throttle_us_per_full = 0;
>>  -
>>       mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
>>       if (mmap_size < 0) {
>>      ret = mmap_size;
>>  diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events
>>  index a25902597b..5558cff0dc 100644
>>  --- a/accel/kvm/trace-events
>>  +++ b/accel/kvm/trace-events
>>  @@ -9,6 +9,10 @@ kvm_device_ioctl(int fd, int type, void *arg) "dev fd %d, type 0x%x, arg %p"
>>   kvm_failed_reg_get(uint64_t id, const char *msg) "Warning: Unable to retrieve ONEREG %" PRIu64 " from KVM: %s"
>>   kvm_failed_reg_set(uint64_t id, const char *msg) "Warning: Unable to set ONEREG %" PRIu64 " to KVM: %s"
>>   kvm_init_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
>>  +kvm_create_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
>>  +kvm_get_vcpu(unsigned long arch_cpu_id) "id: %lu"
>>  +kvm_destroy_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
>>  +kvm_park_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
>>   kvm_irqchip_commit_routes(void) ""
>>   kvm_irqchip_add_msi_route(char *name, int vector, int virq) "dev %s vector %d virq %d"
>>   kvm_irqchip_update_msi_route(int virq) "Updating MSI route virq=%d"
>>  @@ -25,7 +29,6 @@ kvm_dirty_ring_reaper(const char *s) "%s"
>>   kvm_dirty_ring_reap(uint64_t count, int64_t t) "reaped %"PRIu64" pages (took %"PRIi64" us)"
>>   kvm_dirty_ring_reaper_kick(const char *reason) "%s"
>>   kvm_dirty_ring_flush(int finished) "%d"
>>  -kvm_destroy_vcpu(void) ""
>>   kvm_failed_get_vcpu_mmap_size(void) ""
>>   kvm_cpu_exec(void) ""
>>   kvm_interrupt_exit_request(void) ""
>>  diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
>>  index fad9a7e8ff..2ed928aa71 100644
>>  --- a/include/sysemu/kvm.h
>>  +++ b/include/sysemu/kvm.h
>>  @@ -435,6 +435,22 @@ void kvm_set_sigmask_len(KVMState *s, unsigned int sigmask_len);
>>   int kvm_physical_memory_addr_from_host(KVMState *s, void *ram_addr,
>>  >  >  >  >  >       hwaddr *phys_addr);
>>   
>>  +/**
>>  + * kvm_create_vcpu - Gets a parked KVM vCPU or creates a KVM vCPU
>>  + * @cpu: QOM CPUState object for which KVM vCPU has to be fetched/created.
>>  + *
>>  + * @returns: 0 when success, errno (<0) when failed.
>>  + */
>>  +int kvm_create_vcpu(CPUState *cpu);
>>  +
>>  +/**
>>  + * kvm_park_vcpu - Park QEMU KVM vCPU context
>>  + * @cpu: QOM CPUState object for which QEMU KVM vCPU context has to be parked.
>>  + *
>>  + * @returns: none
>>  + */
>>  +void kvm_park_vcpu(CPUState *cpu);
>>  +
>>   #endif /* NEED_CPU_H */
>>   
>>   void kvm_cpu_synchronize_state(CPUState *cpu);
>  Otherwise, Looks good to me.  Feel free to add
>  Reviewed-by: "Vishnu Pajjuri" mailto:vishnu@os.amperecomputing.com
>  Thanks,

Thanks.
Salil



>  -Vishnu

Re: [PATCH V8 1/8] accel/kvm: Extract common KVM vCPU {creation,parking} code

[Philippe Mathieu-Daudé]

On 3/5/24 17:57, Salil Mehta wrote:
> Hi Philippe,
> 
>>   From: Philippe Mathieu-Daudé <philmd@linaro.org>
>>   Sent: Friday, May 3, 2024 10:40 AM
>>   Subject: Re: [PATCH V8 1/8] accel/kvm: Extract common KVM vCPU
>>   {creation,parking} code
>>   
>>   Hi Salil,
>>   
>>   On 12/3/24 02:59, Salil Mehta wrote:
>>   > KVM vCPU creation is done once during the vCPU realization when Qemu
>>   > vCPU thread is spawned. This is common to all the architectures as of now.
>>   >
>>   > Hot-unplug of vCPU results in destruction of the vCPU object in QOM
>>   > but the corresponding KVM vCPU object in the Host KVM is not destroyed
>>   > as KVM doesn't support vCPU removal. Therefore, its representative KVM
>>   > vCPU object/context in Qemu is parked.
>>   >
>>   > Refactor architecture common logic so that some APIs could be reused
>>   > by vCPU Hotplug code of some architectures likes ARM, Loongson etc.
>>   > Update new/old APIs with trace events instead of DPRINTF. No functional
>>   change is intended here.
>>   >
>>   > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
>>   > Reviewed-by: Gavin Shan <gshan@redhat.com>
>>   > Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
>>   > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>   > Tested-by: Xianglai Li <lixianglai@loongson.cn>
>>   > Tested-by: Miguel Luis <miguel.luis@oracle.com>
>>   > Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
>>   > ---
>>   >   accel/kvm/kvm-all.c    | 64 ++++++++++++++++++++++++++++++++------
>>   ----
>>   >   accel/kvm/trace-events |  5 +++-
>>   >   include/sysemu/kvm.h   | 16 +++++++++++
>>   >   3 files changed, 69 insertions(+), 16 deletions(-)
>>   >
>>   > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index
>>   > a8cecd040e..3bc3207bda 100644
>>   > --- a/accel/kvm/kvm-all.c
>>   > +++ b/accel/kvm/kvm-all.c
>>   > @@ -126,6 +126,7 @@ static QemuMutex kml_slots_lock;
>>   >   #define kvm_slots_unlock()  qemu_mutex_unlock(&kml_slots_lock)
>>   >
>>   >   static void kvm_slot_init_dirty_bitmap(KVMSlot *mem);
>>   > +static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id);
>>   >
>>   >   static inline void kvm_resample_fd_remove(int gsi)
>>   >   {
>>   > @@ -314,14 +315,53 @@ err:
>>   >       return ret;
>>   >   }
>>   >
>>   > +void kvm_park_vcpu(CPUState *cpu)
>>   > +{
>>   > +    struct KVMParkedVcpu *vcpu;
>>   > +
>>   > +    trace_kvm_park_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>>   > +
>>   > +    vcpu = g_malloc0(sizeof(*vcpu));
>>   > +    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
>>   > +    vcpu->kvm_fd = cpu->kvm_fd;
>>   > +    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node); }
>>   > +
>>   > +int kvm_create_vcpu(CPUState *cpu)
>>   > +{
>>   > +    unsigned long vcpu_id = kvm_arch_vcpu_id(cpu);
>>   > +    KVMState *s = kvm_state;
>>   > +    int kvm_fd;
>>   > +
>>   > +    trace_kvm_create_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>>   > +
>>   > +    /* check if the KVM vCPU already exist but is parked */
>>   > +    kvm_fd = kvm_get_vcpu(s, vcpu_id);
>>   > +    if (kvm_fd < 0) {
>>   > +        /* vCPU not parked: create a new KVM vCPU */
>>   > +        kvm_fd = kvm_vm_ioctl(s, KVM_CREATE_VCPU, vcpu_id);
>>   > +        if (kvm_fd < 0) {
>>   > +            error_report("KVM_CREATE_VCPU IOCTL failed for vCPU %lu",  vcpu_id);
>>   > +            return kvm_fd;
>>   > +        }
>>   > +    }
>>   > +
>>   > +    cpu->kvm_fd = kvm_fd;
>>   > +    cpu->kvm_state = s;
>>   > +    cpu->vcpu_dirty = true;
>>   > +    cpu->dirty_pages = 0;
>>   > +    cpu->throttle_us_per_full = 0;
>>   > +
>>   > +    return 0;
>>   > +}
>>   
>>   This seems generic enough to be implemented for all accelerators.
>>   
>>   See AccelOpsClass in include/sysemu/accel-ops.h.
>>   
>>   That said, can be done later on top.
> 
> Let me understand correctly. Are you suggesting to implement above even for
> HVF, TCG, QTEST etc?

Not for you to implement the other non-KVM accelerators, but since
you are introducing this, now is a good time to think about a generic
interface.

So far AccelOpsClass::[un]park_vcpu() handlers make sense to me.

> Thanks
> Salil.
> 
> 
> 
> 

RE: [PATCH V8 1/8] accel/kvm: Extract common KVM vCPU {creation, parking} code

[Salil Mehta via]

Hi Harsh,

Sorry for the delay in my reply. I've been off the grid for some time so missed this
earlier mail. Please find my reply below to you query.

Thanks

>  From: Harsh Prateek Bora <harshpb@linux.ibm.com>
>  Sent: Friday, March 22, 2024 8:15 AM
>  
>  + Vaibhav, Shiva
>  
>  Hi Salil,
>  
>  I came across your patch while trying to solve a related problem on spapr.
>  One query below ..
>  
>  On 3/12/24 07:29, Salil Mehta via wrote:
>  > KVM vCPU creation is done once during the vCPU realization when Qemu
>  > vCPU thread is spawned. This is common to all the architectures as of now.
>  >
>  > Hot-unplug of vCPU results in destruction of the vCPU object in QOM
>  > but the corresponding KVM vCPU object in the Host KVM is not destroyed
>  > as KVM doesn't support vCPU removal. Therefore, its representative KVM
>  > vCPU object/context in Qemu is parked.
>  >
>  > Refactor architecture common logic so that some APIs could be reused
>  > by vCPU Hotplug code of some architectures likes ARM, Loongson etc.
>  > Update new/old APIs with trace events instead of DPRINTF. No functional
>  change is intended here.
>  >
>  > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
>  > Reviewed-by: Gavin Shan <gshan@redhat.com>
>  > Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
>  > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>  > Tested-by: Xianglai Li <lixianglai@loongson.cn>
>  > Tested-by: Miguel Luis <miguel.luis@oracle.com>
>  > Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
>  > ---
>  >   accel/kvm/kvm-all.c    | 64 ++++++++++++++++++++++++++++++++------
>  ----
>  >   accel/kvm/trace-events |  5 +++-
>  >   include/sysemu/kvm.h   | 16 +++++++++++
>  >   3 files changed, 69 insertions(+), 16 deletions(-)
>  >
>  > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index
>  > a8cecd040e..3bc3207bda 100644
>  > --- a/accel/kvm/kvm-all.c
>  > +++ b/accel/kvm/kvm-all.c
>  > @@ -126,6 +126,7 @@ static QemuMutex kml_slots_lock;
>  >   #define kvm_slots_unlock()  qemu_mutex_unlock(&kml_slots_lock)
>  >
>  >   static void kvm_slot_init_dirty_bitmap(KVMSlot *mem);
>  > +static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id);
>  >
>  >   static inline void kvm_resample_fd_remove(int gsi)
>  >   {
>  > @@ -314,14 +315,53 @@ err:
>  >       return ret;
>  >   }
>  >
>  > +void kvm_park_vcpu(CPUState *cpu)
>  > +{
>  > +    struct KVMParkedVcpu *vcpu;
>  > +
>  > +    trace_kvm_park_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>  > +
>  > +    vcpu = g_malloc0(sizeof(*vcpu));
>  > +    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
>  > +    vcpu->kvm_fd = cpu->kvm_fd;
>  > +    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node); }
>  > +
>  > +int kvm_create_vcpu(CPUState *cpu)
>  > +{
>  > +    unsigned long vcpu_id = kvm_arch_vcpu_id(cpu);
>  > +    KVMState *s = kvm_state;
>  > +    int kvm_fd;
>  > +
>  > +    trace_kvm_create_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>  > +
>  > +    /* check if the KVM vCPU already exist but is parked */
>  > +    kvm_fd = kvm_get_vcpu(s, vcpu_id);
>  > +    if (kvm_fd < 0) {
>  > +        /* vCPU not parked: create a new KVM vCPU */
>  > +        kvm_fd = kvm_vm_ioctl(s, KVM_CREATE_VCPU, vcpu_id);
>  > +        if (kvm_fd < 0) {
>  > +            error_report("KVM_CREATE_VCPU IOCTL failed for vCPU %lu",
>  vcpu_id);
>  > +            return kvm_fd;
>  > +        }
>  > +    }
>  > +
>  > +    cpu->kvm_fd = kvm_fd;
>  > +    cpu->kvm_state = s;
>  > +    cpu->vcpu_dirty = true;
>  > +    cpu->dirty_pages = 0;
>  > +    cpu->throttle_us_per_full = 0;
>  > +
>  > +    return 0;
>  > +}
>  > +
>  >   static int do_kvm_destroy_vcpu(CPUState *cpu)
>  >   {
>  >       KVMState *s = kvm_state;
>  >       long mmap_size;
>  > -    struct KVMParkedVcpu *vcpu = NULL;
>  >       int ret = 0;
>  >
>  > -    trace_kvm_destroy_vcpu();
>  > +    trace_kvm_destroy_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>  >
>  >       ret = kvm_arch_destroy_vcpu(cpu);
>  >       if (ret < 0) {
>  > @@ -347,10 +387,7 @@ static int do_kvm_destroy_vcpu(CPUState *cpu)
>  >           }
>  >       }
>  >
>  > -    vcpu = g_malloc0(sizeof(*vcpu));
>  > -    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
>  > -    vcpu->kvm_fd = cpu->kvm_fd;
>  > -    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
>  > +    kvm_park_vcpu(cpu);
>  >   err:
>  >       return ret;
>  >   }
>  > @@ -371,6 +408,8 @@ static int kvm_get_vcpu(KVMState *s, unsigned
>  long vcpu_id)
>  >           if (cpu->vcpu_id == vcpu_id) {
>  >               int kvm_fd;
>  >
>  > +            trace_kvm_get_vcpu(vcpu_id);
>  > +
>  >               QLIST_REMOVE(cpu, node);
>  >               kvm_fd = cpu->kvm_fd;
>  >               g_free(cpu);
>  > @@ -378,7 +417,7 @@ static int kvm_get_vcpu(KVMState *s, unsigned
>  long vcpu_id)
>  >           }
>  >       }
>  >
>  > -    return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
>  > +    return -ENOENT;
>  >   }
>  >
>  >   int kvm_init_vcpu(CPUState *cpu, Error **errp) @@ -389,19 +428,14 @@
>  > int kvm_init_vcpu(CPUState *cpu, Error **errp)
>  >
>  >       trace_kvm_init_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>  >
>  > -    ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu));
>  > +    ret = kvm_create_vcpu(cpu);
>  >       if (ret < 0) {
>  > -        error_setg_errno(errp, -ret, "kvm_init_vcpu: kvm_get_vcpu failed
>  (%lu)",
>  > +        error_setg_errno(errp, -ret,
>  > +                         "kvm_init_vcpu: kvm_create_vcpu failed
>  > + (%lu)",
>  >                            kvm_arch_vcpu_id(cpu));
>  
>  If a vcpu hotplug fails due to failure with kvm_create_vcpu ioctl, current
>  behaviour would be to bring down the guest as errp is &error_fatal. Any
>  thoughts on how do we ensure that a failure with kvm_create_vcpu ioctl for
>  hotplugged cpus (only) doesnt bring down the guest and fail gracefully (by
>  reporting error to user on monitor?)?

In the ARM, we are by design pre-creating all the vCPUs in the KVM during the
Qemu/KVM Init. This is to satisfy the constraints posed by ARM architecture
as we are not allowed to meddle with any initialization at KVM level or Guest
kernel level after system has booted. The constraints are mainly coming from
GIC and related per-CPU features which can only be initialized once during init
in the KVM and then their presence is made to felt to the Guest kernel only
once during enumeration of the CPUs and related GIC CPU interfaces. Later
cannot be changed either. Hence, if all of the KVM vCPUs have been created
successfully during init then hot(un)plugging operations later won't have
fatal initialization errors at the KVM as all operation get handled at QOM
level only for the hot(un)plugged vCPUs.

I feel if there is a failure to create KVM vCPU at Qemu KVM Init time then
there is something severally wrong either with the inputs or the system.
Hence, to keep the handling simple I was in favor of aborting the initialization.


But all of above is ARM arch specific. Do you have anything specific in mind
why you need graceful handling at the init time?

Thanks
Salil.

>  
>  regards,
>  Harsh
>  >           goto err;
>  >       }
>  >

RE: [PATCH V8 1/8] accel/kvm: Extract common KVM vCPU {creation, parking} code

[Salil Mehta via]

Hello,

Just replied to your other thread just now. Sorry catching everything late.

Thanks

>  From: Harsh Prateek Bora <harshpb@linux.ibm.com>
>  Sent: Tuesday, April 23, 2024 7:44 AM
>  
>  + Nick
>  
>  Hi Salil,
>  I have posted a patch [1] for ppc which based on this refactoring patch.
>  I see there were some comments from Vishnu on this patch.
>  Are we expecting any further updates on this patch before merge?


Yes, few of them and I'm working towards it. I've received most of the reviews
and SOBs last year itself. There are few minor comments to be addressed before
I can float V9 version of this patch-set.

I'm planning to push that for review in 2 weeks of time along with  RFC V3 of
the architecture specific code.


Thanks
Salil.


>  
>  Thanks
>  Harsh
>  
>  [1]
>  https://lore.kernel.org/qemu-devel/a0f9b2fc-4c8a-4c37-bc36-
>  26bbaa627fec@linux.ibm.com/T/#u
>  
>  On 3/22/24 13:45, Harsh Prateek Bora wrote:
>  > + Vaibhav, Shiva
>  >
>  > Hi Salil,
>  >
>  > I came across your patch while trying to solve a related problem on
>  > spapr. One query below ..
>  >
>  > On 3/12/24 07:29, Salil Mehta via wrote:
>  >> KVM vCPU creation is done once during the vCPU realization when
>  Qemu
>  >> vCPU thread is spawned. This is common to all the architectures as of
>  >> now.
>  >>
>  >> Hot-unplug of vCPU results in destruction of the vCPU object in QOM
>  >> but the corresponding KVM vCPU object in the Host KVM is not
>  >> destroyed as KVM doesn't support vCPU removal. Therefore, its
>  >> representative KVM vCPU object/context in Qemu is parked.
>  >>
>  >> Refactor architecture common logic so that some APIs could be reused
>  >> by vCPU Hotplug code of some architectures likes ARM, Loongson etc.
>  >> Update new/old APIs with trace events instead of DPRINTF. No
>  >> functional change is intended here.
>  >>
>  >> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
>  >> Reviewed-by: Gavin Shan <gshan@redhat.com>
>  >> Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
>  >> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>  >> Tested-by: Xianglai Li <lixianglai@loongson.cn>
>  >> Tested-by: Miguel Luis <miguel.luis@oracle.com>
>  >> Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
>  >> ---
>  >>   accel/kvm/kvm-all.c    | 64
>  >> ++++++++++++++++++++++++++++++++----------
>  >>   accel/kvm/trace-events |  5 +++-
>  >>   include/sysemu/kvm.h   | 16 +++++++++++
>  >>   3 files changed, 69 insertions(+), 16 deletions(-)
>  >>
>  >> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index
>  >> a8cecd040e..3bc3207bda 100644
>  >> --- a/accel/kvm/kvm-all.c
>  >> +++ b/accel/kvm/kvm-all.c
>  >> @@ -126,6 +126,7 @@ static QemuMutex kml_slots_lock;
>  >>   #define kvm_slots_unlock()  qemu_mutex_unlock(&kml_slots_lock)
>  >>   static void kvm_slot_init_dirty_bitmap(KVMSlot *mem);
>  >> +static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id);
>  >>   static inline void kvm_resample_fd_remove(int gsi)
>  >>   {
>  >> @@ -314,14 +315,53 @@ err:
>  >>       return ret;
>  >>   }
>  >> +void kvm_park_vcpu(CPUState *cpu)
>  >> +{
>  >> +    struct KVMParkedVcpu *vcpu;
>  >> +
>  >> +    trace_kvm_park_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>  >> +
>  >> +    vcpu = g_malloc0(sizeof(*vcpu));
>  >> +    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
>  >> +    vcpu->kvm_fd = cpu->kvm_fd;
>  >> +    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
>  }
>  >> +
>  >> +int kvm_create_vcpu(CPUState *cpu)
>  >> +{
>  >> +    unsigned long vcpu_id = kvm_arch_vcpu_id(cpu);
>  >> +    KVMState *s = kvm_state;
>  >> +    int kvm_fd;
>  >> +
>  >> +    trace_kvm_create_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>  >> +
>  >> +    /* check if the KVM vCPU already exist but is parked */
>  >> +    kvm_fd = kvm_get_vcpu(s, vcpu_id);
>  >> +    if (kvm_fd < 0) {
>  >> +        /* vCPU not parked: create a new KVM vCPU */
>  >> +        kvm_fd = kvm_vm_ioctl(s, KVM_CREATE_VCPU, vcpu_id);
>  >> +        if (kvm_fd < 0) {
>  >> +            error_report("KVM_CREATE_VCPU IOCTL failed for vCPU
>  >> +%lu",
>  >> vcpu_id);
>  >> +            return kvm_fd;
>  >> +        }
>  >> +    }
>  >> +
>  >> +    cpu->kvm_fd = kvm_fd;
>  >> +    cpu->kvm_state = s;
>  >> +    cpu->vcpu_dirty = true;
>  >> +    cpu->dirty_pages = 0;
>  >> +    cpu->throttle_us_per_full = 0;
>  >> +
>  >> +    return 0;
>  >> +}
>  >> +
>  >>   static int do_kvm_destroy_vcpu(CPUState *cpu)
>  >>   {
>  >>       KVMState *s = kvm_state;
>  >>       long mmap_size;
>  >> -    struct KVMParkedVcpu *vcpu = NULL;
>  >>       int ret = 0;
>  >> -    trace_kvm_destroy_vcpu();
>  >> +    trace_kvm_destroy_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>  >>       ret = kvm_arch_destroy_vcpu(cpu);
>  >>       if (ret < 0) {
>  >> @@ -347,10 +387,7 @@ static int do_kvm_destroy_vcpu(CPUState *cpu)
>  >>           }
>  >>       }
>  >> -    vcpu = g_malloc0(sizeof(*vcpu));
>  >> -    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
>  >> -    vcpu->kvm_fd = cpu->kvm_fd;
>  >> -    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
>  >> +    kvm_park_vcpu(cpu);
>  >>   err:
>  >>       return ret;
>  >>   }
>  >> @@ -371,6 +408,8 @@ static int kvm_get_vcpu(KVMState *s, unsigned
>  >> long
>  >> vcpu_id)
>  >>           if (cpu->vcpu_id == vcpu_id) {
>  >>               int kvm_fd;
>  >> +            trace_kvm_get_vcpu(vcpu_id);
>  >> +
>  >>               QLIST_REMOVE(cpu, node);
>  >>               kvm_fd = cpu->kvm_fd;
>  >>               g_free(cpu);
>  >> @@ -378,7 +417,7 @@ static int kvm_get_vcpu(KVMState *s, unsigned
>  >> long
>  >> vcpu_id)
>  >>           }
>  >>       }
>  >> -    return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
>  >> +    return -ENOENT;
>  >>   }
>  >>   int kvm_init_vcpu(CPUState *cpu, Error **errp) @@ -389,19 +428,14
>  >> @@ int kvm_init_vcpu(CPUState *cpu, Error **errp)
>  >>       trace_kvm_init_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>  >> -    ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu));
>  >> +    ret = kvm_create_vcpu(cpu);
>  >>       if (ret < 0) {
>  >> -        error_setg_errno(errp, -ret, "kvm_init_vcpu: kvm_get_vcpu
>  >> failed (%lu)",
>  >> +        error_setg_errno(errp, -ret,
>  >> +                         "kvm_init_vcpu: kvm_create_vcpu failed
>  >> +(%lu)",
>  >>                            kvm_arch_vcpu_id(cpu));
>  >
>  > If a vcpu hotplug fails due to failure with kvm_create_vcpu ioctl,
>  > current behaviour would be to bring down the guest as errp is
>  > &error_fatal. Any thoughts on how do we ensure that a failure with
>  > kvm_create_vcpu ioctl for hotplugged cpus (only) doesnt bring down the
>  > guest and fail gracefully (by reporting error to user on monitor?)?
>  >
>  > regards,
>  > Harsh
>  >>           goto err;
>  >>       }
>  >> -    cpu->kvm_fd = ret;
>  >> -    cpu->kvm_state = s;
>  >> -    cpu->vcpu_dirty = true;
>  >> -    cpu->dirty_pages = 0;
>  >> -    cpu->throttle_us_per_full = 0;
>  >> -
>  >>       mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
>  >>       if (mmap_size < 0) {
>  >>           ret = mmap_size;
>  >> diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events index
>  >> a25902597b..5558cff0dc 100644
>  >> --- a/accel/kvm/trace-events
>  >> +++ b/accel/kvm/trace-events
>  >> @@ -9,6 +9,10 @@ kvm_device_ioctl(int fd, int type, void *arg) "dev
>  >> fd %d, type 0x%x, arg %p"
>  >>   kvm_failed_reg_get(uint64_t id, const char *msg) "Warning: Unable
>  >> to retrieve ONEREG %" PRIu64 " from KVM: %s"
>  >>   kvm_failed_reg_set(uint64_t id, const char *msg) "Warning: Unable
>  >> to set ONEREG %" PRIu64 " to KVM: %s"
>  >>   kvm_init_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d
>  >> id: %lu"
>  >> +kvm_create_vcpu(int cpu_index, unsigned long arch_cpu_id) "index:
>  %d
>  >> id: %lu"
>  >> +kvm_get_vcpu(unsigned long arch_cpu_id) "id: %lu"
>  >> +kvm_destroy_vcpu(int cpu_index, unsigned long arch_cpu_id) "index:
>  >> +%d
>  >> id: %lu"
>  >> +kvm_park_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d
>  >> id: %lu"
>  >>   kvm_irqchip_commit_routes(void) ""
>  >>   kvm_irqchip_add_msi_route(char *name, int vector, int virq) "dev %s
>  >> vector %d virq %d"
>  >>   kvm_irqchip_update_msi_route(int virq) "Updating MSI route virq=%d"
>  >> @@ -25,7 +29,6 @@ kvm_dirty_ring_reaper(const char *s) "%s"
>  >>   kvm_dirty_ring_reap(uint64_t count, int64_t t) "reaped %"PRIu64"
>  >> pages (took %"PRIi64" us)"
>  >>   kvm_dirty_ring_reaper_kick(const char *reason) "%s"
>  >>   kvm_dirty_ring_flush(int finished) "%d"
>  >> -kvm_destroy_vcpu(void) ""
>  >>   kvm_failed_get_vcpu_mmap_size(void) ""
>  >>   kvm_cpu_exec(void) ""
>  >>   kvm_interrupt_exit_request(void) ""
>  >> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h index
>  >> fad9a7e8ff..2ed928aa71 100644
>  >> --- a/include/sysemu/kvm.h
>  >> +++ b/include/sysemu/kvm.h
>  >> @@ -435,6 +435,22 @@ void kvm_set_sigmask_len(KVMState *s,
>  unsigned
>  >> int sigmask_len);
>  >>   int kvm_physical_memory_addr_from_host(KVMState *s, void
>  *ram_addr,
>  >>                                          hwaddr *phys_addr);
>  >> +/**
>  >> + * kvm_create_vcpu - Gets a parked KVM vCPU or creates a KVM vCPU
>  >> + * @cpu: QOM CPUState object for which KVM vCPU has to be
>  >> fetched/created.
>  >> + *
>  >> + * @returns: 0 when success, errno (<0) when failed.
>  >> + */
>  >> +int kvm_create_vcpu(CPUState *cpu);
>  >> +
>  >> +/**
>  >> + * kvm_park_vcpu - Park QEMU KVM vCPU context
>  >> + * @cpu: QOM CPUState object for which QEMU KVM vCPU context
>  has to
>  >> be parked.
>  >> + *
>  >> + * @returns: none
>  >> + */
>  >> +void kvm_park_vcpu(CPUState *cpu);
>  >> +
>  >>   #endif /* NEED_CPU_H */
>  >>   void kvm_cpu_synchronize_state(CPUState *cpu);

RE: [PATCH V8 7/8] gdbstub: Add helper function to unregister GDB register space

[Salil Mehta via]

Hi Vishnu,
Sorry for the delay in reply. Still catching up.

>  From: Vishnu Pajjuri <vishnu@amperemail.onmicrosoft.com> 
>  Sent: Thursday, April 4, 2024 3:02 PM
>  To: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org; qemu-arm@nongnu.org
>  
>  Hi Salil,
>  On 12-03-2024 07:29, Salil Mehta wrote:
>>  Add common function to help unregister the GDB register space. This shall be
>>  done in context to the CPU unrealization.
>>  
>>  Signed-off-by: Salil Mehta mailto:salil.mehta@huawei.com
>>  Tested-by: Vishnu Pajjuri mailto:vishnu@os.amperecomputing.com
>>  Reviewed-by: Gavin Shan mailto:gshan@redhat.com
>>  Tested-by: Xianglai Li mailto:lixianglai@loongson.cn
>>  Tested-by: Miguel Luis mailto:miguel.luis@oracle.com
>>  Reviewed-by: Shaoqin Huang mailto:shahuang@redhat.com
>>  ---
>>   gdbstub/gdbstub.c>  | 12 ++++++++++++
>>   include/exec/gdbstub.h |  6 ++++++
>>   2 files changed, 18 insertions(+)
>>  
>>  diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
>>  index 17efcae0d0..a8449dc309 100644
>>  --- a/gdbstub/gdbstub.c
>>  +++ b/gdbstub/gdbstub.c
>>  @@ -615,6 +615,18 @@ void gdb_register_coprocessor(CPUState *cpu,
>>       }
>>   }
>>   
>>  +void gdb_unregister_coprocessor_all(CPUState *cpu)
>>  +{
>>  +    /*
>>  +     * Safe to nuke everything. GDBRegisterState::xml is static const char so
>>  +     * it won't be freed
>>  +     */
>>  +    g_array_free(cpu->gdb_regs, true);
>>  +
>>  +    cpu->gdb_regs = NULL;
>>  +    cpu->gdb_num_g_regs = 0;
>  Likewise, you may need to set gdb_num_regs to zero as well.


Sure, thanks.


>>  +}
>>  +
>>   static void gdb_process_breakpoint_remove_all(GDBProcess *p)
>>   {
>>       CPUState *cpu = gdb_get_first_cpu_in_process(p);
>>  diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
>>  index eb14b91139..249d4d4bc8 100644
>>  --- a/include/exec/gdbstub.h
>>  +++ b/include/exec/gdbstub.h
>>  @@ -49,6 +49,12 @@ void gdb_register_coprocessor(CPUState *cpu,
>>  >  >  >  >  >   gdb_get_reg_cb get_reg, gdb_set_reg_cb set_reg,
>>  >  >  >  >  >   const GDBFeature *feature, int g_pos);
>>   
>>  +/**
>>  + * gdb_unregister_coprocessor_all() - unregisters supplemental set of registers
>>  + * @cpu - the CPU associated with registers
>>  + */
>>  +void gdb_unregister_coprocessor_all(CPUState *cpu);
>>  +
>>   /**
>>    * gdbserver_start: start the gdb server
>>    * @port_or_device: connection spec for gdb
>>  Otherwise, Looks good to me.  Feel free to add
>>  Reviewed-by: "Vishnu Pajjuri" mailto:vishnu@os.amperecomputing.com

Thanks
Salil.


>>  Regards,
>>  -Vishnu

RE: [PATCH V8 3/8] hw/acpi: Update ACPI GED framework to support vCPU Hotplug

[Salil Mehta via]

Hello,

Sorry, I missed this earlier.

>  From: Zhao Liu <zhao1.liu@intel.com>
>  Sent: Wednesday, March 13, 2024 6:14 AM
>  To: Salil Mehta <salil.mehta@huawei.com>
>  
>  Hi Salil,
>  
>  It seems my comment [1] in v7 was missed, but I still hit the same issue. Pls
>  let me paste the previous comment here again.
>  
>  [1]: https://lore.kernel.org/qemu-devel/ZXCqp32ggIFvUweu@intel.com/

Yes, I have this in my mind. 

>  
>  [snip]
>  
>  > @@ -400,6 +411,12 @@ static void acpi_ged_initfn(Object *obj)
>  >      memory_region_init_io(&ged_st->regs, obj, &ged_regs_ops, ged_st,
>  >                            TYPE_ACPI_GED "-regs", ACPI_GED_REG_COUNT);
>  >      sysbus_init_mmio(sbd, &ged_st->regs);
>  > +
>  > +    memory_region_init(&s->container_cpuhp, OBJECT(dev), "cpuhp
>  container",
>  > +                       ACPI_CPU_HOTPLUG_REG_LEN);
>  > +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->container_cpuhp);
>  > +    cpu_hotplug_hw_init(&s->container_cpuhp, OBJECT(dev),
>  > +                        &s->cpuhp_state, 0);
>  >  }
>  >
>  
>  I find this cpu_hotplug_hw_init() can still cause qtest errors (for v8) on x86
>  platforms as you mentioned in v6:
>  https://lore.kernel.org/qemu-devel/15e70616-6abb-63a4-17d0-
>  820f4a254607@opnsrc.net/T/#m108f102b2fe92b7dd7218f2f942f7b233a9d6a
>  f3
>  
>  IIUC, microvm machine has its own 'possible_cpus_arch_ids' and that is
>  inherited from its parent x86 machine.
>  
>  The above error is because device-introspect-test sets the none-machine:
>  
>  # starting QEMU: exec ./qemu-system-i386 -qtest unix:/tmp/qtest-
>  3094820.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-
>  3094820.qmp,id=char0 -mon chardev=char0,mode=control -display none -
>  audio none -nodefaults -machine none -accel qtest
>  
>  So what about just checking mc->possible_cpu_arch_ids instead of an
>  assert in cpu_hotplug_hw_init()?
>  
>  diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c index
>  4b24a2500361..303f1f1f57bc 100644
>  --- a/hw/acpi/cpu.c
>  +++ b/hw/acpi/cpu.c
>  @@ -221,7 +221,10 @@ void cpu_hotplug_hw_init(MemoryRegion *as,
>  Object *owner,
>       const CPUArchIdList *id_list;
>       int i;
>  
>  -    assert(mc->possible_cpu_arch_ids);
>  +    if (!mc->possible_cpu_arch_ids) {
>  +        return;
>  +    }
>  +


Yes, we can do this with some debug print or trace maybe.


>       id_list = mc->possible_cpu_arch_ids(machine);
>       state->dev_count = id_list->len;
>       state->devs = g_new0(typeof(*state->devs), state->dev_count);
>  
>  This check seems to be acceptable in the general code path? Not all
>  machines have possible_cpu_arch_ids, after all.

True. BTW, have you tested this with Qtest?

Thanks
Salil.

RE: [PATCH V8 3/8] hw/acpi: Update ACPI GED framework to support vCPU Hotplug

[Salil Mehta via]

Hi Vishnu,

>  From: Vishnu Pajjuri <vishnu@amperemail.onmicrosoft.com> 
>  Sent: Thursday, April 4, 2024 3:01 PM
>  To: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org; qemu-arm@nongnu.org
>  
>  Hi Salil,
>>  On 12-03-2024 07:29, Salil Mehta wrote:
>>  ACPI GED (as described in the ACPI 6.4 spec) uses an interrupt listed in the
>>  _CRS object of GED to intimate OSPM about an event. Later then demultiplexes the
>>  notified event by evaluating ACPI _EVT method to know the type of event. Use
>>  ACPI GED to also notify the guest kernel about any CPU hot(un)plug events.
>>  
>>  ACPI CPU hotplug related initialization should only happen if ACPI_CPU_HOTPLUG
>>  support has been enabled for particular architecture. Add cpu_hotplug_hw_init()
>>  stub to avoid compilation break.
>>  
>>  Co-developed-by: Keqian Zhu mailto:zhukeqian1@huawei.com
>>  Signed-off-by: Keqian Zhu mailto:zhukeqian1@huawei.com
>>  Signed-off-by: Salil Mehta mailto:salil.mehta@huawei.com
>>  Reviewed-by: Jonathan Cameron mailto:Jonathan.Cameron@huawei.com
>>  Reviewed-by: Gavin Shan mailto:gshan@redhat.com
>>  Reviewed-by: David Hildenbrand mailto:david@redhat.com
>>  Reviewed-by: Shaoqin Huang mailto:shahuang@redhat.com
>>  Tested-by: Vishnu Pajjuri mailto:vishnu@os.amperecomputing.com
>>  Tested-by: Xianglai Li mailto:lixianglai@loongson.cn
>>  Tested-by: Miguel Luis mailto:miguel.luis@oracle.com
>>  ---
>>   hw/acpi/acpi-cpu-hotplug-stub.c>    |  6 ++++++
>>   hw/acpi/generic_event_device.c>     | 17 +++++++++++++++++
>>   include/hw/acpi/generic_event_device.h |  4 ++++
>>   3 files changed, 27 insertions(+)
>>  
>>  diff --git a/hw/acpi/acpi-cpu-hotplug-stub.c b/hw/acpi/acpi-cpu-hotplug-stub.c
>>  index 3fc4b14c26..c6c61bb9cd 100644
>>  --- a/hw/acpi/acpi-cpu-hotplug-stub.c
>>  +++ b/hw/acpi/acpi-cpu-hotplug-stub.c
>>  @@ -19,6 +19,12 @@ void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner,
>>       return;
>>   }
>>   
>>  +void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
>>  +         CPUHotplugState *state, hwaddr base_addr)
>>  +{
>>  +    return;
>>  +}
>>  +
>>   void acpi_cpu_ospm_status(CPUHotplugState *cpu_st, ACPIOSTInfoList ***list)
>>   {
>>       return;
>>  diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
>>  index 2d6e91b124..35a71505a5 100644
>>  --- a/hw/acpi/generic_event_device.c
>>  +++ b/hw/acpi/generic_event_device.c
>>  @@ -12,6 +12,7 @@
>>   #include "qemu/osdep.h"
>>   #include "qapi/error.h"
>>   #include "hw/acpi/acpi.h"
>>  +#include "hw/acpi/cpu.h"
>>   #include "hw/acpi/generic_event_device.h"
>>   #include "hw/irq.h"
>>   #include "hw/mem/pc-dimm.h"
>>  @@ -25,6 +26,7 @@ static const uint32_t ged_supported_events[] = {
>>       ACPI_GED_MEM_HOTPLUG_EVT,
>>       ACPI_GED_PWR_DOWN_EVT,
>>       ACPI_GED_NVDIMM_HOTPLUG_EVT,
>>  +    ACPI_GED_CPU_HOTPLUG_EVT,
>>   };
>>   
>>   /*
>>  @@ -234,6 +236,8 @@ static void acpi_ged_device_plug_cb(HotplugHandler *hotplug_dev,
>>       } else {
>>       acpi_memory_plug_cb(hotplug_dev, &s->memhp_state, dev, errp);
>>       }
>>  +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>  +    acpi_cpu_plug_cb(hotplug_dev, &s->cpuhp_state, dev, errp);
>>       } else {
>>       error_setg(errp, "virt: device plug request for unsupported device"
>>          " type: %s", object_get_typename(OBJECT(dev)));
>>  @@ -248,6 +252,8 @@ static void acpi_ged_unplug_request_cb(HotplugHandler *hotplug_dev,
>>       if ((object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
>>          !(object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)))) {
>>  >     acpi_memory_unplug_request_cb(hotplug_dev, &s->memhp_state, dev, errp);
>>  +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>  +    acpi_cpu_unplug_request_cb(hotplug_dev, &s->cpuhp_state, dev, errp);
>>       } else {
>>       error_setg(errp, "acpi: device unplug request for unsupported device"
>>          " type: %s", object_get_typename(OBJECT(dev)));
>>  @@ -261,6 +267,8 @@ static void acpi_ged_unplug_cb(HotplugHandler *hotplug_dev,
>>   
>>       if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>>       acpi_memory_unplug_cb(&s->memhp_state, dev, errp);
>>  +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>  +    acpi_cpu_unplug_cb(&s->cpuhp_state, dev, errp);
>>       } else {
>>       error_setg(errp, "acpi: device unplug for unsupported device"
>>          " type: %s", object_get_typename(OBJECT(dev)));
>>  @@ -272,6 +280,7 @@ static void acpi_ged_ospm_status(AcpiDeviceIf *adev, ACPIOSTInfoList ***list)
>>       AcpiGedState *s = ACPI_GED(adev);
>>   
>>       acpi_memory_ospm_status(&s->memhp_state, list);
>>  +    acpi_cpu_ospm_status(&s->cpuhp_state, list);
>>   }
>>   
>>   static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
>>  @@ -286,6 +295,8 @@ static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
>>       sel = ACPI_GED_PWR_DOWN_EVT;
>>       } else if (ev & ACPI_NVDIMM_HOTPLUG_STATUS) {
>>       sel = ACPI_GED_NVDIMM_HOTPLUG_EVT;
>>  +    } else if (ev & ACPI_CPU_HOTPLUG_STATUS) {
>>  +    sel = ACPI_GED_CPU_HOTPLUG_EVT;
>>       } else {
>>       /* Unknown event. Return without generating interrupt. */
>>       warn_report("GED: Unsupported event %d. No irq injected", ev);
>>  @@ -400,6 +411,12 @@ static void acpi_ged_initfn(Object *obj)
>>       memory_region_init_io(&ged_st->regs, obj, &ged_regs_ops, ged_st,
>>             TYPE_ACPI_GED "-regs", ACPI_GED_REG_COUNT);
>>       sysbus_init_mmio(sbd, &ged_st->regs);
>>  +
>>  +    memory_region_init(&s->container_cpuhp, OBJECT(dev), "cpuhp container",
>>  +           ACPI_CPU_HOTPLUG_REG_LEN);
>>  +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->container_cpuhp);
>  same sbd can be used here instead of SYS_BUS_DEVICE(dev).


    Yes, we can.
	

>>  +    cpu_hotplug_hw_init(&s->container_cpuhp, OBJECT(dev),
>>  +        &s->cpuhp_state, 0);
>>   }
>>   
>>   static void acpi_ged_class_init(ObjectClass *class, void *data)
>>  diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
>>  index ba84ce0214..90fc41cbb8 100644
>>  --- a/include/hw/acpi/generic_event_device.h
>>  +++ b/include/hw/acpi/generic_event_device.h
>>  @@ -60,6 +60,7 @@
>>   #define HW_ACPI_GENERIC_EVENT_DEVICE_H
>>   
>>   #include "hw/sysbus.h"
>>  +#include "hw/acpi/cpu_hotplug.h"
>>   #include "hw/acpi/memory_hotplug.h"
>>   #include "hw/acpi/ghes.h"
>>   #include "qom/object.h"
>>  @@ -95,6 +96,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
>>   #define ACPI_GED_MEM_HOTPLUG_EVT   0x1
>>   #define ACPI_GED_PWR_DOWN_EVT>  0x2
>>   #define ACPI_GED_NVDIMM_HOTPLUG_EVT 0x4
>>  +#define ACPI_GED_CPU_HOTPLUG_EVT    0x8
>>   
>>   typedef struct GEDState {
>>       MemoryRegion evt;
>>  @@ -106,6 +108,8 @@ struct AcpiGedState {
>>       SysBusDevice parent_obj;
>>       MemHotplugState memhp_state;
>>       MemoryRegion container_memhp;
>>  +    CPUHotplugState cpuhp_state;
>>  +    MemoryRegion container_cpuhp;
>>       GEDState ged_state;
>>       uint32_t ged_event_bitmap;
>>       qemu_irq irq;
>>  Otherwise, Looks good to me.  Feel free to add
>>  Reviewed-by: "Vishnu Pajjuri" mailto:vishnu@os.amperecomputing.com

Thanks.


>>  Regards,
>>  -Vishnu

Re: [PATCH V8 6/8] physmem: Add helper function to destroy CPU AddressSpace

[Salil Mehta]

[-- Attachment #1: Type: text/plain, Size: 1819 bytes --]

Hi Zhukeqian,

On Fri, Mar 15, 2024 at 1:17 AM zhukeqian <zhukeqian1@huawei.com> wrote:

> Hi Salil,
>
> [...]
>
> +void cpu_address_space_destroy(CPUState *cpu, int asidx) {
> +    CPUAddressSpace *cpuas;
> +
> +    assert(cpu->cpu_ases);
> +    assert(asidx >= 0 && asidx < cpu->num_ases);
> +    /* KVM cannot currently support multiple address spaces. */
> +    assert(asidx == 0 || !kvm_enabled());
> +
> +    cpuas = &cpu->cpu_ases[asidx];
> +    if (tcg_enabled()) {
> +        memory_listener_unregister(&cpuas->tcg_as_listener);
> +    }
> +
> +    address_space_destroy(cpuas->as);
> +    g_free_rcu(cpuas->as, rcu);
>
> In address_space_destroy(), it calls call_rcu1() on cpuas->as which will
> set do_address_space_destroy() as the rcu func.
> And g_free_rcu() also calls call_rcu1() on cpuas->as which will overwrite
> the rcu func as g_free().
>
> Then I think the g_free() may be called twice in rcu thread, please verify
> that.
>
> The source code of call_rcu1:
>
> void call_rcu1(struct rcu_head *node, void (*func)(struct rcu_head *node))
> {
>     node->func = func;
>     enqueue(node);
>     qatomic_inc(&rcu_call_count);
>     qemu_event_set(&rcu_call_ready_event);
> }
>


Thanks for testing and identifying this. Let me have a look and will get
back to you.

Thanks
Salil



>
> Thanks,
> Keqian
>
> +
> +    if (asidx == 0) {
> +        /* reset the convenience alias for address space 0 */
> +        cpu->as = NULL;
> +    }
> +
> +    if (--cpu->cpu_ases_count == 0) {
> +        g_free(cpu->cpu_ases);
> +        cpu->cpu_ases = NULL;
> +    }
> +}
> +
>  AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx)  {
>      /* Return the AddressSpace corresponding to the specified index */
> --
> 2.34.1
>
>

[-- Attachment #2: Type: text/html, Size: 2572 bytes --]

Re: [PATCH V8 6/8] physmem: Add helper function to destroy CPU AddressSpace

[Peter Maydell]

On Tue, 12 Mar 2024 at 02:02, Salil Mehta <salil.mehta@huawei.com> wrote:
>
> Virtual CPU Hot-unplug leads to unrealization of a CPU object. This also
> involves destruction of the CPU AddressSpace. Add common function to help
> destroy the CPU AddressSpace.
>
> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
> Reviewed-by: Gavin Shan <gshan@redhat.com>
> Tested-by: Xianglai Li <lixianglai@loongson.cn>
> Tested-by: Miguel Luis <miguel.luis@oracle.com>
> Reviewed-by: Shaoqin Huang <shahuang@redhat.com>

> diff --git a/system/physmem.c b/system/physmem.c
> index 6e9ed97597..61b32ac4f2 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -761,6 +761,7 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
>
>      if (!cpu->cpu_ases) {
>          cpu->cpu_ases = g_new0(CPUAddressSpace, cpu->num_ases);
> +        cpu->cpu_ases_count = cpu->num_ases;
>      }
>
>      newas = &cpu->cpu_ases[asidx];
> @@ -774,6 +775,34 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
>      }
>  }
>
> +void cpu_address_space_destroy(CPUState *cpu, int asidx)
> +{
> +    CPUAddressSpace *cpuas;
> +
> +    assert(cpu->cpu_ases);
> +    assert(asidx >= 0 && asidx < cpu->num_ases);
> +    /* KVM cannot currently support multiple address spaces. */
> +    assert(asidx == 0 || !kvm_enabled());
> +
> +    cpuas = &cpu->cpu_ases[asidx];
> +    if (tcg_enabled()) {
> +        memory_listener_unregister(&cpuas->tcg_as_listener);
> +    }
> +
> +    address_space_destroy(cpuas->as);
> +    g_free_rcu(cpuas->as, rcu);
> +
> +    if (asidx == 0) {
> +        /* reset the convenience alias for address space 0 */
> +        cpu->as = NULL;
> +    }
> +
> +    if (--cpu->cpu_ases_count == 0) {
> +        g_free(cpu->cpu_ases);
> +        cpu->cpu_ases = NULL;
> +    }
> +}

When do we need to destroy a single address space in this way
that means we need to keep a count of how many ASes the CPU
currently has? The commit message talks about the case when we
unrealize the whole CPU object, but in that situation you can
just throw away all the ASes at once (eg by calling some
cpu_destroy_address_spaces() function from cpu_common_unrealizefn()).

Also, if we're leaking stuff here by failing to destroy it,
is that a problem for existing CPU types like x86 that we
can already hotplug?

thanks
-- PMM

Re: [PATCH V8 3/8] hw/acpi: Update ACPI GED framework to support vCPU Hotplug

[Zhao Liu]

Hi Salil,

On Fri, May 03, 2024 at 07:59:32PM +0000, Salil Mehta wrote:
> Date: Fri, 3 May 2024 19:59:32 +0000
> From: Salil Mehta <salil.mehta@huawei.com>
> Subject: RE: [PATCH V8 3/8] hw/acpi: Update ACPI GED framework to support
>  vCPU Hotplug
> 
> Hello,
> 
> Sorry, I missed this earlier.
>
> >  From: Zhao Liu <zhao1.liu@intel.com>
> >  Sent: Wednesday, March 13, 2024 6:14 AM
> >  To: Salil Mehta <salil.mehta@huawei.com>
> >  
> >  Hi Salil,
> >  
> >  It seems my comment [1] in v7 was missed, but I still hit the same issue. Pls
> >  let me paste the previous comment here again.
> >  
> >  [1]: https://lore.kernel.org/qemu-devel/ZXCqp32ggIFvUweu@intel.com/
> 
> Yes, I have this in my mind. 
> 
> >  
> >  [snip]
> >  
> >  > @@ -400,6 +411,12 @@ static void acpi_ged_initfn(Object *obj)
> >  >      memory_region_init_io(&ged_st->regs, obj, &ged_regs_ops, ged_st,
> >  >                            TYPE_ACPI_GED "-regs", ACPI_GED_REG_COUNT);
> >  >      sysbus_init_mmio(sbd, &ged_st->regs);
> >  > +
> >  > +    memory_region_init(&s->container_cpuhp, OBJECT(dev), "cpuhp
> >  container",
> >  > +                       ACPI_CPU_HOTPLUG_REG_LEN);
> >  > +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->container_cpuhp);
> >  > +    cpu_hotplug_hw_init(&s->container_cpuhp, OBJECT(dev),
> >  > +                        &s->cpuhp_state, 0);
> >  >  }
> >  >
> >  
> >  I find this cpu_hotplug_hw_init() can still cause qtest errors (for v8) on x86
> >  platforms as you mentioned in v6:
> >  https://lore.kernel.org/qemu-devel/15e70616-6abb-63a4-17d0-
> >  820f4a254607@opnsrc.net/T/#m108f102b2fe92b7dd7218f2f942f7b233a9d6a
> >  f3
> >  
> >  IIUC, microvm machine has its own 'possible_cpus_arch_ids' and that is
> >  inherited from its parent x86 machine.
> >  
> >  The above error is because device-introspect-test sets the none-machine:
> >  
> >  # starting QEMU: exec ./qemu-system-i386 -qtest unix:/tmp/qtest-
> >  3094820.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-
> >  3094820.qmp,id=char0 -mon chardev=char0,mode=control -display none -
> >  audio none -nodefaults -machine none -accel qtest
> >  
> >  So what about just checking mc->possible_cpu_arch_ids instead of an
> >  assert in cpu_hotplug_hw_init()?
> >  
> >  diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c index
> >  4b24a2500361..303f1f1f57bc 100644
> >  --- a/hw/acpi/cpu.c
> >  +++ b/hw/acpi/cpu.c
> >  @@ -221,7 +221,10 @@ void cpu_hotplug_hw_init(MemoryRegion *as,
> >  Object *owner,
> >       const CPUArchIdList *id_list;
> >       int i;
> >  
> >  -    assert(mc->possible_cpu_arch_ids);
> >  +    if (!mc->possible_cpu_arch_ids) {
> >  +        return;
> >  +    }
> >  +
> 
> 
> Yes, we can do this with some debug print or trace maybe.

Here it is just to return early without touching mc->possible_cpu_arch_ids().
If you adopt this workaround, then in the meantime I suggest adding a comment
to this "if" to clarify that it is for compatibility with certain machines
that do not implement mc->possible_cpu_arch_ids().
 
> >       id_list = mc->possible_cpu_arch_ids(machine);
> >       state->dev_count = id_list->len;
> >       state->devs = g_new0(typeof(*state->devs), state->dev_count);
> >  
> >  This check seems to be acceptable in the general code path? Not all
> >  machines have possible_cpu_arch_ids, after all.
> 
> True. BTW, have you tested this with Qtest?

Yes, by "make check" on x86 platform. This workaround can help us pass
the x86 tests.

Regards,
Zhao

RE: [PATCH V8 6/8] physmem: Add helper function to destroy CPU AddressSpace

[Salil Mehta via]

Hi Peter,

Thanks for the review.

>  From: Peter Maydell <peter.maydell@linaro.org>
>  Sent: Saturday, May 4, 2024 2:41 PM
>  
>  On Tue, 12 Mar 2024 at 02:02, Salil Mehta <salil.mehta@huawei.com>
>  wrote:
>  >
>  > Virtual CPU Hot-unplug leads to unrealization of a CPU object. This
>  > also involves destruction of the CPU AddressSpace. Add common function
>  > to help destroy the CPU AddressSpace.
>  >
>  > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
>  > Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
>  > Reviewed-by: Gavin Shan <gshan@redhat.com>
>  > Tested-by: Xianglai Li <lixianglai@loongson.cn>
>  > Tested-by: Miguel Luis <miguel.luis@oracle.com>
>  > Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
>  
>  > diff --git a/system/physmem.c b/system/physmem.c index
>  > 6e9ed97597..61b32ac4f2 100644
>  > --- a/system/physmem.c
>  > +++ b/system/physmem.c
>  > @@ -761,6 +761,7 @@ void cpu_address_space_init(CPUState *cpu, int
>  > asidx,
>  >
>  >      if (!cpu->cpu_ases) {
>  >          cpu->cpu_ases = g_new0(CPUAddressSpace, cpu->num_ases);
>  > +        cpu->cpu_ases_count = cpu->num_ases;
>  >      }
>  >
>  >      newas = &cpu->cpu_ases[asidx];
>  > @@ -774,6 +775,34 @@ void cpu_address_space_init(CPUState *cpu, int
>  asidx,
>  >      }
>  >  }
>  >
>  > +void cpu_address_space_destroy(CPUState *cpu, int asidx) {
>  > +    CPUAddressSpace *cpuas;
>  > +
>  > +    assert(cpu->cpu_ases);
>  > +    assert(asidx >= 0 && asidx < cpu->num_ases);
>  > +    /* KVM cannot currently support multiple address spaces. */
>  > +    assert(asidx == 0 || !kvm_enabled());
>  > +
>  > +    cpuas = &cpu->cpu_ases[asidx];
>  > +    if (tcg_enabled()) {
>  > +        memory_listener_unregister(&cpuas->tcg_as_listener);
>  > +    }
>  > +
>  > +    address_space_destroy(cpuas->as);
>  > +    g_free_rcu(cpuas->as, rcu);
>  > +
>  > +    if (asidx == 0) {
>  > +        /* reset the convenience alias for address space 0 */
>  > +        cpu->as = NULL;
>  > +    }
>  > +
>  > +    if (--cpu->cpu_ases_count == 0) {
>  > +        g_free(cpu->cpu_ases);
>  > +        cpu->cpu_ases = NULL;
>  > +    }
>  > +}
>  
>  When do we need to destroy a single address space in this way that means
>  we need to keep a count of how many ASes the CPU currently has? The
>  commit message talks about the case when we unrealize the whole CPU
>  object, but in that situation you can just throw away all the ASes at once (eg
>  by calling some
>  cpu_destroy_address_spaces() function from cpu_common_unrealizefn()).


Yes, maybe, we can destroy all at once from common leg as well. I'd prefer this
to be done from the arch specific function for ARM to maintain the clarity &
symmetry of initialization and un-initialization legs.  For now, all of these address
space destruction is happening in context to the arm_cpu_unrealizefn().

It’s a kind of trade-off between little more code and clarity but I'm open to
further suggestions.


>  
>  Also, if we're leaking stuff here by failing to destroy it, is that a problem for
>  existing CPU types like x86 that we can already hotplug?

No we are not. We are taking care of these in the ARM arch specific legs
within functions arm_cpu_(un)realizefn(). 

https://lore.kernel.org/all/20230926103654.34424-1-salil.mehta@huawei.com/

Above change now would be part of Arch specific patch-set RFC V3 being prepared.


Thanks
Salil.

RE: [PATCH V8 3/8] hw/acpi: Update ACPI GED framework to support vCPU Hotplug

[Salil Mehta via]

>  From: Zhao Liu <zhao1.liu@intel.com>
>  Sent: Monday, May 6, 2024 10:06 AM
>  
>  Hi Salil,
>  
>  On Fri, May 03, 2024 at 07:59:32PM +0000, Salil Mehta wrote:
>  > Date: Fri, 3 May 2024 19:59:32 +0000
>  > From: Salil Mehta <salil.mehta@huawei.com>
>  > Subject: RE: [PATCH V8 3/8] hw/acpi: Update ACPI GED framework to
>  > support  vCPU Hotplug
>  >
>  > Hello,
>  >
>  > Sorry, I missed this earlier.
>  >
>  > >  From: Zhao Liu <zhao1.liu@intel.com>
>  > >  Sent: Wednesday, March 13, 2024 6:14 AM
>  > >  To: Salil Mehta <salil.mehta@huawei.com>
>  > >
>  > >  Hi Salil,
>  > >
>  > >  It seems my comment [1] in v7 was missed, but I still hit the same
>  > > issue. Pls  let me paste the previous comment here again.
>  > >
>  > >  [1]: https://lore.kernel.org/qemu- devel/ZXCqp32ggIFvUweu@intel.com/
>  >
>  > Yes, I have this in my mind.
>  >
>  > >
>  > >  [snip]
>  > >
>  > >  > @@ -400,6 +411,12 @@ static void acpi_ged_initfn(Object *obj)
>  > >  >      memory_region_init_io(&ged_st->regs, obj, &ged_regs_ops,  ged_st,
>  > >  >                            TYPE_ACPI_GED "-regs", ACPI_GED_REG_COUNT);
>  > >  >      sysbus_init_mmio(sbd, &ged_st->regs);
>  > >  > +
>  > >  > +    memory_region_init(&s->container_cpuhp, OBJECT(dev), "cpuhp
>  > >  container",
>  > >  > +                       ACPI_CPU_HOTPLUG_REG_LEN);
>  > >  > +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->container_cpuhp);
>  > >  > +    cpu_hotplug_hw_init(&s->container_cpuhp, OBJECT(dev),
>  > >  > +                        &s->cpuhp_state, 0);
>  > >  >  }
>  > >  >
>  > >
>  > >  I find this cpu_hotplug_hw_init() can still cause qtest errors (for
>  > > v8) on x86  platforms as you mentioned in v6:
>  > >  https://lore.kernel.org/qemu-devel/15e70616-6abb-63a4-17d0-820f4a254607@opnsrc.net/T/#m108f102b2fe92b7dd7218f2f942f7b233a9d6af3
>  > >
>  > >  IIUC, microvm machine has its own 'possible_cpus_arch_ids' and that
>  > > is  inherited from its parent x86 machine.
>  > >
>  > >  The above error is because device-introspect-test sets the none-machine:
>  > >
>  > >  # starting QEMU: exec ./qemu-system-i386 -qtest unix:/tmp/qtest-
>  > > 3094820.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-
>  > >  3094820.qmp,id=char0 -mon chardev=char0,mode=control -display none
>  > > -  audio none -nodefaults -machine none -accel qtest
>  > >
>  > >  So what about just checking mc->possible_cpu_arch_ids instead of an
>  > > assert in cpu_hotplug_hw_init()?
>  > >
>  > >  diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c index
>  > > 4b24a2500361..303f1f1f57bc 100644
>  > >  --- a/hw/acpi/cpu.c
>  > >  +++ b/hw/acpi/cpu.c
>  > >  @@ -221,7 +221,10 @@ void cpu_hotplug_hw_init(MemoryRegion *as,
>  > > Object *owner,
>  > >       const CPUArchIdList *id_list;
>  > >       int i;
>  > >
>  > >  -    assert(mc->possible_cpu_arch_ids);
>  > >  +    if (!mc->possible_cpu_arch_ids) {
>  > >  +        return;
>  > >  +    }
>  > >  +
>  >
>  >
>  > Yes, we can do this with some debug print or trace maybe.
>  
>  Here it is just to return early without touching mc->possible_cpu_arch_ids().
>  If you adopt this workaround, then in the meantime I suggest adding a
>  comment to this "if" to clarify that it is for compatibility with certain
>  machines that do not implement mc->possible_cpu_arch_ids().


sure.


>  
>  > >       id_list = mc->possible_cpu_arch_ids(machine);
>  > >       state->dev_count = id_list->len;
>  > >       state->devs = g_new0(typeof(*state->devs), state->dev_count);
>  > >
>  > >  This check seems to be acceptable in the general code path? Not all
>  > > machines have possible_cpu_arch_ids, after all.
>  >
>  > True. BTW, have you tested this with Qtest?
>  
>  Yes, by "make check" on x86 platform. This workaround can help us pass the
>  x86 tests.

thanks.

Best
Salil.

Re: [PATCH V8 6/8] physmem: Add helper function to destroy CPU AddressSpace

[Peter Maydell]

On Mon, 6 May 2024 at 10:06, Salil Mehta <salil.mehta@huawei.com> wrote:
>
> Hi Peter,
>
> Thanks for the review.
>
> >  From: Peter Maydell <peter.maydell@linaro.org>
> >  When do we need to destroy a single address space in this way that means
> >  we need to keep a count of how many ASes the CPU currently has? The
> >  commit message talks about the case when we unrealize the whole CPU
> >  object, but in that situation you can just throw away all the ASes at once (eg
> >  by calling some
> >  cpu_destroy_address_spaces() function from cpu_common_unrealizefn()).
>
>
> Yes, maybe, we can destroy all at once from common leg as well. I'd prefer this
> to be done from the arch specific function for ARM to maintain the clarity &
> symmetry of initialization and un-initialization legs.  For now, all of these address
> space destruction is happening in context to the arm_cpu_unrealizefn().
>
> It’s a kind of trade-off between little more code and clarity but I'm open to
> further suggestions.
>
>
> >
> >  Also, if we're leaking stuff here by failing to destroy it, is that a problem for
> >  existing CPU types like x86 that we can already hotplug?
>
> No we are not. We are taking care of these in the ARM arch specific legs
> within functions arm_cpu_(un)realizefn().

How can you be taking care of *x86* CPU types in the Arm unrealize?

thanks
-- PMM

RE: [PATCH V8 6/8] physmem: Add helper function to destroy CPU AddressSpace

[Salil Mehta via]

>  From: Peter Maydell <peter.maydell@linaro.org>
>  Sent: Monday, May 6, 2024 10:29 AM
>  To: Salil Mehta <salil.mehta@huawei.com>
>  
>  On Mon, 6 May 2024 at 10:06, Salil Mehta <salil.mehta@huawei.com>
>  wrote:
>  >
>  > Hi Peter,
>  >
>  > Thanks for the review.
>  >
>  > >  From: Peter Maydell <peter.maydell@linaro.org>  When do we need to
>  > > destroy a single address space in this way that means  we need to
>  > > keep a count of how many ASes the CPU currently has? The  commit
>  > > message talks about the case when we unrealize the whole CPU
>  > > object, but in that situation you can just throw away all the ASes
>  > > at once (eg  by calling some
>  > >  cpu_destroy_address_spaces() function from
>  cpu_common_unrealizefn()).
>  >
>  >
>  > Yes, maybe, we can destroy all at once from common leg as well. I'd
>  > prefer this to be done from the arch specific function for ARM to
>  > maintain the clarity & symmetry of initialization and
>  > un-initialization legs.  For now, all of these address space destruction is
>  happening in context to the arm_cpu_unrealizefn().
>  >
>  > It’s a kind of trade-off between little more code and clarity but I'm
>  > open to further suggestions.
>  >
>  >
>  > >
>  > >  Also, if we're leaking stuff here by failing to destroy it, is that
>  > > a problem for  existing CPU types like x86 that we can already hotplug?
>  >
>  > No we are not. We are taking care of these in the ARM arch specific
>  > legs within functions arm_cpu_(un)realizefn().
>  
>  How can you be taking care of *x86* CPU types in the Arm unrealize?


Sorry, yes, I missed to reply that clearly. There was indeed a leak with x86 reported
by Phillipe/David last year. In fact, Phillipe floated a patch last year for this.
I thought it was fixed already as part of cpu_common_unrealize() but I just
checked and realized that the below proposed changed still isn’t part of the
mainline

https://lore.kernel.org/qemu-devel/20230918160257.30127-9-philmd@linaro.org/

I can definitely add a common CPU AddressSpace destruction leg as part of this
patch if in case arch specific CPU unrealize does not cleans up its CPU
AddressSpace?


Thanks
Salil.

>  
>  thanks
>  -- PMM

Re: [PATCH V8 6/8] physmem: Add helper function to destroy CPU AddressSpace

[Peter Maydell]

On Tue, 7 May 2024 at 01:11, Salil Mehta <salil.mehta@huawei.com> wrote:
>
> >  From: Peter Maydell <peter.maydell@linaro.org>
> >  Sent: Monday, May 6, 2024 10:29 AM
> >  To: Salil Mehta <salil.mehta@huawei.com>
> >
> >  On Mon, 6 May 2024 at 10:06, Salil Mehta <salil.mehta@huawei.com>
> >  wrote:
> >  >
> >  > Hi Peter,
> >  >
> >  > Thanks for the review.
> >  >
> >  > >  From: Peter Maydell <peter.maydell@linaro.org>  When do we need to
> >  > > destroy a single address space in this way that means  we need to
> >  > > keep a count of how many ASes the CPU currently has? The  commit
> >  > > message talks about the case when we unrealize the whole CPU
> >  > > object, but in that situation you can just throw away all the ASes
> >  > > at once (eg  by calling some
> >  > >  cpu_destroy_address_spaces() function from
> >  cpu_common_unrealizefn()).
> >  >
> >  >
> >  > Yes, maybe, we can destroy all at once from common leg as well. I'd
> >  > prefer this to be done from the arch specific function for ARM to
> >  > maintain the clarity & symmetry of initialization and
> >  > un-initialization legs.  For now, all of these address space destruction is
> >  happening in context to the arm_cpu_unrealizefn().
> >  >
> >  > It’s a kind of trade-off between little more code and clarity but I'm
> >  > open to further suggestions.
> >  >
> >  >
> >  > >
> >  > >  Also, if we're leaking stuff here by failing to destroy it, is that
> >  > > a problem for  existing CPU types like x86 that we can already hotplug?
> >  >
> >  > No we are not. We are taking care of these in the ARM arch specific
> >  > legs within functions arm_cpu_(un)realizefn().
> >
> >  How can you be taking care of *x86* CPU types in the Arm unrealize?
>
>
> Sorry, yes, I missed to reply that clearly. There was indeed a leak with x86 reported
> by Phillipe/David last year. In fact, Phillipe floated a patch last year for this.
> I thought it was fixed already as part of cpu_common_unrealize() but I just
> checked and realized that the below proposed changed still isn’t part of the
> mainline
>
> https://lore.kernel.org/qemu-devel/20230918160257.30127-9-philmd@linaro.org/

That seems like the right way to clean these up -- Philippe, do you want
to fish that bugfix out of your big patchset and submit it separately ?

> I can definitely add a common CPU AddressSpace destruction leg as part of this
> patch if in case arch specific CPU unrealize does not cleans up its CPU
> AddressSpace?

Arch-specific CPU unrealize shouldn't need to do anything -- if we
fix this similarly to Philippe's patch above then that will do
the cleanup required. Handling this kind of cleanup in common code
is more reliable because it doesn't require every target-arch
maintainer to remember it needs to be done, plus it's less code.

thanks
-- PMM

RE: [PATCH V8 6/8] physmem: Add helper function to destroy CPU AddressSpace

[Salil Mehta via]

Hi Peter,

>  From: Peter Maydell <peter.maydell@linaro.org>
>  Sent: Tuesday, May 7, 2024 10:03 AM
>  
>  On Tue, 7 May 2024 at 01:11, Salil Mehta <salil.mehta@huawei.com> wrote:
>  >
>  > >  From: Peter Maydell <peter.maydell@linaro.org>
>  > >  Sent: Monday, May 6, 2024 10:29 AM
>  > >  To: Salil Mehta <salil.mehta@huawei.com>
>  > >
>  > >  On Mon, 6 May 2024 at 10:06, Salil Mehta <salil.mehta@huawei.com>
>  > >  wrote:
>  > >  >
>  > >  > Hi Peter,
>  > >  >
>  > >  > Thanks for the review.
>  > >  >
>  > >  > >  From: Peter Maydell <peter.maydell@linaro.org>  When do we
>  > > need to  > > destroy a single address space in this way that means
>  > > we need to  > > keep a count of how many ASes the CPU currently has?
>  > > The  commit  > > message talks about the case when we unrealize the
>  > > whole CPU  > > object, but in that situation you can just throw away
>  > > all the ASes  > > at once (eg  by calling some  > >
>  > > cpu_destroy_address_spaces() function from
>  > > cpu_common_unrealizefn()).
>  > >  >
>  > >  >
>  > >  > Yes, maybe, we can destroy all at once from common leg as well.
>  > > I'd  > prefer this to be done from the arch specific function for
>  > > ARM to  > maintain the clarity & symmetry of initialization and  >
>  > > un-initialization legs.  For now, all of these address space
>  > > destruction is  happening in context to the arm_cpu_unrealizefn().
>  > >  >
>  > >  > It’s a kind of trade-off between little more code and clarity but
>  > > I'm  > open to further suggestions.
>  > >  >
>  > >  >
>  > >  > >
>  > >  > >  Also, if we're leaking stuff here by failing to destroy it, is
>  > > that  > > a problem for  existing CPU types like x86 that we can already  hotplug?
>  > >  >
>  > >  > No we are not. We are taking care of these in the ARM arch
>  > > specific  > legs within functions arm_cpu_(un)realizefn().
>  > >
>  > >  How can you be taking care of *x86* CPU types in the Arm unrealize?
>  >
>  >
>  > Sorry, yes, I missed to reply that clearly. There was indeed a leak
>  > with x86 reported by Phillipe/David last year. In fact, Phillipe floated a
>  patch last year for this.
>  > I thought it was fixed already as part of cpu_common_unrealize() but I
>  > just checked and realized that the below proposed changed still isn’t
>  > part of the mainline
>  >
>  > https://lore.kernel.org/qemu-devel/20230918160257.30127-9-
>  philmd@linar
>  > o.org/
>  
>  That seems like the right way to clean these up -- Philippe, do you want to
>  fish that bugfix out of your big patchset and submit it separately ?
>  
>  > I can definitely add a common CPU AddressSpace destruction leg as part
>  > of this patch if in case arch specific CPU unrealize does not cleans
>  > up its CPU AddressSpace?
>  
>  Arch-specific CPU unrealize shouldn't need to do anything -- if we fix this
>  similarly to Philippe's patch above then that will do the cleanup required.
>  Handling this kind of cleanup in common code is more reliable because it
>  doesn't require every target-arch maintainer to remember it needs to be
>  done, plus it's less code.


Agreed but It is a trade-off between 'lines of code' and 'clarity of the code'.
Ideally, if someone is adding a initialization part one must consciously verify
the locations where these will get deallocated/deinited as well so I do not
think there is an escape from that kind of maintenance part. 

For arch like x86, where we are doing default AddressSpace allocation from
common realize, it is more obvious to symmetrically destroy from common
leg but for any other architecture including ARM where many types of CPU
AddressSpace are getting allocated it would not be obvious to find its
destruction leg buried somewhere in the common code. 

Hence, I would suggest to make release of the AddressSpace from common
unrealization part optional which by default it is in the code excerpt from the
patch:

https://lore.kernel.org/qemu-devel/20230918160257.30127-9-philmd@linaro.org/

+    /* Destroy CPU address space */
+    for (unsigned idx = 0; idx < cpu->num_ases; idx++) {
+        cpu_address_space_destroy(cpu, idx);
+    }

cpu->num_ases will be 0 in case Arch specific code has already destroyed it.

That said we will still need some modification in the address space destruction
function to gracefully return in case that CPU AddressSpace was already
destroyed or does not exist. This will keep both options open.


Thanks
Salil.

>  
>  thanks
>  -- PMM

Re: [PATCH V8 1/8] accel/kvm: Extract common KVM vCPU {creation,parking} code

[Vishnu Pajjuri]

[-- Attachment #1: Type: text/plain, Size: 10226 bytes --]

Hi Salil,

On 03-05-2024 21:53, Salil Mehta wrote:
> [EXTERNAL EMAIL NOTICE: This email originated from an external sender. Please be mindful of safe email handling and proprietary information protection practices.]
>
>
> Hi Vishnu,
>
>>   From: Vishnu Pajjuri<vishnu@amperemail.onmicrosoft.com>
>>   Sent: Thursday, April 4, 2024 3:00 PM
>>   Subject: Re: [PATCH V8 1/8] accel/kvm: Extract common KVM vCPU {creation,parking} code
>>
>>   Hi Salil,
>>>   On 12-03-2024 07:29, Salil Mehta wrote:
>>>   KVM vCPU creation is done once during the vCPU realization when Qemu vCPU thread
>>>   is spawned. This is common to all the architectures as of now.
>>>
>>>   Hot-unplug of vCPU results in destruction of the vCPU object in QOM but the
>>>   corresponding KVM vCPU object in the Host KVM is not destroyed as KVM doesn't
>>>   support vCPU removal. Therefore, its representative KVM vCPU object/context in
>>>   Qemu is parked.
>>>
>>>   Refactor architecture common logic so that some APIs could be reused by vCPU
>>>   Hotplug code of some architectures likes ARM, Loongson etc. Update new/old APIs
>>>   with trace events instead of DPRINTF. No functional change is intended here.
>>>
>>>   Signed-off-by: Salil Mehtamailto:salil.mehta@huawei.com
>>>   Reviewed-by: Gavin Shanmailto:gshan@redhat.com
>>>   Tested-by: Vishnu Pajjurimailto:vishnu@os.amperecomputing.com
>>>   Reviewed-by: Jonathan Cameronmailto:Jonathan.Cameron@huawei.com
>>>   Tested-by: Xianglai Limailto:lixianglai@loongson.cn
>>>   Tested-by: Miguel Luismailto:miguel.luis@oracle.com
>>>   Reviewed-by: Shaoqin Huangmailto:shahuang@redhat.com
>>>   ---
>>>    accel/kvm/kvm-all.c    | 64 ++++++++++++++++++++++++++++++++----------
>>>    accel/kvm/trace-events |  5 +++-
>>>    include/sysemu/kvm.h   | 16 +++++++++++
>>>    3 files changed, 69 insertions(+), 16 deletions(-)
>>>
>>>   diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>>>   index a8cecd040e..3bc3207bda 100644
>>>   --- a/accel/kvm/kvm-all.c
>>>   +++ b/accel/kvm/kvm-all.c
>>>   @@ -126,6 +126,7 @@ static QemuMutex kml_slots_lock;
>>>    #define kvm_slots_unlock()  qemu_mutex_unlock(&kml_slots_lock)
>>>
>>>    static void kvm_slot_init_dirty_bitmap(KVMSlot *mem);
>>>   +static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id);
>>>
>>>    static inline void kvm_resample_fd_remove(int gsi)
>>>    {
>>>   @@ -314,14 +315,53 @@ err:
>>>   return ret;
>>>    }
>>>
>>>   +void kvm_park_vcpu(CPUState *cpu)
>>>   +{
>>>   +    struct KVMParkedVcpu *vcpu;
>>>   +
>>>   +    trace_kvm_park_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>>   It's good if we add kvm_fd to trace.
>>   It will be useful to cross verify kvm_get_vcpu()'s kvm_fd with parked vcpu.
>
> Agreed. But this is currently called in context to create and destroy vCPU
> where the trace already exists with the info you are seeking. Having
> trace here might duplicate the info and end up increasing the noise.
>
> Let me know if you think otherwise or have something else to add.

This is to provide additional information to the racing only.

The intention here is to trace mapping of vcpu_id<-->kvm_fd while parking

and fetching vcpu. This way we can easily trace what is parked 
(kvm_park_vcpu()) vs fetched (kvm_get_vcpu())

using pair of information.

>
> Thanks
>
>
>>>   +
>>>   +    vcpu = g_malloc0(sizeof(*vcpu));
>>>   +    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
>>>   +    vcpu->kvm_fd = cpu->kvm_fd;
>>>   +    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
>>>   +}
>>>   +
>>>   +int kvm_create_vcpu(CPUState *cpu)
>>>   +{
>>>   +    unsigned long vcpu_id = kvm_arch_vcpu_id(cpu);
>>>   +    KVMState *s = kvm_state;
>>>   +    int kvm_fd;
>>>   +
>>>   +    trace_kvm_create_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>>   vcpu_id can be used instead of kvm_arch_vcpu_id(cpu).
>
> KVM arch VCPU Id ensures that ID being traced is meaningful for that
> architecture. The way CPU ID gets calculated in on different architectures
> could be different. Hence, its value might be quite different.

vcpu_id is already being calculated just above trace call.

I don't think vcpu_id value gets differ by the time of tracing.

>
>>>   +
>>>   +    /* check if the KVM vCPU already exist but is parked */
>>>   +    kvm_fd = kvm_get_vcpu(s, vcpu_id);
>>>   +    if (kvm_fd < 0) {
>>>   +>   /* vCPU not parked: create a new KVM vCPU */
>>>   +>   kvm_fd = kvm_vm_ioctl(s, KVM_CREATE_VCPU, vcpu_id);
>>>   +>   if (kvm_fd < 0) {
>>>   +>       error_report("KVM_CREATE_VCPU IOCTL failed for vCPU %lu", vcpu_id);
>>>   +>       return kvm_fd;
>>>   +>   }
>>>   +    }
>>>   +
>>>   +    cpu->kvm_fd = kvm_fd;
>>>   +    cpu->kvm_state = s;
>>>   +    cpu->vcpu_dirty = true;
>>>   +    cpu->dirty_pages = 0;
>>>   +    cpu->throttle_us_per_full = 0;
>>>   +
>>>   +    return 0;
>>>   +}
>>>   +
>>>    static int do_kvm_destroy_vcpu(CPUState *cpu)
>>>    {
>>>        KVMState *s = kvm_state;
>>>        long mmap_size;
>>>   -    struct KVMParkedVcpu *vcpu = NULL;
>>>        int ret = 0;
>>>
>>>   -    trace_kvm_destroy_vcpu();
>>>   +    trace_kvm_destroy_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>>>
>>>        ret = kvm_arch_destroy_vcpu(cpu);
>>>        if (ret < 0) {
>>>   @@ -347,10 +387,7 @@ static int do_kvm_destroy_vcpu(CPUState *cpu)
>>>   >    }
>>>        }
>>>
>>>   -    vcpu = g_malloc0(sizeof(*vcpu));
>>>   -    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
>>>   -    vcpu->kvm_fd = cpu->kvm_fd;
>>>   -    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
>>>   +    kvm_park_vcpu(cpu);
>>>    err:
>>>        return ret;
>>>    }
>>>   @@ -371,6 +408,8 @@ static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
>>>   >    if (cpu->vcpu_id == vcpu_id) {
>>>   >        int kvm_fd;
>>>
>>>   +>       trace_kvm_get_vcpu(vcpu_id);
>>   It's good if we add kvm_fd to trace.
>>   It will be useful to cross verify kvm_get_vcpu's kvm_fd with parked vcpu.
>
> I can but I'm wondering why you've raised this? Perhaps, I'm not aware of the
> interface you are using to configure the VMs and how traces across diferent
> VMs get reflected. Please help in my understanding.

This is to provide additional information only not specific to any 
interface to configure VMs.

_Regards_,

-Vishnu

>
>>>   +
>>>   >        QLIST_REMOVE(cpu, node);
>>>   >        kvm_fd = cpu->kvm_fd;
>>>   >        g_free(cpu);
>>>   @@ -378,7 +417,7 @@ static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
>>>   >    }
>>>        }
>>>
>>>   -    return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
>>>   +    return -ENOENT;
>>>    }
>>>
>>>    int kvm_init_vcpu(CPUState *cpu, Error **errp)
>>>   @@ -389,19 +428,14 @@ int kvm_init_vcpu(CPUState *cpu, Error **errp)
>>>
>>>        trace_kvm_init_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>>>
>>>   -    ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu));
>>>   +    ret = kvm_create_vcpu(cpu);
>>>        if (ret < 0) {
>>>   -   error_setg_errno(errp, -ret, "kvm_init_vcpu: kvm_get_vcpu failed (%lu)",
>>>   +   error_setg_errno(errp, -ret,
>>>   +          "kvm_init_vcpu: kvm_create_vcpu failed (%lu)",
>>              kvm_arch_vcpu_id(cpu));
>>>       goto err;
>>>        }
>>>
>>>   -    cpu->kvm_fd = ret;
>>>   -    cpu->kvm_state = s;
>>>   -    cpu->vcpu_dirty = true;
>>>   -    cpu->dirty_pages = 0;
>>>   -    cpu->throttle_us_per_full = 0;
>>>   -
>>>        mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
>>>        if (mmap_size < 0) {
>>>       ret = mmap_size;
>>>   diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events
>>>   index a25902597b..5558cff0dc 100644
>>>   --- a/accel/kvm/trace-events
>>>   +++ b/accel/kvm/trace-events
>>>   @@ -9,6 +9,10 @@ kvm_device_ioctl(int fd, int type, void *arg) "dev fd %d, type 0x%x, arg %p"
>>>    kvm_failed_reg_get(uint64_t id, const char *msg) "Warning: Unable to retrieve ONEREG %" PRIu64 " from KVM: %s"
>>>    kvm_failed_reg_set(uint64_t id, const char *msg) "Warning: Unable to set ONEREG %" PRIu64 " to KVM: %s"
>>>    kvm_init_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
>>>   +kvm_create_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
>>>   +kvm_get_vcpu(unsigned long arch_cpu_id) "id: %lu"
>>>   +kvm_destroy_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
>>>   +kvm_park_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
>>>    kvm_irqchip_commit_routes(void) ""
>>>    kvm_irqchip_add_msi_route(char *name, int vector, int virq) "dev %s vector %d virq %d"
>>>    kvm_irqchip_update_msi_route(int virq) "Updating MSI route virq=%d"
>>>   @@ -25,7 +29,6 @@ kvm_dirty_ring_reaper(const char *s) "%s"
>>>    kvm_dirty_ring_reap(uint64_t count, int64_t t) "reaped %"PRIu64" pages (took %"PRIi64" us)"
>>>    kvm_dirty_ring_reaper_kick(const char *reason) "%s"
>>>    kvm_dirty_ring_flush(int finished) "%d"
>>>   -kvm_destroy_vcpu(void) ""
>>>    kvm_failed_get_vcpu_mmap_size(void) ""
>>>    kvm_cpu_exec(void) ""
>>>    kvm_interrupt_exit_request(void) ""
>>>   diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
>>>   index fad9a7e8ff..2ed928aa71 100644
>>>   --- a/include/sysemu/kvm.h
>>>   +++ b/include/sysemu/kvm.h
>>>   @@ -435,6 +435,22 @@ void kvm_set_sigmask_len(KVMState *s, unsigned int sigmask_len);
>>>    int kvm_physical_memory_addr_from_host(KVMState *s, void *ram_addr,
>>>   >  >  >  >  >       hwaddr *phys_addr);
>>>
>>>   +/**
>>>   + * kvm_create_vcpu - Gets a parked KVM vCPU or creates a KVM vCPU
>>>   + * @cpu: QOM CPUState object for which KVM vCPU has to be fetched/created.
>>>   + *
>>>   + * @returns: 0 when success, errno (<0) when failed.
>>>   + */
>>>   +int kvm_create_vcpu(CPUState *cpu);
>>>   +
>>>   +/**
>>>   + * kvm_park_vcpu - Park QEMU KVM vCPU context
>>>   + * @cpu: QOM CPUState object for which QEMU KVM vCPU context has to be parked.
>>>   + *
>>>   + * @returns: none
>>>   + */
>>>   +void kvm_park_vcpu(CPUState *cpu);
>>>   +
>>>    #endif /* NEED_CPU_H */
>>>
>>>    void kvm_cpu_synchronize_state(CPUState *cpu);
>>   Otherwise, Looks good to me.  Feel free to add
>>   Reviewed-by: "Vishnu Pajjuri"mailto:vishnu@os.amperecomputing.com
>>   Thanks,
> Thanks.
> Salil
>
>
>
>>   -Vishnu

[-- Attachment #2: Type: text/html, Size: 12651 bytes --]

Re: [PATCH V8 1/8] accel/kvm: Extract common KVM vCPU {creation,parking} code

[Salil Mehta]

[-- Attachment #1: Type: text/plain, Size: 10413 bytes --]

HI Vishnu,

On Tue, May 7, 2024 at 12:39 PM Vishnu Pajjuri <
vishnu@amperemail.onmicrosoft.com> wrote:

> Hi Salil,
> On 03-05-2024 21:53, Salil Mehta wrote:
>
> [EXTERNAL EMAIL NOTICE: This email originated from an external sender. Please be mindful of safe email handling and proprietary information protection practices.]
>
>
> Hi Vishnu,
>
>
>  From: Vishnu Pajjuri <vishnu@amperemail.onmicrosoft.com> <vishnu@amperemail.onmicrosoft.com>
>  Sent: Thursday, April 4, 2024 3:00 PM
>  Subject: Re: [PATCH V8 1/8] accel/kvm: Extract common KVM vCPU {creation,parking} code
>
>  Hi Salil,
>
>  On 12-03-2024 07:29, Salil Mehta wrote:
>  KVM vCPU creation is done once during the vCPU realization when Qemu vCPU thread
>  is spawned. This is common to all the architectures as of now.
>
>  Hot-unplug of vCPU results in destruction of the vCPU object in QOM but the
>  corresponding KVM vCPU object in the Host KVM is not destroyed as KVM doesn't
>  support vCPU removal. Therefore, its representative KVM vCPU object/context in
>  Qemu is parked.
>
>  Refactor architecture common logic so that some APIs could be reused by vCPU
>  Hotplug code of some architectures likes ARM, Loongson etc. Update new/old APIs
>  with trace events instead of DPRINTF. No functional change is intended here.
>
>  Signed-off-by: Salil Mehta mailto:salil.mehta@huawei.com <salil.mehta@huawei.com>
>  Reviewed-by: Gavin Shan mailto:gshan@redhat.com <gshan@redhat.com>
>  Tested-by: Vishnu Pajjuri mailto:vishnu@os.amperecomputing.com <vishnu@os.amperecomputing.com>
>  Reviewed-by: Jonathan Cameron mailto:Jonathan.Cameron@huawei.com <Jonathan.Cameron@huawei.com>
>  Tested-by: Xianglai Li mailto:lixianglai@loongson.cn <lixianglai@loongson.cn>
>  Tested-by: Miguel Luis mailto:miguel.luis@oracle.com <miguel.luis@oracle.com>
>  Reviewed-by: Shaoqin Huang mailto:shahuang@redhat.com <shahuang@redhat.com>
>  ---
>   accel/kvm/kvm-all.c    | 64 ++++++++++++++++++++++++++++++++----------
>   accel/kvm/trace-events |  5 +++-
>   include/sysemu/kvm.h   | 16 +++++++++++
>   3 files changed, 69 insertions(+), 16 deletions(-)
>
>  diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
>  index a8cecd040e..3bc3207bda 100644
>  --- a/accel/kvm/kvm-all.c
>  +++ b/accel/kvm/kvm-all.c
>  @@ -126,6 +126,7 @@ static QemuMutex kml_slots_lock;
>   #define kvm_slots_unlock()  qemu_mutex_unlock(&kml_slots_lock)
>
>   static void kvm_slot_init_dirty_bitmap(KVMSlot *mem);
>  +static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id);
>
>   static inline void kvm_resample_fd_remove(int gsi)
>   {
>  @@ -314,14 +315,53 @@ err:
>  return ret;
>   }
>
>  +void kvm_park_vcpu(CPUState *cpu)
>  +{
>  +    struct KVMParkedVcpu *vcpu;
>  +
>  +    trace_kvm_park_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>
>  It's good if we add kvm_fd to trace.
>  It will be useful to cross verify kvm_get_vcpu()'s kvm_fd with parked vcpu.
>
> Agreed. But this is currently called in context to create and destroy vCPU
> where the trace already exists with the info you are seeking. Having
> trace here might duplicate the info and end up increasing the noise.
>
> Let me know if you think otherwise or have something else to add.
>
> This is to provide additional information to the racing only.
>
> The intention here is to trace mapping of vcpu_id<-->kvm_fd while parking
>
> and fetching vcpu. This way we can easily trace what is parked
> (kvm_park_vcpu()) vs fetched (kvm_get_vcpu())
>
> using pair of information.
>

Ok, No problem. I will.


> Thanks
>
>
>
>  +
>  +    vcpu = g_malloc0(sizeof(*vcpu));
>  +    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
>  +    vcpu->kvm_fd = cpu->kvm_fd;
>  +    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
>  +}
>  +
>  +int kvm_create_vcpu(CPUState *cpu)
>  +{
>  +    unsigned long vcpu_id = kvm_arch_vcpu_id(cpu);
>  +    KVMState *s = kvm_state;
>  +    int kvm_fd;
>  +
>  +    trace_kvm_create_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>
>  vcpu_id can be used instead of kvm_arch_vcpu_id(cpu).
>
> KVM arch VCPU Id ensures that ID being traced is meaningful for that
> architecture. The way CPU ID gets calculated in on different architectures
> could be different. Hence, its value might be quite different.
>
> vcpu_id is already being calculated just above trace call.
>
> I don't think vcpu_id value gets differ by the time of tracing.
>

sure.


>  +
>  +    /* check if the KVM vCPU already exist but is parked */
>  +    kvm_fd = kvm_get_vcpu(s, vcpu_id);
>  +    if (kvm_fd < 0) {
>  +>   /* vCPU not parked: create a new KVM vCPU */
>  +>   kvm_fd = kvm_vm_ioctl(s, KVM_CREATE_VCPU, vcpu_id);
>  +>   if (kvm_fd < 0) {
>  +>       error_report("KVM_CREATE_VCPU IOCTL failed for vCPU %lu", vcpu_id);
>  +>       return kvm_fd;
>  +>   }
>  +    }
>  +
>  +    cpu->kvm_fd = kvm_fd;
>  +    cpu->kvm_state = s;
>  +    cpu->vcpu_dirty = true;
>  +    cpu->dirty_pages = 0;
>  +    cpu->throttle_us_per_full = 0;
>  +
>  +    return 0;
>  +}
>  +
>   static int do_kvm_destroy_vcpu(CPUState *cpu)
>   {
>       KVMState *s = kvm_state;
>       long mmap_size;
>  -    struct KVMParkedVcpu *vcpu = NULL;
>       int ret = 0;
>
>  -    trace_kvm_destroy_vcpu();
>  +    trace_kvm_destroy_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>
>       ret = kvm_arch_destroy_vcpu(cpu);
>       if (ret < 0) {
>  @@ -347,10 +387,7 @@ static int do_kvm_destroy_vcpu(CPUState *cpu)
>  >    }
>       }
>
>  -    vcpu = g_malloc0(sizeof(*vcpu));
>  -    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
>  -    vcpu->kvm_fd = cpu->kvm_fd;
>  -    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
>  +    kvm_park_vcpu(cpu);
>   err:
>       return ret;
>   }
>  @@ -371,6 +408,8 @@ static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
>  >    if (cpu->vcpu_id == vcpu_id) {
>  >        int kvm_fd;
>
>  +>       trace_kvm_get_vcpu(vcpu_id);
>
>  It's good if we add kvm_fd to trace.
>  It will be useful to cross verify kvm_get_vcpu's kvm_fd with parked vcpu.
>
> I can but I'm wondering why you've raised this? Perhaps, I'm not aware of the
> interface you are using to configure the VMs and how traces across diferent
> VMs get reflected. Please help in my understanding.
>
> This is to provide additional information only not specific to any
> interface to configure VMs.
>


Ok. sure.


Thanks
Salil.



> *Regards*,
>
> -Vishnu
>
>  +
>  >        QLIST_REMOVE(cpu, node);
>  >        kvm_fd = cpu->kvm_fd;
>  >        g_free(cpu);
>  @@ -378,7 +417,7 @@ static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
>  >    }
>       }
>
>  -    return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
>  +    return -ENOENT;
>   }
>
>   int kvm_init_vcpu(CPUState *cpu, Error **errp)
>  @@ -389,19 +428,14 @@ int kvm_init_vcpu(CPUState *cpu, Error **errp)
>
>       trace_kvm_init_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>
>  -    ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu));
>  +    ret = kvm_create_vcpu(cpu);
>       if (ret < 0) {
>  -   error_setg_errno(errp, -ret, "kvm_init_vcpu: kvm_get_vcpu failed (%lu)",
>  +   error_setg_errno(errp, -ret,
>  +          "kvm_init_vcpu: kvm_create_vcpu failed (%lu)",
>
>             kvm_arch_vcpu_id(cpu));
>
>      goto err;
>       }
>
>  -    cpu->kvm_fd = ret;
>  -    cpu->kvm_state = s;
>  -    cpu->vcpu_dirty = true;
>  -    cpu->dirty_pages = 0;
>  -    cpu->throttle_us_per_full = 0;
>  -
>       mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
>       if (mmap_size < 0) {
>      ret = mmap_size;
>  diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events
>  index a25902597b..5558cff0dc 100644
>  --- a/accel/kvm/trace-events
>  +++ b/accel/kvm/trace-events
>  @@ -9,6 +9,10 @@ kvm_device_ioctl(int fd, int type, void *arg) "dev fd %d, type 0x%x, arg %p"
>   kvm_failed_reg_get(uint64_t id, const char *msg) "Warning: Unable to retrieve ONEREG %" PRIu64 " from KVM: %s"
>   kvm_failed_reg_set(uint64_t id, const char *msg) "Warning: Unable to set ONEREG %" PRIu64 " to KVM: %s"
>   kvm_init_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
>  +kvm_create_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
>  +kvm_get_vcpu(unsigned long arch_cpu_id) "id: %lu"
>  +kvm_destroy_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
>  +kvm_park_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
>   kvm_irqchip_commit_routes(void) ""
>   kvm_irqchip_add_msi_route(char *name, int vector, int virq) "dev %s vector %d virq %d"
>   kvm_irqchip_update_msi_route(int virq) "Updating MSI route virq=%d"
>  @@ -25,7 +29,6 @@ kvm_dirty_ring_reaper(const char *s) "%s"
>   kvm_dirty_ring_reap(uint64_t count, int64_t t) "reaped %"PRIu64" pages (took %"PRIi64" us)"
>   kvm_dirty_ring_reaper_kick(const char *reason) "%s"
>   kvm_dirty_ring_flush(int finished) "%d"
>  -kvm_destroy_vcpu(void) ""
>   kvm_failed_get_vcpu_mmap_size(void) ""
>   kvm_cpu_exec(void) ""
>   kvm_interrupt_exit_request(void) ""
>  diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
>  index fad9a7e8ff..2ed928aa71 100644
>  --- a/include/sysemu/kvm.h
>  +++ b/include/sysemu/kvm.h
>  @@ -435,6 +435,22 @@ void kvm_set_sigmask_len(KVMState *s, unsigned int sigmask_len);
>   int kvm_physical_memory_addr_from_host(KVMState *s, void *ram_addr,
>  >  >  >  >  >       hwaddr *phys_addr);
>
>  +/**
>  + * kvm_create_vcpu - Gets a parked KVM vCPU or creates a KVM vCPU
>  + * @cpu: QOM CPUState object for which KVM vCPU has to be fetched/created.
>  + *
>  + * @returns: 0 when success, errno (<0) when failed.
>  + */
>  +int kvm_create_vcpu(CPUState *cpu);
>  +
>  +/**
>  + * kvm_park_vcpu - Park QEMU KVM vCPU context
>  + * @cpu: QOM CPUState object for which QEMU KVM vCPU context has to be parked.
>  + *
>  + * @returns: none
>  + */
>  +void kvm_park_vcpu(CPUState *cpu);
>  +
>   #endif /* NEED_CPU_H */
>
>   void kvm_cpu_synchronize_state(CPUState *cpu);
>
>  Otherwise, Looks good to me.  Feel free to add
>  Reviewed-by: "Vishnu Pajjuri" mailto:vishnu@os.amperecomputing.com <vishnu@os.amperecomputing.com>
>  Thanks,
>
> Thanks.
> Salil
>
>
>
>
>  -Vishnu
>
>

[-- Attachment #2: Type: text/html, Size: 12969 bytes --]

RE: [PATCH V8 1/8] accel/kvm: Extract common KVM vCPU {creation,parking} code

[Salil Mehta via]

Hi Phillipe,

Sorry, I missed this mail earlier.

>  From: Philippe Mathieu-Daudé <philmd@linaro.org>
>  Sent: Friday, May 3, 2024 7:23 PM
>  To: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org;
>  qemu-arm@nongnu.org
>  
>  On 3/5/24 17:57, Salil Mehta wrote:
>  > Hi Philippe,
>  >
>  >>   From: Philippe Mathieu-Daudé <philmd@linaro.org>
>  >>   Sent: Friday, May 3, 2024 10:40 AM
>  >>   Subject: Re: [PATCH V8 1/8] accel/kvm: Extract common KVM vCPU
>  >>   {creation,parking} code
>  >>
>  >>   Hi Salil,
>  >>
>  >>   On 12/3/24 02:59, Salil Mehta wrote:
>  >>   > KVM vCPU creation is done once during the vCPU realization when Qemu
>  >>   > vCPU thread is spawned. This is common to all the architectures as of now.
>  >>   >
>  >>   > Hot-unplug of vCPU results in destruction of the vCPU object in QOM
>  >>   > but the corresponding KVM vCPU object in the Host KVM is not destroyed
>  >>   > as KVM doesn't support vCPU removal. Therefore, its representative KVM
>  >>   > vCPU object/context in Qemu is parked.
>  >>   >
>  >>   > Refactor architecture common logic so that some APIs could be reused
>  >>   > by vCPU Hotplug code of some architectures likes ARM, Loongson etc.
>  >>   > Update new/old APIs with trace events instead of DPRINTF. No functional
>  >>   change is intended here.
>  >>   >
>  >>   > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
>  >>   > Reviewed-by: Gavin Shan <gshan@redhat.com>
>  >>   > Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
>  >>   > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>  >>   > Tested-by: Xianglai Li <lixianglai@loongson.cn>
>  >>   > Tested-by: Miguel Luis <miguel.luis@oracle.com>
>  >>   > Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
>  >>   > ---
>  >>   >   accel/kvm/kvm-all.c    | 64 ++++++++++++++++++++++++++++++++-
>  -----
>  >>   ----
>  >>   >   accel/kvm/trace-events |  5 +++-
>  >>   >   include/sysemu/kvm.h   | 16 +++++++++++
>  >>   >   3 files changed, 69 insertions(+), 16 deletions(-)
>  >>   >
>  >>   > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index
>  >>   > a8cecd040e..3bc3207bda 100644
>  >>   > --- a/accel/kvm/kvm-all.c
>  >>   > +++ b/accel/kvm/kvm-all.c
>  >>   > @@ -126,6 +126,7 @@ static QemuMutex kml_slots_lock;
>  >>   >   #define kvm_slots_unlock()  qemu_mutex_unlock(&kml_slots_lock)
>  >>   >
>  >>   >   static void kvm_slot_init_dirty_bitmap(KVMSlot *mem);
>  >>   > +static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id);
>  >>   >
>  >>   >   static inline void kvm_resample_fd_remove(int gsi)
>  >>   >   {
>  >>   > @@ -314,14 +315,53 @@ err:
>  >>   >       return ret;
>  >>   >   }
>  >>   >
>  >>   > +void kvm_park_vcpu(CPUState *cpu)
>  >>   > +{
>  >>   > +    struct KVMParkedVcpu *vcpu;
>  >>   > +
>  >>   > +    trace_kvm_park_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>  >>   > +
>  >>   > +    vcpu = g_malloc0(sizeof(*vcpu));
>  >>   > +    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
>  >>   > +    vcpu->kvm_fd = cpu->kvm_fd;
>  >>   > +    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node); }
>  >>   > +
>  >>   > +int kvm_create_vcpu(CPUState *cpu)
>  >>   > +{
>  >>   > +    unsigned long vcpu_id = kvm_arch_vcpu_id(cpu);
>  >>   > +    KVMState *s = kvm_state;
>  >>   > +    int kvm_fd;
>  >>   > +
>  >>   > +    trace_kvm_create_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>  >>   > +
>  >>   > +    /* check if the KVM vCPU already exist but is parked */
>  >>   > +    kvm_fd = kvm_get_vcpu(s, vcpu_id);
>  >>   > +    if (kvm_fd < 0) {
>  >>   > +        /* vCPU not parked: create a new KVM vCPU */
>  >>   > +        kvm_fd = kvm_vm_ioctl(s, KVM_CREATE_VCPU, vcpu_id);
>  >>   > +        if (kvm_fd < 0) {
>  >>   > +            error_report("KVM_CREATE_VCPU IOCTL failed for vCPU %lu", vcpu_id);
>  >>   > +            return kvm_fd;
>  >>   > +        }
>  >>   > +    }
>  >>   > +
>  >>   > +    cpu->kvm_fd = kvm_fd;
>  >>   > +    cpu->kvm_state = s;
>  >>   > +    cpu->vcpu_dirty = true;
>  >>   > +    cpu->dirty_pages = 0;
>  >>   > +    cpu->throttle_us_per_full = 0;
>  >>   > +
>  >>   > +    return 0;
>  >>   > +}
>  >>
>  >>   This seems generic enough to be implemented for all accelerators.
>  >>
>  >>   See AccelOpsClass in include/sysemu/accel-ops.h.
>  >>
>  >>   That said, can be done later on top.
>  >
>  > Let me understand correctly. Are you suggesting to implement above
>  > even for HVF, TCG, QTEST etc?
>  
>  Not for you to implement the other non-KVM accelerators, but since you
>  are introducing this, now is a good time to think about a generic interface.
>  
>  So far AccelOpsClass::[un]park_vcpu() handlers make sense to me.

Sure, but what is the advantage of defining these 'supporting' functions
as part of the AccelOpsClass? Each of these functions in any case will need
to be defined individually for different Accelerators or unless we are
planning to extract some common accelerator functions in a separate file
and use them across all the accelerators?

I'm surely missing some key point here.

Thanks
Salil.

Re: [PATCH V8 1/8] accel/kvm: Extract common KVM vCPU {creation,parking} code

[Philippe Mathieu-Daudé]

On 8/5/24 12:46, Salil Mehta wrote:
> Hi Phillipe,
> 
> Sorry, I missed this mail earlier.
> 
>>   From: Philippe Mathieu-Daudé <philmd@linaro.org>
>>   Sent: Friday, May 3, 2024 7:23 PM
>>   To: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org;
>>   qemu-arm@nongnu.org
>>   
>>   On 3/5/24 17:57, Salil Mehta wrote:
>>   > Hi Philippe,
>>   >
>>   >>   From: Philippe Mathieu-Daudé <philmd@linaro.org>
>>   >>   Sent: Friday, May 3, 2024 10:40 AM
>>   >>   Subject: Re: [PATCH V8 1/8] accel/kvm: Extract common KVM vCPU
>>   >>   {creation,parking} code
>>   >>
>>   >>   Hi Salil,
>>   >>
>>   >>   On 12/3/24 02:59, Salil Mehta wrote:
>>   >>   > KVM vCPU creation is done once during the vCPU realization when Qemu
>>   >>   > vCPU thread is spawned. This is common to all the architectures as of now.
>>   >>   >
>>   >>   > Hot-unplug of vCPU results in destruction of the vCPU object in QOM
>>   >>   > but the corresponding KVM vCPU object in the Host KVM is not destroyed
>>   >>   > as KVM doesn't support vCPU removal. Therefore, its representative KVM
>>   >>   > vCPU object/context in Qemu is parked.
>>   >>   >
>>   >>   > Refactor architecture common logic so that some APIs could be reused
>>   >>   > by vCPU Hotplug code of some architectures likes ARM, Loongson etc.
>>   >>   > Update new/old APIs with trace events instead of DPRINTF. No functional
>>   >>   change is intended here.
>>   >>   >
>>   >>   > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
>>   >>   > Reviewed-by: Gavin Shan <gshan@redhat.com>
>>   >>   > Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
>>   >>   > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>   >>   > Tested-by: Xianglai Li <lixianglai@loongson.cn>
>>   >>   > Tested-by: Miguel Luis <miguel.luis@oracle.com>
>>   >>   > Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
>>   >>   > ---
>>   >>   >   accel/kvm/kvm-all.c    | 64 ++++++++++++++++++++++++++++++++-
>>   -----
>>   >>   ----
>>   >>   >   accel/kvm/trace-events |  5 +++-
>>   >>   >   include/sysemu/kvm.h   | 16 +++++++++++
>>   >>   >   3 files changed, 69 insertions(+), 16 deletions(-)
>>   >>   >
>>   >>   > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index
>>   >>   > a8cecd040e..3bc3207bda 100644
>>   >>   > --- a/accel/kvm/kvm-all.c
>>   >>   > +++ b/accel/kvm/kvm-all.c
>>   >>   > @@ -126,6 +126,7 @@ static QemuMutex kml_slots_lock;
>>   >>   >   #define kvm_slots_unlock()  qemu_mutex_unlock(&kml_slots_lock)
>>   >>   >
>>   >>   >   static void kvm_slot_init_dirty_bitmap(KVMSlot *mem);
>>   >>   > +static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id);
>>   >>   >
>>   >>   >   static inline void kvm_resample_fd_remove(int gsi)
>>   >>   >   {
>>   >>   > @@ -314,14 +315,53 @@ err:
>>   >>   >       return ret;
>>   >>   >   }
>>   >>   >
>>   >>   > +void kvm_park_vcpu(CPUState *cpu)
>>   >>   > +{
>>   >>   > +    struct KVMParkedVcpu *vcpu;
>>   >>   > +
>>   >>   > +    trace_kvm_park_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>>   >>   > +
>>   >>   > +    vcpu = g_malloc0(sizeof(*vcpu));
>>   >>   > +    vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
>>   >>   > +    vcpu->kvm_fd = cpu->kvm_fd;
>>   >>   > +    QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node); }
>>   >>   > +
>>   >>   > +int kvm_create_vcpu(CPUState *cpu)
>>   >>   > +{
>>   >>   > +    unsigned long vcpu_id = kvm_arch_vcpu_id(cpu);
>>   >>   > +    KVMState *s = kvm_state;
>>   >>   > +    int kvm_fd;
>>   >>   > +
>>   >>   > +    trace_kvm_create_vcpu(cpu->cpu_index, kvm_arch_vcpu_id(cpu));
>>   >>   > +
>>   >>   > +    /* check if the KVM vCPU already exist but is parked */
>>   >>   > +    kvm_fd = kvm_get_vcpu(s, vcpu_id);
>>   >>   > +    if (kvm_fd < 0) {
>>   >>   > +        /* vCPU not parked: create a new KVM vCPU */
>>   >>   > +        kvm_fd = kvm_vm_ioctl(s, KVM_CREATE_VCPU, vcpu_id);
>>   >>   > +        if (kvm_fd < 0) {
>>   >>   > +            error_report("KVM_CREATE_VCPU IOCTL failed for vCPU %lu", vcpu_id);
>>   >>   > +            return kvm_fd;
>>   >>   > +        }
>>   >>   > +    }
>>   >>   > +
>>   >>   > +    cpu->kvm_fd = kvm_fd;
>>   >>   > +    cpu->kvm_state = s;
>>   >>   > +    cpu->vcpu_dirty = true;
>>   >>   > +    cpu->dirty_pages = 0;
>>   >>   > +    cpu->throttle_us_per_full = 0;
>>   >>   > +
>>   >>   > +    return 0;
>>   >>   > +}
>>   >>
>>   >>   This seems generic enough to be implemented for all accelerators.
>>   >>
>>   >>   See AccelOpsClass in include/sysemu/accel-ops.h.
>>   >>
>>   >>   That said, can be done later on top.
>>   >
>>   > Let me understand correctly. Are you suggesting to implement above
>>   > even for HVF, TCG, QTEST etc?
>>   
>>   Not for you to implement the other non-KVM accelerators, but since you
>>   are introducing this, now is a good time to think about a generic interface.
>>   
>>   So far AccelOpsClass::[un]park_vcpu() handlers make sense to me.
> 
> Sure, but what is the advantage of defining these 'supporting' functions
> as part of the AccelOpsClass? Each of these functions in any case will need
> to be defined individually for different Accelerators or unless we are
> planning to extract some common accelerator functions in a separate file
> and use them across all the accelerators?

kvm_arm_create_host_vcpu() [*] seems generic. Maybe we could do the
same with HVF at least.

[*] 
https://lore.kernel.org/qemu-devel/20230926100436.28284-7-salil.mehta@huawei.com/
> 
> I'm surely missing some key point here.

I started https://etherpad.opendev.org/p/QEMU_vCPU_life to
document the vCPU spagetti code. In that big picture the "park"
method makes sense to me, but we can discuss that later. Again,
certainly not a block for your work, I'm just trying to see
the whole view.

Regards,

Phil.