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>
---
include/exec/cpu-common.h | 8 ++++++++
include/hw/core/cpu.h | 1 +
softmmu/physmem.c | 25 +++++++++++++++++++++++++
3 files changed, 34 insertions(+)
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index 41788c0bdd..eb56a228a2 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/hw/core/cpu.h b/include/hw/core/cpu.h
index 648b5b3586..65d2ae4581 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -355,6 +355,7 @@ struct CPUState {
QSIMPLEQ_HEAD(, qemu_work_item) work_list;
CPUAddressSpace *cpu_ases;
+ int cpu_ases_count;
int num_ases;
AddressSpace *as;
MemoryRegion *memory;
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 4f6ca653b3..4dfa0ca66f 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -761,6 +761,7 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
if (!cpu->cpu_ases) {
cpu->cpu_ases = g_new0(CPUAddressSpace, cpu->num_ases);
+ cpu->cpu_ases_count = cpu->num_ases;
}
newas = &cpu->cpu_ases[asidx];
@@ -774,6 +775,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_count == 1) {
+ g_free(cpu->cpu_ases);
+ cpu->cpu_ases = NULL;
+ }
+
+ cpu->cpu_ases_count--;
+}
+
AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx)
{
/* Return the AddressSpace corresponding to the specified index */
--
2.34.1Hi Salil,
On 10/12/23 05:43, Salil Mehta wrote:
> Virtual CPU Hot-unplug leads to unrealization of a CPU object. This also
> involves destruction of the CPU AddressSpace. Add common function to help
> destroy the CPU AddressSpace.
>
> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
> ---
> include/exec/cpu-common.h | 8 ++++++++
> include/hw/core/cpu.h | 1 +
> softmmu/physmem.c | 25 +++++++++++++++++++++++++
> 3 files changed, 34 insertions(+)
>
I guess you need another respin since 'softmmu/physmem.c' has been
renamed to 'system/physmem.c' by 8d7f2e767d ("system: Rename softmmu/
directory as system/"). So please consider leveraging the respin chance
to address the following minor comments. With that,
Reviewed-by: Gavin Shan <gshan@redhat.com>
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index 41788c0bdd..eb56a228a2 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/hw/core/cpu.h b/include/hw/core/cpu.h
> index 648b5b3586..65d2ae4581 100644
> --- a/include/hw/core/cpu.h
> +++ b/include/hw/core/cpu.h
> @@ -355,6 +355,7 @@ struct CPUState {
> QSIMPLEQ_HEAD(, qemu_work_item) work_list;
>
> CPUAddressSpace *cpu_ases;
> + int cpu_ases_count;
The prefix 'cpu_' is duplicate here because the container (CPUState)
indicates it explicitly. I think it was inherited from @cpu_ases.
Besides, @ases_count seems not better than @remaining_ases. If it
makes sense, please rename it to @remaining_ases
> int num_ases;
> AddressSpace *as;
> MemoryRegion *memory;
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index 4f6ca653b3..4dfa0ca66f 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -761,6 +761,7 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
>
> if (!cpu->cpu_ases) {
> cpu->cpu_ases = g_new0(CPUAddressSpace, cpu->num_ases);
> + cpu->cpu_ases_count = cpu->num_ases;
> }
>
> newas = &cpu->cpu_ases[asidx];
> @@ -774,6 +775,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);
> +
The two asserts on @asidx and @cpu->cpu_ases can be combined
to one so that these 3 asserts can be combined to two.
/* Only one address space is supported by KVM */
assert(asidx == 0 || !kvm_enabled());
assert(asidx >= 0 && asidx < cpu->cpu_ases_count)
> + cpuas = &cpu->cpu_ases[asidx];
> + if (tcg_enabled()) {
> + memory_listener_unregister(&cpuas->tcg_as_listener);
> + }
> +
We need to detach CPUState::as here if @asidx == 0 because the alias was
populated in cpu_address_space_init(). Even the CPUState is going to be
release pretty soon, we have inconsistent states temporarily.
/* Detach the alias to address space 0 */
if (asidx == 0) {
cpu->as = NULL;
}
> + address_space_destroy(cpuas->as);
> + g_free_rcu(cpuas->as, rcu);
> +
> + if (cpu->cpu_ases_count == 1) {
> + g_free(cpu->cpu_ases);
> + cpu->cpu_ases = NULL;
> + }
> +
> + cpu->cpu_ases_count--;
> +}
> +
I'm not sure, but Linux's kref_put() decreases the reference count before
the object is released. In order to follow that pattern, we need something
like below. However, it's something related to personal taste, I guess :)
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 */
Thanks,
Gavin
Hi Gavin,
On 12/10/2023 00:31, Gavin Shan wrote:
> Hi Salil,
>
> On 10/12/23 05:43, Salil Mehta wrote:
>> Virtual CPU Hot-unplug leads to unrealization of a CPU object. This also
>> involves destruction of the CPU AddressSpace. Add common function to help
>> destroy the CPU AddressSpace.
>>
>> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
>> Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
>> ---
>> include/exec/cpu-common.h | 8 ++++++++
>> include/hw/core/cpu.h | 1 +
>> softmmu/physmem.c | 25 +++++++++++++++++++++++++
>> 3 files changed, 34 insertions(+)
>>
>
> I guess you need another respin since 'softmmu/physmem.c' has been
> renamed to 'system/physmem.c' by 8d7f2e767d ("system: Rename softmmu/
> directory as system/").
Gosh. Will do.
So please consider leveraging the respin chance
> to address the following minor comments. With that,
>
> Reviewed-by: Gavin Shan <gshan@redhat.com>
>
>> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
>> index 41788c0bdd..eb56a228a2 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/hw/core/cpu.h b/include/hw/core/cpu.h
>> index 648b5b3586..65d2ae4581 100644
>> --- a/include/hw/core/cpu.h
>> +++ b/include/hw/core/cpu.h
>> @@ -355,6 +355,7 @@ struct CPUState {
>> QSIMPLEQ_HEAD(, qemu_work_item) work_list;
>> CPUAddressSpace *cpu_ases;
>> + int cpu_ases_count;
>
> The prefix 'cpu_' is duplicate here because the container (CPUState)
> indicates it explicitly. I think it was inherited from @cpu_ases.
> Besides, @ases_count seems not better than @remaining_ases. If it
> makes sense, please rename it to @remaining_ases
>
>> int num_ases;
>> AddressSpace *as;
>> MemoryRegion *memory;
>> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
>> index 4f6ca653b3..4dfa0ca66f 100644
>> --- a/softmmu/physmem.c
>> +++ b/softmmu/physmem.c
>> @@ -761,6 +761,7 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
>> if (!cpu->cpu_ases) {
>> cpu->cpu_ases = g_new0(CPUAddressSpace, cpu->num_ases);
>> + cpu->cpu_ases_count = cpu->num_ases;
>> }
>> newas = &cpu->cpu_ases[asidx];
>> @@ -774,6 +775,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);
>> +
>
> The two asserts on @asidx and @cpu->cpu_ases can be combined
> to one so that these 3 asserts can be combined to two.
>
> /* Only one address space is supported by KVM */
> assert(asidx == 0 || !kvm_enabled());
> assert(asidx >= 0 && asidx < cpu->cpu_ases_count)
We can do that.
I am not in favor to remove 'assert(cpu->cpu_ases);' as this can save
lot of debugging.
>
>> + cpuas = &cpu->cpu_ases[asidx];
>> + if (tcg_enabled()) {
>> + memory_listener_unregister(&cpuas->tcg_as_listener);
>> + }
>> +
>
> We need to detach CPUState::as here if @asidx == 0 because the alias was
> populated in cpu_address_space_init(). Even the CPUState is going to be
> release pretty soon, we have inconsistent states temporarily.
>
> /* Detach the alias to address space 0 */
> if (asidx == 0) {
> cpu->as = NULL;
> }
Yes. Good catch.
Thanks.
>> + address_space_destroy(cpuas->as);
>> + g_free_rcu(cpuas->as, rcu);
>> +
>> + if (cpu->cpu_ases_count == 1) {
>> + g_free(cpu->cpu_ases);
>> + cpu->cpu_ases = NULL;
>> + }
>> +
>> + cpu->cpu_ases_count--;
>> +}
>> +
>
> I'm not sure, but Linux's kref_put() decreases the reference count before
> the object is released. In order to follow that pattern, we need something
> like below. However, it's something related to personal taste, I guess :)
>
> if (--cpu->cpu_ases_count == 0) {
> g_free(cpu->cpu_ases);
> cpu->cpu_ases = NULL;
> }
I can see your point but the only way this can cause race is when
multiple CPUs are being destroyed simultaneously. I am not sure that
will ever happen. Hence, this code might not either be required or will
be insufficient to avoid the race you are pointing at.
Anyway, I do not mind changing to above.
Thanks
Salil.
Hi Salil,
On 10/12/23 10:04, Salil Mehta wrote:
> On 12/10/2023 00:31, Gavin Shan wrote:
>> On 10/12/23 05:43, Salil Mehta wrote:
[...]
>>> +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);
>>> +
>>
>> The two asserts on @asidx and @cpu->cpu_ases can be combined
>> to one so that these 3 asserts can be combined to two.
>>
>> /* Only one address space is supported by KVM */
>> assert(asidx == 0 || !kvm_enabled());
>> assert(asidx >= 0 && asidx < cpu->cpu_ases_count)
>
> We can do that.
>
> I am not in favor to remove 'assert(cpu->cpu_ases);' as this can save lot of debugging.
>
Ok, It's fine to keep 'assert(cpu->cpu_ases)', but 'assert(asidx >= 0)' is
still needed? For example, the wrong chunk of memory will be release when
@asidx is smaller than zero, which is out-of-bound to @cpu->cpu_ases[]
Thanks,
Gavin
> From: Gavin Shan <gshan@redhat.com>
> Sent: Thursday, October 12, 2023 1:18 AM
> To: Salil Mehta <salil.mehta@opnsrc.net>; 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;
> philmd@linaro.org; eric.auger@redhat.com; oliver.upton@linux.dev;
> pbonzini@redhat.com; mst@redhat.com; will@kernel.org; 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; zhukeqian <zhukeqian1@huawei.com>; wangxiongfeng
> (C) <wangxiongfeng2@huawei.com>; wangyanan (Y) <wangyanan55@huawei.com>;
> jiakernel2@gmail.com; maobibo@loongson.cn; lixianglai@loongson.cn; Linuxarm
> <linuxarm@huawei.com>
> Subject: Re: [PATCH V5 8/9] physmem: Add helper function to destroy CPU
> AddressSpace
>
> Hi Salil,
>
> On 10/12/23 10:04, Salil Mehta wrote:
> > On 12/10/2023 00:31, Gavin Shan wrote:
> >> On 10/12/23 05:43, Salil Mehta wrote:
>
> [...]
>
> >>> +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);
> >>> +
> >>
> >> The two asserts on @asidx and @cpu->cpu_ases can be combined
> >> to one so that these 3 asserts can be combined to two.
> >>
> >> /* Only one address space is supported by KVM */
> >> assert(asidx == 0 || !kvm_enabled());
> >> assert(asidx >= 0 && asidx < cpu->cpu_ases_count)
> >
> > We can do that.
> >
> > I am not in favor to remove 'assert(cpu->cpu_ases);' as this can save
> lot of debugging.
> >
>
> Ok, It's fine to keep 'assert(cpu->cpu_ases)', but 'assert(asidx >= 0)' is
> still needed? For example, the wrong chunk of memory will be release when
> @asidx is smaller than zero, which is out-of-bound to @cpu->cpu_ases[]
Yes, of course, we can keep that.
Thanks
Salil.
© 2016 - 2026 Red Hat, Inc.