| Message ID | d3e388d55f36a93108d0f7b1736f97435237cb77.1694433326.git.lixianglai@loongson.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Adds CPU hot-plug support to Loongarch | expand |
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); >
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);
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);
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(-)