Merge ~rafaeldtinoco/ubuntu/+source/qemu:lp1828495-disco-devel-qemu into ubuntu/+source/qemu:ubuntu/disco-devel

Proposed by Rafael David Tinoco on 2019-06-28
Status: Merged
Merge reported by: Christian Ehrhardt 
Merged at revision: d4bf86763f4ca913f80016a735e5686c90a52570
Proposed branch: ~rafaeldtinoco/ubuntu/+source/qemu:lp1828495-disco-devel-qemu
Merge into: ubuntu/+source/qemu:ubuntu/disco-devel
Diff against target: 49 lines (+20/-0) (has conflicts)
2 files modified
debian/changelog (+17/-0)
debian/patches/series (+3/-0)
Conflict in debian/changelog
Conflict in debian/patches/series
Reviewer Review Type Date Requested Status
Bryce Harrington 2019-06-28 Approve on 2019-07-23
Christian Ehrhardt  2019-06-28 Approve on 2019-07-01
Ubuntu Virtualization Developers 2019-06-28 Pending
Ubuntu Core Development Team 2019-06-28 Pending
Canonical Server Team 2019-06-28 Pending
Review via email: mp+369470@code.launchpad.net
To post a comment you must log in.
Bryce Harrington (bryce) wrote :

I'm not sure there's any standard or convention, but I've been putting the dep3 headers at the top of the patch file. I'm sure it doesn't matter technically but I figure they'll be more noticeable (and more likely to get updated) by future maintainers there.

I was a little confused by this:

+ Needed patches are in d/p/u/lp1828495-:
+ - 0011-disable-arch-cap-when-no-msr.patch (LP: #1828495):

which I interpreted to mean the patches would be named like

    debian/patches/ubuntu/lp1828495-0011-disable-arch-cap-when-no-msr.patch

However, the series file and the patches themselves are named without the "lp1828495-" prefix.

Also, don't forget the link to the PPA.

I figure Christian will give a much more detailed review. I just checked that the patches are all in good order, the dep3 header elements match the patch, and that the patches are indeed present upstream.

review: Needs Fixing
Rafael David Tinoco (rafaeldtinoco) wrote :

DEP3 at the bottom is usually fine... but you had a good catch on missing lp1828495.. I forgot about it (done in eoan) when renaming patches. TKu! I'll push this again as soon as I finish some execution tests for the same branch!

Rafael David Tinoco (rafaeldtinoco) wrote :

I meant bionic instead of eoan in last comment (Christian is doing an upstream merge for Eoan to solve this).

2bfa74f... by Rafael David Tinoco on 2019-06-29

  - 0011-disable-arch-cap-when-no-msr.patch (LP: #1828495):
    i386: kvm: Disable arch_capabilities if MSR can't be set
    (upstream: 485b1d256bcb, desc: v4.0.0-rc0-1-g485b1d256b)

9d9bce6... by Rafael David Tinoco on 2019-06-29

  - 0012-arch-capabilities-migratable.patch (LP: #1828495):
    i386: Make arch_capabilities migratable
    (upstream: 014018e19b3c, desc: v4.0.0-rc0-2-g014018e19b)

a48e22c... by Rafael David Tinoco on 2019-06-29

  - 0014-remove-cpuid-pconfig.patch
    i386: remove the new CPUID 'PCONFIG' from Icelake-Server CPU model
    (upstream: 76e5a4d58357, desc: v3.1.0-1495-g76e5a4d58

d4bf867... by Rafael David Tinoco on 2019-06-29

changelog

Rafael David Tinoco (rafaeldtinoco) wrote :

Ohh and sorry, the PPA is the same one as I used for the Bionic merge:

https://launchpad.net/~rafaeldtinoco/+archive/ubuntu/lp1828495

I should have mentioned it.

Packages versioned as: 1:3.1+dfsg-2ubuntu3.3~ppa1

Christian Ehrhardt  (paelzer) wrote :

@Bryce - yes the position of dep3 is (AFAIK) only style; e.g. I usually add them along the tags upstream has between signoff and the diffstat.

@Rafael - this LGTM it is the long list we had in Bionic reduced to what still applies to Disco.
I found no special case that would affect this being different in 3.1.

Ready to also create cosmic then - OTOH thinking again this one is so huge and testing will surely take a while. Cosmic is EOL in mid July, so maybe we don't even consider cosmic on this - what is your opinion about that?

review: Approve
Christian Ehrhardt  (paelzer) wrote :

For coordination, next steps will be:
- get qemu 4.0 complete in Eoan (prereq to be in -dev release)
- build PPA combining the fixes for this and similar changes for PPC
  - regression test on this PPA
  - IBMs ack on the PPA with DD 2.3 HW
  - redo your manual tests that you have done with Disco and Eoan
- Start SRU process on these

Rafael David Tinoco (rafaeldtinoco) wrote :

+1 on everything you said.

64e6546... by Rafael David Tinoco on 2019-07-15

  - 0015-remove-cpuid-intel_pt.patch
    i386: remove the 'INTEL_PT' CPUID bit from named CPU models
    (upstream: 4c257911dcc7, desc: v3.1.0-1496-g4c257911dc)

11af69e... by Rafael David Tinoco on 2019-07-15

  - 0016-no-ospke-on-some.patch (LP: #1828495):
    i386: Disable OSPKE on CPU model definitions
    (upstream: bb4928c7cafe, desc: v4.0.0-rc0-3-gbb4928c7ca)

Rafael David Tinoco (rafaeldtinoco) wrote :

Last push was only to fix some fuzz in 0016 that caused trouble with debuild -S.

Rafael David Tinoco (rafaeldtinoco) wrote :

I uploaded source to PPA:

https://launchpad.net/~rafaeldtinoco/+archive/ubuntu/lp1828495?field.series_filter=disco

If you want to test this patchset only.

Christian Ehrhardt  (paelzer) wrote :

I adopted the change to #15/#16 into my branches and PPAs and will respin the testsuite

Bryce Harrington (bryce) :
review: Approve
Rafael David Tinoco (rafaeldtinoco) wrote :

Based on public bug feedback from Intel, looks like I've missed the undocumented MDS-NO mitigation bit from ARCH_CAPABILITIES. I haven't done it because last manual from Intel does not show it, I'm really not sure where to find more documentation about it.. but its a simple upstream change (just the MSR flag) so I backported and I'm pushing the change here.

I'm first dealing with the Bionic SRU, and if it is all good, I'll include the same patch in this change.

Comments from the Bionic merge request:

https://code.launchpad.net/~rafaeldtinoco/ubuntu/+source/qemu/+git/qemu/+merge/368804/comments/969879
https://code.launchpad.net/~rafaeldtinoco/ubuntu/+source/qemu/+git/qemu/+merge/368804/comments/969880

From the other merge request:

"""
This is the upstream change:

https://github.com/qemu/qemu/commit/20140a82c674

This is the public comment from Intel:

https://bugs.launchpad.net/intel/+bug/1828495/comments/40

And commit the patch is:

    - 0017-target-i386-add-MDS-NO-feature.patch (LP: #1828495):
      target/i386: add MDS-NO feature
      (upstream: 20140a82c674, desc: v4.0.0-721-g20140a82c6)
"""

Nish Aravamudan (nacc) wrote :

On Mon, Jul 1, 2019, 02:46 Christian Ehrhardt  <
<email address hidden>> wrote:

> Review: Approve
>
> @Bryce - yes the position of dep3 is (AFAIK) only style; e.g. I usually
> add them along the tags upstream has between signoff and the diffstat.
>

FYI `dep3changelog` while not a policy file may imply that putting headers
anywhere is not supported by all tools. I'm curious what it does in this
case.

-Nish

Christian Ehrhardt  (paelzer) wrote :

yep good thought Nish, so our usual practise is either all wrong or had more than style reasons :-)
But this isn't part of this MP content here so lets keep that aside.

@Rafael - here the same changes as I requested in the Bionic branch please.
Then we can sponsor and ask the SRU Team to get a fast turnaround time on this fixup.

Rafael David Tinoco (rafaeldtinoco) wrote :

Actually the only change needed here was the changelog mentioning the BUG # just once, like I did on Bionic. I pushed it but then I realized we have already this version in -proposed so no changes are needed and we can "verify" the package we have in disco-proposed right now.

All this because you had already included patches in Disco called:

qemu (1:3.1+dfsg-2ubuntu3.1) disco-security; urgency=medium

  * SECURITY UPDATE: Add support for exposing md-clear functionality
    to guests
    - d/p/ubuntu/enable-md-clear.patch
    - d/p/ubuntu/enable-md-no.patch
    ...

And enable-md-no.patch made the trick for MDS-NO bit.

With that, I don't think its worth anything else here if you agree (just the verification in the public bug).

Bryce Harrington (bryce) wrote :

On Mon, Aug 05, 2019 at 01:12:29AM -0000, Nish Aravamudan wrote:
> On Mon, Jul 1, 2019, 02:46 Christian Ehrhardt  <
> <email address hidden>> wrote:
>
> > Review: Approve
> >
> > @Bryce - yes the position of dep3 is (AFAIK) only style; e.g. I usually
> > add them along the tags upstream has between signoff and the diffstat.
> >
>
> FYI `dep3changelog` while not a policy file may imply that putting headers
> anywhere is not supported by all tools. I'm curious what it does in this
> case.

Looking at its source code, it expects to see Description|Subject and
Origin|Author|From somewhere between the top of the file and the --- cut
line but doesn't seem to care where or in what order.

(Personally, I remain unconvinced that it's better to interleave the
DEP3 headers rather than keep them above and separate from the upstream
patch, however all the patches I've seen so far that have DEP3 headers
are interleaving them, so there's at least a consistency value in doing
similarly.)

Bryce

Christian Ehrhardt  (paelzer) wrote :

@Rafael - Ack to Disco being good

And since it is in -proposed it is also merged.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/changelog b/debian/changelog
2index bb2e787..aed6368 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,5 +1,6 @@
6 qemu (1:3.1+dfsg-2ubuntu3.3) disco; urgency=medium
7
8+<<<<<<< debian/changelog
9 [ Christian Ehrhardt ]
10 * d/p/ubuntu/lp-1830243-s390-bios-Skip-bootmap-signature-entries.patch:
11 tolerate guests with secure boot loaders (LP: #1830243)
12@@ -19,6 +20,22 @@ qemu (1:3.1+dfsg-2ubuntu3.3) disco; urgency=medium
13 i386: Disable OSPKE on CPU model definitions
14
15 -- Christian Ehrhardt <christian.ehrhardt@canonical.com> Thu, 04 Jul 2019 14:47:56 +0200
16+=======
17+ * {Ice,Cascade}Lake CPUs + IA32_ARCH_CAPABILITIES support (LP: #1828495)
18+ Needed patches are in d/p/u/lp1828495-:
19+ - 0011-disable-arch-cap-when-no-msr.patch:
20+ i386: kvm: Disable arch_capabilities if MSR can't be set
21+ - 0012-arch-capabilities-migratable.patch:
22+ i386: Make arch_capabilities migratable
23+ - 0014-remove-cpuid-pconfig.patch:
24+ i386: remove the new CPUID 'PCONFIG' from Icelake-Server CPU model
25+ - 0015-remove-cpuid-intel_pt.patch:
26+ i386: remove the 'INTEL_PT' CPUID bit from named CPU models
27+ - 0016-no-ospke-on-some.patch:
28+ i386: Disable OSPKE on CPU model definitions
29+
30+ -- Rafael David Tinoco <rafaeldtinoco@ubuntu.com> Fri, 28 Jun 2019 20:57:00 +0000
31+>>>>>>> debian/changelog
32
33 qemu (1:3.1+dfsg-2ubuntu3.2) disco; urgency=medium
34
35diff --git a/debian/patches/series b/debian/patches/series
36index f8a7ef6..d6705ec 100644
37--- a/debian/patches/series
38+++ b/debian/patches/series
39@@ -25,7 +25,10 @@ ubuntu/CVE-2018-20815.patch
40 ubuntu/CVE-2019-5008.patch
41 ubuntu/CVE-2019-9824.patch
42 ubuntu/lp-1830704-s390x-cpumodel-ignore-csske-for-expansion.patch
43+<<<<<<< debian/patches/series
44 ubuntu/lp-1830243-s390-bios-Skip-bootmap-signature-entries.patch
45+=======
46+>>>>>>> debian/patches/series
47 ubuntu/lp1828495-0011-disable-arch-cap-when-no-msr.patch
48 ubuntu/lp1828495-0012-arch-capabilities-migratable.patch
49 ubuntu/lp1828495-0014-remove-cpuid-pconfig.patch

Subscribers

People subscribed via source and target branches