[v2] Adds CPU hot-plug support to Loongarch

[PATCH v2 03/10] make qdev_disconnect_gpio_out_named() public

Posted by xianglai li 2 years, 5 months ago

It will be reused in loongarch/virt.c for unwiring
the vcpu<->exioi interrupts for the vcpu hot-(un)plug
cases.

Co-authored-by: "Salil Mehta" <salil.mehta@opnsrc.net>
Cc: "Salil Mehta" <salil.mehta@opnsrc.net>
Cc: Xiaojuan Yang <yangxiaojuan@loongson.cn>
Cc: Song Gao <gaosong@loongson.cn>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Ani Sinha <anisinha@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Eduardo Habkost <eduardo@habkost.net>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Cc: "Philippe Mathieu-Daudé" <philmd@linaro.org>
Cc: Yanan Wang <wangyanan55@huawei.com>
Cc: "Daniel P. Berrangé" <berrange@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Bibo Mao <maobibo@loongson.cn>
Signed-off-by: xianglai li <lixianglai@loongson.cn>
---
 hw/core/gpio.c         | 4 ++--
 include/hw/qdev-core.h | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/core/gpio.c b/hw/core/gpio.c
index 80d07a6ec9..4fc6409545 100644
--- a/hw/core/gpio.c
+++ b/hw/core/gpio.c
@@ -143,8 +143,8 @@ qemu_irq qdev_get_gpio_out_connector(DeviceState *dev, const char *name, int n)
 
 /* disconnect a GPIO output, returning the disconnected input (if any) */
 
-static qemu_irq qdev_disconnect_gpio_out_named(DeviceState *dev,
-                                               const char *name, int n)
+qemu_irq qdev_disconnect_gpio_out_named(DeviceState *dev,
+                                        const char *name, int n)
 {
     char *propname = g_strdup_printf("%s[%d]",
                                      name ? name : "unnamed-gpio-out", n);
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 151d968238..32bb54163e 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -739,6 +739,8 @@ qemu_irq qdev_get_gpio_out_connector(DeviceState *dev, const char *name, int n);
  */
 qemu_irq qdev_intercept_gpio_out(DeviceState *dev, qemu_irq icpt,
                                  const char *name, int n);
+qemu_irq qdev_disconnect_gpio_out_named(DeviceState *dev,
+                                        const char *name, int n);
 
 BusState *qdev_get_child_bus(DeviceState *dev, const char *name);
 
-- 
2.39.1

Re: [PATCH v2 03/10] make qdev_disconnect_gpio_out_named() public

Posted by Philippe Mathieu-Daudé 2 years, 5 months ago

Hi,

On 12/9/23 04:11, xianglai li wrote:
> It will be reused in loongarch/virt.c for unwiring
> the vcpu<->exioi interrupts for the vcpu hot-(un)plug
> cases.

Since we never had to use this, I'm surprised we really need it.

QEMU IRQs/GPIOs are similar to hardware ones, and aren't expected
to be rewired at runtime. Usually another hot-pluggable bus layer is
used, and the bus is physically wired toward the hardware.


I suppose you want to add that because a unplugged vCPU is still
receiving IRQs. The question is, why? Before unplugging, I expect
the INTC (IPI) here to be signaled a vCPU is going to be unplugged,
so maybe you are missing handling the unplug event there. See
in loongarch_ipi_writel():

     switch (addr) {
     case CORE_EN_OFF:
         s->en = val;
         break;
     case CORE_SET_OFF:
         s->status |= val;
         if (s->status != 0 && (s->status & s->en) != 0) {
             qemu_irq_raise(s->irq);
         }
         break;
     case CORE_CLEAR_OFF:
         s->status &= ~val;
         if (s->status == 0 && s->en != 0) {
             qemu_irq_lower(s->irq);
         }
         break;

Maybe you need to factor ipi_raise/lower() helpers which check cores
are available & enabled before propagating IRQ?

> Co-authored-by: "Salil Mehta" <salil.mehta@opnsrc.net>
> Cc: "Salil Mehta" <salil.mehta@opnsrc.net>
> Cc: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> Cc: Song Gao <gaosong@loongson.cn>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Ani Sinha <anisinha@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Eduardo Habkost <eduardo@habkost.net>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Cc: "Philippe Mathieu-Daudé" <philmd@linaro.org>
> Cc: Yanan Wang <wangyanan55@huawei.com>
> Cc: "Daniel P. Berrangé" <berrange@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Bibo Mao <maobibo@loongson.cn>
> Signed-off-by: xianglai li <lixianglai@loongson.cn>
> ---
>   hw/core/gpio.c         | 4 ++--
>   include/hw/qdev-core.h | 2 ++
>   2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/core/gpio.c b/hw/core/gpio.c
> index 80d07a6ec9..4fc6409545 100644
> --- a/hw/core/gpio.c
> +++ b/hw/core/gpio.c
> @@ -143,8 +143,8 @@ qemu_irq qdev_get_gpio_out_connector(DeviceState *dev, const char *name, int n)
>   
>   /* disconnect a GPIO output, returning the disconnected input (if any) */
>   
> -static qemu_irq qdev_disconnect_gpio_out_named(DeviceState *dev,
> -                                               const char *name, int n)
> +qemu_irq qdev_disconnect_gpio_out_named(DeviceState *dev,
> +                                        const char *name, int n)
>   {
>       char *propname = g_strdup_printf("%s[%d]",
>                                        name ? name : "unnamed-gpio-out", n);
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 151d968238..32bb54163e 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -739,6 +739,8 @@ qemu_irq qdev_get_gpio_out_connector(DeviceState *dev, const char *name, int n);
>    */
>   qemu_irq qdev_intercept_gpio_out(DeviceState *dev, qemu_irq icpt,
>                                    const char *name, int n);
> +qemu_irq qdev_disconnect_gpio_out_named(DeviceState *dev,
> +                                        const char *name, int n);
>   
>   BusState *qdev_get_child_bus(DeviceState *dev, const char *name);
>

Re: [PATCH v2 03/10] make qdev_disconnect_gpio_out_named() public

Posted by lixianglai 2 years, 4 months ago

Hi Philippe Mathieu-Daudé :
> Hi,
>
> On 12/9/23 04:11, xianglai li wrote:
>> It will be reused in loongarch/virt.c for unwiring
>> the vcpu<->exioi interrupts for the vcpu hot-(un)plug
>> cases.
>
> Since we never had to use this, I'm surprised we really need it.
>
> QEMU IRQs/GPIOs are similar to hardware ones, and aren't expected
> to be rewired at runtime. Usually another hot-pluggable bus layer is
> used, and the bus is physically wired toward the hardware.
>
>
> I suppose you want to add that because a unplugged vCPU is still
> receiving IRQs. The question is, why? Before unplugging, I expect
> the INTC (IPI) here to be signaled a vCPU is going to be unplugged,
> so maybe you are missing handling the unplug event there. See


I did not receive an interrupt signal after pulling out the vcpu.

Whether to use the qdev_disconnect_gpio_out_named function does not 
affect the cpu hot swap function.

The qdev_disconnect_gpio_out_named function is used only to make

loongarch_cpu_irq_init and loongarch_cpu_irq_uninit symmetrical.

I thought it would make the whole process more regular and nice.

If this causes confusion I think with the next patch I can remove the 
reference to this function.

Thanks,

xianglai.


> in loongarch_ipi_writel():
>
>     switch (addr) {
>     case CORE_EN_OFF:
>         s->en = val;
>         break;
>     case CORE_SET_OFF:
>         s->status |= val;
>         if (s->status != 0 && (s->status & s->en) != 0) {
>             qemu_irq_raise(s->irq);
>         }
>         break;
>     case CORE_CLEAR_OFF:
>         s->status &= ~val;
>         if (s->status == 0 && s->en != 0) {
>             qemu_irq_lower(s->irq);
>         }
>         break;
>
> Maybe you need to factor ipi_raise/lower() helpers which check cores
> are available & enabled before propagating IRQ?
>
>> Co-authored-by: "Salil Mehta" <salil.mehta@opnsrc.net>
>> Cc: "Salil Mehta" <salil.mehta@opnsrc.net>
>> Cc: Xiaojuan Yang <yangxiaojuan@loongson.cn>
>> Cc: Song Gao <gaosong@loongson.cn>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Ani Sinha <anisinha@redhat.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Richard Henderson <richard.henderson@linaro.org>
>> Cc: Eduardo Habkost <eduardo@habkost.net>
>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>> Cc: "Philippe Mathieu-Daudé" <philmd@linaro.org>
>> Cc: Yanan Wang <wangyanan55@huawei.com>
>> Cc: "Daniel P. Berrangé" <berrange@redhat.com>
>> Cc: Peter Xu <peterx@redhat.com>
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: Bibo Mao <maobibo@loongson.cn>
>> Signed-off-by: xianglai li <lixianglai@loongson.cn>
>> ---
>>   hw/core/gpio.c         | 4 ++--
>>   include/hw/qdev-core.h | 2 ++
>>   2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/core/gpio.c b/hw/core/gpio.c
>> index 80d07a6ec9..4fc6409545 100644
>> --- a/hw/core/gpio.c
>> +++ b/hw/core/gpio.c
>> @@ -143,8 +143,8 @@ qemu_irq qdev_get_gpio_out_connector(DeviceState 
>> *dev, const char *name, int n)
>>     /* disconnect a GPIO output, returning the disconnected input (if 
>> any) */
>>   -static qemu_irq qdev_disconnect_gpio_out_named(DeviceState *dev,
>> -                                               const char *name, int n)
>> +qemu_irq qdev_disconnect_gpio_out_named(DeviceState *dev,
>> +                                        const char *name, int n)
>>   {
>>       char *propname = g_strdup_printf("%s[%d]",
>>                                        name ? name : 
>> "unnamed-gpio-out", n);
>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
>> index 151d968238..32bb54163e 100644
>> --- a/include/hw/qdev-core.h
>> +++ b/include/hw/qdev-core.h
>> @@ -739,6 +739,8 @@ qemu_irq qdev_get_gpio_out_connector(DeviceState 
>> *dev, const char *name, int n);
>>    */
>>   qemu_irq qdev_intercept_gpio_out(DeviceState *dev, qemu_irq icpt,
>>                                    const char *name, int n);
>> +qemu_irq qdev_disconnect_gpio_out_named(DeviceState *dev,
>> +                                        const char *name, int n);
>>     BusState *qdev_get_child_bus(DeviceState *dev, const char *name);