Code review comment for ~paelzer/ubuntu/+source/qemu:SRU-lp-1836154-z14-hw-cpu-model-bionic

You already know that, but this is a review, I should mention everything I see and let you decide things.. so here it goes:

We just pushed a fix to Bionic (for MDS-NO) which means that you have to rebase this merge request to the one currently in -proposed. I see
you've also included the boot signature toleration patch in previous version,
I confess I missed that from the current -proposed for Bionic.

------------

About the backported patches:

1) lp-1836154-01-MIMIMAL-header-sync-against-5.2.patch

Despite the small typo for "minimal", I could not find this commit as being a upstream QEMU commit. The commit I found, related to this change, was:

commit d9cb4336159a00bd0d9c81b93f02874ef3626057
Author: Cornelia Huck <email address hidden>
Date: Thu May 16 14:10:36 2019

    linux headers: update against Linux 5.2-rc1

    commit a188339ca5a396acc588e5851ed7e19f66b0ebd9

    Signed-off-by: Cornelia Huck <email address hidden>

diff --git a/linux-headers/asm-s390/kvm.h b/linux-headers/asm-s390/kvm.h
index 0265482f8f..03ab5968c7 100644
--- a/linux-headers/asm-s390/kvm.h
+++ b/linux-headers/asm-s390/kvm.h
@@ -152,7 +152,10 @@ struct kvm_s390_vm_cpu_subfunc {
        __u8 pcc[16]; /* with MSA4 */
        __u8 ppno[16]; /* with MSA5 */
        __u8 kma[16]; /* with MSA8 */
- __u8 reserved[1808];
+ __u8 kdsa[16]; /* with MSA9 */
+ __u8 sortl[32]; /* with STFLE.150 */
+ __u8 dfltcc[32]; /* with STFLE.151 */
+ __u8 reserved[1728];
 };

 /* kvm attributes for crypto */

(the exact same content) as given by IBM DE in the backport list.
Did you get this patch (w/ diff desc) somewhere else ?

2) lp-1836154-02-s390x-cpumodel-Miscellaneous-Instruction-Extensions-.patch

Where is the commit ID coming from ? Your local repo ? Correct if I'm wrong but, in this case, using DEP-3 compliant headers is the correct choice, no ?

I saw you added (cherry picked from commit ...) message but that implies in having "Origin:"" header flag (according to DEP3), and changing commit Author, at least it seems that way according to (https://dep-team.pages.debian.net/deps/dep3/).

Patches contents are good and equivalent to the upstream ones.

3) lp-1836154-03-s390x-cpumodel-msa9-facility.patch

Same as (1), (2), (3), ...

I believe you did the same thing for:

lp-1836154-01-mimimal-header-sync-against-5.2.patch (d9cb4336159a)
lp-1836154-02-s390x-cpumodel-...-Extensions-.patch (2ec038836fa0)
lp-1836154-03-s390x-cpumodel-msa9-facility.patch (5dacbe23d23c)
lp-1836154-04-s390x-cpumodel-vector-enhancements.patch (54d65de0b525)
lp-1836154-05-s390x-cpumodel-enhanced-sort-facility.patch (d220fabf1609)
lp-1836154-06-s390x-cpumodel-add-Def...ion-facility.patch (afc7b8666b62)
lp-1836154-07-s390x-cpumodel-add-gen15-defintions.patch (caef62430fed)
lp-1836154-08-s390x-cpumodel-wire...-as-gen15-machin.patch (c657e84faee4)
lp-1836154-09-s390-cpumodel-fix-desc...new-vector-fac.patch (d05be57ddc2e)

and then for the next two:

lp-1836154-s390x-cpumodel-remove-esort-from-the-default-model.patch
lp-1836154-s390x-cpumodel-also-change-name-of-vxbeh.patch

you have the correct cherry-picked patch with a DEP3 header, and it isn't mandatory since the clean upstream patch format is acceptable as delta.

Shouldn't this be the opposite ? All 01-09 patches to have the DEP3 header and
the (cherry-picked from XXXXX, like they have) line in the patch AND the last 2 could have DEP3 or not.

I don't want to be annoying with minimal details that you're possibly already aware, and sorry if I'm being, specially for this package since you are the one that will be maintaining it.

----- compilation and usability:

I was able to generate s390x package and use it inside a Ubuntu Bionic LXD
container in a Ubuntu Disco LPAR.

inaddy@kguest:~$ sudo lscpu
Architecture: s390x
CPU op-mode(s): 32-bit, 64-bit
Byte Order: Big Endian
CPU(s): 4
On-line CPU(s) list: 0-3
Thread(s) per core: 1
Core(s) per socket: 1
Socket(s) per book: 1
Book(s) per drawer: 1
Drawer(s): 4
NUMA node(s): 1
Vendor ID: IBM/S390
Machine type: 2964
CPU dynamic MHz: 5000
CPU static MHz: 5000
BogoMIPS: 3033.00
Hypervisor: KVM/Linux
Hypervisor vendor: KVM
Virtualization type: full
Dispatching mode: horizontal
L1d cache: 128K
L1i cache: 96K
L2d cache: 2048K
L2i cache: 2048K
L3 cache: 65536K
L4 cache: 491520K
NUMA node0 CPU(s): 0-3
Flags: esan3 zarch stfle msa ldisp eimm dfp edat etf3eh highgprs te vx

qemu correctly report added CPU flags:

https://pastebin.ubuntu.com/p/HDn7g9H9sk/

And I believe it is safe that IBM has tested the CPU functionalities (specially when we don't have the HW they're enabling).