| Message ID | 20230926100436.28284-32-salil.mehta@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support of Virtual CPU Hotplug for ARMv8 Arch | expand |
Hi Salil, On 26/9/23 12:04, Salil Mehta wrote: > Supporting vCPU Hotplug for ARM arch also means introducing new functionality of > unrealizing the ARMCPU. This requires some new common functions. > > Defining them as part of architecture independent change so that this code could > be reused by other interested parties. > > Signed-off-by: Salil Mehta <salil.mehta@huawei.com> > --- > gdbstub/gdbstub.c | 13 +++++++++++++ > include/exec/cpu-common.h | 8 ++++++++ > include/exec/gdbstub.h | 1 + > include/hw/core/cpu.h | 1 + > softmmu/physmem.c | 25 +++++++++++++++++++++++++ > 5 files changed, 48 insertions(+) > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h > index dab572c9bd..ffd815a0d8 100644 > --- a/include/hw/core/cpu.h > +++ b/include/hw/core/cpu.h > @@ -366,6 +366,7 @@ struct CPUState { > QSIMPLEQ_HEAD(, qemu_work_item) work_list; > > CPUAddressSpace *cpu_ases; > + int cpu_ases_ref_count; > int num_ases; > AddressSpace *as; > MemoryRegion *memory; > diff --git a/softmmu/physmem.c b/softmmu/physmem.c > index 3df73542e1..a93ae783af 100644 > --- a/softmmu/physmem.c > +++ b/softmmu/physmem.c > @@ -762,6 +762,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_ref_count = cpu->num_ases; > } > > newas = &cpu->cpu_ases[asidx]; > @@ -775,6 +776,30 @@ void cpu_address_space_init(CPUState *cpu, int asidx, > } > } > > +void cpu_address_space_destroy(CPUState *cpu, int asidx) > +{ > + CPUAddressSpace *cpuas; > + > + assert(asidx < cpu->num_ases); > + assert(asidx == 0 || !kvm_enabled()); > + assert(cpu->cpu_ases); > + > + 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 (cpu->cpu_ases_ref_count == 1) { > + g_free(cpu->cpu_ases); > + cpu->cpu_ases = NULL; > + } > + > + cpu->cpu_ases_ref_count--; See Richard comment from: https://lore.kernel.org/qemu-devel/594b2550-9a73-684f-6e54-29401dc6cd7a@linaro.org/ "I think it would be better to destroy all address spaces at once, "so that you don't need to invent a reference count that isn't used "for anything else. > +} > + > AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx) > { > /* Return the AddressSpace corresponding to the specified index */
Hi Phil, > From: Philippe Mathieu-Daudé <philmd@linaro.org> > Sent: Tuesday, October 3, 2023 7:34 AM > To: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org; qemu- > arm@nongnu.org > Cc: maz@kernel.org; jean-philippe@linaro.org; Jonathan Cameron > <jonathan.cameron@huawei.com>; lpieralisi@kernel.org; > peter.maydell@linaro.org; richard.henderson@linaro.org; > imammedo@redhat.com; andrew.jones@linux.dev; david@redhat.com; > eric.auger@redhat.com; will@kernel.org; ardb@kernel.org; > oliver.upton@linux.dev; pbonzini@redhat.com; mst@redhat.com; > gshan@redhat.com; rafael@kernel.org; borntraeger@linux.ibm.com; > 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 > <zhukeqian1@huawei.com>; wangxiongfeng (C) <wangxiongfeng2@huawei.com>; > wangyanan (Y) <wangyanan55@huawei.com>; jiakernel2@gmail.com; > maobibo@loongson.cn; lixianglai@loongson.cn > Subject: Re: [PATCH RFC V2 31/37] physmem,gdbstub: Common helping > funcs/changes to *unrealize* vCPU > > Hi Salil, > > On 26/9/23 12:04, Salil Mehta wrote: > > Supporting vCPU Hotplug for ARM arch also means introducing new > functionality of > > unrealizing the ARMCPU. This requires some new common functions. > > > > Defining them as part of architecture independent change so that this > code could > > be reused by other interested parties. > > > > Signed-off-by: Salil Mehta <salil.mehta@huawei.com> > > --- > > gdbstub/gdbstub.c | 13 +++++++++++++ > > include/exec/cpu-common.h | 8 ++++++++ > > include/exec/gdbstub.h | 1 + > > include/hw/core/cpu.h | 1 + > > softmmu/physmem.c | 25 +++++++++++++++++++++++++ > > 5 files changed, 48 insertions(+) > > > > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h > > index dab572c9bd..ffd815a0d8 100644 > > --- a/include/hw/core/cpu.h > > +++ b/include/hw/core/cpu.h > > @@ -366,6 +366,7 @@ struct CPUState { > > QSIMPLEQ_HEAD(, qemu_work_item) work_list; > > > > CPUAddressSpace *cpu_ases; > > + int cpu_ases_ref_count; > > int num_ases; > > AddressSpace *as; > > MemoryRegion *memory; > > diff --git a/softmmu/physmem.c b/softmmu/physmem.c > > index 3df73542e1..a93ae783af 100644 > > --- a/softmmu/physmem.c > > +++ b/softmmu/physmem.c > > @@ -762,6 +762,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_ref_count = cpu->num_ases; > > } > > > > newas = &cpu->cpu_ases[asidx]; > > @@ -775,6 +776,30 @@ void cpu_address_space_init(CPUState *cpu, int > asidx, > > } > > } > > > > +void cpu_address_space_destroy(CPUState *cpu, int asidx) > > +{ > > + CPUAddressSpace *cpuas; > > + > > + assert(asidx < cpu->num_ases); > > + assert(asidx == 0 || !kvm_enabled()); > > + assert(cpu->cpu_ases); > > + > > + 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 (cpu->cpu_ases_ref_count == 1) { > > + g_free(cpu->cpu_ases); > > + cpu->cpu_ases = NULL; > > + } > > + > > + cpu->cpu_ases_ref_count--; > > See Richard comment from: > https://lore.kernel.org/qemu-devel/594b2550-9a73-684f-6e54- > 29401dc6cd7a@linaro.org/ > > "I think it would be better to destroy all address spaces at once, > "so that you don't need to invent a reference count that isn't used > "for anything else. Yes, we can do that and remove the reference count. The only reason I did it was because I was not sure if it is safe to assume that all the AddressSpace will always be destroyed *together*. And now since this is being ported to other architectures will the same hold true everywhere? Thanks Salil.
Hi Phil/Richard, > From: qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org <qemu-arm- > bounces+salil.mehta=huawei.com@nongnu.org> On Behalf Of Salil Mehta via > Sent: Tuesday, October 3, 2023 11:23 AM > > Hi Phil, > > > From: Philippe Mathieu-Daudé <philmd@linaro.org> > > Sent: Tuesday, October 3, 2023 7:34 AM > > To: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org; qemu- > > arm@nongnu.org > > Cc: maz@kernel.org; jean-philippe@linaro.org; Jonathan Cameron > > <jonathan.cameron@huawei.com>; lpieralisi@kernel.org; > > peter.maydell@linaro.org; richard.henderson@linaro.org; > > imammedo@redhat.com; andrew.jones@linux.dev; david@redhat.com; > > eric.auger@redhat.com; will@kernel.org; ardb@kernel.org; > > oliver.upton@linux.dev; pbonzini@redhat.com; mst@redhat.com; > > gshan@redhat.com; rafael@kernel.org; borntraeger@linux.ibm.com; > > 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 > > <zhukeqian1@huawei.com>; wangxiongfeng (C) <wangxiongfeng2@huawei.com>; > > wangyanan (Y) <wangyanan55@huawei.com>; jiakernel2@gmail.com; > > maobibo@loongson.cn; lixianglai@loongson.cn > > Subject: Re: [PATCH RFC V2 31/37] physmem,gdbstub: Common helping > > funcs/changes to *unrealize* vCPU > > > > Hi Salil, > > > > On 26/9/23 12:04, Salil Mehta wrote: > > > Supporting vCPU Hotplug for ARM arch also means introducing new > > functionality of > > > unrealizing the ARMCPU. This requires some new common functions. > > > > > > Defining them as part of architecture independent change so that this > > code could > > > be reused by other interested parties. > > > > > > Signed-off-by: Salil Mehta <salil.mehta@huawei.com> > > > --- > > > gdbstub/gdbstub.c | 13 +++++++++++++ > > > include/exec/cpu-common.h | 8 ++++++++ > > > include/exec/gdbstub.h | 1 + > > > include/hw/core/cpu.h | 1 + > > > softmmu/physmem.c | 25 +++++++++++++++++++++++++ > > > 5 files changed, 48 insertions(+) > > > > > > > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h > > > index dab572c9bd..ffd815a0d8 100644 > > > --- a/include/hw/core/cpu.h > > > +++ b/include/hw/core/cpu.h > > > @@ -366,6 +366,7 @@ struct CPUState { > > > QSIMPLEQ_HEAD(, qemu_work_item) work_list; > > > > > > CPUAddressSpace *cpu_ases; > > > + int cpu_ases_ref_count; > > > int num_ases; > > > AddressSpace *as; > > > MemoryRegion *memory; > > > diff --git a/softmmu/physmem.c b/softmmu/physmem.c > > > index 3df73542e1..a93ae783af 100644 > > > --- a/softmmu/physmem.c > > > +++ b/softmmu/physmem.c > > > @@ -762,6 +762,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_ref_count = cpu->num_ases; > > > } > > > > > > newas = &cpu->cpu_ases[asidx]; > > > @@ -775,6 +776,30 @@ void cpu_address_space_init(CPUState *cpu, int > > asidx, > > > } > > > } > > > > > > +void cpu_address_space_destroy(CPUState *cpu, int asidx) > > > +{ > > > + CPUAddressSpace *cpuas; > > > + > > > + assert(asidx < cpu->num_ases); > > > + assert(asidx == 0 || !kvm_enabled()); > > > + assert(cpu->cpu_ases); > > > + > > > + 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 (cpu->cpu_ases_ref_count == 1) { > > > + g_free(cpu->cpu_ases); > > > + cpu->cpu_ases = NULL; > > > + } > > > + > > > + cpu->cpu_ases_ref_count--; > > > > See Richard comment from: > > https://lore.kernel.org/qemu-devel/594b2550-9a73-684f-6e54- > > 29401dc6cd7a@linaro.org/ > > > > "I think it would be better to destroy all address spaces at once, > > "so that you don't need to invent a reference count that isn't used > > "for anything else. > > Yes, we can do that and remove the reference count. The only reason I > did it was because I was not sure if it is safe to assume that all > the AddressSpace will always be destroyed *together*. And now since > this is being ported to other architectures will the same hold > true everywhere? (sorry, I missed key point) To make things clear further for ARM, presence of tagged/secure memory is optional (and I am not even sure all of these are supported with accel=KVM). The Address Space destruction function is common to all architectures and hence it is not safe to destroy all of these together. https://lore.kernel.org/qemu-devel/20230926100436.28284-1-salil.mehta@huawei.com/T/#mfb2a525081c412917a0026d558e72f48875e386d +static void arm_cpu_unrealizefn(DeviceState *dev) +{ + ARMCPUClass *acc = ARM_CPU_GET_CLASS(dev); + ARMCPU *cpu = ARM_CPU(dev); + CPUARMState *env = &cpu->env; + CPUState *cs = CPU(dev); + bool has_secure; + + has_secure = cpu->has_el3 || arm_feature(env, ARM_FEATURE_M_SECURITY); + + /* rock 'n' un-roll, whatever happened in the arm_cpu_realizefn cleanly */ + cpu_address_space_destroy(cs, ARMASIdx_NS); + + if (cpu->tag_memory != NULL) { + cpu_address_space_destroy(cs, ARMASIdx_TagNS); + if (has_secure) { + cpu_address_space_destroy(cs, ARMASIdx_TagS); + } + } + + if (has_secure) { + cpu_address_space_destroy(cs, ARMASIdx_S); + } [...] } @Richard, please let me know if I understood your comment correctly? Thanks Salil.
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c index 5f28d5cf57..ddbcb4f115 100644 --- a/gdbstub/gdbstub.c +++ b/gdbstub/gdbstub.c @@ -491,6 +491,19 @@ void gdb_register_coprocessor(CPUState *cpu, } } +void gdb_unregister_coprocessor_all(CPUState *cpu) +{ + GDBRegisterState *s, *p; + + p = cpu->gdb_regs; + while (p) { + s = p; + p = p->next; + g_free(s); + } + cpu->gdb_regs = NULL; +} + static void gdb_process_breakpoint_remove_all(GDBProcess *p) { CPUState *cpu = gdb_get_first_cpu_in_process(p); diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h index 87dc9a752c..27cd4d32b1 100644 --- a/include/exec/cpu-common.h +++ b/include/exec/cpu-common.h @@ -120,6 +120,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/exec/gdbstub.h b/include/exec/gdbstub.h index 7d743fe1e9..a22f0875e2 100644 --- a/include/exec/gdbstub.h +++ b/include/exec/gdbstub.h @@ -17,6 +17,7 @@ typedef int (*gdb_set_reg_cb)(CPUArchState *env, uint8_t *buf, int reg); void gdb_register_coprocessor(CPUState *cpu, gdb_get_reg_cb get_reg, gdb_set_reg_cb set_reg, int num_regs, const char *xml, int g_pos); +void gdb_unregister_coprocessor_all(CPUState *cpu); /** * gdbserver_start: start the gdb server diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h index dab572c9bd..ffd815a0d8 100644 --- a/include/hw/core/cpu.h +++ b/include/hw/core/cpu.h @@ -366,6 +366,7 @@ struct CPUState { QSIMPLEQ_HEAD(, qemu_work_item) work_list; CPUAddressSpace *cpu_ases; + int cpu_ases_ref_count; int num_ases; AddressSpace *as; MemoryRegion *memory; diff --git a/softmmu/physmem.c b/softmmu/physmem.c index 3df73542e1..a93ae783af 100644 --- a/softmmu/physmem.c +++ b/softmmu/physmem.c @@ -762,6 +762,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_ref_count = cpu->num_ases; } newas = &cpu->cpu_ases[asidx]; @@ -775,6 +776,30 @@ void cpu_address_space_init(CPUState *cpu, int asidx, } } +void cpu_address_space_destroy(CPUState *cpu, int asidx) +{ + CPUAddressSpace *cpuas; + + assert(asidx < cpu->num_ases); + assert(asidx == 0 || !kvm_enabled()); + assert(cpu->cpu_ases); + + 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 (cpu->cpu_ases_ref_count == 1) { + g_free(cpu->cpu_ases); + cpu->cpu_ases = NULL; + } + + cpu->cpu_ases_ref_count--; +} + AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx) { /* Return the AddressSpace corresponding to the specified index */
Supporting vCPU Hotplug for ARM arch also means introducing new functionality of unrealizing the ARMCPU. This requires some new common functions. Defining them as part of architecture independent change so that this code could be reused by other interested parties. Signed-off-by: Salil Mehta <salil.mehta@huawei.com> --- gdbstub/gdbstub.c | 13 +++++++++++++ include/exec/cpu-common.h | 8 ++++++++ include/exec/gdbstub.h | 1 + include/hw/core/cpu.h | 1 + softmmu/physmem.c | 25 +++++++++++++++++++++++++ 5 files changed, 48 insertions(+)