| Message ID | 6f3c91ecab2d61c2cfb2d1a34a0f2120138e28ed.1689837093.git.lixianglai@loongson.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Adds CPU hot-plug support to Loongarch | expand |
On Thu, 20 Jul 2023 15:15:08 +0800 xianglai li <lixianglai@loongson.cn> wrote: > It introduces a new function to unwire the > vcpu<->exioi interrupts for the vcpu hot-(un)plug cases. it's not a new function. You probably wanted to say: subj: make foo() public it will be reused .someplace. for ... > > 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> > 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 884c726a87..992f5419fa 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); watch for proper alignment have you tried to run ./scripts/checkpatch.pl on you series? (it should catch such cases) > > BusState *qdev_get_child_bus(DeviceState *dev, const char *name); >
On Thu, 20 Jul 2023 at 08:16, xianglai li <lixianglai@loongson.cn> wrote: > > It introduces a new function to unwire the > vcpu<->exioi interrupts for the vcpu hot-(un)plug cases. > > 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> > 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); This function as currently written is intended only for use in qdev_intercept_gpio_out(), which in turn is there only for qtest test case use. I would be a bit cautious about whether there are unexpected issues with trying to use it in "production" functionality with a running QEMU (eg when perhaps some device might be trying to signal the gpio line in another thread while this one is trying to disconnect it). thanks -- PMM
Hi, Igor Mammedov: On 7/28/23 7:59 PM, Igor Mammedov wrote: > On Thu, 20 Jul 2023 15:15:08 +0800 > xianglai li <lixianglai@loongson.cn> wrote: > >> It introduces a new function to unwire the >> vcpu<->exioi interrupts for the vcpu hot-(un)plug cases. > it's not a new function. > You probably wanted to say: > > subj: make foo() public > > it will be reused .someplace. for ... Ok, I'll fix it in the next version of patch. >> 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> >> 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 884c726a87..992f5419fa 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); > watch for proper alignment > > have you tried to run ./scripts/checkpatch.pl on you series? > (it should catch such cases) Ok, I'll fix it in the next version of patch. I ran ./scripts/checkpatch.pl on my patch.pl but didn't check for the problem. >> >> BusState *qdev_get_child_bus(DeviceState *dev, const char *name); >> Thanks, xianglai.
Hi,Peter Maydell : On 7/28/23 8:38 PM, Peter Maydell wrote: > On Thu, 20 Jul 2023 at 08:16, xianglai li <lixianglai@loongson.cn> wrote: >> It introduces a new function to unwire the >> vcpu<->exioi interrupts for the vcpu hot-(un)plug cases. >> >> 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> >> 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); > This function as currently written is intended only for > use in qdev_intercept_gpio_out(), which in turn is there > only for qtest test case use. I would be a bit cautious > about whether there are unexpected issues with trying > to use it in "production" functionality with a running > QEMU (eg when perhaps some device might be trying to > signal the gpio line in another thread while this one > is trying to disconnect it). That's how I understand it: Take Loongarch's extended interrupt controller, for example: 1.The interrupt pin of the extended interrupt controller is initialized in static void loongarch_extioi_instance_init(Object *obj) { .... for (cpu = 0; cpu < EXTIOI_CPUS; cpu++) { .... for (pin = 0; pin < LS3A_INTC_IP; pin++) { qdev_init_gpio_out(DEVICE(obj), &s->parent_irq[cpu][pin], 1); } } ..... } The above function binds the extended interrupt pin to the variable s->parent_irq[cpu][pin] pointer. 2.The assignment of the interrupt pin of the extended interrupt controller operates in static LoongArchCPU *loongarch_cpu_create(MachineState *machine, LoongArchCPU *cpu, Error **errp) { ... for (pin = 0; pin < LS3A_INTC_IP; pin++) { qdev_connect_gpio_out(extioi, (cpu_index * 8 + pin), qdev_get_gpio_in(cpudev, pin + 2)); } ... } The above function binds the extended interrupt pin to the return value of the qdev_get_gpio_in(cpudev, pin + 2). You actually do the following: s->parent_irq[cpu][pin] = qdev_get_gpio_in(cpudev, pin + 2); Variables are only accessed when the interrupt controller's IO space is extended to read and write. There are QEMU thread locks when reading and writing in IO space： static inline void glue(address_space_stw_internal, SUFFIX)(ARG1_DECL, hwaddr addr, uint16_t val, MemTxAttrs attrs, MemTxResult *result, enum device_endian endian) { ... RCU_READ_LOCK(); ... release_lock |= prepare_mmio_access(mr); r = memory_region_dispatch_write(mr, addr1, val, MO_16 | devend_memop(endian), attrs); ... if (release_lock) { qemu_mutex_unlock_iothread(); } RCU_READ_UNLOCK(); ... } Therefore, both are locked by the thread of QEMU when the CPU is unplugged or interrupted sent, which can ensure that the two are mutually exclusive. From the call stack below, it can be seen that interrupt sending and CPU unplugging are in two worker threads, but both worker threads are mutexe. for example: Thread 1 "qemu-system-loo" hit Breakpoint 5, qemu_set_irq 41 if (!irq) (gdb) bt #0 0x000000aaab1e3630 in qemu_set_irq #1 0x000000aaab1324d8 in loongarch_msi_mem_write #2 0x000000aaab1835e0 in memory_region_write_accessor #3 0x000000aaab182e38 in access_with_adjusted_size #4 0x000000aaab18323c in memory_region_dispatch_write #5 0x000000aaab190cf8 in address_space_stl_internal #6 0x000000aaab190cf8 in address_space_stl_le #7 0x000000aaab3714a8 in aio_dispatch_handler #8 0x000000aaab371e20 in aio_dispatch_handlers #9 0x000000aaab371e20 in aio_dispatch #10 0x000000aaab388ae0 in aio_ctx_dispatch #11 0x000000fff724d334 in g_main_context_dispatch #12 0x000000aaab38a390 in glib_pollfds_poll #13 0x000000aaab38a390 in os_host_main_loop_wait #14 0x000000aaab38a390 in main_loop_wait #15 0x000000aaab05aa0c in qemu_main_loop #16 0x000000aaab1ddf2c in qemu_default_main #17 0x000000fff60d9670 in __libc_start_main #18 0x000000aaaae719fc in _start () (gdb) call qemu_mutex_iothread_locked() $5 = true Thread 4 "qemu-system-loo" hit Breakpoint 2, loongarch_cpu_destroy 1177 LoongArchMachineState *lsms = LOONGARCH_MACHINE(machine); (gdb) bt #0 0x000000aaab0eeb60 in loongarch_cpu_destroy #1 0x000000aaab0eeb60 in loongarch_cpu_unplug #2 0x000000aaab0eeb60 in virt_machine_device_unplug #3 0x000000aaab0eeb60 in virt_machine_device_unplug #4 0x000000aaaaeca0d8 in cpu_hotplug_wr #5 0x000000aaab1835e0 in memory_region_write_accessor #6 0x000000aaab182e38 in access_with_adjusted_size #7 0x000000aaab18323c in memory_region_dispatch_write #8 0x000000aaab18a914 in flatview_write_continue #9 0x000000aaab18aae0 in flatview_write #10 0x000000aaab18ab7c in subpage_write #11 0x000000aaab183328 in memory_region_write_with_attrs_accessor #12 0x000000aaab182e38 in access_with_adjusted_size #13 0x000000aaab18323c in memory_region_dispatch_write #14 0x000000aaab1ca714 in io_writex #15 0x000000aaab1cfdfc in helper_stb_mmu #16 0x000000ffa55a2224 in code_gen_buffer () #17 0x000000aaab1be7d0 in tb_lookup #18 0x000000aaab1be7d0 in cpu_exec_loop #19 0x000000aaab1bf04c in cpu_exec_setjmp #20 0x000000aaab1bf0f8 in cpu_exec #21 0x000000aaab1da660 in tcg_cpus_exec #22 0x000000aaab1da7e0 in mttcg_cpu_thread_fn #23 0x000000aaab374fd4 in qemu_thread_start #24 0x000000fff623ddc8 in start_thread #25 0x000000fff618d8fc in __thread_start (gdb) call qemu_mutex_iothread_locked() $6 = true Thanks, xianglai > thanks > -- PMM
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 884c726a87..992f5419fa 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 introduces a new function to unwire the vcpu<->exioi interrupts for the vcpu hot-(un)plug cases. 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> 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(-)