| Message ID | 20200915194416.107460-1-walling@linux.ibm.com |
|---|---|
| Headers | show |
| Series | s390: Extended-Length SCCB & DIAGNOSE 0x318 | expand |
Patchew URL: https://patchew.org/QEMU/20200915194416.107460-1-walling@linux.ibm.com/ Hi, This series failed build test on FreeBSD host. Please find the details below. === TEST SCRIPT BEGIN === #!/bin/bash # Testing script will be invoked under the git checkout with # HEAD pointing to a commit that has the patches applied on top of "base" # branch if qemu-system-x86_64 --help >/dev/null 2>&1; then QEMU=qemu-system-x86_64 elif /usr/libexec/qemu-kvm --help >/dev/null 2>&1; then QEMU=/usr/libexec/qemu-kvm else exit 1 fi make vm-build-freebsd J=21 QEMU=$QEMU exit 0 === TEST SCRIPT END === The full log is available at http://patchew.org/logs/20200915194416.107460-1-walling@linux.ibm.com/testing.FreeBSD/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
On Tue, 15 Sep 2020 12:57:37 -0700 (PDT) no-reply@patchew.org wrote: > Patchew URL: https://patchew.org/QEMU/20200915194416.107460-1-walling@linux.ibm.com/ > > > > Hi, > > This series failed build test on FreeBSD host. Please find the details below. > > === TEST SCRIPT BEGIN === > #!/bin/bash > # Testing script will be invoked under the git checkout with > # HEAD pointing to a commit that has the patches applied on top of "base" > # branch > if qemu-system-x86_64 --help >/dev/null 2>&1; then > QEMU=qemu-system-x86_64 > elif /usr/libexec/qemu-kvm --help >/dev/null 2>&1; then > QEMU=/usr/libexec/qemu-kvm > else > exit 1 > fi > make vm-build-freebsd J=21 QEMU=$QEMU > exit 0 > === TEST SCRIPT END === "fatal: unable to write new index file" Is patchew out of disk space? [And it's a bit annoying that the actual error message has been snipped from the email; is that intentional?] > > > > The full log is available at > http://patchew.org/logs/20200915194416.107460-1-walling@linux.ibm.com/testing.FreeBSD/?type=message. > --- > Email generated automatically by Patchew [https://patchew.org/]. > Please send your feedback to patchew-devel@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.
On 9/16/20 11:53 AM, Cornelia Huck wrote: [...] >> > > Thanks, applied. > > Thanks Conny. Much appreciated for everyone's patience and review. The only thing I'd like to hold out on for now is for someone to take a peek at patch #3 with respect to the protected virtualization stuff. I don't know too much about it, honestly, and I want to ensure that dynamically allocating memory for the SCCB makes sense there. The alternative would be to allocate a static 4K for the work_sccb.
On 9/16/20 1:15 PM, Collin Walling wrote: > On 9/16/20 11:53 AM, Cornelia Huck wrote: > > [...] > >>> >> >> Thanks, applied. >> >> > > Thanks Conny. > > Much appreciated for everyone's patience and review. The only thing I'd > like to hold out on for now is for someone to take a peek at patch #3 > with respect to the protected virtualization stuff. I don't know too > much about it, honestly, and I want to ensure that dynamically > allocating memory for the SCCB makes sense there. The alternative would > be to allocate a static 4K for the work_sccb. > I had someone take a look at the patch for PV and was told everything looks sane. Since the patches have already been applied, it seems like it's too late to add a reviewed-by from someone? Either way: thanks to everyone for the journey on getting these patches through!
On Fri, 25 Sep 2020 11:13:49 -0400 Collin Walling <walling@linux.ibm.com> wrote: > On 9/16/20 1:15 PM, Collin Walling wrote: > > On 9/16/20 11:53 AM, Cornelia Huck wrote: > > > > [...] > > > >>> > >> > >> Thanks, applied. > >> > >> > > > > Thanks Conny. > > > > Much appreciated for everyone's patience and review. The only thing I'd > > like to hold out on for now is for someone to take a peek at patch #3 > > with respect to the protected virtualization stuff. I don't know too > > much about it, honestly, and I want to ensure that dynamically > > allocating memory for the SCCB makes sense there. The alternative would > > be to allocate a static 4K for the work_sccb. > > > > I had someone take a look at the patch for PV and was told everything > looks sane. Since the patches have already been applied, it seems like > it's too late to add a reviewed-by from someone? Have the reviewer reply with their R-b, and I'll happily add it, as I rebase s390-next before doing a pull req anyway :) > > Either way: thanks to everyone for the journey on getting these patches > through! >
On Fri, 25 Sep 2020 17:18:55 +0200 Cornelia Huck <cohuck@redhat.com> wrote: > On Fri, 25 Sep 2020 11:13:49 -0400 > Collin Walling <walling@linux.ibm.com> wrote: > > > On 9/16/20 1:15 PM, Collin Walling wrote: > > > On 9/16/20 11:53 AM, Cornelia Huck wrote: > > > > > > [...] > > > > > >>> > > >> > > >> Thanks, applied. > > >> > > >> > > > > > > Thanks Conny. > > > > > > Much appreciated for everyone's patience and review. The only > > > thing I'd like to hold out on for now is for someone to take a > > > peek at patch #3 with respect to the protected virtualization > > > stuff. I don't know too much about it, honestly, and I want to > > > ensure that dynamically allocating memory for the SCCB makes > > > sense there. The alternative would be to allocate a static 4K for > > > the work_sccb. > > > > I had someone take a look at the patch for PV and was told > > everything looks sane. Since the patches have already been applied, > > it seems like it's too late to add a reviewed-by from someone? > > Have the reviewer reply with their R-b, and I'll happily add it, as I > rebase s390-next before doing a pull req anyway :) well it was me :) you can add a Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com> for the first 6 patches, and an Acked-by: Claudio Imbrenda <imbrenda@linux.ibm.com> for the last one thanks!
On Fri, 25 Sep 2020 17:32:05 +0200 Claudio Imbrenda <imbrenda@linux.ibm.com> wrote: > On Fri, 25 Sep 2020 17:18:55 +0200 > Cornelia Huck <cohuck@redhat.com> wrote: > > > On Fri, 25 Sep 2020 11:13:49 -0400 > > Collin Walling <walling@linux.ibm.com> wrote: > > > > > On 9/16/20 1:15 PM, Collin Walling wrote: > > > > On 9/16/20 11:53 AM, Cornelia Huck wrote: > > > > > > > > [...] > > > > > > > >>> > > > >> > > > >> Thanks, applied. > > > >> > > > >> > > > > > > > > Thanks Conny. > > > > > > > > Much appreciated for everyone's patience and review. The only > > > > thing I'd like to hold out on for now is for someone to take a > > > > peek at patch #3 with respect to the protected virtualization > > > > stuff. I don't know too much about it, honestly, and I want to > > > > ensure that dynamically allocating memory for the SCCB makes > > > > sense there. The alternative would be to allocate a static 4K for > > > > the work_sccb. > > > > > > I had someone take a look at the patch for PV and was told > > > everything looks sane. Since the patches have already been applied, > > > it seems like it's too late to add a reviewed-by from someone? > > > > Have the reviewer reply with their R-b, and I'll happily add it, as I > > rebase s390-next before doing a pull req anyway :) > > well it was me :) > > you can add a > > Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > > for the first 6 patches, and an > > Acked-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > > for the last one > > > thanks! > Thanks, updated and pushed out.
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(-)