Re: [PATCH 5/8] Adds basic CPU hot-(un)plug support for Loongarch
Hi Igor Mammedov:
On 7/28/23 9:21 PM, Igor Mammedov wrote:
On Thu, 20 Jul 2023 15:15:10 +0800
xianglai li wrote:
1.Add CPU topology related functions
2.Add CPU hot-plug related hook functions
3.Update the in-place CPU creation process at machine initialization
patch is to large, split it at least on those ^^ 3 parts,
which would do a single distinct thing.
After that it will be easier to review this.
Ok, I'll split this patch further.
Also looking at hw/loongarch/acpi-build.c
you have cpu_index == arch_id == core_id /according to comments/
and they are mixed/used interchangeably. which is confusing
at least. So clean it up first to use arch_id consistently
then a separate patches to introduce socket/core/thread support
with proper documentation/pointers to specs as to how arch_id
should be calculated.
And once that is ready, add hotplug on top of it.
Okay, I'll do it according to your suggestion.
Cc: Xiaojuan Yang
Cc: Song Gao
Cc: "Michael S. Tsirkin"
Cc: Igor Mammedov
Cc: Ani Sinha
Cc: Paolo Bonzini
Cc: Richard Henderson
Cc: Eduardo Habkost
Cc: Marcel Apfelbaum
Cc: "Philippe Mathieu-Daudé"
Cc: Yanan Wang
Cc: "Daniel P. Berrangé"
Cc: Peter Xu
Cc: David Hildenbrand
Signed-off-by: xianglai li
---
hw/loongarch/virt.c | 381 ++--
include/hw/loongarch/virt.h | 11 +-
target/loongarch/cpu.h | 4 +
3 files changed, 382 insertions(+), 14 deletions(-)
diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index e19b042ce8..5919389f42 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -46,6 +46,9 @@
#include "hw/block/flash.h"
#include "qemu/error-report.h"
+static int virt_get_socket_id(const MachineState *ms, int cpu_index);
+static int virt_get_core_id(const MachineState *ms, int cpu_index);
+static int virt_get_thread_id(const MachineState *ms, int cpu_index);
static void virt_flash_create(LoongArchMachineState *lams)
{
@@ -447,12 +450,12 @@ static DeviceState *create_acpi_ged(DeviceState *pch_pic,
LoongArchMachineState
{
DeviceState *dev;
MachineState *ms = MACHINE(lams);
-uint32_t event = ACPI_GED_PWR_DOWN_EVT;
+uint32_t event = ACPI_GED_PWR_DOWN_EVT | ACPI_GED_CPU_HOTPLUG_EVT;
if (ms->ram_slots) {
event |= ACPI_GED_MEM_HOTPLUG_EVT;
}
-dev = qdev_new(TYPE_ACPI_GED);
+dev = qdev_new(TYPE_ACPI_GED_LOONGARCH);
qdev_prop_set_uint32(dev, "ged-event", event);
/* ged event */
@@ -461,6 +464,7 @@ static DeviceState *create_acpi_ged(DeviceState *pch_pic,
LoongArchMachineState
sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, VIRT_GED_MEM_ADDR);
/* ged regs used for reset and power down */
sysbus_mmio_map(SYS_BUS_DEVICE(dev), 2, VIRT_GED_REG_ADDR);
+sysbus_mmio_map(SYS_BUS_DEVICE(dev), 3, VIRT_GED_CPUHP_ADDR);
sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0,
qdev_get_gpio_in(pch_pic, VIRT_SCI_IRQ -
VIRT_GSI_BASE));
@@ -583,6 +587,7 @@ static void loongarch_irq_init(LoongArchMachineState *lams)
extioi = qdev_new(TYPE_LOONGARCH_EXTIOI);
sysbus_realize_and_unref(SYS_BUS_DEVICE(extioi), &error_fatal);
+lams->extioi = extioi;
/*
* The connection of interrupts:
@@ -624,11 +629,11 @@ static void loongarch_irq_init(LoongArchMachineState
*lams)
sysbus_mmio_get_region(SYS_BUS_DEVICE(ipi),
1));
/*
-* extioi iocsr memory region
-* only one extioi is added on loongarch virt machine
-* external device interrupt can only be routed to cpu 0-3
-*/
- if (cpu < EXTIOI_CPUS)
+ * extioi iocsr memory region
+ * only one extioi is added on loongarch virt machine
+ * external device interrupt can only be routed to cpu 0-3
+ */
+if (cpu < EXTIOI_CPUS)
memory_region_add_subregion(&env->system_iocsr, APIC_BASE,
sysbus_mmio_get_region(SYS_BUS_DEVICE(extioi),
cpu));
@@ -789,7 +794,6 @@ static void loongarch_init(MachineState *machine)
NodeInfo *numa_info = machine->numa_state->nodes;
int i;
hwaddr fdt_base;
-const CPUArchIdList *possible_cpus;
MachineClass *mc = MACHINE_GET_CLASS(machine);
CPUState *cpu;
char *ramName = NULL;
@@ -810,12 +814,40 @@ static void loongarch_init(MachineState *machine)
create_fdt(lams);
/* Init CPUs */
-possible_cpus = mc->possible_cpu_arch_ids(machine);
-for (i = 0; i < possible_cpus->len; i++) {
-cpu = cpu_create(machine->cpu_type);
+mc->possible_cpu_arch_ids(machine);
+
+for (i = 0; i < machine->smp.cpus; i++) {
+Object *cpuobj;
+cpuobj = object_new(machine->cpu_type);
+
+cpu = CPU(cpuobj);
cpu->cpu_index = i;
I'd move this to foo_cpu_pre_plug()
I guess I don't need to assign a value to the cpu
Re: [PATCH 5/8] Adds basic CPU hot-(un)plug support for Loongarch
On Thu, 20 Jul 2023 15:15:10 +0800
xianglai li wrote:
> 1.Add CPU topology related functions
> 2.Add CPU hot-plug related hook functions
> 3.Update the in-place CPU creation process at machine initialization
patch is to large, split it at least on those ^^ 3 parts,
which would do a single distinct thing.
After that it will be easier to review this.
Also looking at hw/loongarch/acpi-build.c
you have cpu_index == arch_id == core_id /according to comments/
and they are mixed/used interchangeably. which is confusing
at least. So clean it up first to use arch_id consistently
then a separate patches to introduce socket/core/thread support
with proper documentation/pointers to specs as to how arch_id
should be calculated.
And once that is ready, add hotplug on top of it.
>
> Cc: Xiaojuan Yang
> Cc: Song Gao
> Cc: "Michael S. Tsirkin"
> Cc: Igor Mammedov
> Cc: Ani Sinha
> Cc: Paolo Bonzini
> Cc: Richard Henderson
> Cc: Eduardo Habkost
> Cc: Marcel Apfelbaum
> Cc: "Philippe Mathieu-Daudé"
> Cc: Yanan Wang
> Cc: "Daniel P. Berrangé"
> Cc: Peter Xu
> Cc: David Hildenbrand
> Signed-off-by: xianglai li
> ---
> hw/loongarch/virt.c | 381 ++--
> include/hw/loongarch/virt.h | 11 +-
> target/loongarch/cpu.h | 4 +
> 3 files changed, 382 insertions(+), 14 deletions(-)
>
> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
> index e19b042ce8..5919389f42 100644
> --- a/hw/loongarch/virt.c
> +++ b/hw/loongarch/virt.c
> @@ -46,6 +46,9 @@
> #include "hw/block/flash.h"
> #include "qemu/error-report.h"
>
> +static int virt_get_socket_id(const MachineState *ms, int cpu_index);
> +static int virt_get_core_id(const MachineState *ms, int cpu_index);
> +static int virt_get_thread_id(const MachineState *ms, int cpu_index);
>
> static void virt_flash_create(LoongArchMachineState *lams)
> {
> @@ -447,12 +450,12 @@ static DeviceState *create_acpi_ged(DeviceState
> *pch_pic, LoongArchMachineState
> {
> DeviceState *dev;
> MachineState *ms = MACHINE(lams);
> -uint32_t event = ACPI_GED_PWR_DOWN_EVT;
> +uint32_t event = ACPI_GED_PWR_DOWN_EVT | ACPI_GED_CPU_HOTPLUG_EVT;
>
> if (ms->ram_slots) {
> event |= ACPI_GED_MEM_HOTPLUG_EVT;
> }
> -dev = qdev_new(TYPE_ACPI_GED);
> +dev = qdev_new(TYPE_ACPI_GED_LOONGARCH);
> qdev_prop_set_uint32(dev, "ged-event", event);
>
> /* ged event */
> @@ -461,6 +464,7 @@ static DeviceState *create_acpi_ged(DeviceState *pch_pic,
> LoongArchMachineState
> sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, VIRT_GED_MEM_ADDR);
> /* ged regs used for reset and power down */
> sysbus_mmio_map(SYS_BUS_DEVICE(dev), 2, VIRT_GED_REG_ADDR);
> +sysbus_mmio_map(SYS_BUS_DEVICE(dev), 3, VIRT_GED_CPUHP_ADDR);
>
> sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0,
> qdev_get_gpio_in(pch_pic, VIRT_SCI_IRQ -
> VIRT_GSI_BASE));
> @@ -583,6 +587,7 @@ static void loongarch_irq_init(LoongArchMachineState
> *lams)
>
> extioi = qdev_new(TYPE_LOONGARCH_EXTIOI);
> sysbus_realize_and_unref(SYS_BUS_DEVICE(extioi), &error_fatal);
> +lams->extioi = extioi;
>
> /*
> * The connection of interrupts:
> @@ -624,11 +629,11 @@ static void loongarch_irq_init(LoongArchMachineState
> *lams)
>
> sysbus_mmio_get_region(SYS_BUS_DEVICE(ipi),
> 1));
> /*
> - * extioi iocsr memory region
> - * only one extioi is added on loongarch virt machine
> - * external device interrupt can only be routed to cpu 0-3
> - */
> - if (cpu < EXTIOI_CPUS)
> + * extioi iocsr memory region
> + * only one extioi is added on loongarch virt machine
> + * external device interrupt can only be routed to cpu 0-3
> + */
> +if (cpu < EXTIOI_CPUS)
> memory_region_add_subregion(&env->system_iocsr, APIC_BASE,
>
> sysbus_mmio_get_region(SYS_BUS_DEVICE(extioi),
> cpu));
> @@ -789,7 +794,6 @@ static void loongarch_init(MachineState *machine)
> NodeInfo *numa_info = machine->numa_state->nodes;
> int i;
> hwaddr fdt_base;
> -const CPUArchIdList *possible_cpus;
> MachineClass *mc = MACHINE_GET_CLASS(machine);
> CPUState *cpu;
> char *ramName = NULL;
> @@ -810,12 +814,40 @@ static void loongarch_init(MachineState *machine)
> create_fdt(lams);
> /* Init CPUs */
>
> -possible_cpus = mc->possible_cpu_arch_ids(machine);
> -for (i = 0; i < possible_cpus->len; i++) {
> -cpu = cpu_create(machine->cpu_type);
> +mc->possible_cpu_arch_ids(machine);
> +
> +for (i = 0; i < machine->smp.cpus; i++) {
> +Object *cpuobj;
> +cpuobj = object_new(machine->cpu_type);
> +
> +cpu = CPU(cpuobj);
> cpu->cpu_index = i;
I'd move this to foo_cpu_pre_plug()
> -machine->possible_cpu
[PATCH 5/8] Adds basic CPU hot-(un)plug support for Loongarch
1.Add CPU topology related functions
2.Add CPU hot-plug related hook functions
3.Update the in-place CPU creation process at machine initialization
Cc: Xiaojuan Yang
Cc: Song Gao
Cc: "Michael S. Tsirkin"
Cc: Igor Mammedov
Cc: Ani Sinha
Cc: Paolo Bonzini
Cc: Richard Henderson
Cc: Eduardo Habkost
Cc: Marcel Apfelbaum
Cc: "Philippe Mathieu-Daudé"
Cc: Yanan Wang
Cc: "Daniel P. Berrangé"
Cc: Peter Xu
Cc: David Hildenbrand
Signed-off-by: xianglai li
---
hw/loongarch/virt.c | 381 ++--
include/hw/loongarch/virt.h | 11 +-
target/loongarch/cpu.h | 4 +
3 files changed, 382 insertions(+), 14 deletions(-)
diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index e19b042ce8..5919389f42 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -46,6 +46,9 @@
#include "hw/block/flash.h"
#include "qemu/error-report.h"
+static int virt_get_socket_id(const MachineState *ms, int cpu_index);
+static int virt_get_core_id(const MachineState *ms, int cpu_index);
+static int virt_get_thread_id(const MachineState *ms, int cpu_index);
static void virt_flash_create(LoongArchMachineState *lams)
{
@@ -447,12 +450,12 @@ static DeviceState *create_acpi_ged(DeviceState *pch_pic,
LoongArchMachineState
{
DeviceState *dev;
MachineState *ms = MACHINE(lams);
-uint32_t event = ACPI_GED_PWR_DOWN_EVT;
+uint32_t event = ACPI_GED_PWR_DOWN_EVT | ACPI_GED_CPU_HOTPLUG_EVT;
if (ms->ram_slots) {
event |= ACPI_GED_MEM_HOTPLUG_EVT;
}
-dev = qdev_new(TYPE_ACPI_GED);
+dev = qdev_new(TYPE_ACPI_GED_LOONGARCH);
qdev_prop_set_uint32(dev, "ged-event", event);
/* ged event */
@@ -461,6 +464,7 @@ static DeviceState *create_acpi_ged(DeviceState *pch_pic,
LoongArchMachineState
sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, VIRT_GED_MEM_ADDR);
/* ged regs used for reset and power down */
sysbus_mmio_map(SYS_BUS_DEVICE(dev), 2, VIRT_GED_REG_ADDR);
+sysbus_mmio_map(SYS_BUS_DEVICE(dev), 3, VIRT_GED_CPUHP_ADDR);
sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0,
qdev_get_gpio_in(pch_pic, VIRT_SCI_IRQ -
VIRT_GSI_BASE));
@@ -583,6 +587,7 @@ static void loongarch_irq_init(LoongArchMachineState *lams)
extioi = qdev_new(TYPE_LOONGARCH_EXTIOI);
sysbus_realize_and_unref(SYS_BUS_DEVICE(extioi), &error_fatal);
+lams->extioi = extioi;
/*
* The connection of interrupts:
@@ -624,11 +629,11 @@ static void loongarch_irq_init(LoongArchMachineState
*lams)
sysbus_mmio_get_region(SYS_BUS_DEVICE(ipi),
1));
/*
-* extioi iocsr memory region
-* only one extioi is added on loongarch virt machine
-* external device interrupt can only be routed to cpu 0-3
-*/
- if (cpu < EXTIOI_CPUS)
+ * extioi iocsr memory region
+ * only one extioi is added on loongarch virt machine
+ * external device interrupt can only be routed to cpu 0-3
+ */
+if (cpu < EXTIOI_CPUS)
memory_region_add_subregion(&env->system_iocsr, APIC_BASE,
sysbus_mmio_get_region(SYS_BUS_DEVICE(extioi),
cpu));
@@ -789,7 +794,6 @@ static void loongarch_init(MachineState *machine)
NodeInfo *numa_info = machine->numa_state->nodes;
int i;
hwaddr fdt_base;
-const CPUArchIdList *possible_cpus;
MachineClass *mc = MACHINE_GET_CLASS(machine);
CPUState *cpu;
char *ramName = NULL;
@@ -810,12 +814,40 @@ static void loongarch_init(MachineState *machine)
create_fdt(lams);
/* Init CPUs */
-possible_cpus = mc->possible_cpu_arch_ids(machine);
-for (i = 0; i < possible_cpus->len; i++) {
-cpu = cpu_create(machine->cpu_type);
+mc->possible_cpu_arch_ids(machine);
+
+for (i = 0; i < machine->smp.cpus; i++) {
+Object *cpuobj;
+cpuobj = object_new(machine->cpu_type);
+
+cpu = CPU(cpuobj);
cpu->cpu_index = i;
-machine->possible_cpus->cpus[i].cpu = OBJECT(cpu);
+
+object_property_set_int(cpuobj, "socket-id",
+virt_get_socket_id(machine, i), NULL);
+object_property_set_int(cpuobj, "core-id",
+virt_get_core_id(machine, i), NULL);
+object_property_set_int(cpuobj, "thread-id",
+virt_get_thread_id(machine, i), NULL);
+/*
+ * The CPU in place at the time of machine startup will also enter
+ * the CPU hot-plug process when it is created, but at this time,
+ * the GED device has not been created, resulting in exit in the CPU
+ * hot-plug process, which can avoid the incumbent CPU repeatedly
+ * applying for resources.
+ *
+ * The interrupt resource of the in-place CPU will be requested at
+ * the current function call loongarch_irq_
