[PATCH v7 0/2] hw/loongarch/virt: Replace destination error with error_abort

[PATCH v7 0/2] hw/loongarch/virt: Replace destination error with error_abort

[Bibo Mao]

In function virt_cpu_plug() and virt_cpu_unplug(), the error is
impossile. Destination error is not propagated and replaced with
error_abort. With this, the logic is simple.

Also there is bugfix in function virt_cpu_plug(), Object cpuslot::cpu is
set at last only when there is no any error.

---
  v6 ... v7:
    1. Replace dest error from parameter errp with error_abort since the
       error is impossible.
    2. Some patches are merged, refresh the pending two patches.

  v5 ... v6:
    1. If there is nested error report when restore from error in function
       virt_cpu_plug(), set output Error object with &error_abort rather
       than NULL, since it is almost impossible now.
    2. If there is nested error report when restore from error in function
       virt_cpu_unplug(), set output Error object with &error_abort rather
       than NULL, since it is almost impossible now.

  v4 ... v5:
    1. Split patch2 in v4 into three small patches, two are fixup for error
       handing when cpu plug/unplug fails so that system can continue to
       run, one is to remove error_propagate() and refresh title.
    2. Refresh changelog in last patch and remove fixes information
       since it is impossible to happen.

  v3 ... v4:
    1. Add missed this cleanup with error and remove some local error
       object.
    2. Replace local error object with error_abort object in
       virt_cpu_irq_init(), since its return value is not checked.

  v2 ... v3:
    1. Add missing modification replacing error_propagate() + error_setg()
      with error_setg().
    2. Some enhancement about error handling, handling error
       symmetrically in many places

  v1 ... v2:
    1. Add fixes tag and change title with fix prefix in patch 1.
    2. Replace error_propagate() with error_setg(), and return directly
       for any error.
---
Bibo Mao (2):
  hw/loongarch/virt: Fix cpuslot::cpu set at last in virt_cpu_plug()
  hw/loongarch/virt: Replace destination error with error_abort

 hw/loongarch/virt.c | 43 +++++++++----------------------------------
 1 file changed, 9 insertions(+), 34 deletions(-)


base-commit: 71119ed3651622e1c531d1294839e9f3341adaf5
-- 
2.39.3

[PATCH v7 1/2] hw/loongarch/virt: Fix cpuslot::cpu set at last in virt_cpu_plug()

[Bibo Mao]

In function virt_cpu_plug(), Object cpuslot::cpu is set at last
only when there is no any error, otherwise it is problematic that
cpuslot::cpu is set in advance however it returns because of error.

Fixes: ab9935d2991e (hw/loongarch/virt: Implement cpu plug interface)
Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
 hw/loongarch/virt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index e25864214f..504f8755a0 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -973,8 +973,6 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
     LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(hotplug_dev);
     Error *err = NULL;
 
-    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
-    cpu_slot->cpu = CPU(dev);
     if (lvms->ipi) {
         hotplug_handler_plug(HOTPLUG_HANDLER(lvms->ipi), dev, &err);
         if (err) {
@@ -998,6 +996,8 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
         }
     }
 
+    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
+    cpu_slot->cpu = CPU(dev);
     return;
 }
 
-- 
2.39.3

Re: [PATCH v7 1/2] hw/loongarch/virt: Fix cpuslot::cpu set at last in virt_cpu_plug()

[Markus Armbruster]

Bibo Mao <maobibo@loongson.cn> writes:

> In function virt_cpu_plug(), Object cpuslot::cpu is set at last
> only when there is no any error, otherwise it is problematic that
> cpuslot::cpu is set in advance however it returns because of error.
>
> Fixes: ab9935d2991e (hw/loongarch/virt: Implement cpu plug interface)
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> ---
>  hw/loongarch/virt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
> index e25864214f..504f8755a0 100644
> --- a/hw/loongarch/virt.c
> +++ b/hw/loongarch/virt.c
> @@ -973,8 +973,6 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
>      LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(hotplug_dev);
>      Error *err = NULL;
>  
> -    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
> -    cpu_slot->cpu = CPU(dev);
>      if (lvms->ipi) {
>          hotplug_handler_plug(HOTPLUG_HANDLER(lvms->ipi), dev, &err);
>          if (err) {
> @@ -998,6 +996,8 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
>          }
>      }
>  
> +    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
> +    cpu_slot->cpu = CPU(dev);
>      return;
>  }

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v7 1/2] hw/loongarch/virt: Fix cpuslot::cpu set at last in virt_cpu_plug()

[bibo mao]

Markus,

Thanks for your reviewing and guidance.

Regards
Bibo Mao

On 2025/3/24 下午2:05, Markus Armbruster wrote:
> Bibo Mao <maobibo@loongson.cn> writes:
> 
>> In function virt_cpu_plug(), Object cpuslot::cpu is set at last
>> only when there is no any error, otherwise it is problematic that
>> cpuslot::cpu is set in advance however it returns because of error.
>>
>> Fixes: ab9935d2991e (hw/loongarch/virt: Implement cpu plug interface)
>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>> ---
>>   hw/loongarch/virt.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
>> index e25864214f..504f8755a0 100644
>> --- a/hw/loongarch/virt.c
>> +++ b/hw/loongarch/virt.c
>> @@ -973,8 +973,6 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
>>       LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(hotplug_dev);
>>       Error *err = NULL;
>>   
>> -    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
>> -    cpu_slot->cpu = CPU(dev);
>>       if (lvms->ipi) {
>>           hotplug_handler_plug(HOTPLUG_HANDLER(lvms->ipi), dev, &err);
>>           if (err) {
>> @@ -998,6 +996,8 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
>>           }
>>       }
>>   
>> +    cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
>> +    cpu_slot->cpu = CPU(dev);
>>       return;
>>   }
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 

Re: [PATCH v7 1/2] hw/loongarch/virt: Fix cpuslot::cpu set at last in virt_cpu_plug()

[Markus Armbruster]

bibo mao <maobibo@loongson.cn> writes:

> Markus,
>
> Thanks for your reviewing and guidance.

You're welcome!

[PATCH v7 2/2] hw/loongarch/virt: Replace destination error with error_abort

[Bibo Mao]

In function virt_cpu_plug() and virt_cpu_unplug(), the error is
impossile. Destination error is not propagated and replaced with
error_abort. With this, the logic is simple.

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
 hw/loongarch/virt.c | 39 +++++++--------------------------------
 1 file changed, 7 insertions(+), 32 deletions(-)

diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index 504f8755a0..65c9027feb 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -936,29 +936,15 @@ static void virt_cpu_unplug(HotplugHandler *hotplug_dev,
                             DeviceState *dev, Error **errp)
 {
     CPUArchId *cpu_slot;
-    Error *err = NULL;
     LoongArchCPU *cpu = LOONGARCH_CPU(dev);
     LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(hotplug_dev);
 
     /* Notify ipi and extioi irqchip to remove interrupt routing to CPU */
-    hotplug_handler_unplug(HOTPLUG_HANDLER(lvms->ipi), dev, &err);
-    if (err) {
-        error_propagate(errp, err);
-        return;
-    }
-
-    hotplug_handler_unplug(HOTPLUG_HANDLER(lvms->extioi), dev, &err);
-    if (err) {
-        error_propagate(errp, err);
-        return;
-    }
+    hotplug_handler_unplug(HOTPLUG_HANDLER(lvms->ipi), dev, &error_abort);
+    hotplug_handler_unplug(HOTPLUG_HANDLER(lvms->extioi), dev, &error_abort);
 
     /* Notify acpi ged CPU removed */
-    hotplug_handler_unplug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
-    if (err) {
-        error_propagate(errp, err);
-        return;
-    }
+    hotplug_handler_unplug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &error_abort);
 
     cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
     cpu_slot->cpu = NULL;
@@ -971,29 +957,18 @@ static void virt_cpu_plug(HotplugHandler *hotplug_dev,
     CPUArchId *cpu_slot;
     LoongArchCPU *cpu = LOONGARCH_CPU(dev);
     LoongArchVirtMachineState *lvms = LOONGARCH_VIRT_MACHINE(hotplug_dev);
-    Error *err = NULL;
 
     if (lvms->ipi) {
-        hotplug_handler_plug(HOTPLUG_HANDLER(lvms->ipi), dev, &err);
-        if (err) {
-            error_propagate(errp, err);
-            return;
-        }
+        hotplug_handler_plug(HOTPLUG_HANDLER(lvms->ipi), dev, &error_abort);
     }
 
     if (lvms->extioi) {
-        hotplug_handler_plug(HOTPLUG_HANDLER(lvms->extioi), dev, &err);
-        if (err) {
-            error_propagate(errp, err);
-            return;
-        }
+        hotplug_handler_plug(HOTPLUG_HANDLER(lvms->extioi), dev, &error_abort);
     }
 
     if (lvms->acpi_ged) {
-        hotplug_handler_plug(HOTPLUG_HANDLER(lvms->acpi_ged), dev, &err);
-        if (err) {
-            error_propagate(errp, err);
-        }
+        hotplug_handler_plug(HOTPLUG_HANDLER(lvms->acpi_ged), dev,
+                             &error_abort);
     }
 
     cpu_slot = virt_find_cpu_slot(MACHINE(lvms), cpu->phy_id);
-- 
2.39.3

Re: [PATCH v7 2/2] hw/loongarch/virt: Replace destination error with error_abort

[Markus Armbruster]

Bibo Mao <maobibo@loongson.cn> writes:

> In function virt_cpu_plug() and virt_cpu_unplug(), the error is
> impossile. Destination error is not propagated and replaced with
> error_abort. With this, the logic is simple.
>
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>

I did not double-check myself that errors are impossible.

Acked-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v7 0/2] hw/loongarch/virt: Replace destination error with error_abort

[gaosong]

hi, Markus,  I had pick up this series for my 'loongarch bug fix for 
10.0. 'branch

thanks.
Song Gao
在 2025/3/24 上午11:01, Bibo Mao 写道:
> In function virt_cpu_plug() and virt_cpu_unplug(), the error is
> impossile. Destination error is not propagated and replaced with
> error_abort. With this, the logic is simple.
>
> Also there is bugfix in function virt_cpu_plug(), Object cpuslot::cpu is
> set at last only when there is no any error.
>
> ---
>    v6 ... v7:
>      1. Replace dest error from parameter errp with error_abort since the
>         error is impossible.
>      2. Some patches are merged, refresh the pending two patches.
>
>    v5 ... v6:
>      1. If there is nested error report when restore from error in function
>         virt_cpu_plug(), set output Error object with &error_abort rather
>         than NULL, since it is almost impossible now.
>      2. If there is nested error report when restore from error in function
>         virt_cpu_unplug(), set output Error object with &error_abort rather
>         than NULL, since it is almost impossible now.
>
>    v4 ... v5:
>      1. Split patch2 in v4 into three small patches, two are fixup for error
>         handing when cpu plug/unplug fails so that system can continue to
>         run, one is to remove error_propagate() and refresh title.
>      2. Refresh changelog in last patch and remove fixes information
>         since it is impossible to happen.
>
>    v3 ... v4:
>      1. Add missed this cleanup with error and remove some local error
>         object.
>      2. Replace local error object with error_abort object in
>         virt_cpu_irq_init(), since its return value is not checked.
>
>    v2 ... v3:
>      1. Add missing modification replacing error_propagate() + error_setg()
>        with error_setg().
>      2. Some enhancement about error handling, handling error
>         symmetrically in many places
>
>    v1 ... v2:
>      1. Add fixes tag and change title with fix prefix in patch 1.
>      2. Replace error_propagate() with error_setg(), and return directly
>         for any error.
> ---
> Bibo Mao (2):
>    hw/loongarch/virt: Fix cpuslot::cpu set at last in virt_cpu_plug()
>    hw/loongarch/virt: Replace destination error with error_abort
>
>   hw/loongarch/virt.c | 43 +++++++++----------------------------------
>   1 file changed, 9 insertions(+), 34 deletions(-)
>
>
> base-commit: 71119ed3651622e1c531d1294839e9f3341adaf5