Re: [Qemu-devel] [PATCH v1 00/27] s390x: SMP for TCG (+ cleanups)

[David Hildenbrand <david@redhat.com>]

On 18.09.2017 19:31, Christian Borntraeger wrote:
> David,
> 
> can you outline how much you tested KVM after these changes?  I assume we should
> review/test the whole series properly?

Related to my tests:

I did the usual shutdown/reboot/system_reset/online-offline test with
KVM. I remember also playing with CPU hotplug again. However, no
kvm-unit-tests so far (also not in my pipeline).


Related to the patches:

I tried to modify as little KVM code as possible. Most stuff really is
just moving code around. But still, subtle errors can happen.

E.g. for all new SIGP instructions, I keep existing behavior for KVM by
checking for tcg/kvm directly at the start at the handlers. So most
s390x/tcg patches should have no influence.

So reviewing all s390x/kvm patches and !tcg patches would be great. I
will certainly not complain if some IBM folks would also review the TCG
patches ;)

Special care should need:
- "target/s390x: proper cpu->be convertion in s390_store_status()"
-- to make sure I haven't done any stupid mistakes while converting
   (just realized s/convertion/conversion/ :) )
- "s390x/kvm: factor out SIGP code into sigp.c"
-- to make sure the new kvm_s390_handle_sigp() and handle_sigp() do the
   same thing as handle_sigp() did before.

"s390x/tcg: STOPPED cpus can never wake up" and
"s390x/tcg: implement STOP and RESET interrupts for TCG" modify a
function called also for kvm (s390_cpu_has_work()). It is expected to
return always "false" for kvm. This behavior should still be that way.

Thanks!

> 
> On 09/18/2017 05:59 PM, David Hildenbrand wrote:
>> This series contains:
>> - properly implement local external interrupts for TCG
>> - factor out KVM SIGP handling code into common code
>> - implement missing SIGP orders for TCG handled by the kernel for KVM
>>   (including STOP and RESTART interrupts)
>> - make TCG use the new SIGP code - experimental SMP support for s390x TCG
>> - refactor STFL(E) implementation for TCG
>> - bunch of cleanups
>>
>> Basically all SIGP instructions are fully supported.
>>
>> Thanks to Aurelien Jarno for the initital prototype and showcasing that
>> supporting experimental SMP code can be implemented quite easily.
>>
>> TCG SMP on s390x - what works?
>> - "-smp X,maxcpus=X" with both, single and multi threaded TCG
>> - "-smp ... -device qemu-s390-cpu,id=cpuX,core-id=X"
>> - system_powerdown, system_reset, shutdown, reboot, NMI
>> - online/offline of CPUs from inside the guest
>>
>> TCG SMP on s390x - what does not work?
>> - Floating interrupts all target CPU 0. Don't offline it.
>> - CPU hotplug after the machine/main loop has been fully setup
>> -- the new CPU comes up, answers and sends emergency signals, but suddenly
>>    the VM gets stuck. This is strange, as ordinary online/offline works
>>    just fine.
>> -- can be triggered by "cpu-add 1" + "system_reset". The system will hang
>>    when trying to online CPUs. (note: in Linux code they are fully up and
>>    running and already executed code)
>> -- also if hotplugging with "-S", before anything has run. This is strange,
>>    as "-device qemu-s390-cpu" works just fine.
>> -- does not seem to be related to CPU setup/reset code, I checked that
>> -- seems to be related to some TCG internals (as broken for single and
>>    multi threaded TCG).
>> -- common code seems to be somehow broken, not sure if this is even
>>    expected to work (e.g. for single threaded TCG, hotplugged CPUs will
>>    never get set "cpu->created = true". But doesn't seem to be related to
>>    this)
>>
>>
>> Based on: https://github.com/cohuck/qemu.git s390-next
>> Available on: git@github.com:davidhildenbrand/qemu.git s390x-queue
>>
>>
>> David Hildenbrand (27):
>>   s390x: raise CPU hotplug irq after really hotplugged
>>   s390x/cpumodel: fix max STFL(E) bit number
>>   target/s390x: get rid of next_core_id
>>   s390x: introduce and use S390_MAX_CPUS
>>   s390/tcg: turn INTERRUPT_EXT into a mask
>>   s390x/tcg: injection of emergency signals and extarnal calls
>>   s390x/tcg: STOPPED cpus can never wake up
>>   s390x/tcg: a CPU cannot switch state due to an interrupt
>>   target/s390x: factor out handling of WAIT PSW into handle_wait()
>>   s390x/kvm: pass ipb directly into handle_sigp()
>>   s390x/kvm: generalize SIGP stop and restart interrupt injection
>>   s390x/kvm: factor out storing of CPU status
>>   target/s390x: proper cpu->be convertion in s390_store_status()
>>   s390x/kvm: factor out storing of adtl CPU status
>>   s390x/kvm: drop two debug prints
>>   s390x/kvm: factor out SIGP code into sigp.c
>>   s390x/kvm: factor out actual handling of STOP interrupts
>>   s390x/tcg: implement SIGP SENSE RUNNING STATUS
>>   s390x/tcg: implement SIGP SENSE
>>   s390x/tcg: implement SIGP EXTERNAL CALL
>>   s390x/tcg: implement SIGP EMERGENCY SIGNAL
>>   s390x/tcg: implement SIGP CONDITIONAL EMERGENCY SIGNAL
>>   s390x/tcg: implement STOP and RESET interrupts for TCG
>>   s390x/tcg: flush the tlb on SIGP SET PREFIX
>>   s390x/tcg: switch to new SIGP handling code
>>   s390x/tcg: unlock NMI
>>   s390x/tcg: refactor stfl(e) to use s390_get_feat_block()
>>
>>  hw/s390x/s390-virtio-ccw.c  |  17 +-
>>  target/s390x/Makefile.objs  |   1 +
>>  target/s390x/cpu-qom.h      |   2 -
>>  target/s390x/cpu.c          |  40 ++--
>>  target/s390x/cpu.h          |  36 +++-
>>  target/s390x/cpu_features.c |   2 +-
>>  target/s390x/cpu_models.c   |   2 +
>>  target/s390x/excp_helper.c  |  98 ++++++---
>>  target/s390x/helper.c       | 115 ++++++++--
>>  target/s390x/helper.h       |   4 +-
>>  target/s390x/internal.h     |  15 ++
>>  target/s390x/interrupt.c    |  70 +++++-
>>  target/s390x/kvm-stub.c     |  13 +-
>>  target/s390x/kvm.c          | 470 +++--------------------------------------
>>  target/s390x/kvm_s390x.h    |   3 +-
>>  target/s390x/misc_helper.c  | 114 ++++------
>>  target/s390x/sigp.c         | 504 ++++++++++++++++++++++++++++++++++++++++++++
>>  target/s390x/trace-events   |   4 +-
>>  target/s390x/translate.c    |   6 +-
>>  19 files changed, 896 insertions(+), 620 deletions(-)
>>  create mode 100644 target/s390x/sigp.c
>>
> 


-- 

Thanks,

David