Re: [PATCH v6 0/8] s390: Extended-Length SCCB & DIAGNOSE 0x318

[Cornelia Huck <cohuck@redhat.com>]

On Tue, 15 Sep 2020 15:44:08 -0400
Collin Walling <walling@linux.ibm.com> wrote:

> Changelog:
> 
>     v6
> 
>     • sccb_verify_boundary function:
>         • s/len/sccb_len
>         • removed the endian check/conversion of the sccb_len from within 
>           this function (caller is now responsible)
> 
>     • proper endian conversion when using header length to malloc
> 
>     • use g_autofree for work_sccb
> 
>     • added r-b's and acks (thanks!)
> 
>     • added a feature-check fence within the diag_318_handler to ensure
>         the handler does not complete without proper feature support
>         • will throw a program exception if handler is invoked but
>           feature is not enabled
> 
>     
> 
>     v5 (comment below pertains to version 5)
> 
>     Janosch, Thomas, Conny: I've removed your r-b's from patch #3 since I
>     added some g_mallocs in place and I'd like to make sure things are
>     done properly there (explained in changelog, but let me know if further
>     explanation is necessary).
> 
>     Janosch, please let me know if the changes to #3 are safe under PV.
> 
>     Thanks.
> 
>     • removed sccb_verify_length function
>         - will simply use the length check code that was in place before
> 
>     • introduced a macro for calculating required SCCB length
>         - takes a struct and max # of cpus as args
> 
>     • work_sccb size is now dynamically allocated based on the length
>       provided by the guest kernel, instead of always using a static
>       4K size
>         - as such, the SCCB will have to be read twice:
>             - first time to retrieve the header
>             - second time with proper size after space for work_sccb 
>               is allocated
> 
> 
> 
>     v4
>     
>     • added r-b's and ack's (thanks, everyone!)
> 
>     • renamed boundary and length function
> 
>     • updated header sync to reflect a change discussed in the respective
>         KVM patches
> 
>     • s/data_len/offset_cpu
> 
>     • added /* fallthrough */ comment in boundary check
> 
> 
> 
>     v3
> 
>     • Device IOCTLs removed
>         - diag 318 info is now communicated via sync_regs
> 
>     • Reset code removed
>         - this is now handled in KVM
>         - diag318_info is stored within the CPU reset portion of the
>             S390CPUState
> 
>     • Various cleanups for ELS preliminary patches
> 
> 
> 
>     v2
> 
>     • QEMU now handles the instruction call
>         - as such, the "enable diag 318" IOCTL has been removed
> 
>     • patch #1 now changes the read scp/cpu info functions to
>       retrieve the machine state once
>         - as such, I have not added any ack's or r-bs since this
>           patch differs from the previous version
> 
>     • patch #3 introduces a new "get_read_scp_info_data_len"
>       function in order clean-up the variable data length assignment
>       in patch #7
>         - a comment above this function should help clarify what's
>           going on to make things a bit easier to read
> 
>     • other misc clean ups and fixes
>         - s/diag318/diag_318 in order to keep the naming scheme
>           consistent with Linux and other diag-related code
>         - s/byte_134/fac134 to align naming scheme with Linux
> 
> -----------------------------------------------------------------------
> 
> This patch series introduces two features for an s390 KVM quest:
>     - Extended-Length SCCB (els) for the Read SCP/CPU Info SCLP 
>         commands
>     - DIAGNOSE 0x318 (diag_318) enabling / migration handling
> 
> The diag 318 feature depends on els and KVM support.
> 
> The els feature is handled entirely with QEMU, and does not require 
> KVM support.
> 
> Both features are made available starting with the zEC12-full model.
> 
> These patches are introduced together for two main reasons:
>     - els allows diag 318 to exist while retaining the original 248 
>         VCPU max
>     - diag 318 is presented to show how els is useful
> 
> Full els support is dependant on the Linux kernel, which must react
> to the SCLP response code and set an appropriate-length SCCB. 
> 
> A user should take care when tuning the CPU model for a VM.
> If a user defines a VM with els support and specifies 248 CPUs, but
> the guest Linux kernel cannot react to the SCLP response code, then
> the guest will crash immediately upon kernel startup.
> 
> Collin L. Walling (8):
>   s390/sclp: get machine once during read scp/cpu info
>   s390/sclp: rework sclp boundary checks
>   s390/sclp: read sccb from mem based on provided length
>   s390/sclp: check sccb len before filling in data
>   s390/sclp: use cpu offset to locate cpu entries
>   s390/sclp: add extended-length sccb support for kvm guest
>   s390/kvm: header sync for diag318
>   s390: guest support for diagnose 0x318
> 
>  hw/s390x/event-facility.c           |   2 +-
>  hw/s390x/sclp.c                     | 142 ++++++++++++++++++++--------
>  include/hw/s390x/sclp.h             |  11 ++-
>  linux-headers/asm-s390/kvm.h        |   7 +-
>  linux-headers/linux/kvm.h           |   1 +
>  target/s390x/cpu.h                  |   2 +
>  target/s390x/cpu_features.h         |   1 +
>  target/s390x/cpu_features_def.h.inc |   4 +
>  target/s390x/cpu_models.c           |   1 +
>  target/s390x/gen-features.c         |   2 +
>  target/s390x/kvm.c                  |  47 +++++++++
>  target/s390x/machine.c              |  17 ++++
>  12 files changed, 194 insertions(+), 43 deletions(-)
> 

Thanks, applied.