s390x: flic rework, tcg flic support and tcg
cover s390x: flic rework, tcg flic support and tcg
This patch series implements floating interrupt support for TCG and fixes STSI so we can remove warnings related to s390x SMP and MTTCG.
KVM code has to be touched in order to factor out the injection routines into the flic ("s390x/flic: factor out injection of floating interrupts"). Basic testing didn't reveal any problems so far.
With this series I am now able to run fedora 26/27 and Ubuntu 17.10+ with 16 VCPUs (MTTCG) on a 8CPU host, doing a make -j16 in the guest. I got nasty stalls in the guest beforehand.
I already sent the patches
- "cpus: make pause_all_cpus() play with SMP on single threaded TCG"
- "cpu-exec: fix missed CPU kick during interrupt injection"
separately, but as nobody commented so far (e.g. due to 2.11) I am including them here as they are required to make SMP work reliably.
Based on: https://github.com/cohuck/qemu.git s390x-next Available at: https://github.com/davidhildenbrand/qemu.git s390x-queue
David Hildenbrand (15):
cpus: make pause_all_cpus() play with SMP on single threaded TCG
cpu-exec: fix missed CPU kick during interrupt injection s390x/tcg: deliver multiple interrupts in a row s390x/flic: simplify flic initialization
s390x/tcg: simplify machine check handling s390x/flic: factor out injection of floating interrupts s390x/tcg: tolerate wrong wakeups due to floating interrupts s390x/flic: make floating interrupts on TCG actually floating s390x/tcg: implement TEST PENDING INTERRUPTION s390x/flic: implement qemu_s390_clear_io_flic() s390x/flic: optimize CPU wakeup for TCG
s390x/tcg: fix size + content of STSI blocks s390x/tcg: STSI overhaul
s390x/tcg: remove SMP warning
configure: s390x supports mttcg now
accel/tcg/cpu-exec.c | 12 +- configure | 1 + cpus.c | 32 +++--- hw/intc/s390_flic.c | 222 +++++++++++++++++++++++++++++++++++- hw/intc/s390_flic_kvm.c | 75 +++++++++--- hw/s390x/s390-virtio-ccw.c | 4 - include/hw/s390x/s390_flic.h | 55 +++++++-- target/s390x/cpu.c | 10 -- target/s390x/cpu.h | 75 ++++++------ target/s390x/excp_helper.c | 147 +++++++++--------------- target/s390x/helper.h | 1 + target/s390x/insn-data.def | 1 + target/s390x/internal.h | 5 - target/s390x/interrupt.c | 100 ++++------------ target/s390x/kvm-stub.c | 13 --- target/s390x/kvm.c | 68 +---------- target/s390x/kvm_s390x.h | 10 +- target/s390x/misc_helper.c | 266 +++++++++++++++++++++++++++---------------- target/s390x/translate.c | 8 ++ 19 files changed, 644 insertions(+), 461 deletions(-)
Signature · 1 line
2.14.3
PATCH 1/15 cpus: make pause_all_cpus() play with SMP on single threaded TCG
pause_all_cpus() is sometimes called from a VCPU thread (e.g. s390x during special reset). It cannot deal with multiple VCPUs per Thread (single threaded TCG) yet.
Booting an s390x guest with -smp 2 and single threaded TCG from disk currently fails. The DIAG 308 will issue a pause_all_cpus() and wait forever for the CPUs to actually stop. But it is waiting for itself.
So let's stop all VCPUs belonging to the current thread. Factor out stopping of a VCPU.
--- cpus.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-)
cpus.c +19 -13
@@ -1057,13 +1057,22 @@ static void qemu_tcg_destroy_vcpu(CPUState *cpu) { } +static void qemu_cpu_stop (CPUState *cpu,bool exit) +{ +g_assert (qemu_cpu_is_self (cpu)); + cpu->stop = false; + cpu->stopped = true; +if (exit) { +cpu_exit (cpu); + } +qemu_cond_broadcast (&qemu_pause_cond); +} +static void qemu_wait_io_event_common (CPUState *cpu) {atomic_mb_set (&cpu->thread_kicked, false);if (cpu->stop) { - cpu->stop = false; - cpu->stopped = true; -qemu_cond_broadcast (&qemu_pause_cond); +qemu_cpu_stop (cpu, false); }process_queued_cpu_work (cpu); }
@@ -1610,12 +1619,12 @@ void pause_all_vcpus(void)qemu_clock_enable (QEMU_CLOCK_VIRTUAL , false);CPU_FOREACH (cpu) { - cpu->stop = true; -qemu_cpu_kick (cpu); - } - -if (qemu_in_vcpu_thread ()) { -cpu_stop_current (); +if (qemu_cpu_is_self (cpu)) { +qemu_cpu_stop (cpu, true); + }else { + cpu->stop = true; +qemu_cpu_kick (cpu); + } }while (!all_vcpus_paused ()) {
@@ -1799,10 +1808,7 @@ void qemu_init_vcpu(CPUState *cpu)void cpu_stop_current (void ) {if (current_cpu) { - current_cpu->stop = false; - current_cpu->stopped = true; -cpu_exit (current_cpu); -qemu_cond_broadcast (&qemu_pause_cond); +qemu_cpu_stop (current_cpu, true); } }
Signature · 1 line
2.14.3
PATCH 2/15 cpu-exec: fix missed CPU kick during interrupt injection
In reply to David HildenbrandThe conditional memory barrier not only looks strange but actually is wrong.
On s390x, I can reproduce interrupts via cpu_interrupt() not leading to a proper kick out of emulation every now and then. cpu_interrupt() is especially used for inter CPU communication via SIGP (esp. external calls and emergency interrupts).
With this patch, I was not able to reproduce. (esp. no stalls or hangs in the guest).
My setup is s390x MTTCG with 16 VCPUs on 8 CPU host, running make -j16.
--- accel/tcg/cpu-exec.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
accel/tcg/cpu-exec.c +4 -8
@@ -525,19 +525,15 @@ static inline bool cpu_handle_interrupt(CPUState *cpu, TranslationBlock **last_tb) {CPUClass *cc =CPU_GET_CLASS (cpu); -int32_t insns_left;/* Clear the interrupt flag now since we're processing * cpu->interrupt_request and cpu->exit_request. */ - insns_left =atomic_read (&cpu->icount_decr.u32);atomic_set (&cpu->icount_decr.u16.high,0 ); -if (unlikely (insns_left <0 )) { -/* Ensure the zeroing of icount_decr comes before the next read -* of cpu->exit_request or cpu->interrupt_request. -*/ -smp_mb (); - } +/* Ensure zeroing happens before reading cpu->exit_request or +* cpu->interrupt_request. (also see cpu_exit()) +*/ +smp_mb ();if (unlikely (atomic_read (&cpu->interrupt_request))) {int interrupt_request;
Signature · 1 line
2.14.3
PATCH 3/15 s390x/tcg: deliver multiple interrupts in a row
In reply to David HildenbrandWe have to consider all deliverable interrupts.
We now have to take care of the special scenario, where we first inject an interrupt with a WAIT PSW, followed by a !WAIT PSW. (very unlikely but possible)
--- target/s390x/excp_helper.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)
target/s390x/excp_helper.c +16 -4
@@ -433,10 +433,12 @@ void s390_cpu_do_interrupt(CPUState *cs) {S390CPU *cpu =S390_CPU (cs);CPUS390XState *env = &cpu->env; +bool stopped = false;qemu_log_mask (CPU_LOG_INT ,"%s: %d at pc=%" PRIx64"\n" , __func__, cs->exception_index, env->psw.addr); +try_deliver:/* handle machine checks */ if (cs->exception_index ==-1 &&s390_cpu_has_mcck_int (cpu)) { cs->exception_index =EXCP_MCHK ;
@@ -479,13 +481,14 @@ void s390_cpu_do_interrupt(CPUState *cs)break ;case EXCP_STOP :do_stop_interrupt (env); + stopped = true;break ; } -/* WAIT PSW during interrupt injection or STOP interrupt */ -if (cs->exception_index ==EXCP_HLT ) { -/* don't trigger a cpu_loop_exit(), use an interrupt instead */ -cpu_interrupt (CPU (cpu),CPU_INTERRUPT_HALT ); +if (cs->exception_index !=-1 && !stopped) { +/* check if there are more pending interrupts to deliver */ + cs->exception_index =-1 ; + goto try_deliver; } cs->exception_index =-1 ;
@@ -493,6 +496,15 @@ void s390_cpu_do_interrupt(CPUState *cs)if (!env->pending_int) { cs->interrupt_request &= ~CPU_INTERRUPT_HARD ; } + +/* WAIT PSW during interrupt injection or STOP interrupt */ +if ((env->psw.mask &PSW_MASK_WAIT ) || stopped) { +/* don't trigger a cpu_loop_exit(), use an interrupt instead */ +cpu_interrupt (CPU (cpu),CPU_INTERRUPT_HALT ); + }else if (cs->halted) { +/* unhalt if we had a WAIT PSW somehwere in our injection chain */ +s390_cpu_unhalt (cpu); + } }bool s390_cpu_exec_interrupt (CPUState *cs,int interrupt_request)
Signature · 1 line
2.14.3
PATCH 4/15 s390x/flic: simplify flic initialization
In reply to David HildenbrandThis makes it clearer, which device is used for which accelerator.
--- hw/intc/s390_flic.c | 9 +++++++-- hw/intc/s390_flic_kvm.c | 12 ------------ include/hw/s390x/s390_flic.h | 9 --------- 3 files changed, 7 insertions(+), 23 deletions(-)
hw/intc/s390_flic.c +7 -2
@@ -40,11 +40,16 @@ void s390_flic_init(void) {DeviceState *dev; - dev =s390_flic_kvm_create (); -if (!dev) { +if (kvm_enabled ()) { + dev =qdev_create (NULL ,TYPE_KVM_S390_FLIC ); +object_property_add_child (qdev_get_machine (),TYPE_KVM_S390_FLIC , +OBJECT (dev),NULL ); + }else if (tcg_enabled ()) { dev =qdev_create (NULL ,TYPE_QEMU_S390_FLIC );object_property_add_child (qdev_get_machine (),TYPE_QEMU_S390_FLIC ,OBJECT (dev),NULL ); + }else { +g_assert_not_reached (); }qdev_init_nofail (dev); }
hw/intc/s390_flic_kvm.c +0 -12
@@ -35,18 +35,6 @@ typedef struct KVMS390FLICState {bool clear_io_supported; } KVMS390FLICState; -DeviceState *s390_flic_kvm_create (void ) -{ -DeviceState *dev =NULL ; - -if (kvm_enabled ()) { - dev =qdev_create (NULL ,TYPE_KVM_S390_FLIC ); -object_property_add_child (qdev_get_machine (),TYPE_KVM_S390_FLIC , -OBJECT (dev),NULL ); - } -return dev; -} -/** * flic_get_all_irqs - store all pending irqs in buffer * @buf: pointer to buffer which is passed to kernel
include/hw/s390x/s390_flic.h +0 -9
@@ -91,13 +91,4 @@ void s390_flic_init(void);S390FLICState *s390_get_flic (void );bool ais_needed (void *opaque); -#ifdef CONFIG_KVM -DeviceState *s390_flic_kvm_create (void ); -#else -static inline DeviceState *s390_flic_kvm_create (void ) -{ -return NULL ; -} -#endif -#endif /* HW_S390_FLIC_H */
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v1 for-2-12 04/15] s390x/flic: simplify flic initialization
Quoted 3 lines · David Hildenbrand
This makes it clearer, which device is used for which accelerator.
Signed-off-by David Hildenbrand <[email protected]>
nice.
Quoted 71 lines
--- hw/intc/s390_flic.c | 9 +++++++-- hw/intc/s390_flic_kvm.c | 12 ------------ include/hw/s390x/s390_flic.h | 9 --------- 3 files changed, 7 insertions(+), 23 deletions(-)hw/intc/s390_flic.c +7
-2@@ -40,11 +40,16 @@ void s390_flic_init(void) {DeviceState *dev; - dev =s390_flic_kvm_create (); -if (!dev) { +if (kvm_enabled ()) { + dev =qdev_create (NULL ,TYPE_KVM_S390_FLIC ); +object_property_add_child (qdev_get_machine (),TYPE_KVM_S390_FLIC , +OBJECT (dev),NULL ); + }else if (tcg_enabled ()) { dev =qdev_create (NULL ,TYPE_QEMU_S390_FLIC );object_property_add_child (qdev_get_machine (),TYPE_QEMU_S390_FLIC ,OBJECT (dev),NULL ); + }else { +g_assert_not_reached (); }qdev_init_nofail (dev); }hw/intc/s390_flic_kvm.c +0
-12@@ -35,18 +35,6 @@ typedef struct KVMS390FLICState {bool clear_io_supported; } KVMS390FLICState; -DeviceState *s390_flic_kvm_create (void ) -{ -DeviceState *dev =NULL ; - -if (kvm_enabled ()) { - dev =qdev_create (NULL ,TYPE_KVM_S390_FLIC ); -object_property_add_child (qdev_get_machine (),TYPE_KVM_S390_FLIC , -OBJECT (dev),NULL ); - } -return dev; -} -/** * flic_get_all_irqs - store all pending irqs in buffer * @buf: pointer to buffer which is passed to kernel include/hw/s390x/s390_flic.h +0
-9@@ -91,13 +91,4 @@ void s390_flic_init(void);S390FLICState *s390_get_flic (void );bool ais_needed (void *opaque); -#ifdef CONFIG_KVM -DeviceState *s390_flic_kvm_create (void ); -#else -static inline DeviceState *s390_flic_kvm_create (void ) -{ -return NULL ; -} -#endif -#endif /* HW_S390_FLIC_H */
Re: [Qemu-devel] [PATCH v1 for-2-12 04/15] s390x/flic: simplify flic initialization
In reply to David HildenbrandQuoted 17 lines · On Mon, 11 Dec 2017 14:47:29 +0100 David Hildenbrand <[email protected]> wrote:
This makes it clearer, which device is used for which accelerator.
Signed-off-by David Hildenbrand <[email protected]>--- hw/intc/s390_flic.c | 9 +++++++-- hw/intc/s390_flic_kvm.c | 12 ------------ include/hw/s390x/s390_flic.h | 9 --------- 3 files changed, 7 insertions(+), 23 deletions(-)hw/intc/s390_flic.c +0
-0@@ -40,11 +40,16 @@ void s390_flic_init(void) {DeviceState *dev;
hw/intc/s390_flic.c void s390_flic_init(void) · line 50
Quoted 10 lines
hw/intc/s390_flic.c +5
-2- dev =s390_flic_kvm_create (); -if (!dev) { +if (kvm_enabled ()) { + dev =qdev_create (NULL ,TYPE_KVM_S390_FLIC ); +object_property_add_child (qdev_get_machine (),TYPE_KVM_S390_FLIC , +OBJECT (dev),NULL ); + }else if (tcg_enabled ()) { dev =qdev_create (NULL ,TYPE_QEMU_S390_FLIC );object_property_add_child (qdev_get_machine (),TYPE_QEMU_S390_FLIC ,OBJECT (dev),NULL );
Can you use TYPE_S390_FLIC_COMMON for attaching the flic to the machine?
hw/intc/s390_flic.c void s390_flic_init(void) · line 52
Quoted 2 lines
hw/intc/s390_flic.c +2
-0+ }else { +g_assert_not_reached ();
Checking for tcg_enabled() explicitly does not seem the common pattern, although it is fine with me (I doubt we'll support other accelerators for s390x in the foreseeable future).
hw/intc/s390_flic.c void s390_flic_init(void) · line 55
Quoted 3 lines
hw/intc/s390_flic.c +0
-0}qdev_init_nofail (dev); }
Do we want to switch to the same pattern for the storage attribute device as well?
Change looks fine to me.
Re: [Qemu-devel] [PATCH v1 for-2-12 04/15] s390x/flic: simplify flic initialization
Quoted 20 lines · Cornelia Huck
Quoted 17 lines · On Mon, 11 Dec 2017 14:47:29 +0100 David Hildenbrand <[email protected]> wrote:
This makes it clearer, which device is used for which accelerator.
Signed-off-by David Hildenbrand <[email protected]>--- hw/intc/s390_flic.c | 9 +++++++-- hw/intc/s390_flic_kvm.c | 12 ------------ include/hw/s390x/s390_flic.h | 9 --------- 3 files changed, 7 insertions(+), 23 deletions(-)hw/intc/s390_flic.c +0
-0@@ -40,11 +40,16 @@ void s390_flic_init(void) {DeviceState *dev;
Quoted 12 lines
hw/intc/s390_flic.c void s390_flic_init(void) · line 50
Quoted 10 lines
hw/intc/s390_flic.c +5
-2- dev =s390_flic_kvm_create (); -if (!dev) { +if (kvm_enabled ()) { + dev =qdev_create (NULL ,TYPE_KVM_S390_FLIC ); +object_property_add_child (qdev_get_machine (),TYPE_KVM_S390_FLIC , +OBJECT (dev),NULL ); + }else if (tcg_enabled ()) { dev =qdev_create (NULL ,TYPE_QEMU_S390_FLIC );object_property_add_child (qdev_get_machine (),TYPE_QEMU_S390_FLIC ,OBJECT (dev),NULL );Can you use
TYPE_S390_FLIC_COMMONfor attaching the flic to the machine?
I suggest doing that in a separate patch. (I remember that changing the name should not harm migration).
Quoted 7 lines
hw/intc/s390_flic.c void s390_flic_init(void) · line 52
Quoted 3 lines
hw/intc/s390_flic.c +2
-0+ }else { +g_assert_not_reached ();Checking for
tcg_enabled()explicitly does not seem the common pattern, although it is fine with me (I doubt we'll support other accelerators for s390x in the foreseeable future).
Indeed, I can drop that.
Quoted 7 lines
hw/intc/s390_flic.c void s390_flic_init(void) · line 55
Quoted 4 lines
hw/intc/s390_flic.c +0
-0}qdev_init_nofail (dev); }Do we want to switch to the same pattern for the storage attribute device as well?
Yes, can have a look, thanks!
Quoted 3 lines
Change looks fine to me.
Signature · 3 lines
Thanks, David / dhildenb
Re: [Qemu-devel] [PATCH v1 for-2-12 04/15] s390x/flic: simplify flic initialization
Quoted 34 lines · On Mon, 11 Dec 2017 21:34:20 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 20 lines · Cornelia Huck
Quoted 17 lines · On Mon, 11 Dec 2017 14:47:29 +0100 David Hildenbrand <[email protected]> wrote:
This makes it clearer, which device is used for which accelerator.
Signed-off-by David Hildenbrand <[email protected]>--- hw/intc/s390_flic.c | 9 +++++++-- hw/intc/s390_flic_kvm.c | 12 ------------ include/hw/s390x/s390_flic.h | 9 --------- 3 files changed, 7 insertions(+), 23 deletions(-)hw/intc/s390_flic.c +0
-0@@ -40,11 +40,16 @@ void s390_flic_init(void) {DeviceState *dev;Quoted 12 lines
hw/intc/s390_flic.c void s390_flic_init(void) · line 50
Quoted 10 lines
hw/intc/s390_flic.c +5
-2- dev =s390_flic_kvm_create (); -if (!dev) { +if (kvm_enabled ()) { + dev =qdev_create (NULL ,TYPE_KVM_S390_FLIC ); +object_property_add_child (qdev_get_machine (),TYPE_KVM_S390_FLIC , +OBJECT (dev),NULL ); + }else if (tcg_enabled ()) { dev =qdev_create (NULL ,TYPE_QEMU_S390_FLIC );object_property_add_child (qdev_get_machine (),TYPE_QEMU_S390_FLIC ,OBJECT (dev),NULL );Can you use
TYPE_S390_FLIC_COMMONfor attaching the flic to the machine?
Quoted 2 lines
I suggest doing that in a separate patch. (I remember that changing the name should not harm migration).
I don't think it harms migration, as we hook up the same device as before (just via the common flic device).
Probably does not make sense as a separate patch, though, as you touch this code anyway.
Quoted 10 lines
Quoted 8 lines
hw/intc/s390_flic.c void s390_flic_init(void) · line 52
Quoted 4 lines
hw/intc/s390_flic.c +2
-0+ }else { +g_assert_not_reached ();Checking for
tcg_enabled()explicitly does not seem the common pattern, although it is fine with me (I doubt we'll support other accelerators for s390x in the foreseeable future).Indeed, I can drop that.
Yup.
Re: [Qemu-devel] [PATCH v1 for-2-12 04/15] s390x/flic: simplify flic initialization
Quoted 37 lines · Cornelia Huck
Quoted 34 lines · On Mon, 11 Dec 2017 21:34:20 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 32 lines · Cornelia Huck
Quoted 17 lines · On Mon, 11 Dec 2017 14:47:29 +0100 David Hildenbrand <[email protected]> wrote:
This makes it clearer, which device is used for which accelerator.
Signed-off-by David Hildenbrand <[email protected]>--- hw/intc/s390_flic.c | 9 +++++++-- hw/intc/s390_flic_kvm.c | 12 ------------ include/hw/s390x/s390_flic.h | 9 --------- 3 files changed, 7 insertions(+), 23 deletions(-)hw/intc/s390_flic.c +0
-0@@ -40,11 +40,16 @@ void s390_flic_init(void) {DeviceState *dev;hw/intc/s390_flic.c void s390_flic_init(void) · line 50
Quoted 10 lines
hw/intc/s390_flic.c +5
-2- dev =s390_flic_kvm_create (); -if (!dev) { +if (kvm_enabled ()) { + dev =qdev_create (NULL ,TYPE_KVM_S390_FLIC ); +object_property_add_child (qdev_get_machine (),TYPE_KVM_S390_FLIC , +OBJECT (dev),NULL ); + }else if (tcg_enabled ()) { dev =qdev_create (NULL ,TYPE_QEMU_S390_FLIC );object_property_add_child (qdev_get_machine (),TYPE_QEMU_S390_FLIC ,OBJECT (dev),NULL );Can you use
TYPE_S390_FLIC_COMMONfor attaching the flic to the machine?
Quoted 8 lines
Quoted 2 lines
I suggest doing that in a separate patch. (I remember that changing the name should not harm migration).
I don't think it harms migration, as we hook up the same device as before (just via the common flic device).
Probably does not make sense as a separate patch, though, as you touch this code anyway.
Please be aware that for object_property_add_child(), TYPE... is for our example the name of the child
/machine/s390-flic-qemu/
/machine/s390x-flic-kvm/
Not a type. And that would mean a change.
Signature · 3 lines
Thanks, David / dhildenb
Re: [Qemu-devel] [PATCH v1 for-2-12 04/15] s390x/flic: simplify flic initialization
Quoted 47 lines · On Tue, 12 Dec 2017 10:58:12 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 37 lines · Cornelia Huck
Quoted 34 lines · On Mon, 11 Dec 2017 21:34:20 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 32 lines · Cornelia Huck
Quoted 17 lines · On Mon, 11 Dec 2017 14:47:29 +0100 David Hildenbrand <[email protected]> wrote:
This makes it clearer, which device is used for which accelerator.
Signed-off-by David Hildenbrand <[email protected]>--- hw/intc/s390_flic.c | 9 +++++++-- hw/intc/s390_flic_kvm.c | 12 ------------ include/hw/s390x/s390_flic.h | 9 --------- 3 files changed, 7 insertions(+), 23 deletions(-)hw/intc/s390_flic.c +0
-0@@ -40,11 +40,16 @@ void s390_flic_init(void) {DeviceState *dev;hw/intc/s390_flic.c void s390_flic_init(void) · line 50
Quoted 10 lines
hw/intc/s390_flic.c +5
-2- dev =s390_flic_kvm_create (); -if (!dev) { +if (kvm_enabled ()) { + dev =qdev_create (NULL ,TYPE_KVM_S390_FLIC ); +object_property_add_child (qdev_get_machine (),TYPE_KVM_S390_FLIC , +OBJECT (dev),NULL ); + }else if (tcg_enabled ()) { dev =qdev_create (NULL ,TYPE_QEMU_S390_FLIC );object_property_add_child (qdev_get_machine (),TYPE_QEMU_S390_FLIC ,OBJECT (dev),NULL );Can you use
TYPE_S390_FLIC_COMMONfor attaching the flic to the machine?Quoted 8 lines
Quoted 2 lines
I suggest doing that in a separate patch. (I remember that changing the name should not harm migration).
I don't think it harms migration, as we hook up the same device as before (just via the common flic device).
Probably does not make sense as a separate patch, though, as you touch this code anyway.
Quoted 9 lines
Please be aware that for
object_property_add_child(), TYPE... is for our example the name of the child/machine/s390-flic-qemu/
/machine/s390x-flic-kvm/Not a type. And that would mean a change.
Ugh. Ok, let's keep it. The skeys device (for example) used the common name from the start.
David Hildenbrand
Reviewed-by Christian BorntraegerRe: [Qemu-devel] [PATCH v1 for-2-12 04/15] s390x/flic: simplify flic initialization
In reply to Cornelia HuckQuoted 37 lines · Cornelia Huck
Quoted 34 lines · On Mon, 11 Dec 2017 21:34:20 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 32 lines · Cornelia Huck
Quoted 17 lines · On Mon, 11 Dec 2017 14:47:29 +0100 David Hildenbrand <[email protected]> wrote:
This makes it clearer, which device is used for which accelerator.
Signed-off-by David Hildenbrand <[email protected]>--- hw/intc/s390_flic.c | 9 +++++++-- hw/intc/s390_flic_kvm.c | 12 ------------ include/hw/s390x/s390_flic.h | 9 --------- 3 files changed, 7 insertions(+), 23 deletions(-)hw/intc/s390_flic.c +0
-0@@ -40,11 +40,16 @@ void s390_flic_init(void) {DeviceState *dev;hw/intc/s390_flic.c void s390_flic_init(void) · line 50
Quoted 10 lines
hw/intc/s390_flic.c +5
-2- dev =s390_flic_kvm_create (); -if (!dev) { +if (kvm_enabled ()) { + dev =qdev_create (NULL ,TYPE_KVM_S390_FLIC ); +object_property_add_child (qdev_get_machine (),TYPE_KVM_S390_FLIC , +OBJECT (dev),NULL ); + }else if (tcg_enabled ()) { dev =qdev_create (NULL ,TYPE_QEMU_S390_FLIC );object_property_add_child (qdev_get_machine (),TYPE_QEMU_S390_FLIC ,OBJECT (dev),NULL );Can you use
TYPE_S390_FLIC_COMMONfor attaching the flic to the machine?
Quoted 9 lines
Quoted 2 lines
I suggest doing that in a separate patch. (I remember that changing the name should not harm migration).
I don't think it harms migration, as we hook up the same device as before (just via the common flic device).
Probably does not make sense as a separate patch, though, as you touch this code anyway.
This is what it looks like now:
commit dec1ff5cfc72fa0998e28a25dd23f0695ddfe21b
Author: David Hildenbrand <[email protected]> Date: Mon Nov 13 23:09:56 2017 +0100
s390x/flic: simplify flic initialization
This makes it clearer, which device is used for which accelerator.
We can directly attach both to /machine/s390-flic/ instead of two different locations (/machine/s390x-flic[qemu|kvm]). Should not harm migration.
hw/intc/s390_flic.c +5 -4
@@ -40,12 +40,13 @@ void s390_flic_init(void) {DeviceState *dev; - dev =s390_flic_kvm_create (); -if (!dev) { +if (kvm_enabled ()) { + dev =qdev_create (NULL ,TYPE_KVM_S390_FLIC ); + }else { dev =qdev_create (NULL ,TYPE_QEMU_S390_FLIC ); -object_property_add_child (qdev_get_machine (),TYPE_QEMU_S390_FLIC , -OBJECT (dev),NULL ); } +object_property_add_child (qdev_get_machine (),TYPE_S390_FLIC_COMMON , +OBJECT (dev),NULL );qdev_init_nofail (dev); }
...
Signature · 3 lines
Thanks, David / dhildenb
Re: [Qemu-devel] [PATCH v1 for-2-12 04/15] s390x/flic: simplify flic initialization
Quoted 6 lines · On Tue, 12 Dec 2017 11:45:22 +0100 David Hildenbrand <[email protected]> wrote:
This is what it looks like now:
commit
dec1ff5cfc72fa0998e28a25dd23f0695ddfe21bAuthor: David Hildenbrand <[email protected]> Date: Mon Nov 13 23:09:56 2017 +0100
Quoted 7 lines
s390x/flic: simplify flic initialization
This makes it clearer, which device is used for which accelerator.
We can directly attach both to /machine/s390-flic/ instead of two different locations (/machine/s390x-flic[qemu|kvm]). Should not harm migration.
Not sure about libvirt usage, though. Let's keep it as-is?
Quoted 13 lines
Reviewed-by Christian Borntraeger <[email protected]>Signed-off-by David Hildenbrand <[email protected]>hw/intc/s390_flic.c +0
-1@@ -40,12 +40,13 @@ void s390_flic_init(void) {DeviceState *dev; - dev =s390_flic_kvm_create ();
hw/intc/s390_flic.c void s390_flic_init(void) · line 49
Quoted 10 lines
hw/intc/s390_flic.c +5
-3-if (!dev) { +if (kvm_enabled ()) { + dev =qdev_create (NULL ,TYPE_KVM_S390_FLIC ); + }else { dev =qdev_create (NULL ,TYPE_QEMU_S390_FLIC ); -object_property_add_child (qdev_get_machine (),TYPE_QEMU_S390_FLIC , -OBJECT (dev),NULL ); } +object_property_add_child (qdev_get_machine (),TYPE_S390_FLIC_COMMON , +OBJECT (dev),NULL );
Dropping the check for tcg_enabled() is fine with me, though.
Quoted 5 lines
hw/intc/s390_flic.c +0
-0qdev_init_nofail (dev); }
Re: [Qemu-devel] [PATCH v1 for-2-12 04/15] s390x/flic: simplify flic initialization
In reply to Cornelia HuckQuoted 4 lines
Do we want to switch to the same pattern for the storage attribute device as well?
Unfortunately not that easy due to the KVM capability check. Will leave it as is for now.
Quoted 2 lines
Change looks fine to me.
Signature · 3 lines
Thanks, David / dhildenb
PATCH 5/15 s390x/tcg: simplify machine check handling
In reply to David HildenbrandWe currently only support CRW machine checks. This is a preparation for real floating interrupt support.
Get rid of the queue and handle it via the bit INTERRUPT_MCHK. We don't rename it for now, as it will be soon gone (when moving crw machine checks into the flic).
Please note that this is the same way also KVM handles it: only one instance of a machine check can be pending at a time. So no need for a queue.
While at it, make sure we try to deliver only if env->cregs[14] actually indicates that CRWs are accepted.
Drop two unused defines on the way (we already have PSW_MASK_...).
--- target/s390x/cpu.c | 2 -- target/s390x/cpu.h | 10 ---------- target/s390x/excp_helper.c | 29 +++++------------------------ target/s390x/interrupt.c | 18 +++++++----------- 4 files changed, 12 insertions(+), 47 deletions(-)
target/s390x/cpu.c +0 -2
@@ -118,7 +118,6 @@ static void s390_cpu_initial_reset(CPUState *s)for (i =0 ; i <ARRAY_SIZE (env->io_index); i++) { env->io_index[i] =-1 ; } - env->mchk_index =-1 ;/* tininess for underflow is detected before rounding */ set_float_detect_tininess(float_tininess_before_rounding,
@@ -155,7 +154,6 @@ static void s390_cpu_full_reset(CPUState *s)for (i =0 ; i <ARRAY_SIZE (env->io_index); i++) { env->io_index[i] =-1 ; } - env->mchk_index =-1 ;/* tininess for underflow is detected before rounding */ set_float_detect_tininess(float_tininess_before_rounding,
target/s390x/cpu.h +0 -10
@@ -54,10 +54,6 @@#define MMU_USER_IDX 0#define MAX_IO_QUEUE 16 -#define MAX_MCHK_QUEUE 16 - -#define PSW_MCHK_MASK 0x0004000000000000 -#define PSW_IO_MASK 0x0200000000000000#define S390_MAX_CPUS 248
@@ -73,10 +69,6 @@ typedef struct IOIntQueue {uint32_t word; } IOIntQueue; -typedef struct MchkQueue { -uint16_t type; -}MchkQueue ; -struct CPUS390XState {uint64_t regs[16 ];/* GP registers */ /*
@@ -122,14 +114,12 @@ struct CPUS390XState {uint64_t cregs[16 ];/* control registers */ IOIntQueue io_queue[MAX_IO_QUEUE ][8 ]; -MchkQueue mchk_queue[MAX_MCHK_QUEUE ];int pending_int;uint32_t service_param;uint16_t external_call_addr;DECLARE_BITMAP (emergency_signals,S390_MAX_CPUS );int io_index[8 ]; -int mchk_index;uint64_t ckc;uint64_t cputm;
target/s390x/excp_helper.c +5 -24
@@ -368,30 +368,16 @@ static void do_io_interrupt(CPUS390XState *env)static void do_mchk_interrupt (CPUS390XState *env) { -S390CPU *cpu =s390_env_get_cpu (env);uint64_t mask, addr;LowCore *lowcore; -MchkQueue *q;int i; -if (!(env->psw.mask &PSW_MASK_MCHECK )) { -cpu_abort (CPU (cpu),"Machine check w/o mchk mask\n" ); - } +/* for now we only support channel report machine checks (floating) */ +g_assert (env->psw.mask &PSW_MASK_MCHECK ); +g_assert (env->cregs[14 ] &CR0_SERVICE_SC ); -if (env->mchk_index <0 || env->mchk_index >=MAX_MCHK_QUEUE ) { -cpu_abort (CPU (cpu),"Mchk queue overrun: %d\n" , env->mchk_index); - } - - q = &env->mchk_queue[env->mchk_index]; - -if (q->type !=1 ) { -/* Don't know how to handle this... */ -cpu_abort (CPU (cpu),"Unknown machine check type %d\n" , q->type); - } -if (!(env->cregs[14 ] & (1 <<28 ))) { -/* CRW machine checks disabled */ -return ; - } +g_assert (env->pending_int &INTERRUPT_MCHK ); + env->pending_int &= ~INTERRUPT_MCHK ; lowcore =cpu_map_lowcore (env);
@@ -418,11 +404,6 @@ static void do_mchk_interrupt(CPUS390XState *env)cpu_unmap_lowcore (lowcore); - env->mchk_index--; -if (env->mchk_index ==-1 ) { - env->pending_int &= ~INTERRUPT_MCHK ; - } -DPRINTF ("%s: %" PRIx64" %" PRIx64"\n" , __func__, env->psw.mask, env->psw.addr);
target/s390x/interrupt.c +7 -11
@@ -162,16 +162,6 @@ static void cpu_inject_crw_mchk(S390CPU *cpu) {CPUS390XState *env = &cpu->env; -if (env->mchk_index ==MAX_MCHK_QUEUE -1 ) { -/* ugh - can't queue anymore. Let's drop. */ -return ; - } - - env->mchk_index++; -assert (env->mchk_index <MAX_MCHK_QUEUE ); - - env->mchk_queue[env->mchk_index].type =1 ; - env->pending_int |=INTERRUPT_MCHK ;cpu_interrupt (CPU (cpu),CPU_INTERRUPT_HARD ); }
@@ -225,7 +215,13 @@ bool s390_cpu_has_mcck_int(S390CPU *cpu)return false; } -return env->pending_int &INTERRUPT_MCHK ; +/* for now we only support channel report machine checks (floating) */ +if ((env->pending_int &INTERRUPT_MCHK ) && + (env->cregs[14 ] &CR14_CHANNEL_REPORT_SC )) { +return true; + } + +return false; }bool s390_cpu_has_ext_int (S390CPU *cpu)
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v1 for-2-12 05/15] s390x/tcg: simplify machine check handling
Quoted 10 lines · On Mon, 11 Dec 2017 14:47:30 +0100 David Hildenbrand <[email protected]> wrote:
We currently only support CRW machine checks. This is a preparation for real floating interrupt support.
Get rid of the queue and handle it via the bit
INTERRUPT_MCHK. We don't rename it for now, as it will be soon gone (when moving crw machine checks into the flic).Please note that this is the same way also KVM handles it: only one instance of a machine check can be pending at a time. So no need for a queue.
That's basically architecture (IIRC, it's been a while). More pending machine checks are handled by merging into the mcic.
Quoted 13 lines
While at it, make sure we try to deliver only if env->cregs[14] actually indicates that CRWs are accepted.
Drop two unused defines on the way (we already have
PSW_MASK_...).Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/cpu.c | 2 -- target/s390x/cpu.h | 10 ---------- target/s390x/excp_helper.c | 29 +++++------------------------ target/s390x/interrupt.c | 18 +++++++----------- 4 files changed, 12 insertions(+), 47 deletions(-)
PATCH 6/15 s390x/flic: factor out injection of floating interrupts
In reply to David HildenbrandLet the flic device handle it internally. This will allow us to later on store floating interrupts in the flic for the TCG case.
This now also simplifies kvm.c. All that's left is the fallback interface for floating interrupts, which is no triggered directly via the flic in case anything goes wrong.
--- hw/intc/s390_flic.c | 31 ++++++++++++++++++++ hw/intc/s390_flic_kvm.c | 63 ++++++++++++++++++++++++++++++++++++---- include/hw/s390x/s390_flic.h | 5 ++++ target/s390x/cpu.h | 7 ++++- target/s390x/interrupt.c | 42 +++++++++++---------------- target/s390x/kvm-stub.c | 13 --------- target/s390x/kvm.c | 68 ++++---------------------------------------- target/s390x/kvm_s390x.h | 10 +------ 8 files changed, 123 insertions(+), 116 deletions(-)
hw/intc/s390_flic.c +31 -0
@@ -131,6 +131,34 @@ static int qemu_s390_inject_airq(S390FLICState *fs, uint8_t type,return 0 ; } +static void qemu_s390_inject_service (S390FLICState *fs,uint32_t parm) +{ + +S390CPU *dummy_cpu =s390_cpu_addr2state (0 ); + +/* FIXME: don't inject into dummy CPU */ +cpu_inject_service (dummy_cpu, parm); +} + +static void qemu_s390_inject_io (S390FLICState *fs,uint16_t subchannel_id, +uint16_t subchannel_nr,uint32_t io_int_parm, +uint32_t io_int_word) +{ +S390CPU *dummy_cpu =s390_cpu_addr2state (0 ); + +/* FIXME: don't inject into dummy CPU */ +cpu_inject_io (dummy_cpu, subchannel_id, subchannel_nr, io_int_parm, + io_int_word); +} + +static void qemu_s390_inject_crw_mchk (S390FLICState *fs) +{ +S390CPU *dummy_cpu =s390_cpu_addr2state (0 ); + +/* FIXME: don't inject into dummy CPU */ +cpu_inject_crw_mchk (dummy_cpu); +} +static void qemu_s390_flic_reset (DeviceState *dev) {QEMUS390FLICState *flic =QEMU_S390_FLIC (dev);
@@ -172,6 +200,9 @@ static void qemu_s390_flic_class_init(ObjectClass *oc, void *data) fsc->clear_io_irq = qemu_s390_clear_io_flic; fsc->modify_ais_mode = qemu_s390_modify_ais_mode; fsc->inject_airq = qemu_s390_inject_airq; + fsc->inject_service = qemu_s390_inject_service; + fsc->inject_io = qemu_s390_inject_io; + fsc->inject_crw_mchk = qemu_s390_inject_crw_mchk; }static Property s390_flic_common_properties[] = {
hw/intc/s390_flic_kvm.c +58 -5
78 lines not on this page · raw message
include/hw/s390x/s390_flic.h +5 -0
@@ -66,6 +66,11 @@ typedef struct S390FLICStateClass {int (*modify_ais_mode)(S390FLICState *fs,uint8_t isc,uint16_t mode);int (*inject_airq)(S390FLICState *fs,uint8_t type,uint8_t isc,uint8_t flags); +void (*inject_service)(S390FLICState *fs,uint32_t parm); +void (*inject_io)(S390FLICState *fs,uint16_t subchannel_id, +uint16_t subchannel_nr,uint32_t io_int_parm, +uint32_t io_int_word); +void (*inject_crw_mchk)(S390FLICState *fs); } S390FLICStateClass;#define TYPE_KVM_S390_FLIC "s390-flic-kvm"
target/s390x/cpu.h +6 -1
@@ -740,7 +740,12 @@ void s390_program_interrupt(CPUS390XState *env, uint32_t code, int ilen, uintptr_t ra);/* service interrupts are floating therefore we must not pass an cpustate */ void s390_sclp_extint (uint32_t parm); - +/* FIXME: remove once we have proper floating interrupts in TCG */ +void cpu_inject_service (S390CPU *cpu,uint32_t param); +void cpu_inject_crw_mchk (S390CPU *cpu); +void cpu_inject_io (S390CPU *cpu,uint16_t subchannel_id, +uint16_t subchannel_number,uint32_t io_int_parm, +uint32_t io_int_word);/* mmu_helper.c */ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf,
target/s390x/interrupt.c +17 -25
84 lines not on this page · raw message
target/s390x/kvm-stub.c +0 -13
@@ -12,10 +12,6 @@#include "cpu.h" #include "kvm_s390x.h" -void kvm_s390_service_interrupt (uint32_t parm) -{ -} -void kvm_s390_access_exception (S390CPU *cpu,uint16_t code,uint64_t te_code) { }
@@ -30,15 +26,6 @@ void kvm_s390_program_interrupt(S390CPU *cpu, uint16_t code) { } -void kvm_s390_io_interrupt (uint16_t subchannel_id,uint16_t subchannel_nr, -uint32_t io_int_parm,uint32_t io_int_word) -{ -} - -void kvm_s390_crw_mchk (void ) -{ -} -int kvm_s390_set_cpu_state (S390CPU *cpu,uint8_t cpu_state) {return -ENOSYS ;
target/s390x/kvm.c +5 -63
92 lines not on this page · raw message
target/s390x/kvm_s390x.h +1 -9
@@ -12,17 +12,12 @@struct kvm_s390_irq ; -void kvm_s390_floating_interrupt (struct kvm_s390_irq *irq); -void kvm_s390_service_interrupt (uint32_t parm); +void kvm_s390_floating_interrupt_legacy (struct kvm_s390_irq *irq);void kvm_s390_vcpu_interrupt (S390CPU *cpu,struct kvm_s390_irq *irq);void kvm_s390_access_exception (S390CPU *cpu,uint16_t code,uint64_t te_code);int kvm_s390_mem_op (S390CPU *cpu,vaddr addr,uint8_t ar,void *hostbuf,int len,bool is_write);void kvm_s390_program_interrupt (S390CPU *cpu,uint16_t code); -void kvm_s390_io_interrupt (uint16_t subchannel_id, -uint16_t subchannel_nr,uint32_t io_int_parm, -uint32_t io_int_word); -void kvm_s390_crw_mchk (void );int kvm_s390_set_cpu_state (S390CPU *cpu,uint8_t cpu_state);void kvm_s390_vcpu_interrupt_pre_save (S390CPU *cpu);int kvm_s390_vcpu_interrupt_post_load (S390CPU *cpu);
@@ -44,7 +39,4 @@ void kvm_s390_crypto_reset(void);void kvm_s390_restart_interrupt (S390CPU *cpu);void kvm_s390_stop_interrupt (S390CPU *cpu); -/* implemented outside of target/s390x/ */ -int kvm_s390_inject_flic (struct kvm_s390_irq *irq); -#endif /* KVM_S390X_H */
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v1 for-2-12 06/15] s390x/flic: factor out injection of floating interrupts
Quoted 5 lines · On Mon, 11 Dec 2017 14:47:31 +0100 David Hildenbrand <[email protected]> wrote:
Let the flic device handle it internally. This will allow us to later on store floating interrupts in the flic for the TCG case.
This now also simplifies
kvm.c. All that's left is the fallback interface for floating interrupts, which is no triggered directly via
s/no/now/
Quoted 54 lines
the flic in case anything goes wrong.
Signed-off-by David Hildenbrand <[email protected]>--- hw/intc/s390_flic.c | 31 ++++++++++++++++++++ hw/intc/s390_flic_kvm.c | 63 ++++++++++++++++++++++++++++++++++++---- include/hw/s390x/s390_flic.h | 5 ++++ target/s390x/cpu.h | 7 ++++- target/s390x/interrupt.c | 42 +++++++++++---------------- target/s390x/kvm-stub.c | 13 --------- target/s390x/kvm.c | 68 ++++---------------------------------------- target/s390x/kvm_s390x.h | 10 +------ 8 files changed, 123 insertions(+), 116 deletions(-)hw/intc/s390_flic.c +28
-0@@ -131,6 +131,34 @@ static int qemu_s390_inject_airq(S390FLICState *fs, uint8_t type,return 0 ; } +static void qemu_s390_inject_service (S390FLICState *fs,uint32_t parm) +{ + +S390CPU *dummy_cpu =s390_cpu_addr2state (0 ); + +/* FIXME: don't inject into dummy CPU */ +cpu_inject_service (dummy_cpu, parm); +} + +static void qemu_s390_inject_io (S390FLICState *fs,uint16_t subchannel_id, +uint16_t subchannel_nr,uint32_t io_int_parm, +uint32_t io_int_word) +{ +S390CPU *dummy_cpu =s390_cpu_addr2state (0 ); + +/* FIXME: don't inject into dummy CPU */ +cpu_inject_io (dummy_cpu, subchannel_id, subchannel_nr, io_int_parm, + io_int_word); +} + +static void qemu_s390_inject_crw_mchk (S390FLICState *fs) +{ +S390CPU *dummy_cpu =s390_cpu_addr2state (0 ); + +/* FIXME: don't inject into dummy CPU */ +cpu_inject_crw_mchk (dummy_cpu); +} +static void qemu_s390_flic_reset (DeviceState *dev) {QEMUS390FLICState *flic =QEMU_S390_FLIC (dev);
hw/intc/s390_flic.c static void qemu_s390_flic_class_init(ObjectClass *oc, void *data) · line 206
Quoted 7 lines
hw/intc/s390_flic.c +3
-0@@ -172,6 +200,9 @@ static void qemu_s390_flic_class_init(ObjectClass *oc, void *data) fsc->clear_io_irq = qemu_s390_clear_io_flic; fsc->modify_ais_mode = qemu_s390_modify_ais_mode; fsc->inject_airq = qemu_s390_inject_airq; + fsc->inject_service = qemu_s390_inject_service; + fsc->inject_io = qemu_s390_inject_io; + fsc->inject_crw_mchk = qemu_s390_inject_crw_mchk; }
As you now have a callback for ->inject_io(), make qemu_s390_inject_airq() invoke it directly instead of going the detour through s390_io_interrupt()?
hw/intc/s390_flic.c static void qemu_s390_flic_class_init(ObjectClass *oc, void *data) · line 208
Quoted 2 lines
hw/intc/s390_flic.c +0
-0static Property s390_flic_common_properties[] = {
(...)
Generally looks sane.
One thing I noticed: You removed the caching of the flic (in the old kvm inject routine), and you generally do more qom invocations (first, to find the common flic; then, to translate to the qemu or kvm flic). Not sure if this might be a problem (probably not).
Re: [Qemu-devel] [PATCH v1 for-2-12 06/15] s390x/flic: factor out injection of floating interrupts
Quoted 80 lines · Cornelia Huck
Quoted 5 lines · On Mon, 11 Dec 2017 14:47:31 +0100 David Hildenbrand <[email protected]> wrote:
Let the flic device handle it internally. This will allow us to later on store floating interrupts in the flic for the TCG case.
This now also simplifies
kvm.c. All that's left is the fallback interface for floating interrupts, which is no triggered directly vias/no/now/
Quoted 54 lines
the flic in case anything goes wrong.
Signed-off-by David Hildenbrand <[email protected]>--- hw/intc/s390_flic.c | 31 ++++++++++++++++++++ hw/intc/s390_flic_kvm.c | 63 ++++++++++++++++++++++++++++++++++++---- include/hw/s390x/s390_flic.h | 5 ++++ target/s390x/cpu.h | 7 ++++- target/s390x/interrupt.c | 42 +++++++++++---------------- target/s390x/kvm-stub.c | 13 --------- target/s390x/kvm.c | 68 ++++---------------------------------------- target/s390x/kvm_s390x.h | 10 +------ 8 files changed, 123 insertions(+), 116 deletions(-)hw/intc/s390_flic.c +28
-0@@ -131,6 +131,34 @@ static int qemu_s390_inject_airq(S390FLICState *fs, uint8_t type,return 0 ; } +static void qemu_s390_inject_service (S390FLICState *fs,uint32_t parm) +{ + +S390CPU *dummy_cpu =s390_cpu_addr2state (0 ); + +/* FIXME: don't inject into dummy CPU */ +cpu_inject_service (dummy_cpu, parm); +} + +static void qemu_s390_inject_io (S390FLICState *fs,uint16_t subchannel_id, +uint16_t subchannel_nr,uint32_t io_int_parm, +uint32_t io_int_word) +{ +S390CPU *dummy_cpu =s390_cpu_addr2state (0 ); + +/* FIXME: don't inject into dummy CPU */ +cpu_inject_io (dummy_cpu, subchannel_id, subchannel_nr, io_int_parm, + io_int_word); +} + +static void qemu_s390_inject_crw_mchk (S390FLICState *fs) +{ +S390CPU *dummy_cpu =s390_cpu_addr2state (0 ); + +/* FIXME: don't inject into dummy CPU */ +cpu_inject_crw_mchk (dummy_cpu); +} +static void qemu_s390_flic_reset (DeviceState *dev) {QEMUS390FLICState *flic =QEMU_S390_FLIC (dev);hw/intc/s390_flic.c static void qemu_s390_flic_class_init(ObjectClass *oc, void *data) · line 206
Quoted 7 lines
hw/intc/s390_flic.c +3
-0@@ -172,6 +200,9 @@ static void qemu_s390_flic_class_init(ObjectClass *oc, void *data) fsc->clear_io_irq = qemu_s390_clear_io_flic; fsc->modify_ais_mode = qemu_s390_modify_ais_mode; fsc->inject_airq = qemu_s390_inject_airq; + fsc->inject_service = qemu_s390_inject_service; + fsc->inject_io = qemu_s390_inject_io; + fsc->inject_crw_mchk = qemu_s390_inject_crw_mchk; }As you now have a callback for ->inject_io(), make
qemu_s390_inject_airq()invoke it directly instead of going the detour throughs390_io_interrupt()?hw/intc/s390_flic.c static void qemu_s390_flic_class_init(ObjectClass *oc, void *data) · line 208
Quoted 2 lines
hw/intc/s390_flic.c +0
-0static Property s390_flic_common_properties[] = {
Quoted 8 lines
(...)
Generally looks sane.
One thing I noticed: You removed the caching of the flic (in the old kvm inject routine), and you generally do more qom invocations (first, to find the common flic; then, to translate to the qemu or kvm flic). Not sure if this might be a problem (probably not).
Is any of these calls on a potential fast path (e.g. guest without adapter interrupts)? If yes, then QOM is a no-go since it is really slow.
Re: [Qemu-devel] [PATCH v1 for-2-12 06/15] s390x/flic: factor out injection of floating interrupts
Quoted 9 lines · On Tue, 12 Dec 2017 15:13:46 +0100 Christian Borntraeger…
Quoted 4 lines · Cornelia Huck
One thing I noticed: You removed the caching of the flic (in the old kvm inject routine), and you generally do more qom invocations (first, to find the common flic; then, to translate to the qemu or kvm flic). Not sure if this might be a problem (probably not).
Is any of these calls on a potential fast path (e.g. guest without adapter interrupts)? If yes, then QOM is a no-go since it is really slow.
At least the new airq interface was using QOM without caching before.
It's basically about any interrupt; but otoh we are (for kvm) in userspace already. Caching the flic and just keeping the casting to the specialized flic might be ok (I'd guess that the lookup is the slowest path.)
Re: [Qemu-devel] [PATCH v1 for-2-12 06/15] s390x/flic: factor out injection of floating interrupts
Quoted 13 lines · Cornelia Huck
Quoted 9 lines · On Tue, 12 Dec 2017 15:13:46 +0100 Christian Borntraeger…
Quoted 4 lines · Cornelia Huck
One thing I noticed: You removed the caching of the flic (in the old kvm inject routine), and you generally do more qom invocations (first, to find the common flic; then, to translate to the qemu or kvm flic). Not sure if this might be a problem (probably not).
Is any of these calls on a potential fast path (e.g. guest without adapter interrupts)? If yes, then QOM is a no-go since it is really slow.
Quoted 7 lines
At least the new airq interface was using QOM without caching before.
It's basically about any interrupt; but otoh we are (for kvm) in userspace already. Caching the flic and just keeping the casting to the specialized flic might be ok (I'd guess that the lookup is the slowest path.)
Please note that the lookup is already cached in s390_get_flic(); That should be sufficient, as it does the expensive lookup. One cache should be enough, no?
The other conversions should be cheap (and already were in place in a couple of places before).
Thanks!
Signature · 3 lines
Thanks, David / dhildenb
Re: [Qemu-devel] [PATCH v1 for-2-12 06/15] s390x/flic: factor out injection of floating interrupts
Quoted 22 lines · On Tue, 12 Dec 2017 16:17:17 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 13 lines · Cornelia Huck
Quoted 9 lines · On Tue, 12 Dec 2017 15:13:46 +0100 Christian Borntraeger…
Quoted 4 lines · Cornelia Huck
One thing I noticed: You removed the caching of the flic (in the old kvm inject routine), and you generally do more qom invocations (first, to find the common flic; then, to translate to the qemu or kvm flic). Not sure if this might be a problem (probably not).
Is any of these calls on a potential fast path (e.g. guest without adapter interrupts)? If yes, then QOM is a no-go since it is really slow.
Quoted 7 lines
At least the new airq interface was using QOM without caching before.
It's basically about any interrupt; but otoh we are (for kvm) in userspace already. Caching the flic and just keeping the casting to the specialized flic might be ok (I'd guess that the lookup is the slowest path.)
Quoted 3 lines
Please note that the lookup is already cached in
s390_get_flic(); That should be sufficient, as it does the expensive lookup. One cache should be enough, no?
Ah, missed that. So the old code actually did double caching...
Quoted 3 lines
The other conversions should be cheap (and already were in place in a couple of places before).
Yes, object_resolve_path() is probably the most expensive one.
Did anyone ever check if the (existing) conversions are actually measurable?
Re: [Qemu-devel] [PATCH v1 for-2-12 06/15] s390x/flic: factor out injection of floating interrupts
Quoted 32 lines · Cornelia Huck
Quoted 22 lines · On Tue, 12 Dec 2017 16:17:17 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 13 lines · Cornelia Huck
Quoted 9 lines · On Tue, 12 Dec 2017 15:13:46 +0100 Christian Borntraeger…
Quoted 4 lines · Cornelia Huck
One thing I noticed: You removed the caching of the flic (in the old kvm inject routine), and you generally do more qom invocations (first, to find the common flic; then, to translate to the qemu or kvm flic). Not sure if this might be a problem (probably not).
Is any of these calls on a potential fast path (e.g. guest without adapter interrupts)? If yes, then QOM is a no-go since it is really slow.
Quoted 7 lines
At least the new airq interface was using QOM without caching before.
It's basically about any interrupt; but otoh we are (for kvm) in userspace already. Caching the flic and just keeping the casting to the specialized flic might be ok (I'd guess that the lookup is the slowest path.)
Quoted 3 lines
Please note that the lookup is already cached in
s390_get_flic(); That should be sufficient, as it does the expensive lookup. One cache should be enough, no?Ah, missed that. So the old code actually did double caching...
Quoted 1 line
Quoted 7 lines
Quoted 2 lines
The other conversions should be cheap (and already were in place in a couple of places before).
Yes,
object_resolve_path()is probably the most expensive one.Did anyone ever check if the (existing) conversions are actually measurable?
Did some quick measurement.
the initial object resolve takes about 20000ns, the cached ones then is 2-5ns. (measuring s390_get_flic function).
The other conversions (S390FLICStateClass *fsc = S390_FLIC_COMMON_GET_CLASS(fs);) takes about 15-25ns (up to 100 cycles).
For irqfd users this does not matter, but things like plan9fs might benefit if we speedup this lookup as well?
PS: We can improve the initial object_resolve_path by doing the resolve not for TYPE_KVM_S390_FLIC
but
"/machine/" TYPE_KVM_S390_FLIC
(2500ns instead of 20000)
but its still way too slow.
Re: [Qemu-devel] [PATCH v1 for-2-12 06/15] s390x/flic: factor out injection of floating interrupts
Quoted 48 lines · Christian Borntraeger
Quoted 32 lines · Cornelia Huck
Quoted 22 lines · On Tue, 12 Dec 2017 16:17:17 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 13 lines · Cornelia Huck
Quoted 9 lines · On Tue, 12 Dec 2017 15:13:46 +0100 Christian Borntraeger…
Quoted 4 lines · Cornelia Huck
One thing I noticed: You removed the caching of the flic (in the old kvm inject routine), and you generally do more qom invocations (first, to find the common flic; then, to translate to the qemu or kvm flic). Not sure if this might be a problem (probably not).
Is any of these calls on a potential fast path (e.g. guest without adapter interrupts)? If yes, then QOM is a no-go since it is really slow.
Quoted 7 lines
At least the new airq interface was using QOM without caching before.
It's basically about any interrupt; but otoh we are (for kvm) in userspace already. Caching the flic and just keeping the casting to the specialized flic might be ok (I'd guess that the lookup is the slowest path.)
Quoted 3 lines
Please note that the lookup is already cached in
s390_get_flic(); That should be sufficient, as it does the expensive lookup. One cache should be enough, no?Ah, missed that. So the old code actually did double caching...
Quoted 1 line
Quoted 7 lines
Quoted 2 lines
The other conversions should be cheap (and already were in place in a couple of places before).
Yes,
object_resolve_path()is probably the most expensive one.Did anyone ever check if the (existing) conversions are actually measurable?
Did some quick measurement.
the initial object resolve takes about 20000ns, the cached ones then is 2-5ns. (measurings390_get_flicfunction).
Quoted 4 lines
The other conversions (S390FLICStateClass *fsc =
S390_FLIC_COMMON_GET_CLASS(fs);) takes about 15-25ns (up to 100 cycles).
For irqfd users this does not matter, but things like plan9fs might benefit if we speedup this lookup as well?
Did you also measure the state conversion like QEMU_S390_FLIC() or KVM_S390_FLIC() ?
As we call e.g. QEMU_S390_FLIC() on a regular basis in TCG, it might then also make sense to speed that up.
a) cache the (converted) state and class in every function. Somewhat uglifies the code.
b) introduce new functions that return the cached value
Not sure what's better. I prefer to do this as a separate addon patch and can prepare something.
Quoted 9 lines
PS: We can improve the initial
object_resolve_pathby doing the resolve not forTYPE_KVM_S390_FLIC
but
"/machine/"TYPE_KVM_S390_FLIC
(2500ns instead of 20000)
but its still way too slow.
Specifying the absolute path would be even faster I guess.
/machine/s390-flic-qemu/ ...
Signature · 3 lines
Thanks, David / dhildenb
Re: [Qemu-devel] [PATCH v1 for-2-12 06/15] s390x/flic: factor out injection of floating interrupts
Quoted 49 lines · David Hildenbrand
Quoted 48 lines · Christian Borntraeger
Quoted 32 lines · Cornelia Huck
Quoted 22 lines · On Tue, 12 Dec 2017 16:17:17 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 13 lines · Cornelia Huck
Quoted 9 lines · On Tue, 12 Dec 2017 15:13:46 +0100 Christian Borntraeger…
Quoted 4 lines · Cornelia Huck
One thing I noticed: You removed the caching of the flic (in the old kvm inject routine), and you generally do more qom invocations (first, to find the common flic; then, to translate to the qemu or kvm flic). Not sure if this might be a problem (probably not).
Is any of these calls on a potential fast path (e.g. guest without adapter interrupts)? If yes, then QOM is a no-go since it is really slow.
Quoted 7 lines
At least the new airq interface was using QOM without caching before.
It's basically about any interrupt; but otoh we are (for kvm) in userspace already. Caching the flic and just keeping the casting to the specialized flic might be ok (I'd guess that the lookup is the slowest path.)
Quoted 3 lines
Please note that the lookup is already cached in
s390_get_flic(); That should be sufficient, as it does the expensive lookup. One cache should be enough, no?Ah, missed that. So the old code actually did double caching...
Quoted 1 line
Quoted 7 lines
Quoted 2 lines
The other conversions should be cheap (and already were in place in a couple of places before).
Yes,
object_resolve_path()is probably the most expensive one.Did anyone ever check if the (existing) conversions are actually measurable?
Did some quick measurement.
the initial object resolve takes about 20000ns, the cached ones then is 2-5ns. (measurings390_get_flicfunction).
Quoted 7 lines
Quoted 4 lines
The other conversions (S390FLICStateClass *fsc =
S390_FLIC_COMMON_GET_CLASS(fs);) takes about 15-25ns (up to 100 cycles).
For irqfd users this does not matter, but things like plan9fs might benefit if we speedup this lookup as well?Did you also measure the state conversion like
QEMU_S390_FLIC()orKVM_S390_FLIC()?
somewhat between 4 and 39 ns.
Re: [Qemu-devel] [PATCH v1 for-2-12 06/15] s390x/flic: factor out injection of floating interrupts
In reply to David HildenbrandQuoted 60 lines · On Wed, 13 Dec 2017 10:31:58 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 48 lines · Christian Borntraeger
Quoted 32 lines · Cornelia Huck
Quoted 22 lines · On Tue, 12 Dec 2017 16:17:17 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 13 lines · Cornelia Huck
Quoted 9 lines · On Tue, 12 Dec 2017 15:13:46 +0100 Christian Borntraeger…
Quoted 4 lines · Cornelia Huck
One thing I noticed: You removed the caching of the flic (in the old kvm inject routine), and you generally do more qom invocations (first, to find the common flic; then, to translate to the qemu or kvm flic). Not sure if this might be a problem (probably not).
Is any of these calls on a potential fast path (e.g. guest without adapter interrupts)? If yes, then QOM is a no-go since it is really slow.
Quoted 7 lines
At least the new airq interface was using QOM without caching before.
It's basically about any interrupt; but otoh we are (for kvm) in userspace already. Caching the flic and just keeping the casting to the specialized flic might be ok (I'd guess that the lookup is the slowest path.)
Quoted 3 lines
Please note that the lookup is already cached in
s390_get_flic(); That should be sufficient, as it does the expensive lookup. One cache should be enough, no?Ah, missed that. So the old code actually did double caching...
Quoted 1 line
Quoted 7 lines
Quoted 2 lines
The other conversions should be cheap (and already were in place in a couple of places before).
Yes,
object_resolve_path()is probably the most expensive one.Did anyone ever check if the (existing) conversions are actually measurable?
Did some quick measurement.
the initial object resolve takes about 20000ns, the cached ones then is 2-5ns. (measurings390_get_flicfunction).Quoted 4 lines
The other conversions (S390FLICStateClass *fsc =
S390_FLIC_COMMON_GET_CLASS(fs);) takes about 15-25ns (up to 100 cycles).
For irqfd users this does not matter, but things like plan9fs might benefit if we speedup this lookup as well?Did you also measure the state conversion like
QEMU_S390_FLIC()orKVM_S390_FLIC()?As we call e.g.
QEMU_S390_FLIC()on a regular basis in TCG, it might then also make sense to speed that up.
Quoted 7 lines
a) cache the (converted) state and class in every function. Somewhat uglifies the code.
b) introduce new functions that return the cached value
Not sure what's better. I prefer to do this as a separate addon patch and can prepare something.
If you want to do it as addon, I vote for option b).
Quoted 11 lines
Quoted 10 lines
PS: We can improve the initial
object_resolve_pathby doing the resolve not forTYPE_KVM_S390_FLIC
but
"/machine/"TYPE_KVM_S390_FLIC
(2500ns instead of 20000)
but its still way too slow.
Quoted 3 lines
Specifying the absolute path would be even faster I guess.
/machine/s390-flic-qemu/ ...
I don't think we really need to speed up the initial lookup.
Re: [Qemu-devel] [PATCH v1 for-2-12 06/15] s390x/flic: factor out injection of floating interrupts
In reply to Christian BorntraegerQuoted 41 lines · On Wed, 13 Dec 2017 10:16:34 +0100 Christian Borntraeger…
Quoted 32 lines · Cornelia Huck
Quoted 22 lines · On Tue, 12 Dec 2017 16:17:17 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 13 lines · Cornelia Huck
Quoted 9 lines · On Tue, 12 Dec 2017 15:13:46 +0100 Christian Borntraeger…
Quoted 4 lines · Cornelia Huck
One thing I noticed: You removed the caching of the flic (in the old kvm inject routine), and you generally do more qom invocations (first, to find the common flic; then, to translate to the qemu or kvm flic). Not sure if this might be a problem (probably not).
Is any of these calls on a potential fast path (e.g. guest without adapter interrupts)? If yes, then QOM is a no-go since it is really slow.
Quoted 7 lines
At least the new airq interface was using QOM without caching before.
It's basically about any interrupt; but otoh we are (for kvm) in userspace already. Caching the flic and just keeping the casting to the specialized flic might be ok (I'd guess that the lookup is the slowest path.)
Quoted 3 lines
Please note that the lookup is already cached in
s390_get_flic(); That should be sufficient, as it does the expensive lookup. One cache should be enough, no?Ah, missed that. So the old code actually did double caching...
Quoted 1 line
Quoted 7 lines
Quoted 2 lines
The other conversions should be cheap (and already were in place in a couple of places before).
Yes,
object_resolve_path()is probably the most expensive one.Did anyone ever check if the (existing) conversions are actually measurable?
Quoted 7 lines
Did some quick measurement.
the initial object resolve takes about 20000ns, the cached ones then is 2-5ns. (measurings390_get_flicfunction).The other conversions (S390FLICStateClass *fsc =
S390_FLIC_COMMON_GET_CLASS(fs);) takes about 15-25ns (up to 100 cycles).
That's probably the debug checks, the rest should not take many cycles.
Quoted 2 lines
For irqfd users this does not matter, but things like plan9fs might benefit if we speedup this lookup as well?
We can probably cache the fsc as well. There are still the casts from the common flic to the qemu/kvm flics; we can cache them in the individual callbacks. Looks a bit odd in the code though, I guess.
An alternative would be using "fast" casts that bypass QOM.
Quoted 9 lines
PS: We can improve the initial
object_resolve_pathby doing the resolve not forTYPE_KVM_S390_FLIC
but
"/machine/"TYPE_KVM_S390_FLIC
(2500ns instead of 20000)
but its still way too slow.
Yeah, caching looks like the saner approach in that case.
Re: [Qemu-devel] [PATCH v1 for-2-12 06/15] s390x/flic: factor out injection of floating interrupts
In reply to Cornelia HuckQuoted 68 lines · Cornelia Huck
Quoted 5 lines · On Mon, 11 Dec 2017 14:47:31 +0100 David Hildenbrand <[email protected]> wrote:
Let the flic device handle it internally. This will allow us to later on store floating interrupts in the flic for the TCG case.
This now also simplifies
kvm.c. All that's left is the fallback interface for floating interrupts, which is no triggered directly vias/no/now/
Quoted 57 lines
the flic in case anything goes wrong.
Signed-off-by David Hildenbrand <[email protected]>--- hw/intc/s390_flic.c | 31 ++++++++++++++++++++ hw/intc/s390_flic_kvm.c | 63 ++++++++++++++++++++++++++++++++++++---- include/hw/s390x/s390_flic.h | 5 ++++ target/s390x/cpu.h | 7 ++++- target/s390x/interrupt.c | 42 +++++++++++---------------- target/s390x/kvm-stub.c | 13 --------- target/s390x/kvm.c | 68 ++++---------------------------------------- target/s390x/kvm_s390x.h | 10 +------ 8 files changed, 123 insertions(+), 116 deletions(-)hw/intc/s390_flic.c +28
-0@@ -131,6 +131,34 @@ static int qemu_s390_inject_airq(S390FLICState *fs, uint8_t type,return 0 ; } +static void qemu_s390_inject_service (S390FLICState *fs,uint32_t parm) +{ + +S390CPU *dummy_cpu =s390_cpu_addr2state (0 ); + +/* FIXME: don't inject into dummy CPU */ +cpu_inject_service (dummy_cpu, parm); +} + +static void qemu_s390_inject_io (S390FLICState *fs,uint16_t subchannel_id, +uint16_t subchannel_nr,uint32_t io_int_parm, +uint32_t io_int_word) +{ +S390CPU *dummy_cpu =s390_cpu_addr2state (0 ); + +/* FIXME: don't inject into dummy CPU */ +cpu_inject_io (dummy_cpu, subchannel_id, subchannel_nr, io_int_parm, + io_int_word); +} + +static void qemu_s390_inject_crw_mchk (S390FLICState *fs) +{ +S390CPU *dummy_cpu =s390_cpu_addr2state (0 ); + +/* FIXME: don't inject into dummy CPU */ +cpu_inject_crw_mchk (dummy_cpu); +} +static void qemu_s390_flic_reset (DeviceState *dev) {QEMUS390FLICState *flic =QEMU_S390_FLIC (dev);@@ -172,6 +200,9 @@ static void qemu_s390_flic_class_init(ObjectClass *oc, void *data) fsc->clear_io_irq = qemu_s390_clear_io_flic; fsc->modify_ais_mode = qemu_s390_modify_ais_mode; fsc->inject_airq = qemu_s390_inject_airq;
Quoted 8 lines
hw/intc/s390_flic.c static void qemu_s390_flic_class_init(ObjectClass *oc, void *data) · line 206
Quoted 4 lines
hw/intc/s390_flic.c +3
-0+ fsc->inject_service = qemu_s390_inject_service; + fsc->inject_io = qemu_s390_inject_io; + fsc->inject_crw_mchk = qemu_s390_inject_crw_mchk; }As you now have a callback for ->inject_io(), make
qemu_s390_inject_airq()invoke it directly instead of going the detour throughs390_io_interrupt()?
Indeed, that makes sense.
Signature · 3 lines
Thanks, David / dhildenb
PATCH 7/15 s390x/tcg: tolerate wrong wakeups due to floating interrupts
In reply to David HildenbrandThis is a preparation for floating interrupt support and only applies to MTTCG, single threaded TCG works just fine. If a floating interrupt wakes up a VCPU and the CPU thinks it can run (clearing cs->halted), at the point where the interrupt would be delivered, already another VCPU might have picked up the interrupt, resulting in a wakeup without an interrupt (executing wrong code).
It is wrong to let the VCPU continue to execute (the WAIT PSW). Instead, we have to put the VCPU back to sleep.
--- target/s390x/excp_helper.c | 5 +++++ 1 file changed, 5 insertions(+)
target/s390x/excp_helper.c +5 -0
@@ -503,6 +503,11 @@ bool s390_cpu_exec_interrupt(CPUState *cs, int interrupt_request)s390_cpu_do_interrupt (cs);return true; } +if (env->psw.mask &PSW_MASK_WAIT ) { +/* Woken up because of a floating interrupt but it has already +* been delivered. Go back to sleep. */ +cpu_interrupt (CPU (cpu),CPU_INTERRUPT_HALT ); + } }return false; }
Signature · 1 line
2.14.3
PATCH 8/15 s390x/flic: make floating interrupts on TCG actually floating
In reply to David HildenbrandMove floating interrupt handling into the flic. Floating interrupts will now be considered by all CPUs, not just CPU #0. While at it, convert I/O interrupts to use a list and make sure we properly consider I/O sub-classes in s390_cpu_has_io_int().
--- hw/intc/s390_flic.c | 144 ++++++++++++++++++++++++++++++++++++++++--- include/hw/s390x/s390_flic.h | 41 ++++++++++++ target/s390x/cpu.c | 8 --- target/s390x/cpu.h | 22 ------- target/s390x/excp_helper.c | 97 ++++++++++------------------- target/s390x/interrupt.c | 52 ++-------------- 6 files changed, 212 insertions(+), 152 deletions(-)
hw/intc/s390_flic.c +134 -10
@@ -131,40 +131,153 @@ static int qemu_s390_inject_airq(S390FLICState *fs, uint8_t type,return 0 ; } +static void qemu_s390_flic_notify (uint32_t type) +{ +CPUState *cs; + +/* +* We have to make all CPUs see CPU_INTERRUPT_HARD, so they might +* consider it. TODO: don't kick/wakeup all VCPUs but try to be +* smarter (using the interrupt type). +*/ +CPU_FOREACH (cs) { +cpu_interrupt (cs,CPU_INTERRUPT_HARD ); + } +} + +uint32_t qemu_s390_flic_dequeue_service (QEMUS390FLICState *flic) +{ +uint32_t tmp; + +g_assert (qemu_mutex_iothread_locked ()); +g_assert (flic->pending &FLIC_PENDING_SERVICE ); + tmp = flic->service_param; + flic->service_param =0 ; + flic->pending &= ~FLIC_PENDING_SERVICE ; + +return tmp; +} + +/* caller has to free the returned object */ +QEMUS390FlicIO *qemu_s390_flic_dequeue_io (QEMUS390FLICState *flic,uint64_t cr6) +{ +QEMUS390FlicIO *io; +uint8_t isc; + +g_assert (qemu_mutex_iothread_locked ()); +if (!(flic->pending &CR6_TO_PENDING_IO (cr6))) { +return NULL ; + } + +for (isc =0 ; isc <8 ; isc++) { +if (QLIST_EMPTY (&flic->io[isc]) || !(cr6 &ISC_TO_ISC_BITS (isc))) { +continue ; + } + io =QLIST_FIRST (&flic->io[isc]); +QLIST_REMOVE (io, next); + +/* update our indicator bit */ +if (QLIST_EMPTY (&flic->io[isc])) { + flic->pending &= ~ISC_TO_PENDING_IO (isc); + } +return io; + } + +return NULL ; +} + +void qemu_s390_flic_dequeue_crw_mchk (QEMUS390FLICState *flic) +{ +g_assert (qemu_mutex_iothread_locked ()); +g_assert (flic->pending &FLIC_PENDING_MCHK_CR ); + flic->pending &= ~FLIC_PENDING_MCHK_CR ; +} +static void qemu_s390_inject_service (S390FLICState *fs,uint32_t parm) { +QEMUS390FLICState *flic =QEMU_S390_FLIC (fs); -S390CPU *dummy_cpu =s390_cpu_addr2state (0 ); +g_assert (qemu_mutex_iothread_locked ()); +/* multiplexing is good enough for sclp - kvm does it internally as well */ + flic->service_param |= parm; + flic->pending |=FLIC_PENDING_SERVICE ; -/* FIXME: don't inject into dummy CPU */ -cpu_inject_service (dummy_cpu, parm); +qemu_s390_flic_notify (FLIC_PENDING_SERVICE ); }static void qemu_s390_inject_io (S390FLICState *fs,uint16_t subchannel_id,uint16_t subchannel_nr,uint32_t io_int_parm,uint32_t io_int_word) { -S390CPU *dummy_cpu =s390_cpu_addr2state (0 ); +const uint8_t isc =IO_INT_WORD_ISC (io_int_word); +QEMUS390FLICState *flic =QEMU_S390_FLIC (fs); +QEMUS390FlicIO *io; -/* FIXME: don't inject into dummy CPU */ -cpu_inject_io (dummy_cpu, subchannel_id, subchannel_nr, io_int_parm, - io_int_word); +g_assert (qemu_mutex_iothread_locked ()); + io =g_new0 (QEMUS390FlicIO,1 ); + io->id = subchannel_id; + io->nr = subchannel_nr; + io->parm = io_int_parm; + io->word = io_int_word; + +QLIST_INSERT_HEAD (&flic->io[isc], io, next); + flic->pending |=ISC_TO_PENDING_IO (isc); + +qemu_s390_flic_notify (ISC_TO_PENDING_IO (isc)); }static void qemu_s390_inject_crw_mchk (S390FLICState *fs) { -S390CPU *dummy_cpu =s390_cpu_addr2state (0 ); +QEMUS390FLICState *flic =QEMU_S390_FLIC (fs); + +g_assert (qemu_mutex_iothread_locked ()); + flic->pending |=FLIC_PENDING_MCHK_CR ; + +qemu_s390_flic_notify (FLIC_PENDING_MCHK_CR ); +} + +bool qemu_s390_flic_has_service (QEMUS390FLICState *flic) +{ +/* called without lock via cc->has_work, will be validated under lock */ +return !!(flic->pending &FLIC_PENDING_SERVICE ); +} + +bool qemu_s390_flic_has_io (QEMUS390FLICState *flic,uint64_t cr6) +{ +/* called without lock via cc->has_work, will be validated under lock */ +return !!(flic->pending &CR6_TO_PENDING_IO (cr6)); +} + +bool qemu_s390_flic_has_crw_mchk (QEMUS390FLICState *flic) +{ +/* called without lock via cc->has_work, will be validated under lock */ +return !!(flic->pending &FLIC_PENDING_MCHK_CR ); +} -/* FIXME: don't inject into dummy CPU */ -cpu_inject_crw_mchk (dummy_cpu); +bool qemu_s390_flic_has_any (QEMUS390FLICState *flic) +{ +g_assert (qemu_mutex_iothread_locked ()); +return !!flic->pending; }static void qemu_s390_flic_reset (DeviceState *dev) {QEMUS390FLICState *flic =QEMU_S390_FLIC (dev); +QEMUS390FlicIO *cur, *next; +int isc; +g_assert (qemu_mutex_iothread_locked ()); flic->simm =0 ; flic->nimm =0 ; + flic->pending =0 ; + +/* remove all pending io interrupts */ +for (isc =0 ; isc <8 ; isc++) { + QLIST_FOREACH_SAFE(cur, &flic->io[isc], next, next) { +QLIST_REMOVE (cur, next); +g_free (cur); + } + } }bool ais_needed (void *opaque)
@@ -186,6 +299,16 @@ static const VMStateDescription qemu_s390_flic_vmstate = { } }; +static void qemu_s390_flic_instance_init (Object *obj) +{ +QEMUS390FLICState *flic =QEMU_S390_FLIC (obj); +int isc; + +for (isc =0 ; isc <8 ; isc++) { +QLIST_INIT (&flic->io[isc]); + } +} +static void qemu_s390_flic_class_init (ObjectClass *oc,void *data) {DeviceClass *dc =DEVICE_CLASS (oc);
@@ -237,6 +360,7 @@ static const TypeInfo qemu_s390_flic_info = { .name = TYPE_QEMU_S390_FLIC, .parent = TYPE_S390_FLIC_COMMON, .instance_size = sizeof(QEMUS390FLICState), + .instance_init = qemu_s390_flic_instance_init, .class_init = qemu_s390_flic_class_init, };
include/hw/s390x/s390_flic.h +41 -0
59 lines not on this page · raw message
target/s390x/cpu.c +0 -8
@@ -99,7 +99,6 @@ static void s390_cpu_initial_reset(CPUState *s) {S390CPU *cpu =S390_CPU (s);CPUS390XState *env = &cpu->env; -int i;s390_cpu_reset (s);/* initial reset does not clear everything! */
@@ -115,9 +114,6 @@ static void s390_cpu_initial_reset(CPUState *s) env->gbea =1 ; env->pfault_token =-1UL ; -for (i =0 ; i <ARRAY_SIZE (env->io_index); i++) { - env->io_index[i] =-1 ; - }/* tininess for underflow is detected before rounding */ set_float_detect_tininess(float_tininess_before_rounding,
@@ -135,7 +131,6 @@ static void s390_cpu_full_reset(CPUState *s)S390CPU *cpu =S390_CPU (s);S390CPUClass *scc =S390_CPU_GET_CLASS (cpu);CPUS390XState *env = &cpu->env; -int i; scc->parent_reset (s); cpu->env.sigp_order =0 ;
@@ -151,9 +146,6 @@ static void s390_cpu_full_reset(CPUState *s) env->gbea =1 ; env->pfault_token =-1UL ; -for (i =0 ; i <ARRAY_SIZE (env->io_index); i++) { - env->io_index[i] =-1 ; - }/* tininess for underflow is detected before rounding */ set_float_detect_tininess(float_tininess_before_rounding,
target/s390x/cpu.h +0 -22
@@ -53,8 +53,6 @@#define MMU_USER_IDX 0 -#define MAX_IO_QUEUE 16 -#define S390_MAX_CPUS 248 typedef struct PSW {
@@ -62,13 +60,6 @@ typedef struct PSW {uint64_t addr; }PSW ; -typedef struct IOIntQueue { -uint16_t id; -uint16_t nr; -uint32_t parm; -uint32_t word; -}IOIntQueue ; -struct CPUS390XState {uint64_t regs[16 ];/* GP registers */ /*
@@ -113,13 +104,9 @@ struct CPUS390XState {uint64_t cregs[16 ];/* control registers */ -IOIntQueue io_queue[MAX_IO_QUEUE ][8 ]; -int pending_int; -uint32_t service_param;uint16_t external_call_addr;DECLARE_BITMAP (emergency_signals,S390_MAX_CPUS ); -int io_index[8 ];uint64_t ckc;uint64_t cputm;
@@ -398,9 +385,6 @@ static inline void cpu_get_tb_cpu_state(CPUS390XState* env, target_ulong *pc,#define EXCP_IO 7/* I/O interrupt */ #define EXCP_MCHK 8/* machine check */ -#define INTERRUPT_IO (1 << 0) -#define INTERRUPT_MCHK (1 << 1) -#define INTERRUPT_EXT_SERVICE (1 << 2)#define INTERRUPT_EXT_CPU_TIMER (1 << 3)#define INTERRUPT_EXT_CLOCK_COMPARATOR (1 << 4)#define INTERRUPT_EXTERNAL_CALL (1 << 5)
@@ -740,12 +724,6 @@ void s390_program_interrupt(CPUS390XState *env, uint32_t code, int ilen, uintptr_t ra);/* service interrupts are floating therefore we must not pass an cpustate */ void s390_sclp_extint (uint32_t parm); -/* FIXME: remove once we have proper floating interrupts in TCG */ -void cpu_inject_service (S390CPU *cpu,uint32_t param); -void cpu_inject_crw_mchk (S390CPU *cpu); -void cpu_inject_io (S390CPU *cpu,uint16_t subchannel_id, -uint16_t subchannel_number,uint32_t io_int_parm, -uint32_t io_int_word);/* mmu_helper.c */ int s390_cpu_virt_mem_rw(S390CPU *cpu, vaddr laddr, uint8_t ar, void *hostbuf,
target/s390x/excp_helper.c +31 -66
152 lines not on this page · raw message
target/s390x/interrupt.c +6 -46
100 lines not on this page · raw message
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v1 for-2-12 08/15] s390x/flic: make floating interrupts on TCG actually floating
Quoted 29 lines · On Mon, 11 Dec 2017 14:47:33 +0100 David Hildenbrand <[email protected]> wrote:
Move floating interrupt handling into the flic. Floating interrupts will now be considered by all CPUs, not just CPU #0. While at it, convert I/O interrupts to use a list and make sure we properly consider I/O sub-classes in
s390_cpu_has_io_int().Signed-off-by David Hildenbrand <[email protected]>--- hw/intc/s390_flic.c | 144 ++++++++++++++++++++++++++++++++++++++++--- include/hw/s390x/s390_flic.h | 41 ++++++++++++ target/s390x/cpu.c | 8 --- target/s390x/cpu.h | 22 ------- target/s390x/excp_helper.c | 97 ++++++++++------------------- target/s390x/interrupt.c | 52 ++-------------- 6 files changed, 212 insertions(+), 152 deletions(-)include/hw/s390x/s390_flic.h +1
-0@@ -16,6 +16,7 @@#include "hw/sysbus.h" #include "hw/s390x/adapter.h" #include "hw/virtio/virtio.h" +#include "qemu/queue.h" /* * Reserve enough gsis to accommodate all virtio devices.
include/hw/s390x/s390_flic.h typedef struct S390FLICStateClass { · line 92
Quoted 7 lines
include/hw/s390x/s390_flic.h +4
-0@@ -85,12 +86,52 @@ typedef struct S390FLICStateClass {#define SIC_IRQ_MODE_SINGLE 1#define AIS_MODE_MASK (isc) (0x80 >> isc) +#define ISC_TO_PENDING_IO (_isc) (0x80 >> (_isc)) +#define CR6_TO_PENDING_IO (_cr6) (((_cr6) >> 24) & 0xff) + +/* the ISC bits are organized in a way that above makros work */
/* organize the ISC bits so that the macros above work */
?
Quoted 15 lines
include/hw/s390x/s390_flic.h +15
-0+#define FLIC_PENDING_IO_ISC7 (1 << 0) +#define FLIC_PENDING_IO_ISC6 (1 << 1) +#define FLIC_PENDING_IO_ISC5 (1 << 2) +#define FLIC_PENDING_IO_ISC4 (1 << 3) +#define FLIC_PENDING_IO_ISC3 (1 << 4) +#define FLIC_PENDING_IO_ISC2 (1 << 5) +#define FLIC_PENDING_IO_ISC1 (1 << 6) +#define FLIC_PENDING_IO_ISC0 (1 << 7) +#define FLIC_PENDING_SERVICE (1 << 8) +#define FLIC_PENDING_MCHK_CR (1 << 9) + +#define FLIC_PENDING_IO (FLIC_PENDING_IO_ISC0 | FLIC_PENDING_IO_ISC1 | \ + FLIC_PENDING_IO_ISC2 | FLIC_PENDING_IO_ISC3 | \ + FLIC_PENDING_IO_ISC4 | FLIC_PENDING_IO_ISC5 | \ + FLIC_PENDING_IO_ISC6 | FLIC_PENDING_IO_ISC7)
PATCH 9/15 s390x/tcg: implement TEST PENDING INTERRUPTION
In reply to David HildenbrandUse s390_cpu_virt_mem_write() so we can actually revert what we did (re-inject the dequeued IO interrupt).
--- target/s390x/helper.h | 1 + target/s390x/insn-data.def | 1 + target/s390x/misc_helper.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++ target/s390x/translate.c | 8 +++++++ 4 files changed, 63 insertions(+)
target/s390x/helper.h +1 -0
@@ -170,6 +170,7 @@ DEF_HELPER_4(schm, void, env, i64, i64, i64) DEF_HELPER_3(ssch, void, env, i64, i64) DEF_HELPER_2(stcrw, void, env, i64) DEF_HELPER_3(stsch, void, env, i64, i64) +DEF_HELPER_2(tpi, i32, env, i64) DEF_HELPER_3(tsch, void, env, i64, i64) DEF_HELPER_2(chsc, void, env, i64)#endif
target/s390x/insn-data.def +1 -0
@@ -1063,6 +1063,7 @@ C(0xb233, SSCH, S, Z, 0, insn, 0, 0, ssch, 0) C(0xb239, STCRW, S, Z, 0, insn, 0, 0, stcrw, 0) C(0xb234, STSCH, S, Z, 0, insn, 0, 0, stsch, 0) + C(0xb236, TPI , S, Z, la2, 0, 0, 0, tpi, 0) C(0xb235, TSCH, S, Z, 0, insn, 0, 0, tsch, 0) /* ??? Not listed in PoO ninth edition, but there's a linux driver that uses it: "A CHSC subchannel is usually present on LPAR only." */
target/s390x/misc_helper.c +53 -0
59 lines not on this page · raw message
target/s390x/translate.c +8 -0
@@ -4201,6 +4201,14 @@ static ExitStatus op_stcrw(DisasContext *s, DisasOps *o)return NO_EXIT ; } +static ExitStatus op_tpi (DisasContext *s,DisasOps *o) +{ +check_privileged (s); +gen_helper_tpi (cc_op, cpu_env, o->addr1); +set_cc_static (s); +return NO_EXIT ; +} +static ExitStatus op_tsch (DisasContext *s,DisasOps *o) {check_privileged (s);
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v1 for-2-12 09/15] s390x/tcg: implement TEST PENDING INTERRUPTION
Quoted 31 lines · On Mon, 11 Dec 2017 14:47:34 +0100 David Hildenbrand <[email protected]> wrote:
Use
s390_cpu_virt_mem_write()so we can actually revert what we did (re-inject the dequeued IO interrupt).Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/helper.h | 1 + target/s390x/insn-data.def | 1 + target/s390x/misc_helper.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++ target/s390x/translate.c | 8 +++++++ 4 files changed, 63 insertions(+)target/s390x/misc_helper.c +19
-0+uint32_t HELPER (tpi )(CPUS390XState *env,uint64_t addr) +{ +const uintptr_t ra =GETPC (); +S390CPU *cpu =s390_env_get_cpu (env); +QEMUS390FLICState *flic =QEMU_S390_FLIC (s390_get_flic ()); +QEMUS390FlicIO *io =NULL ; +LowCore *lowcore; + +if (addr &0x3 ) { +s390_program_interrupt (env,PGM_SPECIFICATION ,4 , ra); + } + +qemu_mutex_lock_iothread (); + io =qemu_s390_flic_dequeue_io (flic, env->cregs[6 ]); +if (!io) { +qemu_mutex_unlock_iothread (); +return 0 ; + } +
target/s390x/misc_helper.c void HELPER(stsch)(CPUS390XState *env, uint64_t r1, uint64_t inst) · line 460
Quoted 10 lines
target/s390x/misc_helper.c +10
-0+if (addr) { +struct { +uint16_t id; +uint16_t nr; +uint32_t parm; + } tmp = { + .id =cpu_to_be16 (io->id), + .nr =cpu_to_be16 (io->nr), + .parm =cpu_to_be32 (io->parm), + };
That's a two-word interruption code; can you call this something better than 'tmp'?
Quoted 27 lines
target/s390x/misc_helper.c +24
-0+ +if (s390_cpu_virt_mem_write (cpu, addr,0 , &tmp,sizeof (tmp))) { +/* writing failed, reinject and properly clean up */ +s390_io_interrupt (io->id, io->nr, io->parm, io->word); +qemu_mutex_unlock_iothread (); +g_free (io); +s390_cpu_virt_mem_handle_exc (cpu, ra); +return 0 ; + } + }else { +/* no protection applies */ + lowcore =cpu_map_lowcore (env); + lowcore->subchannel_id =cpu_to_be16 (io->id); + lowcore->subchannel_nr =cpu_to_be16 (io->nr); + lowcore->io_int_parm =cpu_to_be32 (io->parm); + lowcore->io_int_word =cpu_to_be32 (io->word); +cpu_unmap_lowcore (lowcore); + } + +g_free (io); +qemu_mutex_unlock_iothread (); +return 1 ; +} +void HELPER (tsch )(CPUS390XState *env,uint64_t r1,uint64_t inst) {S390CPU *cpu =s390_env_get_cpu (env);
Re: [Qemu-devel] [PATCH v1 for-2-12 09/15] s390x/tcg: implement TEST PENDING INTERRUPTION
Quoted 39 lines · Cornelia Huck
Quoted 36 lines · On Mon, 11 Dec 2017 14:47:34 +0100 David Hildenbrand <[email protected]> wrote:
Use
s390_cpu_virt_mem_write()so we can actually revert what we did (re-inject the dequeued IO interrupt).Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/helper.h | 1 + target/s390x/insn-data.def | 1 + target/s390x/misc_helper.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++ target/s390x/translate.c | 8 +++++++ 4 files changed, 63 insertions(+)target/s390x/misc_helper.c +24
-0+uint32_t HELPER (tpi )(CPUS390XState *env,uint64_t addr) +{ +const uintptr_t ra =GETPC (); +S390CPU *cpu =s390_env_get_cpu (env); +QEMUS390FLICState *flic =QEMU_S390_FLIC (s390_get_flic ()); +QEMUS390FlicIO *io =NULL ; +LowCore *lowcore; + +if (addr &0x3 ) { +s390_program_interrupt (env,PGM_SPECIFICATION ,4 , ra); + } + +qemu_mutex_lock_iothread (); + io =qemu_s390_flic_dequeue_io (flic, env->cregs[6 ]); +if (!io) { +qemu_mutex_unlock_iothread (); +return 0 ; + } + +if (addr) { +struct { +uint16_t id; +uint16_t nr; +uint32_t parm;
Quoted 8 lines
target/s390x/misc_helper.c void HELPER(stsch)(CPUS390XState *env, uint64_t r1, uint64_t inst) · line 460
Quoted 5 lines
target/s390x/misc_helper.c +5
-0+ } tmp = { + .id = cpu_to_be16(io->id), + .nr = cpu_to_be16(io->nr), + .parm = cpu_to_be32(io->parm), + };That's a two-word interruption code; can you call this something better than 'tmp'?
IMHO from the context we have here it should be pretty clear what is happening. I mean we are defining and initializing the temporary data structure.
But I can change the name if you can come up with a catchy variable name. :)
Thanks!
Quoted 28 lines
Quoted 28 lines
target/s390x/misc_helper.c +24
-0+ +if (s390_cpu_virt_mem_write (cpu, addr,0 , &tmp,sizeof (tmp))) { +/* writing failed, reinject and properly clean up */ +s390_io_interrupt (io->id, io->nr, io->parm, io->word); +qemu_mutex_unlock_iothread (); +g_free (io); +s390_cpu_virt_mem_handle_exc (cpu, ra); +return 0 ; + } + }else { +/* no protection applies */ + lowcore =cpu_map_lowcore (env); + lowcore->subchannel_id =cpu_to_be16 (io->id); + lowcore->subchannel_nr =cpu_to_be16 (io->nr); + lowcore->io_int_parm =cpu_to_be32 (io->parm); + lowcore->io_int_word =cpu_to_be32 (io->word); +cpu_unmap_lowcore (lowcore); + } + +g_free (io); +qemu_mutex_unlock_iothread (); +return 1 ; +} +void HELPER (tsch )(CPUS390XState *env,uint64_t r1,uint64_t inst) {S390CPU *cpu =s390_env_get_cpu (env);
Signature · 3 lines
Thanks, David / dhildenb
Re: [Qemu-devel] [PATCH v1 for-2-12 09/15] s390x/tcg: implement TEST PENDING INTERRUPTION
Quoted 49 lines · On Mon, 11 Dec 2017 21:32:58 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 39 lines · Cornelia Huck
Quoted 36 lines · On Mon, 11 Dec 2017 14:47:34 +0100 David Hildenbrand <[email protected]> wrote:
Use
s390_cpu_virt_mem_write()so we can actually revert what we did (re-inject the dequeued IO interrupt).Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/helper.h | 1 + target/s390x/insn-data.def | 1 + target/s390x/misc_helper.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++ target/s390x/translate.c | 8 +++++++ 4 files changed, 63 insertions(+)target/s390x/misc_helper.c +24
-0+uint32_t HELPER (tpi )(CPUS390XState *env,uint64_t addr) +{ +const uintptr_t ra =GETPC (); +S390CPU *cpu =s390_env_get_cpu (env); +QEMUS390FLICState *flic =QEMU_S390_FLIC (s390_get_flic ()); +QEMUS390FlicIO *io =NULL ; +LowCore *lowcore; + +if (addr &0x3 ) { +s390_program_interrupt (env,PGM_SPECIFICATION ,4 , ra); + } + +qemu_mutex_lock_iothread (); + io =qemu_s390_flic_dequeue_io (flic, env->cregs[6 ]); +if (!io) { +qemu_mutex_unlock_iothread (); +return 0 ; + } + +if (addr) { +struct { +uint16_t id; +uint16_t nr; +uint32_t parm;Quoted 8 lines
target/s390x/misc_helper.c void HELPER(stsch)(CPUS390XState *env, uint64_t r1, uint64_t inst) · line 460
Quoted 5 lines
target/s390x/misc_helper.c +5
-0+ } tmp = { + .id = cpu_to_be16(io->id), + .nr = cpu_to_be16(io->nr), + .parm = cpu_to_be32(io->parm), + };That's a two-word interruption code; can you call this something better than 'tmp'?
Quoted 6 lines
IMHO from the context we have here it should be pretty clear what is happening. I mean we are defining and initializing the temporary data structure.
But I can change the name if you can come up with a catchy variable name. :)
int_code?
Re: [Qemu-devel] [PATCH v1 for-2-12 09/15] s390x/tcg: implement TEST PENDING INTERRUPTION
Quoted 52 lines · Cornelia Huck
Quoted 49 lines · On Mon, 11 Dec 2017 21:32:58 +0100 David Hildenbrand <[email protected]> wrote:
Quoted 47 lines · Cornelia Huck
Quoted 31 lines · On Mon, 11 Dec 2017 14:47:34 +0100 David Hildenbrand <[email protected]> wrote:
Use
s390_cpu_virt_mem_write()so we can actually revert what we did (re-inject the dequeued IO interrupt).Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/helper.h | 1 + target/s390x/insn-data.def | 1 + target/s390x/misc_helper.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++ target/s390x/translate.c | 8 +++++++ 4 files changed, 63 insertions(+)target/s390x/misc_helper.c +19
-0+uint32_t HELPER (tpi )(CPUS390XState *env,uint64_t addr) +{ +const uintptr_t ra =GETPC (); +S390CPU *cpu =s390_env_get_cpu (env); +QEMUS390FLICState *flic =QEMU_S390_FLIC (s390_get_flic ()); +QEMUS390FlicIO *io =NULL ; +LowCore *lowcore; + +if (addr &0x3 ) { +s390_program_interrupt (env,PGM_SPECIFICATION ,4 , ra); + } + +qemu_mutex_lock_iothread (); + io =qemu_s390_flic_dequeue_io (flic, env->cregs[6 ]); +if (!io) { +qemu_mutex_unlock_iothread (); +return 0 ; + } +target/s390x/misc_helper.c void HELPER(stsch)(CPUS390XState *env, uint64_t r1, uint64_t inst) · line 460
Quoted 10 lines
target/s390x/misc_helper.c +10
-0+if (addr) { +struct { +uint16_t id; +uint16_t nr; +uint32_t parm; + } tmp = { + .id =cpu_to_be16 (io->id), + .nr =cpu_to_be16 (io->nr), + .parm =cpu_to_be32 (io->parm), + };That's a two-word interruption code; can you call this something better than 'tmp'?
Quoted 9 lines
Quoted 6 lines
IMHO from the context we have here it should be pretty clear what is happening. I mean we are defining and initializing the temporary data structure.
But I can change the name if you can come up with a catchy variable name. :)
int_code?
makes the line exceed 80c. I'll use intc. Thanks!
Signature · 3 lines
Thanks, David / dhildenb
PATCH 10/15 s390x/flic: implement qemu_s390_clear_io_flic()
In reply to David HildenbrandNow that we have access to the io interrupts, we can implement clear_io_irq() for TCG.
--- hw/intc/s390_flic.c | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-)
hw/intc/s390_flic.c +29 -2
@@ -83,8 +83,35 @@ static void qemu_s390_release_adapter_routes(S390FLICState *fs,static int qemu_s390_clear_io_flic (S390FLICState *fs,uint16_t subchannel_id,uint16_t subchannel_nr) { -/* Fixme TCG */ -return -ENOSYS ; +QEMUS390FLICState *flic =QEMU_S390_FLIC (fs); +QEMUS390FlicIO *cur, *next; +uint8_t isc; + +g_assert (qemu_mutex_iothread_locked ()); +if (!(flic->pending &FLIC_PENDING_IO )) { +return 0 ; + } + +/* check all iscs */ +for (isc =0 ; isc <8 ; isc++) { +if (QLIST_EMPTY (&flic->io[isc])) { +continue ; + } + +/* search and delete any matching one */ + QLIST_FOREACH_SAFE(cur, &flic->io[isc], next, next) { +if (cur->id == subchannel_id && cur->nr == subchannel_nr) { +QLIST_REMOVE (cur, next); +g_free (cur); + } + } + +/* update our indicator bit */ +if (QLIST_EMPTY (&flic->io[isc])) { + flic->pending &= ~ISC_TO_PENDING_IO (isc); + } + } +return 0 ; } static int qemu_s390_modify_ais_mode(S390FLICState *fs, uint8_t isc,
Signature · 1 line
2.14.3
PATCH 11/15 s390x/flic: optimize CPU wakeup for TCG
In reply to David HildenbrandKicking all CPUs on every floating interrupt is far from efficient. Let's optimize it at least a little bit.
--- hw/intc/s390_flic.c | 31 +++++++++++++++++++++++++++++-- target/s390x/cpu.h | 4 ++++ target/s390x/internal.h | 5 ----- 3 files changed, 33 insertions(+), 7 deletions(-)
hw/intc/s390_flic.c +29 -2
@@ -164,10 +164,37 @@ static void qemu_s390_flic_notify(uint32_t type)/* * We have to make all CPUs see CPU_INTERRUPT_HARD, so they might -* consider it. TODO: don't kick/wakeup all VCPUs but try to be -* smarter (using the interrupt type). +* consider it. We will kick all running CPUs and only relevant +* sleeping ones. */ CPU_FOREACH (cs) { +S390CPU *cpu =S390_CPU (cs); + + cs->interrupt_request |=CPU_INTERRUPT_HARD ; + +/* ignore CPUs that are not sleeping */ +if (s390_cpu_get_state (cpu) !=CPU_STATE_OPERATING && +s390_cpu_get_state (cpu) !=CPU_STATE_LOAD ) { +continue ; + } + +/* we always kick running CPUs for now, this is tricky */ +if (cs->halted) { +/* don't check for subclasses, CPUs double check when waking up */ +if (type &FLIC_PENDING_SERVICE ) { +if (!(cpu->env.psw.mask &PSW_MASK_EXT )) { +continue ; + } + }else if (type &FLIC_PENDING_IO ) { +if (!(cpu->env.psw.mask &PSW_MASK_IO )) { +continue ; + } + }else if (type &FLIC_PENDING_MCHK_CR ) { +if (!(cpu->env.psw.mask &PSW_MASK_MCHECK )) { +continue ; + } + } + }cpu_interrupt (cs,CPU_INTERRUPT_HARD ); } }
target/s390x/cpu.h +4 -0
@@ -691,6 +691,10 @@ static inline unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu) return 0; }#endif /* CONFIG_USER_ONLY */ +static inline uint8_t s390_cpu_get_state (S390CPU *cpu) +{ +return cpu->env.cpu_state; +}/* cpu_models.c */
target/s390x/internal.h +0 -5
@@ -278,11 +278,6 @@ static inline void s390_do_cpu_full_reset(CPUState *cs, run_on_cpu_data arg)cpu_reset (cs); } -static inline uint8_t s390_cpu_get_state (S390CPU *cpu) -{ -return cpu->env.cpu_state; -} -/* arch_dump.c */ int s390_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
Signature · 1 line
2.14.3
PATCH 12/15 s390x/tcg: fix size + content of STSI blocks
In reply to David HildenbrandAll blocks are 4k in size, which is only true for two of them right now. Also some reserved fields were wrong, fix it and convert all reserved fields to u8.
This also fixes the LPAR part output in /proc/sysinfo under TCG. (for now, everything was indicated as 0)
--- target/s390x/cpu.h | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)
target/s390x/cpu.h +14 -8
@@ -437,25 +437,27 @@ static inline void setcc(S390CPU *cpu, uint64_t cc)/* Basic Machine Configuration */ struct sysib_111 { -uint32_t res1[8 ]; +uint8_t res1[32 ];uint8_t manuf[16 ];uint8_t type[4 ];uint8_t res2[12 ];uint8_t model[16 ];uint8_t sequence[16 ];uint8_t plant[4 ]; -uint8_t res3[156 ]; +uint8_t res3[3996 ]; }; +QEMU_BUILD_BUG_ON (sizeof (struct sysib_111 ) !=4096 );/* Basic Machine CPU */ struct sysib_121 { -uint32_t res1[80 ]; +uint8_t res1[80 ];uint8_t sequence[16 ];uint8_t plant[4 ];uint8_t res2[2 ];uint16_t cpu_addr; -uint8_t res3[152 ]; +uint8_t res3[3992 ]; }; +QEMU_BUILD_BUG_ON (sizeof (struct sysib_121 ) !=4096 );/* Basic Machine CPUs */ struct sysib_122 {
@@ -467,20 +469,22 @@ struct sysib_122 {uint16_t reserved_cpus;uint16_t adjustments[2026 ]; }; +QEMU_BUILD_BUG_ON (sizeof (struct sysib_122 ) !=4096 );/* LPAR CPU */ struct sysib_221 { -uint32_t res1[80 ]; +uint8_t res1[80 ];uint8_t sequence[16 ];uint8_t plant[4 ];uint16_t cpu_id;uint16_t cpu_addr; -uint8_t res3[152 ]; +uint8_t res3[3992 ]; }; +QEMU_BUILD_BUG_ON (sizeof (struct sysib_221 ) !=4096 );/* LPAR CPUs */ struct sysib_222 { -uint32_t res1[32 ]; +uint8_t res1[32 ];uint16_t lpar_num;uint8_t res2;uint8_t lcpuc;
@@ -493,8 +497,9 @@ struct sysib_222 {uint8_t res3[16 ];uint16_t dedicated_cpus;uint16_t shared_cpus; -uint8_t res4[180 ]; +uint8_t res4[4020 ]; }; +QEMU_BUILD_BUG_ON (sizeof (struct sysib_222 ) !=4096 );/* VM CPUs */ struct sysib_322 {
@@ -517,6 +522,7 @@ struct sysib_322 {uint8_t res4[1504 ];uint8_t ext_names[8 ][256 ]; }; +QEMU_BUILD_BUG_ON (sizeof (struct sysib_322 ) !=4096 );/* MMU defines */ #define _ASCE_ORIGIN ~0xfffULL/* segment table origin */
Signature · 1 line
2.14.3
Re: [Qemu-devel] [qemu-s390x] [PATCH v1 for-2-12 12/15] s390x/tcg: fix size + content of STSI blocks
Quoted 11 lines · David Hildenbrand
All blocks are 4k in size, which is only true for two of them right now. Also some reserved fields were wrong, fix it and convert all reserved fields to u8.
This also fixes the LPAR part output in
/proc/sysinfounder TCG. (for now, everything was indicated as 0)Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/cpu.h | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)
PATCH 13/15 s390x/tcg: STSI overhaul
In reply to David HildenbrandCurrent STSI implementation is a mess, so let's rewrite it.
Problems fixed by this patch:
1) The order of exceptions/when recognized is wrong.
2) We have to store to virtual address space, not absolute.
3) Alignment check of the block is missing.
3) The SMP information is not indicated.
While at it:
a) Make the code look nicer
- get rid of nesting levels
- use struct initialization instead of initializing to zero
- rename a misspelled field and rename function code defines
- use a union and have only one write statement
- use cpu_to_beX()
b) Indicate the VM name/extended name + UUID just like KVM does
c) Indicate that all LPAR CPUs we fake are dedicated
d) Add a comment why we fake being a KVM guest
e) Give our guest as default the name "TCGguest"
f) Fake the same CPU information we have in our Guest for all layers
While at it, get rid of "potential_page_fault()" by forwarding the retaddr properly.
The result is best verified by looking at "/proc/sysinfo" in the guest when specifying on the qemu command line
-uuid "74738ff5-5367-5958-9aee-98fffdcd1876" \
-name "extra long guest name"
--- target/s390x/cpu.h | 22 +++-- target/s390x/misc_helper.c | 213 ++++++++++++++++++++++++--------------------- 2 files changed, 132 insertions(+), 103 deletions(-)
target/s390x/cpu.h +16 -6
@@ -425,11 +425,11 @@ static inline void setcc(S390CPU *cpu, uint64_t cc) }/* STSI */ -#define STSI_LEVEL_MASK 0x00000000f0000000ULL -#define STSI_LEVEL_CURRENT 0x0000000000000000ULL -#define STSI_LEVEL_1 0x0000000010000000ULL -#define STSI_LEVEL_2 0x0000000020000000ULL -#define STSI_LEVEL_3 0x0000000030000000ULL +#define STSI_R0_FC_MASK 0x00000000f0000000ULL +#define STSI_R0_FC_CURRENT 0x0000000000000000ULL +#define STSI_R0_FC_LEVEL_1 0x0000000010000000ULL +#define STSI_R0_FC_LEVEL_2 0x0000000020000000ULL +#define STSI_R0_FC_LEVEL_3 0x0000000030000000ULL#define STSI_R0_RESERVED_MASK 0x000000000fffff00ULL#define STSI_R0_SEL1_MASK 0x00000000000000ffULL#define STSI_R1_RESERVED_MASK 0x00000000ffff0000ULL
@@ -464,7 +464,7 @@ struct sysib_122 {uint8_t res1[32 ];uint32_t capability;uint16_t total_cpus; -uint16_t active_cpus; +uint16_t conf_cpus;uint16_t standby_cpus;uint16_t reserved_cpus;uint16_t adjustments[2026 ];
@@ -524,6 +524,16 @@ struct sysib_322 { };QEMU_BUILD_BUG_ON (sizeof (struct sysib_322 ) !=4096 ); +union sysib { +struct sysib_111 sysib_111; +struct sysib_121 sysib_121; +struct sysib_122 sysib_122; +struct sysib_221 sysib_221; +struct sysib_222 sysib_222; +struct sysib_322 sysib_322; +}; +QEMU_BUILD_BUG_ON (sizeof (union sysib ) !=4096 ); +/* MMU defines */ #define _ASCE_ORIGIN ~0xfffULL/* segment table origin */ #define _ASCE_SUBSPACE 0x200/* subspace group control */
target/s390x/misc_helper.c +116 -97
254 lines not on this page · raw message
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v1 for-2-12 13/15] s390x/tcg: STSI overhaul
Quoted 67 lines · On Mon, 11 Dec 2017 14:47:38 +0100 David Hildenbrand <[email protected]> wrote:
Current STSI implementation is a mess, so let's rewrite it.
Problems fixed by this patch:
1) The order of exceptions/when recognized is wrong.
2) We have to store to virtual address space, not absolute.
3) Alignment check of the block is missing.
3) The SMP information is not indicated.While at it:
a) Make the code look nicer
- get rid of nesting levels
- use struct initialization instead of initializing to zero
- rename a misspelled field and rename function code defines
- use a union and have only one write statement
- usecpu_to_beX()
b) Indicate the VM name/extended name + UUID just like KVM does
c) Indicate that all LPAR CPUs we fake are dedicated
d) Add a comment why we fake being a KVM guest
e) Give our guest as default the name "TCGguest"
f) Fake the same CPU information we have in our Guest for all layersWhile at it, get rid of "
potential_page_fault()" by forwarding the retaddr properly.The result is best verified by looking at "
/proc/sysinfo" in the guest when specifying on the qemu command line
-uuid "74738ff5-5367-5958-9aee-98fffdcd1876" \
-name "extra long guest name"Signed-off-by David Hildenbrand <[email protected]>--- target/s390x/cpu.h | 22 +++-- target/s390x/misc_helper.c | 213 ++++++++++++++++++++++++--------------------- 2 files changed, 132 insertions(+), 103 deletions(-)target/s390x/cpu.h +6
-6@@ -425,11 +425,11 @@ static inline void setcc(S390CPU *cpu, uint64_t cc) }/* STSI */ -#define STSI_LEVEL_MASK 0x00000000f0000000ULL -#define STSI_LEVEL_CURRENT 0x0000000000000000ULL -#define STSI_LEVEL_1 0x0000000010000000ULL -#define STSI_LEVEL_2 0x0000000020000000ULL -#define STSI_LEVEL_3 0x0000000030000000ULL +#define STSI_R0_FC_MASK 0x00000000f0000000ULL +#define STSI_R0_FC_CURRENT 0x0000000000000000ULL +#define STSI_R0_FC_LEVEL_1 0x0000000010000000ULL +#define STSI_R0_FC_LEVEL_2 0x0000000020000000ULL +#define STSI_R0_FC_LEVEL_3 0x0000000030000000ULL#define STSI_R0_RESERVED_MASK 0x000000000fffff00ULL#define STSI_R0_SEL1_MASK 0x00000000000000ffULL#define STSI_R1_RESERVED_MASK 0x00000000ffff0000ULL@@ -464,7 +464,7 @@ struct sysib_122 {uint8_t res1[32 ];uint32_t capability;uint16_t total_cpus; -uint16_t active_cpus; +uint16_t conf_cpus;uint16_t standby_cpus;uint16_t reserved_cpus;uint16_t adjustments[2026 ];@@ -524,6 +524,16 @@ struct sysib_322 { };
target/s390x/cpu.h struct sysib_322 { · line 534
Quoted 10 lines
target/s390x/cpu.h +8
-0QEMU_BUILD_BUG_ON (sizeof (struct sysib_322 ) !=4096 ); +union sysib { +struct sysib_111 sysib_111; +struct sysib_121 sysib_121; +struct sysib_122 sysib_122; +struct sysib_221 sysib_221; +struct sysib_222 sysib_222; +struct sysib_322 sysib_322; +};
Wondering whether you should add some typedefery to make this more qemu-y.
Quoted 5 lines
target/s390x/cpu.h +2
-0+QEMU_BUILD_BUG_ON (sizeof (union sysib ) !=4096 ); +/* MMU defines */ #define _ASCE_ORIGIN ~0xfffULL/* segment table origin */ #define _ASCE_SUBSPACE 0x200/* subspace group control */
(...)
target/s390x/cpu.h
Quoted 6 lines
target/s390x/cpu.h +6
-0+/* +* In theory, we could report Level 1 / Level 2 as current. However, +* the Linux kernel will detect then assume that we have a sclp +* linemode console (which is not the default for QEMU), therefore not +* displaying boot messages and making booting a Linux kernel under +* TCG harder
I know what you mean, but this reads a bit confusing. What about:
"However, the Linux kernel will detect this as running under LPAR and assume that we have a sclp linemode console (which is always present on LPAR, but not the default for QEMU), therefore not displaying boot messages and making booting a Linux kernel under TCG harder."
Quoted 9 lines
target/s390x/cpu.h +4
-0+* +* For now we fake the same SMP configuration on all levels. +* +* TODO: We could later make the level configurable via the machine
+ * and change defaults (linemode console) based on machine
typetarget/s390x/cpu.h +2
-0+* and accelerator. +*/
Re: [Qemu-devel] [qemu-s390x] [PATCH v1 for-2-12 13/15] s390x/tcg: STSI overhaul
Quoted 12 lines
target/s390x/cpu.h struct sysib_322 { · line 534
Quoted 9 lines
target/s390x/cpu.h +8
-0+union sysib { +struct sysib_111 sysib_111; +struct sysib_121 sysib_121; +struct sysib_122 sysib_122; +struct sysib_221 sysib_221; +struct sysib_222 sysib_222; +struct sysib_322 sysib_322; +};Wondering whether you should add some typedefery to make this more qemu-y.
I'll include that in the previous patch. (typedef ... )
Thanks!
Signature · 3 lines
Thanks, David / dhildenb
PATCH 14/15 s390x/tcg: remove SMP warning
In reply to David HildenbrandWe should be pretty good in shape now. Floating interrupts are working and atomic instructions should be atomic.
--- hw/s390x/s390-virtio-ccw.c | 4 ---- 1 file changed, 4 deletions(-)
hw/s390x/s390-virtio-ccw.c +0 -4
@@ -78,10 +78,6 @@ static void s390_init_cpus(MachineState *machine)MachineClass *mc =MACHINE_GET_CLASS (machine);int i; -if (tcg_enabled () && max_cpus >1 ) { -error_report ("WARNING: SMP support on s390x is experimental!" ); - } -/* initialize possible_cpus */ mc->possible_cpu_arch_ids (machine);
Signature · 1 line
2.14.3
PATCH 15/15 configure: s390x supports mttcg now
In reply to David Hildenbrands390x is ready. Most likely we are missing some pieces, but it should already be in pretty good shape now.
--- configure | 1 + 1 file changed, 1 insertion(+)
configure +1 -0
@@ -6508,6 +6508,7 @@ case "$target_name" in echo "TARGET_ABI32=y" >> $config_target_mak ;; s390x) + mttcg=yes gdb_xml_files="s390x-core64.xml s390-acr.xml s390-fpr.xml s390-vx.xml s390-cr.xml s390-virt.xml s390-gs.xml" ;; tilegx)
Signature · 1 line
2.14.3
Quoted 11 lines · On Mon, 11 Dec 2017 14:47:25 +0100 David Hildenbrand <[email protected]> wrote:
This patch series implements floating interrupt support for TCG and fixes STSI so we can remove warnings related to s390x SMP and MTTCG.
KVM code has to be touched in order to factor out the injection routines into the flic ("s390x/flic: factor out injection of floating interrupts"). Basic testing didn't reveal any problems so far.
With this series I am now able to run fedora 26/27 and Ubuntu 17.10+ with 16 VCPUs (MTTCG) on a 8CPU host, doing a make -j16 in the guest. I got nasty stalls in the guest beforehand.
Quoted 8 lines
I already sent the patches
- "cpus: makepause_all_cpus()play with SMP on single threaded TCG"
- "cpu-exec: fix missed CPU kick during interrupt injection"
separately, but as nobody commented so far (e.g. due to 2.11) I am including them here as they are required to make SMP work reliably.Based on: https://github.com/cohuck/qemu.git s390x-next Available at: https://github.com/davidhildenbrand/qemu.git s390x-queue
Generally, this looks good to me. I have commented on the individual patches.
Looking forward to a v2 :)