diff mbox series

[V5,8/9] physmem: Add helper function to destroy CPU AddressSpace

Message ID 20231011194355.15628-9-salil.mehta@huawei.com (mailing list archive)
State New, archived
Headers show
Series Add architecture agnostic code to support vCPU Hotplug | expand

Commit Message

Salil Mehta Oct. 11, 2023, 7:43 p.m. UTC 
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(+)

Comments

Gavin Shan Oct. 11, 2023, 11:31 p.m. UTC | #1 
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/"). 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
Salil Mehta Oct. 12, 2023, 12:04 a.m. UTC | #2 
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.
Gavin Shan Oct. 12, 2023, 12:18 a.m. UTC | #3 
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
Salil Mehta Oct. 12, 2023, 9:22 a.m. UTC | #4 
> 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.
diff mbox series

Patch

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 */