* [PATCH V10 0/8] Add architecture agnostic code to support vCPU Hotplug
@ 2024-05-20 23:32 Salil Mehta via
2024-05-20 23:32 ` [PATCH V10 1/8] accel/kvm: Extract common KVM vCPU {creation, parking} code Salil Mehta via
` (7 more replies)
0 siblings, 8 replies; 15+ messages in thread
From: Salil Mehta via @ 2024-05-20 23:32 UTC (permalink / raw
To: qemu-devel, qemu-arm
Cc: salil.mehta, maz, jean-philippe, jonathan.cameron, lpieralisi,
peter.maydell, richard.henderson, imammedo, andrew.jones, david,
philmd, eric.auger, oliver.upton, pbonzini, mst, will, gshan,
rafael, alex.bennee, linux, darren, ilkka, vishnu, karl.heubaum,
miguel.luis, salil.mehta, zhukeqian1, wangxiongfeng2, wangyanan55,
jiakernel2, maobibo, lixianglai, npiggin, harshpb, linuxarm
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-v3.arch.agnostic.v10
NOTE: For ARM, above will work in combination of the architecture specific part based on
RFC V2 [1]. This architecture specific patch-set RFC V3 shall be floated soon and is present
at below location
[*] https://github.com/salil-mehta/qemu/tree/virt-cpuhp-armv8/rfc-v3-rc1
Revision History:
Patch-set V9 -> V10
1. Addressed Nicholas Piggin's (IBM) & Philippe Mathieu-Daudé (Linaro) comments
- carved out kvm_unpark_vcpu and added its trace
- Widened the scope of the kvm_unpark_vcpu so that it can be used by generic framework
being thought out
Link: https://lore.kernel.org/qemu-devel/20240519210620.228342-1-salil.mehta@huawei.com/
Link: https://lore.kernel.org/qemu-devel/e94b0e14-efee-4050-9c9f-08382a36b63a@linaro.org/
Patch-set V8 -> V9
1. Addressed Vishnu Pajjuri's (Ampere) comments
- Added kvm_fd to trace in kvm_create_vcpu
- Some clean ups: arch vcpu-id and sbd variable
- Added the missed initialization of cpu->gdb_num_regs
2. Addressed the commnet from Zhao Liu (Intel)
- Make initialization of CPU Hotplug state conditional (possible_cpu_arch_ids!=NULL)
Link: https://lore.kernel.org/qemu-devel/20240312020000.12992-1-salil.mehta@huawei.com/
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 | 97 +++++++++++++++++---------
accel/kvm/kvm-cpus.h | 23 ++++++
accel/kvm/trace-events | 5 +-
docs/specs/acpi_hw_reduced_hotplug.rst | 3 +-
gdbstub/gdbstub.c | 13 ++++
hw/acpi/acpi-cpu-hotplug-stub.c | 6 ++
hw/acpi/cpu.c | 33 ++++++---
hw/acpi/generic_event_device.c | 21 ++++++
hw/core/cpu-common.c | 1 -
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 +
system/physmem.c | 29 ++++++++
17 files changed, 214 insertions(+), 48 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH V10 1/8] accel/kvm: Extract common KVM vCPU {creation, parking} code
2024-05-20 23:32 [PATCH V10 0/8] Add architecture agnostic code to support vCPU Hotplug Salil Mehta via
@ 2024-05-20 23:32 ` Salil Mehta via
2024-05-22 1:25 ` [PATCH V10 1/8] accel/kvm: Extract common KVM vCPU {creation,parking} code Nicholas Piggin
2024-05-20 23:32 ` [PATCH V10 2/8] hw/acpi: Move CPU ctrl-dev MMIO region len macro to common header file Salil Mehta via
` (6 subsequent siblings)
7 siblings, 1 reply; 15+ messages in thread
From: Salil Mehta via @ 2024-05-20 23:32 UTC (permalink / raw
To: qemu-devel, qemu-arm
Cc: salil.mehta, maz, jean-philippe, jonathan.cameron, lpieralisi,
peter.maydell, richard.henderson, imammedo, andrew.jones, david,
philmd, eric.auger, oliver.upton, pbonzini, mst, will, gshan,
rafael, alex.bennee, linux, darren, ilkka, vishnu, karl.heubaum,
miguel.luis, salil.mehta, zhukeqian1, wangxiongfeng2, wangyanan55,
jiakernel2, maobibo, lixianglai, npiggin, harshpb, linuxarm,
Jonathan Cameron, Shaoqin Huang
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. 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>
Reviewed-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
---
accel/kvm/kvm-all.c | 97 ++++++++++++++++++++++++++++--------------
accel/kvm/kvm-cpus.h | 23 ++++++++++
accel/kvm/trace-events | 5 ++-
3 files changed, 92 insertions(+), 33 deletions(-)
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index c0be9f5eed..a8f93078dc 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -340,14 +340,73 @@ 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_unpark_vcpu(KVMState *s, unsigned long vcpu_id)
+{
+ struct KVMParkedVcpu *cpu;
+
+ QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
+ if (cpu->vcpu_id == vcpu_id) {
+ int kvm_fd;
+
+ trace_kvm_unpark_vcpu(vcpu_id);
+
+ QLIST_REMOVE(cpu, node);
+ kvm_fd = cpu->kvm_fd;
+ g_free(cpu);
+ return kvm_fd;
+ }
+ }
+
+ return -ENOENT;
+}
+
+int kvm_create_vcpu(CPUState *cpu)
+{
+ unsigned long vcpu_id = kvm_arch_vcpu_id(cpu);
+ KVMState *s = kvm_state;
+ int kvm_fd;
+
+ /* check if the KVM vCPU already exist but is parked */
+ kvm_fd = kvm_unpark_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;
+ }
+ }
+
+ trace_kvm_create_vcpu(cpu->cpu_index, vcpu_id, 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) {
@@ -373,10 +432,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;
}
@@ -389,24 +445,6 @@ void kvm_destroy_vcpu(CPUState *cpu)
}
}
-static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
-{
- struct KVMParkedVcpu *cpu;
-
- QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
- if (cpu->vcpu_id == vcpu_id) {
- int kvm_fd;
-
- QLIST_REMOVE(cpu, node);
- kvm_fd = cpu->kvm_fd;
- g_free(cpu);
- return kvm_fd;
- }
- }
-
- return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
-}
-
int kvm_init_vcpu(CPUState *cpu, Error **errp)
{
KVMState *s = kvm_state;
@@ -415,19 +453,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/kvm-cpus.h b/accel/kvm/kvm-cpus.h
index ca40add32c..2e6bb38b5d 100644
--- a/accel/kvm/kvm-cpus.h
+++ b/accel/kvm/kvm-cpus.h
@@ -22,5 +22,28 @@ bool kvm_supports_guest_debug(void);
int kvm_insert_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr len);
int kvm_remove_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr len);
void kvm_remove_all_breakpoints(CPUState *cpu);
+/**
+ * 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);
+
+/**
+ * kvm_unpark_vcpu - unpark QEMU KVM vCPU context
+ * @s: KVM State
+ * @cpu: Architecture vCPU ID of the parked vCPU
+ *
+ * @returns: KVM fd
+ */
+int kvm_unpark_vcpu(KVMState *s, unsigned long vcpu_id);
#endif /* KVM_CPUS_H */
diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events
index 681ccb667d..bd43a0ef26 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, int kvm_fd) "index: %d, id: %lu, kvm fd: %d"
+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_unpark_vcpu(unsigned long arch_cpu_id) "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) ""
--
2.34.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH V10 2/8] hw/acpi: Move CPU ctrl-dev MMIO region len macro to common header file
2024-05-20 23:32 [PATCH V10 0/8] Add architecture agnostic code to support vCPU Hotplug Salil Mehta via
2024-05-20 23:32 ` [PATCH V10 1/8] accel/kvm: Extract common KVM vCPU {creation, parking} code Salil Mehta via
@ 2024-05-20 23:32 ` Salil Mehta via
2024-05-20 23:32 ` [PATCH V10 3/8] hw/acpi: Update ACPI GED framework to support vCPU Hotplug Salil Mehta via
` (5 subsequent siblings)
7 siblings, 0 replies; 15+ messages in thread
From: Salil Mehta via @ 2024-05-20 23:32 UTC (permalink / raw
To: qemu-devel, qemu-arm
Cc: salil.mehta, maz, jean-philippe, jonathan.cameron, lpieralisi,
peter.maydell, richard.henderson, imammedo, andrew.jones, david,
philmd, eric.auger, oliver.upton, pbonzini, mst, will, gshan,
rafael, alex.bennee, linux, darren, ilkka, vishnu, karl.heubaum,
miguel.luis, salil.mehta, zhukeqian1, wangxiongfeng2, wangyanan55,
jiakernel2, maobibo, lixianglai, npiggin, harshpb, linuxarm,
Jonathan Cameron, Shaoqin Huang
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH V10 3/8] hw/acpi: Update ACPI GED framework to support vCPU Hotplug
2024-05-20 23:32 [PATCH V10 0/8] Add architecture agnostic code to support vCPU Hotplug Salil Mehta via
2024-05-20 23:32 ` [PATCH V10 1/8] accel/kvm: Extract common KVM vCPU {creation, parking} code Salil Mehta via
2024-05-20 23:32 ` [PATCH V10 2/8] hw/acpi: Move CPU ctrl-dev MMIO region len macro to common header file Salil Mehta via
@ 2024-05-20 23:32 ` Salil Mehta via
2024-05-20 23:32 ` [PATCH V10 4/8] hw/acpi: Update GED _EVT method AML with CPU scan Salil Mehta via
` (4 subsequent siblings)
7 siblings, 0 replies; 15+ messages in thread
From: Salil Mehta via @ 2024-05-20 23:32 UTC (permalink / raw
To: qemu-devel, qemu-arm
Cc: salil.mehta, maz, jean-philippe, jonathan.cameron, lpieralisi,
peter.maydell, richard.henderson, imammedo, andrew.jones, david,
philmd, eric.auger, oliver.upton, pbonzini, mst, will, gshan,
rafael, alex.bennee, linux, darren, ilkka, vishnu, karl.heubaum,
miguel.luis, salil.mehta, zhukeqian1, wangxiongfeng2, wangyanan55,
jiakernel2, maobibo, lixianglai, npiggin, harshpb, linuxarm,
Jonathan Cameron, Shaoqin Huang
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>
Reviewed-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
---
hw/acpi/acpi-cpu-hotplug-stub.c | 6 ++++++
hw/acpi/cpu.c | 6 +++++-
hw/acpi/generic_event_device.c | 17 +++++++++++++++++
include/hw/acpi/generic_event_device.h | 4 ++++
4 files changed, 32 insertions(+), 1 deletion(-)
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/cpu.c b/hw/acpi/cpu.c
index 69aaa563db..473b37ba88 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -221,7 +221,11 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
const CPUArchIdList *id_list;
int i;
- assert(mc->possible_cpu_arch_ids);
+ /* hotplug might not be available for all types like x86/microvm etc. */
+ 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);
diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index 2d6e91b124..54d3b4bf9d 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(sbd, &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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH V10 4/8] hw/acpi: Update GED _EVT method AML with CPU scan
2024-05-20 23:32 [PATCH V10 0/8] Add architecture agnostic code to support vCPU Hotplug Salil Mehta via
` (2 preceding siblings ...)
2024-05-20 23:32 ` [PATCH V10 3/8] hw/acpi: Update ACPI GED framework to support vCPU Hotplug Salil Mehta via
@ 2024-05-20 23:32 ` Salil Mehta via
2024-05-20 23:32 ` [PATCH V10 5/8] hw/acpi: Update CPUs AML with cpu-(ctrl)dev change Salil Mehta via
` (3 subsequent siblings)
7 siblings, 0 replies; 15+ messages in thread
From: Salil Mehta via @ 2024-05-20 23:32 UTC (permalink / raw
To: qemu-devel, qemu-arm
Cc: salil.mehta, maz, jean-philippe, jonathan.cameron, lpieralisi,
peter.maydell, richard.henderson, imammedo, andrew.jones, david,
philmd, eric.auger, oliver.upton, pbonzini, mst, will, gshan,
rafael, alex.bennee, linux, darren, ilkka, vishnu, karl.heubaum,
miguel.luis, salil.mehta, zhukeqian1, wangxiongfeng2, wangyanan55,
jiakernel2, maobibo, lixianglai, npiggin, harshpb, linuxarm,
Jonathan Cameron, Shaoqin Huang
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 473b37ba88..af2b6655d2 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -327,7 +327,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 54d3b4bf9d..63226b0040 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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH V10 5/8] hw/acpi: Update CPUs AML with cpu-(ctrl)dev change
2024-05-20 23:32 [PATCH V10 0/8] Add architecture agnostic code to support vCPU Hotplug Salil Mehta via
` (3 preceding siblings ...)
2024-05-20 23:32 ` [PATCH V10 4/8] hw/acpi: Update GED _EVT method AML with CPU scan Salil Mehta via
@ 2024-05-20 23:32 ` Salil Mehta via
2024-05-20 23:32 ` [PATCH V10 6/8] physmem: Add helper function to destroy CPU AddressSpace Salil Mehta via
` (2 subsequent siblings)
7 siblings, 0 replies; 15+ messages in thread
From: Salil Mehta via @ 2024-05-20 23:32 UTC (permalink / raw
To: qemu-devel, qemu-arm
Cc: salil.mehta, maz, jean-philippe, jonathan.cameron, lpieralisi,
peter.maydell, richard.henderson, imammedo, andrew.jones, david,
philmd, eric.auger, oliver.upton, pbonzini, mst, will, gshan,
rafael, alex.bennee, linux, darren, ilkka, vishnu, karl.heubaum,
miguel.luis, salil.mehta, zhukeqian1, wangxiongfeng2, wangyanan55,
jiakernel2, maobibo, lixianglai, npiggin, harshpb, linuxarm,
Jonathan Cameron, Shaoqin Huang
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 af2b6655d2..4c63514b16 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -343,9 +343,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;
@@ -370,13 +371,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,
@@ -700,9 +707,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 53f804ac16..b73b136605 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1537,7 +1537,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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH V10 6/8] physmem: Add helper function to destroy CPU AddressSpace
2024-05-20 23:32 [PATCH V10 0/8] Add architecture agnostic code to support vCPU Hotplug Salil Mehta via
` (4 preceding siblings ...)
2024-05-20 23:32 ` [PATCH V10 5/8] hw/acpi: Update CPUs AML with cpu-(ctrl)dev change Salil Mehta via
@ 2024-05-20 23:32 ` Salil Mehta via
2024-05-20 23:32 ` [PATCH V10 7/8] gdbstub: Add helper function to unregister GDB register space Salil Mehta via
2024-05-20 23:32 ` [PATCH V10 8/8] docs/specs/acpi_hw_reduced_hotplug: Add the CPU Hotplug Event Bit Salil Mehta via
7 siblings, 0 replies; 15+ messages in thread
From: Salil Mehta via @ 2024-05-20 23:32 UTC (permalink / raw
To: qemu-devel, qemu-arm
Cc: salil.mehta, maz, jean-philippe, jonathan.cameron, lpieralisi,
peter.maydell, richard.henderson, imammedo, andrew.jones, david,
philmd, eric.auger, oliver.upton, pbonzini, mst, will, gshan,
rafael, alex.bennee, linux, darren, ilkka, vishnu, karl.heubaum,
miguel.luis, salil.mehta, zhukeqian1, wangxiongfeng2, wangyanan55,
jiakernel2, maobibo, lixianglai, npiggin, harshpb, linuxarm,
Shaoqin Huang
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 815342d043..240ee04369 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -129,6 +129,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 bb398e8237..60b160d0b4 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -486,6 +486,7 @@ struct CPUState {
QSIMPLEQ_HEAD(, qemu_work_item) work_list;
struct 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 342b7a8fd4..146f17826a 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -763,6 +763,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];
@@ -776,6 +777,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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH V10 7/8] gdbstub: Add helper function to unregister GDB register space
2024-05-20 23:32 [PATCH V10 0/8] Add architecture agnostic code to support vCPU Hotplug Salil Mehta via
` (5 preceding siblings ...)
2024-05-20 23:32 ` [PATCH V10 6/8] physmem: Add helper function to destroy CPU AddressSpace Salil Mehta via
@ 2024-05-20 23:32 ` Salil Mehta via
2024-05-21 12:44 ` Alex Bennée
2024-05-20 23:32 ` [PATCH V10 8/8] docs/specs/acpi_hw_reduced_hotplug: Add the CPU Hotplug Event Bit Salil Mehta via
7 siblings, 1 reply; 15+ messages in thread
From: Salil Mehta via @ 2024-05-20 23:32 UTC (permalink / raw
To: qemu-devel, qemu-arm
Cc: salil.mehta, maz, jean-philippe, jonathan.cameron, lpieralisi,
peter.maydell, richard.henderson, imammedo, andrew.jones, david,
philmd, eric.auger, oliver.upton, pbonzini, mst, will, gshan,
rafael, alex.bennee, linux, darren, ilkka, vishnu, karl.heubaum,
miguel.luis, salil.mehta, zhukeqian1, wangxiongfeng2, wangyanan55,
jiakernel2, maobibo, lixianglai, npiggin, harshpb, linuxarm,
Shaoqin Huang
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>
Reviewed-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
---
gdbstub/gdbstub.c | 13 +++++++++++++
hw/core/cpu-common.c | 1 -
include/exec/gdbstub.h | 6 ++++++
3 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index b3574997ea..1949b09240 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -617,6 +617,19 @@ 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_regs = 0;
+ 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/hw/core/cpu-common.c b/hw/core/cpu-common.c
index 0f0a247f56..e5140b4bc1 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -274,7 +274,6 @@ static void cpu_common_finalize(Object *obj)
{
CPUState *cpu = CPU(obj);
- g_array_free(cpu->gdb_regs, TRUE);
qemu_lockcnt_destroy(&cpu->in_ioctl_lock);
qemu_mutex_destroy(&cpu->work_mutex);
}
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH V10 8/8] docs/specs/acpi_hw_reduced_hotplug: Add the CPU Hotplug Event Bit
2024-05-20 23:32 [PATCH V10 0/8] Add architecture agnostic code to support vCPU Hotplug Salil Mehta via
` (6 preceding siblings ...)
2024-05-20 23:32 ` [PATCH V10 7/8] gdbstub: Add helper function to unregister GDB register space Salil Mehta via
@ 2024-05-20 23:32 ` Salil Mehta via
7 siblings, 0 replies; 15+ messages in thread
From: Salil Mehta via @ 2024-05-20 23:32 UTC (permalink / raw
To: qemu-devel, qemu-arm
Cc: salil.mehta, maz, jean-philippe, jonathan.cameron, lpieralisi,
peter.maydell, richard.henderson, imammedo, andrew.jones, david,
philmd, eric.auger, oliver.upton, pbonzini, mst, will, gshan,
rafael, alex.bennee, linux, darren, ilkka, vishnu, karl.heubaum,
miguel.luis, salil.mehta, zhukeqian1, wangxiongfeng2, wangyanan55,
jiakernel2, maobibo, lixianglai, npiggin, harshpb, linuxarm
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH V10 7/8] gdbstub: Add helper function to unregister GDB register space
2024-05-20 23:32 ` [PATCH V10 7/8] gdbstub: Add helper function to unregister GDB register space Salil Mehta via
@ 2024-05-21 12:44 ` Alex Bennée
2024-05-21 14:55 ` Salil Mehta via
0 siblings, 1 reply; 15+ messages in thread
From: Alex Bennée @ 2024-05-21 12:44 UTC (permalink / raw
To: Salil Mehta
Cc: qemu-devel, qemu-arm, maz, jean-philippe, jonathan.cameron,
lpieralisi, peter.maydell, richard.henderson, imammedo,
andrew.jones, david, philmd, eric.auger, oliver.upton, pbonzini,
mst, will, gshan, rafael, linux, darren, ilkka, vishnu,
karl.heubaum, miguel.luis, salil.mehta, zhukeqian1,
wangxiongfeng2, wangyanan55, jiakernel2, maobibo, lixianglai,
npiggin, harshpb, linuxarm, Shaoqin Huang
Salil Mehta <salil.mehta@huawei.com> writes:
> 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>
> Reviewed-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
> ---
> gdbstub/gdbstub.c | 13 +++++++++++++
> hw/core/cpu-common.c | 1 -
> include/exec/gdbstub.h | 6 ++++++
> 3 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index b3574997ea..1949b09240 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -617,6 +617,19 @@ 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_regs = 0;
> + 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/hw/core/cpu-common.c b/hw/core/cpu-common.c
> index 0f0a247f56..e5140b4bc1 100644
> --- a/hw/core/cpu-common.c
> +++ b/hw/core/cpu-common.c
> @@ -274,7 +274,6 @@ static void cpu_common_finalize(Object *obj)
> {
> CPUState *cpu = CPU(obj);
>
> - g_array_free(cpu->gdb_regs, TRUE);
Is this patch missing something? As far as I can tell the new function
never gets called.
> qemu_lockcnt_destroy(&cpu->in_ioctl_lock);
> qemu_mutex_destroy(&cpu->work_mutex);
> }
> 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
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH V10 7/8] gdbstub: Add helper function to unregister GDB register space
2024-05-21 12:44 ` Alex Bennée
@ 2024-05-21 14:55 ` Salil Mehta via
2024-05-21 15:22 ` Alex Bennée
0 siblings, 1 reply; 15+ messages in thread
From: Salil Mehta via @ 2024-05-21 14:55 UTC (permalink / raw
To: Alex Bennée
Cc: qemu-devel@nongnu.org, qemu-arm@nongnu.org, maz@kernel.org,
jean-philippe@linaro.org, Jonathan Cameron, lpieralisi@kernel.org,
peter.maydell@linaro.org, richard.henderson@linaro.org,
imammedo@redhat.com, andrew.jones@linux.dev, david@redhat.com,
philmd@linaro.org, eric.auger@redhat.com, oliver.upton@linux.dev,
pbonzini@redhat.com, mst@redhat.com, will@kernel.org,
gshan@redhat.com, rafael@kernel.org, linux@armlinux.org.uk,
darren@os.amperecomputing.com, ilkka@os.amperecomputing.com,
vishnu@os.amperecomputing.com, karl.heubaum@oracle.com,
miguel.luis@oracle.com, salil.mehta@opnsrc.net, zhukeqian,
wangxiongfeng (C), wangyanan (Y), jiakernel2@gmail.com,
maobibo@loongson.cn, lixianglai@loongson.cn, npiggin@gmail.com,
harshpb@linux.ibm.com, Linuxarm, Shaoqin Huang
Hi Alex,
> From: Alex Bennée <alex.bennee@linaro.org>
> Sent: Tuesday, May 21, 2024 1:45 PM
> To: Salil Mehta <salil.mehta@huawei.com>
>
> Salil Mehta <salil.mehta@huawei.com> writes:
>
> > 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>
> > Reviewed-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
> > ---
> > gdbstub/gdbstub.c | 13 +++++++++++++
> > hw/core/cpu-common.c | 1 -
> > include/exec/gdbstub.h | 6 ++++++
> > 3 files changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c index
> > b3574997ea..1949b09240 100644
> > --- a/gdbstub/gdbstub.c
> > +++ b/gdbstub/gdbstub.c
> > @@ -617,6 +617,19 @@ 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_regs = 0;
> > + 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/hw/core/cpu-common.c b/hw/core/cpu-common.c index
> > 0f0a247f56..e5140b4bc1 100644
> > --- a/hw/core/cpu-common.c
> > +++ b/hw/core/cpu-common.c
> > @@ -274,7 +274,6 @@ static void cpu_common_finalize(Object *obj) {
> > CPUState *cpu = CPU(obj);
> >
> > - g_array_free(cpu->gdb_regs, TRUE);
>
> Is this patch missing something? As far as I can tell the new function never
> gets called.
Above was causing double free because eventually this free'ng of 'gdb_regs' is being
done in context to un-realization of ARM CPU. Function ' gdb_unregister_coprocessor_all'
will be used by loongson arch as well. Hence, I placed this newly added function
in the arch agnostic patch-set
https://lore.kernel.org/qemu-devel/20230926103654.34424-1-salil.mehta@huawei.com/
Another approach could be to keep it but make above free'ing as conditional?
/* in case architecture specific code did not do its job */
if (cpu->gdb_regs)
g_array_free(cpu->gdb_regs, TRUE);
Best regards
Salil.
>
> > qemu_lockcnt_destroy(&cpu->in_ioctl_lock);
> > qemu_mutex_destroy(&cpu->work_mutex);
> > }
> > 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
>
> --
> Alex Bennée
> Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V10 7/8] gdbstub: Add helper function to unregister GDB register space
2024-05-21 14:55 ` Salil Mehta via
@ 2024-05-21 15:22 ` Alex Bennée
2024-05-21 18:47 ` Salil Mehta via
0 siblings, 1 reply; 15+ messages in thread
From: Alex Bennée @ 2024-05-21 15:22 UTC (permalink / raw
To: Salil Mehta
Cc: qemu-devel@nongnu.org, qemu-arm@nongnu.org, maz@kernel.org,
jean-philippe@linaro.org, Jonathan Cameron, lpieralisi@kernel.org,
peter.maydell@linaro.org, richard.henderson@linaro.org,
imammedo@redhat.com, andrew.jones@linux.dev, david@redhat.com,
philmd@linaro.org, eric.auger@redhat.com, oliver.upton@linux.dev,
pbonzini@redhat.com, mst@redhat.com, will@kernel.org,
gshan@redhat.com, rafael@kernel.org, linux@armlinux.org.uk,
darren@os.amperecomputing.com, ilkka@os.amperecomputing.com,
vishnu@os.amperecomputing.com, karl.heubaum@oracle.com,
miguel.luis@oracle.com, salil.mehta@opnsrc.net, zhukeqian,
wangxiongfeng (C), wangyanan (Y), jiakernel2@gmail.com,
maobibo@loongson.cn, lixianglai@loongson.cn, npiggin@gmail.com,
harshpb@linux.ibm.com, Linuxarm, Shaoqin Huang
Salil Mehta <salil.mehta@huawei.com> writes:
> Hi Alex,
>
>> From: Alex Bennée <alex.bennee@linaro.org>
>> Sent: Tuesday, May 21, 2024 1:45 PM
>> To: Salil Mehta <salil.mehta@huawei.com>
>>
>> Salil Mehta <salil.mehta@huawei.com> writes:
>>
>> > 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>
>> > Reviewed-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
>> > ---
>> > gdbstub/gdbstub.c | 13 +++++++++++++
>> > hw/core/cpu-common.c | 1 -
>> > include/exec/gdbstub.h | 6 ++++++
>> > 3 files changed, 19 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c index
>> > b3574997ea..1949b09240 100644
>> > --- a/gdbstub/gdbstub.c
>> > +++ b/gdbstub/gdbstub.c
>> > @@ -617,6 +617,19 @@ 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_regs = 0;
>> > + 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/hw/core/cpu-common.c b/hw/core/cpu-common.c index
>> > 0f0a247f56..e5140b4bc1 100644
>> > --- a/hw/core/cpu-common.c
>> > +++ b/hw/core/cpu-common.c
>> > @@ -274,7 +274,6 @@ static void cpu_common_finalize(Object *obj) {
>> > CPUState *cpu = CPU(obj);
>> >
>> > - g_array_free(cpu->gdb_regs, TRUE);
>>
>> Is this patch missing something? As far as I can tell the new function never
>> gets called.
>
>
> Above was causing double free because eventually this free'ng of 'gdb_regs' is being
> done in context to un-realization of ARM CPU. Function ' gdb_unregister_coprocessor_all'
> will be used by loongson arch as well. Hence, I placed this newly added function
> in the arch agnostic patch-set
>
> https://lore.kernel.org/qemu-devel/20230926103654.34424-1-salil.mehta@huawei.com/
>
> Another approach could be to keep it but make above free'ing as conditional?
>
> /* in case architecture specific code did not do its job */
> if (cpu->gdb_regs)
> g_array_free(cpu->gdb_regs, TRUE);
No I don't object to moving it to a function. But I would expect the
patch that adds the function and plumbs it in to also be the patch that
removes the inline call. Otherwise the tree will be broken in behaviour
between patches.
Just make it clear in the header that the series needs the pre-requisite
patches.
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH V10 7/8] gdbstub: Add helper function to unregister GDB register space
2024-05-21 15:22 ` Alex Bennée
@ 2024-05-21 18:47 ` Salil Mehta via
0 siblings, 0 replies; 15+ messages in thread
From: Salil Mehta via @ 2024-05-21 18:47 UTC (permalink / raw
To: Alex Bennée
Cc: qemu-devel@nongnu.org, qemu-arm@nongnu.org, maz@kernel.org,
jean-philippe@linaro.org, Jonathan Cameron, lpieralisi@kernel.org,
peter.maydell@linaro.org, richard.henderson@linaro.org,
imammedo@redhat.com, andrew.jones@linux.dev, david@redhat.com,
philmd@linaro.org, eric.auger@redhat.com, oliver.upton@linux.dev,
pbonzini@redhat.com, mst@redhat.com, will@kernel.org,
gshan@redhat.com, rafael@kernel.org, linux@armlinux.org.uk,
darren@os.amperecomputing.com, ilkka@os.amperecomputing.com,
vishnu@os.amperecomputing.com, karl.heubaum@oracle.com,
miguel.luis@oracle.com, salil.mehta@opnsrc.net, zhukeqian,
wangxiongfeng (C), wangyanan (Y), jiakernel2@gmail.com,
maobibo@loongson.cn, lixianglai@loongson.cn, npiggin@gmail.com,
harshpb@linux.ibm.com, Linuxarm, Shaoqin Huang
> From: Alex Bennée <alex.bennee@linaro.org>
> Sent: Tuesday, May 21, 2024 4:23 PM
> To: Salil Mehta <salil.mehta@huawei.com>
>
> Salil Mehta <salil.mehta@huawei.com> writes:
>
> > Hi Alex,
> >
> >> From: Alex Bennée <alex.bennee@linaro.org>
> >> Sent: Tuesday, May 21, 2024 1:45 PM
> >> To: Salil Mehta <salil.mehta@huawei.com>
> >>
> >> Salil Mehta <salil.mehta@huawei.com> writes:
> >>
> >> > 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> > Reviewed-by: Vishnu Pajjuri
> >> <vishnu@os.amperecomputing.com> > ---
> >> > gdbstub/gdbstub.c | 13 +++++++++++++
> >> > hw/core/cpu-common.c | 1 -
> >> > include/exec/gdbstub.h | 6 ++++++ > 3 files changed, 19
> >> insertions(+), 1 deletion(-) > > diff --git a/gdbstub/gdbstub.c
> >> b/gdbstub/gdbstub.c index > b3574997ea..1949b09240 100644 > ---
> >> a/gdbstub/gdbstub.c > +++ b/gdbstub/gdbstub.c > @@ -617,6 +617,19
> >> @@ 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_regs = 0;
> >> > + 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/hw/core/cpu-common.c b/hw/core/cpu-common.c index >
> >> 0f0a247f56..e5140b4bc1 100644 > --- a/hw/core/cpu-common.c > +++
> >> b/hw/core/cpu-common.c > @@ -274,7 +274,6 @@ static void
> >> cpu_common_finalize(Object *obj) {
> >> > CPUState *cpu = CPU(obj);
> >> >
> >> > - g_array_free(cpu->gdb_regs, TRUE);
> >>
> >> Is this patch missing something? As far as I can tell the new
> >> function never gets called.
> >
> >
> > Above was causing double free because eventually this free'ng of
> > 'gdb_regs' is being done in context to un-realization of ARM CPU. Function
> ' gdb_unregister_coprocessor_all'
> > will be used by loongson arch as well. Hence, I placed this newly
> > added function in the arch agnostic patch-set
> >
> > https://lore.kernel.org/qemu-devel/20230926103654.34424-1-
> salil.mehta@
> > huawei.com/
> >
> > Another approach could be to keep it but make above free'ing as
> conditional?
> >
> > /* in case architecture specific code did not do its job */ if
> > (cpu->gdb_regs)
> > g_array_free(cpu->gdb_regs, TRUE);
>
> No I don't object to moving it to a function. But I would expect the patch
> that adds the function and plumbs it in to also be the patch that removes
> the inline call. Otherwise the tree will be broken in behaviour between
> patches.
>
Ok.
> Just make it clear in the header that the series needs the pre-requisite
> patches.
Sure, I will also add it in the header of this patch. Though, I did mention
about such dependency in the cover letter for this entire series.
Pasting from the cover-letter:
[*] https://github.com/salil-mehta/qemu.git virt-cpuhp-armv8/rfc-v3.arch.agnostic.v10
NOTE: For ARM, above will work in combination of the architecture specific part based on
RFC V2 [1]. This architecture specific patch-set RFC V3 shall be floated soon and is present
at below location
[*] https://github.com/salil-mehta/qemu/tree/virt-cpuhp-armv8/rfc-v3-rc1
Thanks
Salil.
>
> --
> Alex Bennée
> Virtualisation Tech Lead @ Linaro
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V10 1/8] accel/kvm: Extract common KVM vCPU {creation,parking} code
2024-05-20 23:32 ` [PATCH V10 1/8] accel/kvm: Extract common KVM vCPU {creation, parking} code Salil Mehta via
@ 2024-05-22 1:25 ` Nicholas Piggin
2024-05-22 10:01 ` Salil Mehta via
0 siblings, 1 reply; 15+ messages in thread
From: Nicholas Piggin @ 2024-05-22 1:25 UTC (permalink / raw
To: Salil Mehta, qemu-devel, qemu-arm
Cc: maz, jean-philippe, jonathan.cameron, lpieralisi, peter.maydell,
richard.henderson, imammedo, andrew.jones, david, philmd,
eric.auger, oliver.upton, pbonzini, mst, will, gshan, rafael,
alex.bennee, linux, darren, ilkka, vishnu, karl.heubaum,
miguel.luis, salil.mehta, zhukeqian1, wangxiongfeng2, wangyanan55,
jiakernel2, maobibo, lixianglai, harshpb, linuxarm,
Jonathan Cameron, Shaoqin Huang
On Tue May 21, 2024 at 9:32 AM AEST, 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. 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>
> Reviewed-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
> ---
> accel/kvm/kvm-all.c | 97 ++++++++++++++++++++++++++++--------------
> accel/kvm/kvm-cpus.h | 23 ++++++++++
> accel/kvm/trace-events | 5 ++-
> 3 files changed, 92 insertions(+), 33 deletions(-)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index c0be9f5eed..a8f93078dc 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -340,14 +340,73 @@ 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_unpark_vcpu(KVMState *s, unsigned long vcpu_id)
> +{
> + struct KVMParkedVcpu *cpu;
> +
> + QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
> + if (cpu->vcpu_id == vcpu_id) {
> + int kvm_fd;
> +
> + trace_kvm_unpark_vcpu(vcpu_id);
Just an aside, but unfortunately tracing is not entirely consistent.
Often a function-level trace point is done at the beginning of the
function regardless of the result. But I actually like this style
of tracing at the end and providing result too. OTOH you don't see
the -ENOENT case.
In any case it's nice to have something here.
Other unforunate thing is some confusion between attaching a KVM
context for QEMU vCPU, and actually making the KVM_CREATE_VCPU
ioctl call, and kvm_create_vcpu is not the counterpart of
kvm_destroy_vcpu, etc.. It is not your fault the existing naming
makes this a bit confusing. Fortunately it's pretty well contained
to small amount of code.
I hate to nitpick it but since the functions are being exported,
would it be a better name somthing like kvm_attach_vcpu()?
Just a thought, but no big deal. Either way,
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> +
> + QLIST_REMOVE(cpu, node);
> + kvm_fd = cpu->kvm_fd;
> + g_free(cpu);
> + return kvm_fd;
> + }
> + }
> +
> + return -ENOENT;
> +}
> +
> +int kvm_create_vcpu(CPUState *cpu)
> +{
> + unsigned long vcpu_id = kvm_arch_vcpu_id(cpu);
> + KVMState *s = kvm_state;
> + int kvm_fd;
> +
> + /* check if the KVM vCPU already exist but is parked */
> + kvm_fd = kvm_unpark_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;
> + }
> + }
> +
> + trace_kvm_create_vcpu(cpu->cpu_index, vcpu_id, 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) {
> @@ -373,10 +432,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;
> }
> @@ -389,24 +445,6 @@ void kvm_destroy_vcpu(CPUState *cpu)
> }
> }
>
> -static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
> -{
> - struct KVMParkedVcpu *cpu;
> -
> - QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
> - if (cpu->vcpu_id == vcpu_id) {
> - int kvm_fd;
> -
> - QLIST_REMOVE(cpu, node);
> - kvm_fd = cpu->kvm_fd;
> - g_free(cpu);
> - return kvm_fd;
> - }
> - }
> -
> - return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
> -}
> -
> int kvm_init_vcpu(CPUState *cpu, Error **errp)
> {
> KVMState *s = kvm_state;
> @@ -415,19 +453,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/kvm-cpus.h b/accel/kvm/kvm-cpus.h
> index ca40add32c..2e6bb38b5d 100644
> --- a/accel/kvm/kvm-cpus.h
> +++ b/accel/kvm/kvm-cpus.h
> @@ -22,5 +22,28 @@ bool kvm_supports_guest_debug(void);
> int kvm_insert_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr len);
> int kvm_remove_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr len);
> void kvm_remove_all_breakpoints(CPUState *cpu);
> +/**
> + * 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);
> +
> +/**
> + * kvm_unpark_vcpu - unpark QEMU KVM vCPU context
> + * @s: KVM State
> + * @cpu: Architecture vCPU ID of the parked vCPU
> + *
> + * @returns: KVM fd
> + */
> +int kvm_unpark_vcpu(KVMState *s, unsigned long vcpu_id);
> #endif /* KVM_CPUS_H */
> diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events
> index 681ccb667d..bd43a0ef26 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, int kvm_fd) "index: %d, id: %lu, kvm fd: %d"
> +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_unpark_vcpu(unsigned long arch_cpu_id) "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) ""
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH V10 1/8] accel/kvm: Extract common KVM vCPU {creation,parking} code
2024-05-22 1:25 ` [PATCH V10 1/8] accel/kvm: Extract common KVM vCPU {creation,parking} code Nicholas Piggin
@ 2024-05-22 10:01 ` Salil Mehta via
0 siblings, 0 replies; 15+ messages in thread
From: Salil Mehta via @ 2024-05-22 10:01 UTC (permalink / raw
To: Nicholas Piggin, qemu-devel@nongnu.org, qemu-arm@nongnu.org
Cc: maz@kernel.org, jean-philippe@linaro.org, Jonathan Cameron,
lpieralisi@kernel.org, peter.maydell@linaro.org,
richard.henderson@linaro.org, imammedo@redhat.com,
andrew.jones@linux.dev, david@redhat.com, philmd@linaro.org,
eric.auger@redhat.com, oliver.upton@linux.dev,
pbonzini@redhat.com, mst@redhat.com, will@kernel.org,
gshan@redhat.com, rafael@kernel.org, alex.bennee@linaro.org,
linux@armlinux.org.uk, darren@os.amperecomputing.com,
ilkka@os.amperecomputing.com, vishnu@os.amperecomputing.com,
karl.heubaum@oracle.com, miguel.luis@oracle.com,
salil.mehta@opnsrc.net, zhukeqian, wangxiongfeng (C),
wangyanan (Y), jiakernel2@gmail.com, maobibo@loongson.cn,
lixianglai@loongson.cn, harshpb@linux.ibm.com, Linuxarm,
Jonathan Cameron, Shaoqin Huang
> From: Nicholas Piggin <npiggin@gmail.com>
> Sent: Wednesday, May 22, 2024 2:25 AM
> To: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org;
> qemu-arm@nongnu.org
>
> On Tue May 21, 2024 at 9:32 AM AEST, 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. 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>
> > Reviewed-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
> > ---
> > accel/kvm/kvm-all.c | 97 ++++++++++++++++++++++++++++-------------
> -
> > accel/kvm/kvm-cpus.h | 23 ++++++++++
> > accel/kvm/trace-events | 5 ++-
> > 3 files changed, 92 insertions(+), 33 deletions(-)
> >
> > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index
> > c0be9f5eed..a8f93078dc 100644
> > --- a/accel/kvm/kvm-all.c
> > +++ b/accel/kvm/kvm-all.c
> > @@ -340,14 +340,73 @@ 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_unpark_vcpu(KVMState *s, unsigned long vcpu_id) {
> > + struct KVMParkedVcpu *cpu;
> > +
> > + QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
> > + if (cpu->vcpu_id == vcpu_id) {
> > + int kvm_fd;
> > +
> > + trace_kvm_unpark_vcpu(vcpu_id);
>
> Just an aside, but unfortunately tracing is not entirely consistent.
> Often a function-level trace point is done at the beginning of the function
> regardless of the result. But I actually like this style of tracing at the end and
> providing result too. OTOH you don't see the -ENOENT case.
>
> In any case it's nice to have something here.
I can definitely move it to the end. You mean you wish to include the case where
vCPU was not found already parked?
>
> Other unforunate thing is some confusion between attaching a KVM
> context for QEMU vCPU, and actually making the KVM_CREATE_VCPU ioctl
> call, and kvm_create_vcpu is not the counterpart of kvm_destroy_vcpu,
> etc.. It is not your fault the existing naming makes this a bit confusing.
> Fortunately it's pretty well contained to small amount of code.
>
> I hate to nitpick it but since the functions are being exported, would it be a
> better name somthing like kvm_attach_vcpu()?
>
> Just a thought, but no big deal. Either way,
Sure, I'm getting your point but KVM does not supports destruction of KVM vCPUs
and hence as you rightly pointed creation and destruction legs at Qemu are
not symmetrical.
Can we live with existing conventions for now otherwise this change can add a
noise to this patch?
>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
Thank you.
Salil
>
> > +
> > + QLIST_REMOVE(cpu, node);
> > + kvm_fd = cpu->kvm_fd;
> > + g_free(cpu);
> > + return kvm_fd;
> > + }
> > + }
> > +
> > + return -ENOENT;
> > +}
> > +
> > +int kvm_create_vcpu(CPUState *cpu)
> > +{
> > + unsigned long vcpu_id = kvm_arch_vcpu_id(cpu);
> > + KVMState *s = kvm_state;
> > + int kvm_fd;
> > +
> > + /* check if the KVM vCPU already exist but is parked */
> > + kvm_fd = kvm_unpark_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;
> > + }
> > + }
> > +
> > + trace_kvm_create_vcpu(cpu->cpu_index, vcpu_id, 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) {
> > @@ -373,10 +432,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;
> > }
> > @@ -389,24 +445,6 @@ void kvm_destroy_vcpu(CPUState *cpu)
> > }
> > }
> >
> > -static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id) -{
> > - struct KVMParkedVcpu *cpu;
> > -
> > - QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
> > - if (cpu->vcpu_id == vcpu_id) {
> > - int kvm_fd;
> > -
> > - QLIST_REMOVE(cpu, node);
> > - kvm_fd = cpu->kvm_fd;
> > - g_free(cpu);
> > - return kvm_fd;
> > - }
> > - }
> > -
> > - return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
> > -}
> > -
> > int kvm_init_vcpu(CPUState *cpu, Error **errp) {
> > KVMState *s = kvm_state;
> > @@ -415,19 +453,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/kvm-cpus.h b/accel/kvm/kvm-cpus.h index
> > ca40add32c..2e6bb38b5d 100644
> > --- a/accel/kvm/kvm-cpus.h
> > +++ b/accel/kvm/kvm-cpus.h
> > @@ -22,5 +22,28 @@ bool kvm_supports_guest_debug(void); int
> > kvm_insert_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr len);
> > int kvm_remove_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr
> > len); void kvm_remove_all_breakpoints(CPUState *cpu);
> > +/**
> > + * 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);
> > +
> > +/**
> > + * kvm_unpark_vcpu - unpark QEMU KVM vCPU context
> > + * @s: KVM State
> > + * @cpu: Architecture vCPU ID of the parked vCPU
> > + *
> > + * @returns: KVM fd
> > + */
> > +int kvm_unpark_vcpu(KVMState *s, unsigned long vcpu_id);
> > #endif /* KVM_CPUS_H */
> > diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events index
> > 681ccb667d..bd43a0ef26 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, int
> kvm_fd) "index: %d, id: %lu, kvm fd: %d"
> > +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_unpark_vcpu(unsigned long arch_cpu_id) "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) ""
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2024-05-22 10:01 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-20 23:32 [PATCH V10 0/8] Add architecture agnostic code to support vCPU Hotplug Salil Mehta via
2024-05-20 23:32 ` [PATCH V10 1/8] accel/kvm: Extract common KVM vCPU {creation, parking} code Salil Mehta via
2024-05-22 1:25 ` [PATCH V10 1/8] accel/kvm: Extract common KVM vCPU {creation,parking} code Nicholas Piggin
2024-05-22 10:01 ` Salil Mehta via
2024-05-20 23:32 ` [PATCH V10 2/8] hw/acpi: Move CPU ctrl-dev MMIO region len macro to common header file Salil Mehta via
2024-05-20 23:32 ` [PATCH V10 3/8] hw/acpi: Update ACPI GED framework to support vCPU Hotplug Salil Mehta via
2024-05-20 23:32 ` [PATCH V10 4/8] hw/acpi: Update GED _EVT method AML with CPU scan Salil Mehta via
2024-05-20 23:32 ` [PATCH V10 5/8] hw/acpi: Update CPUs AML with cpu-(ctrl)dev change Salil Mehta via
2024-05-20 23:32 ` [PATCH V10 6/8] physmem: Add helper function to destroy CPU AddressSpace Salil Mehta via
2024-05-20 23:32 ` [PATCH V10 7/8] gdbstub: Add helper function to unregister GDB register space Salil Mehta via
2024-05-21 12:44 ` Alex Bennée
2024-05-21 14:55 ` Salil Mehta via
2024-05-21 15:22 ` Alex Bennée
2024-05-21 18:47 ` Salil Mehta via
2024-05-20 23:32 ` [PATCH V10 8/8] docs/specs/acpi_hw_reduced_hotplug: Add the CPU Hotplug Event Bit Salil Mehta via
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.