Merge ~rafaeldtinoco/ubuntu/+source/qemu:lp1828495-bionic-devel-qemu into ubuntu/+source/qemu:ubuntu/bionic-devel
- Git
- lp:~rafaeldtinoco/ubuntu/+source/qemu
- lp1828495-bionic-devel-qemu
- Merge into ubuntu/bionic-devel
Status: | Merged |
---|---|
Approved by: | Christian Ehrhardt |
Approved revision: | edd2d3b8ce014a2371adc122136c61f072e23dee |
Merge reported by: | Christian Ehrhardt |
Merged at revision: | e6891e753cdc70f2e265ee08afa91d75314afd5f |
Proposed branch: | ~rafaeldtinoco/ubuntu/+source/qemu:lp1828495-bionic-devel-qemu |
Merge into: | ubuntu/+source/qemu:ubuntu/bionic-devel |
Diff against target: |
101 lines (+56/-0) (has conflicts) 3 files modified
debian/changelog (+18/-0) debian/patches/series (+7/-0) debian/patches/ubuntu/lp1828495-0017-target-i386-add-MDS-NO-feature.patch (+31/-0) Conflict in debian/changelog Conflict in debian/patches/series |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Christian Ehrhardt (community) | Needs Fixing | ||
Canonical Server | Pending | ||
Review via email: mp+368804@code.launchpad.net |
Commit message
I have provided the following PPA:
https:/
And I'll upload the source code so it can be easier tested.
Description of the change
Rafael David Tinoco (rafaeldtinoco) wrote : | # |
Christian Ehrhardt (paelzer) wrote : | # |
The lines in the changelog do not wrap pre 80 chars, the should do so.
This means some of your patch names won't fit in a line.
It is up to you, but I usually "dquilt rename" those to something that fits.
Then you can squash the original and the renaming commit (to not double the commits) and force push after you also adapted the changelog.
I wondered - if you want to keep the patch names - if we might still get away by making the changelog like
...
fixes are in d/p/u/lp1828495-:
- 0001-docs-
bla bla
But e.g. for patch 0003 even that isn't enough. So lets just shorten them accordingly.
Christian Ehrhardt (paelzer) wrote : | # |
Not a general need, but since this is the exploration work to do so in 2.11 (bionic) 2.12 (cosmic) and 3.0 (disco/eoan) it might be rather useful to state in the commit message and maybe as note in the patch-header as well (doesn't have to be in the changelog) which qemu released version a patch was in. That eases the rebase onto 2.12 / 3.0 later on - and since I asked you above to touch all commits anyway that will not be a lot of extra work.
In the patch header [1] that is defined like:
Applied-Upstream: 3.0
And you can copy that line to the commit message as well.
Further it will help us to decide if for Eoan I'll just merge 4.0 and if so if/which patches we need even on top of 4.0.
Christian Ehrhardt (paelzer) wrote : | # |
Patch #1 seems ok, but we will need to make it available to be of any use.
Any qemu prior to 3.1 (there is is already) will need to cherry pick this along patch #1
https:/
Christian Ehrhardt (paelzer) wrote : | # |
Some general patch (header) guidance on the example of patch #1
You have added:
16 Description: docs: add guidance on configuring CPU models for x86
17 Author: Daniel P. Berrangé <email address hidden>
18 Origin: upstream, https:/
19 Bug-Ubuntu: https:/
20 Reviewed-by: Rafael David Tinoco <email address hidden>
21 Last-Update: 2019-06-13
And don't get me wrong, it is great to have that. But as usual the devil is in the details.
Description: docs: add guidance on configuring CPU models for x86
This line is only needed for new patches, since you start with soemthing like a git format-patch you don't need to add it "again" here.
Origin: upstream, https:/
When you have taken it as-is then this is correct.
But if you had to modify the patch so that it applies then the line would be:
Origin: backport, https:/
For "full" compliance you'd in the case of a backport also change the Author to become Original-Author and add yourself as the (latest) Author. Like:
15 Author: Rafael David Tinoco <email address hidden>
16 Original-Author: Daniel P. Berrangé <email address hidden>
The other details are slightly different than I'd do it but nothing with a need to change that.
And please don't get me wrong with all the comments - things look great!
Christian Ehrhardt (paelzer) wrote : | # |
#2 ok
Christian Ehrhardt (paelzer) wrote : | # |
gladly we have debian/
That makes #3 apply as it is :-)
I was double checking to be sure as the headers are different
static FeatureWordInfo feature_
static FeatureWordInfo feature_
Ok things are save. The security patch is the one being wrong as it seems from a pre-upstream patch. Both are fine and apply correctly.
#3 ok
Christian Ehrhardt (paelzer) wrote : | # |
The two line comment in #4 look odd - but it is as upstream committed it.
#4 ok
Christian Ehrhardt (paelzer) wrote : | # |
#5
You changed your patch style here.
Drop the first three lines in patches like this please
git format-patch headers are fine in general IF it is the export of the original.
Here it is the export of your patch from a backport branch of yours.
Due to that the hash and From: header are misleading - hence the request to remove.
Further I assume at least this would be a case for "Origin: backport..." I guess.
For a moment I was scared about impacts to phys-bits, but they kept all as is.
I see CPUID_7_
But I also see a drop of CPUID_7_
That change seems from bundling the later fixup in upstream 76e5a4d58357b9d
I wonder if instead of the current patch #5 we can't just make it three patches based on upstream
8a11c62da914
76e5a4d58357b9d
4c257911dcc7c41
The latter two do the removal of CPUID_7_
Using those three from upstream should be more clean than backporting things our own - opinions?
If cleanup is needed do it after those three.
For example apply those three as three individual patches and IF NEEDED then we do a custom patch not based on upstream and expplain there what/why we clean up.
Christian Ehrhardt (paelzer) wrote : | # |
#6 please take that patch as-is from upstream
Add an extra patch before it with the full content of upstreams 9f2d175db5c29b2
In general keeping upstream patches as-is is preferred when applicable.
We have done so before for the same case, see
debian/
debian/
Christian Ehrhardt (paelzer) wrote : | # |
Further #6 has also upstreams f57bceb6ab5163d
Again please make #6 to just be 3fc7c73139d2 as it is called that way in the patch headers.
Then add f57bceb6ab5163d
Christian Ehrhardt (paelzer) wrote : | # |
#7 seems to have whitespace damage.
E.g.
+ CPUID_FEATURE_WORD,
+ MSR_FEATURE_WORD,
Are indented "one space further" then in the upstream commit.
Similar issue further down as well.
None of it is functional, but what happened here?
I haven't found a functional change, but there was a lot of noise.
Please just double check this one to be sure.
You mention that you had to exchange accel_uses_
First there is an accident and the line
feat_word_str = feature_
got duplicated which is a bug - please fix that along clarifying where the whitespace came from.
And as mentioned before we also need to port this to 2.12/3.0.
If you could apply the upstream patch 075859234859 as-is first, and then add another patch that will change accel_uses_
That way we have to benefits:
1. 075859234859 should look exactly as upstream (better to verify on review, and to maintain later)
2. for Cosmic/Disco we can just drop the extra commit doing the cleanup.
I think this is a great case why I prefer to do the backport-adaptions in extra patches in not all but many cases.
Christian Ehrhardt (paelzer) wrote : | # |
#8 - ok
Christian Ehrhardt (paelzer) wrote : | # |
#9 - ok
Christian Ehrhardt (paelzer) wrote : | # |
#10 - ok
Christian Ehrhardt (paelzer) wrote : | # |
#11 - ok
Christian Ehrhardt (paelzer) wrote : | # |
#12 - has again dropped CPUID_7_
But it wasn't mentioned in the patch description.
Here - as in the other patch dropping CPUID_7_
And then add an extra cleanup patch (in an extra commit) dropping CPUID_7_
Again having this in an extra commit/patch will allow to more easily port it to Cosmic/Disco/Eoan.
In fact the "4c257911dcc7c4
That meand #5 and #12 could become four patches:
8a11c62da914
c7a88b52f62
76e5a4d58357b9d
4c257911dcc7c41
And those would not even need any special 2.12 dependent cleanup.
Christian Ehrhardt (paelzer) wrote : | # |
#13 - this only changes code that is present and active post qemu 3.1.
It does set compat of 3.1 to off (which is wrong, needs to be on I think) and the named types.
I know this is a uncommon and deprecated feature, but we don't even have to touch it.
This should be a no-op at best for 2.11-3.1 and worst case a bug.
Please drop this patch completely for this backport effort.
Christian Ehrhardt (paelzer) wrote : | # |
#14 - ok
Christian Ehrhardt (paelzer) wrote : | # |
#15 - ok
This one could have been dangerous, as I don't want to introduce
bug 1825195 with the SRU. But isn't related to the actual support drop [1][2] which is in qemu 3.1
Instead it only does change the default bits of cascade lake. Which on the new qemu would trigger the bug mentioned in the commit and in old qemu would not be detected.
For migratability of the guests I think it is ok to keep #15 applied as is.
[1]: https:/
[2]: https:/
Christian Ehrhardt (paelzer) wrote : | # |
E165: Cannot go beyond last file
Ok, it seems I'm done for now.
Sorry for so much requests to change, but I think it overall is already good.
Yet doing as I requested will fix some remaining issues and make the porting to other Ubuntu Releases easier. So I hope you can forgive me :-/
I'd recommend you to keep this branch as reference - otherwise my comments which refer to patch numbers might be hard to track.
I saved it as lp1828495-
Please feel free to discuss if anything is unclear and ping me when I should re-review this.
Rafael David Tinoco (rafaeldtinoco) wrote : | # |
Christian,
I believe I have address everything you mentioned. Only caveat would be:
(A)
- 0005-new-
i386: Add new CPU model Icelake-
(upstream: 8a11c62da914, desc: v3.0.0-
1) patch header (d/p/u/*.patch) is wrong (local git repo header)
2) DEP3 is backport if it applies
3) description was re-wrapped 1) and 2) items
4) CPUID_7_
5) CPUID_7_
upstream patches:
4c257911dcc7 - i386: remove the 'INTEL_PT' CPUID bit from
76e5a4d58357 - i386: remove the new CPUID 'PCONFIG'...
to "backport" (cleanup) only if needed after those 2
From patch 0005 and beyond, you can't cleanly compile the source package because missing INTEL_PT and PCONFIG options removal, done together with:
- 0013-cascadelak
i386: Add new model of Cascadelake-Server
(upstream: c7a88b52f62, desc: v3.0.0-
* - 0014-remove-
i386: remove the new CPUID 'PCONFIG' from Icelake-Server CPU model
(upstream: 76e5a4d58357, desc: v3.1.0-
* - 0015-remove-
i386: remove the 'INTEL_PT' CPUID bit from named CPU models
(upstream: 4c257911dcc7, desc: v3.1.0-
like you suggested. Code is cleaner and better to be supported this way. Because we are not backporting MPX disablement, I had to make some adjustments in patch 0015
(B)
NOW, regarding MSX disablement... I agree with you! Just wanted to document that:
IF someone is running QEMU on Bionic, in a {Sky,Cascade}Lake CPU, has MPX on by default, a possibly different (than Ubuntu) guest OS (other Linux distro ?) AND is running a object containing instructions of that ABI extension (GCC in between 5 and 9 compiled objects and kernel 3.19 - 4.20) WHILE doing a live migration from this QEMU version to higher one, then.. this person would face a bug :o).
PS: I know its a huge corner case, just wanted to document.
Rafael David Tinoco (rafaeldtinoco) wrote : | # |
I have also just uploaded the source package to:
https:/
So in a few min you might have a compiled version of this MR.
Christian Ehrhardt (paelzer) wrote : | # |
Changelog:
- mention the bug "once" not on each patch line. Not only not necesary, some tools get driven
nuts by that.
- the upstream references are great to have in the patch header, but somewhat overdose in the
changelog
Example:
3 * {Ice,Cascade}Lake CPUs + IA32_ARCH_
4 Needed patches are in d/p/u/lp1828495-:
5 - 0001-guidance-
6 docs: add guidance on configuring CPU models for x86
7 (upstream: 2544e9e4aa2b, desc: v3.0.0-
8 - 0002-msr-
Better:
3 * {Ice,Cascade}Lake CPUs + IA32_ARCH_
(LP: #1828495)
4 Needed patches are in d/p/u/lp1828495-:
5 - 0001-guidance-
6 docs: add guidance on configuring CPU models for x86
8 - 0002-msr-
Could you fix the above across all of your new entry in debian/changelog please?
#1 header seems right now - thanks
- yet I have not seen this in the commits
https:/
#2 - #4 - ok
#5 I see you now left it as-is in favor of applying the real upstream change later on in the new #14/#15 - thanks.
#6 - ok - full header is closer to what upstream has, I found no backport issue, header good now
#7 - ok
#8 - I need to double check on that backport.
Nothing found on the first pass, maybe checking again later ..
#9 - #12 - ok
#13 - ok for itself.
I double checked if we need https:/
#14 - ok
I'm somewhat torn on how to best handle
b0a19803 i386: Update stepping of Cascadelake-Server
-> we certainly have no pc_compat_3_1
-> but what is the better fallback than just not adding it?
ecb85fe48cacb2f
-> also setting pc_compat_3_1
-> should we add that in a modified way and if so how to best do so?
=> Both changes to pc_compat_3_1 mean that on anything 3.1 and early MPX is on
So if you run (even on a newer qemu) such a type and get it from an earlier machine then it would e.g. have MPX on even it was dropped.
I think the right way to do this is to add all these changes that were done to pc_compat_3_1 in the upstream patches to the respective newest types of the release.
Because we change the code with the patches, and that means we'd otherwise change behavior.
Only >3.1 is it ok to really have MPX off by default.
That would mean we move those changes to
Bionic - pc_compat_2_11
Cosmic - pc_compat_2_12
Disco - we keep 3.1 as that is Disco atm.
That would ensure that the type doesn't change until a release post >=4.x
And even on a migration between those the old Bionic qemu would have MPX off (a 2.11 type) and the receiving qemu would know that (it knows all <4.0 are that way).
TODO:
- add ecb85fe48cacb...
Christian Ehrhardt (paelzer) wrote : | # |
after discussion - do NOT - add ecb85fe48cacb2f
Everything else as written above
Rafael David Tinoco (rafaeldtinoco) wrote : | # |
> => Both changes to pc_compat_3_1 mean that on anything 3.1 and early MPX is on
> So if you run (even on a newer qemu) such a type and get it from an earlier
> machine then it would e.g. have MPX on even it was dropped.
> I think the right way to do this is to add all these changes that were done to
> pc_compat_3_1 in the upstream patches to the respective newest types of the
> release.
> Because we change the code with the patches, and that means we'd otherwise
> change behavior.
> Only >3.1 is it ok to really have MPX off by default.
>
> That would mean we move those changes to
> Bionic - pc_compat_2_11
> Cosmic - pc_compat_2_12
> Disco - we keep 3.1 as that is Disco atm.
>
> That would ensure that the type doesn't change until a release post >=4.x
> And even on a migration between those the old Bionic qemu would have MPX off
> (a 2.11 type) and the receiving qemu would know that (it knows all <4.0 are
> that way).
>
> TODO:
> - add ecb85fe48cacb2f
> this
> - do not drop the changes to pc_compat_3_1 but instead move it to the most
> recent compat of the qemu versions used
> - 4c257911dcc7 / #15
> - b0a1980384fc / #16
Alright, just a quick observation.. since PC_COMPAT_2_11 doesn't exist yet, the biggest COMPAT definition being used in 2.11 is "2.10", we should use it, instead of using "2.11", no ? Because defining a PC_COMPAT_2_11 in Bionic's 2.11 won't have any effect without changes to the 2 functions bellow. Changing PC_COMPAT_2_10 would mean that ANYTHING before our 2.11 has mpx on, right ?
This can be seen in:
static void pc_i440fx_
and
static void pc_q35_
as
SET_
So, if we are saying that "mlx" is "off" from 2.11 release and beyond, with included patch ecb85fe48cac, then it should be "on" for the previous releases (2.10 and before), declared in Bionic, Cosmic and Disco as:
BIONIC: PC_COMPAT_2_10: mpx = on.
COSMIC: PC_COMPAT_2_10: mpx = on.
DISCO: PC_COMPAT_2_10: mpx = on.
Is that so ? Could you confirm ? I'm preparing the patches like this but I can change later if needed.
POSSIBLE SCENARIOS
- START VM in Bionic (2.11), mpx=off by default.
- MIGRATE VM to Cosmic (2.12) mpx=off by default.
- START VM in Artful (2.10) or before, mpx=on by default.
- MIGRATE VM to Bionic. PC_COMPAT_2_10 takes a play here. mpx stays off.
- START VM in Cosmic (2.12), mpx=off by default.
- MIGRATE VM to Artful (2.10) or before <- can't do.
Rafael David Tinoco (rafaeldtinoco) wrote : | # |
Christian,
I have to fix the following error:
----
patching file debian/
Reversed (or previously applied) patch detected! Skipping patch.
1 out of 1 hunk ignored
dpkg-source: info: the patch has fuzz which is not allowed, or is malformed
dpkg-source: info: if patch 'ubuntu/
dpkg-source: error: LC_ALL=C patch -t -F 0 -N -p1 -u -V never -E -b -B .pc/ubuntu/
dpkg-buildpackage: error: dpkg-source -b qemu subprocess returned exit status 2
debuild: fatal error at line 1152:
dpkg-buildpackage -rfakeroot -us -uc -ui -S failed
----
It seems that this happens only when trying to generate the source package, quilt push -a works, compilation works, its all good but this minor error. I had this before and re-generated the patch but, for this case, It seems not to be working, I might have to drop 0000 and add it at the end, before changelog, are you okay with that ?
Best,
Rafael
Christian Ehrhardt (paelzer) wrote : | # |
Hi,
yes the HW compat is complex.
Essentially we have:
- the code (which our patches change) and that usually represents the "current" types.
- and we have the Compat structs which are applied to older types to represent the difference of older types from the current code
- our case is special, since we do backports we want even the current code to behave "like it did".
I think we could as well consider doing that:
- for anything that makes a change to _compat_ structs (representing a change in the defaults) we will modify said default instead
- as an example if post 3.1 it did
feat = FOO_ON; // new code with new default added
compat_
Then we want to take the functional code change but with the old default:
feat = FOO_OFF; // new code, but keeping the old default setting
Would that approach work for the patches in your queue that change defaults?
Christian Ehrhardt (paelzer) wrote : | # |
For the other issue, I tihnk the size of the patch stack let you forgot that there is more than d/p/* :-)
Any changes to debian* are just commits.
The quilt stack in debian/patches is only needed for changes outside the debian/* directory to represent changes that are to be applied after the tarball is unpacked.
Due to that
- 0000-add-
has to be removed. Instead it should really just be a cherry pick, not converted to a quilt patch.
Rafael David Tinoco (rafaeldtinoco) wrote : | # |
On Tue, Jun 18, 2019, 02:27 Christian Ehrhardt ui<
<email address hidden>> wrote:
> For the other issue, I tihnk the size of the patch stack let you forgot
> that there is more than d/p/* :-)
>
> Any changes to debian* are just commits.
> The quilt stack in debian/patches is only needed for changes outside the
> debian/* directory to represent changes that are to be applied after the
> tarball is unpacked.
>
of course, let's pretend I never did this :)
- 350e6a9... by Rafael David Tinoco
-
- 0001-guidance-
cpu-models. patch (LP: #1828495):
docs: add guidance on configuring CPU models for x86
(upstream: 2544e9e4aa2b, desc: v3.0.0-151-g2544e9e4aa ) + d/qemu-
system- common. install: include man/man7/ qemu-cpu- models. 7
(debian: debian/qemu_2. 12+dfsg- 3-4740- g2a17895ee2) - 46b6eff... by Rafael David Tinoco
-
- 0002-msr-
new-msr- indices. patch (LP: #1828495):
i386: Add new MSR indices for IA32_PRED_CMD and IA32_ARCH_CAPABILITIES
(upstream: 8c80c99fccea, desc: v3.0.0-152-g8c80c99fcc ) - aa0985d... by Rafael David Tinoco
-
- 0003-cpuid-
feature- ia32-arch- capabilities. patch (LP: #1828495):
i386: Add CPUID bit and feature words for IA32_ARCH_CAPABILITIES MSR
(upstream: 3fc7c73139d2, desc: v3.0.0-153-g3fc7c73139 ) - 326d8a4... by Rafael David Tinoco
-
- 0004-cpuid-
bit-for- wbnoinvd. patch (LP: #1828495):
i386: Add CPUID bit for WBNOINVD
(upstream: 59a80a19ca31, desc: v3.0.0-155-g59a80a19ca ) - 53f1116... by Rafael David Tinoco
-
- 0005-new-
cpu-model- for-icelake. patch (LP: #1828495):
i386: Add new CPU model Icelake-{Server, Client}
(upstream: 8a11c62da914, desc: v3.0.0-156-g8a11c62da9 ) - 60e0dcf... by Rafael David Tinoco
-
- 0006-update-
headers- to-4.16- rc5.patch (LP: #1828495)
update Linux headers to 4.16-rc5
(upstream: 9f2d175db5c2, desc: v2.11.0-2283-g9f2d175db 5) - 4de280a... by Rafael David Tinoco
-
- 0007-kvm-
get-msr- feature- index_list. patch (LP: #1828495):
kvm: Add support to KVM_GET_MSR_FEATURE_ INDEX_LIST and
(upstream: f57bceb6ab51, desc: v3.0.0-1659-gf57bceb6a b) - 566f3ba... by Rafael David Tinoco
-
- 0008-x86-
msr-related- data-structure- changes. patch (LP: #1828495):
x86: Data structure changes to support MSR based features
(upstream: 075859234859, desc: v3.0.0-1660-g075859234 8) - 5add821... by Rafael David Tinoco
-
- 0009-feature-
wordS-arch- capabilities. patch (LP: #1828495):
x86: define a new MSR based feature word -- FEATURE_WORDS_ARCH
(upstream: d86f963694df, desc: v3.0.0-1661-gd86f96369 4) - 2e57f48... by Rafael David Tinoco
-
- 0010-use-
kvm-get- msr-index- list.patch (LP: #1828495):
kvm: Use KVM_GET_MSR_INDEX_ LIST for MSR_IA32_ ARCH_CAPABILITI ES support
(upstream: aec5e9c3a94c, desc: v3.1.0-rc2-32- gaec5e9c3a9) - 89184a1... by Rafael David Tinoco
-
- 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-g485b1d25 6b) - 474c8a6... by Rafael David Tinoco
-
- 0012-arch-
capabilities- migratable. patch (LP: #1828495):
i386: Make arch_capabilities migratable
(upstream: 014018e19b3c, desc: v4.0.0-rc0-2-g014018e1 9b) - a093463... by Rafael David Tinoco
-
- 0013-cascadelak
e-server. patch (LP: #1828495):
i386: Add new model of Cascadelake-Server
(upstream: c7a88b52f62, desc: v3.0.0-1662-gc7a88b52f 6) - 9dbfb9e... by Rafael David Tinoco
-
- 0014-remove-
cpuid-pconfig. patch
i386: remove the new CPUID 'PCONFIG' from Icelake-Server CPU model
(upstream: 76e5a4d58357, desc: v3.1.0-1495-g76e5a4d58 3)
Rafael David Tinoco (rafaeldtinoco) wrote : | # |
# DOCUMENTING IRC DISCUSSION:
08:54 <rafaeldtinoco> cpaelzer: with your suggestion, we would have to up PC_COMPAT every release by putting the same value in the new one
08:54 <rafaeldtinoco> no ?
08:56 <cpaelzer> no
08:57 <cpaelzer> we'd change the default that gets added
08:57 <cpaelzer> since out active code is "before" compat 3.1
08:57 <rafaeldtinoco> cpaelzer: alright, lets do one example together
08:58 <rafaeldtinoco> just to make sure we covered it all
08:58 <cpaelzer> may I take the easiest one :-)
08:58 <cpaelzer> :-P
08:58 <cpaelzer> b0a1980384fc265
08:58 <cpaelzer> the upstream cahnge will make it be stepping 6 AFTER 3.1
08:58 <cpaelzer> since we are before we will NOT change it to 6
08:58 <cpaelzer> which means we will drop the patch completely
08:59 <rafaeldtinoco> but cascadelake wasnt even supported in 2_10/2_11
08:59 <rafaeldtinoco> thats the point of backporting its support
08:59 <rafaeldtinoco> and without stepping
08:59 <rafaeldtinoco> the features arent even supported
08:59 <rafaeldtinoco> (the ones we are adding)
08:59 <cpaelzer> sure, but it still defines it like that
08:59 <cpaelzer> so a qemu 4.0 would expect an old Cascadelake-Server to be stepping 5
08:59 <cpaelzer> and that is what we will make it by -not- adding this patch
08:59 <rafaeldtinoco> cpaelzer: without IA32 feature
08:59 <rafaeldtinoco> but we will have it
09:00 <rafaeldtinoco> if you dont bump the stepping
09:00 <rafaeldtinoco> feature is not enabled (for cascade model)
09:00 <rafaeldtinoco> the extra mitigation tags
09:00 <rafaeldtinoco> (ARCH_CAPABILIT
09:01 <rafaeldtinoco> that is what tricked me ^
09:01 <cpaelzer> just now read the spec on that stepping
09:01 <cpaelzer> yeah this is silly
09:01 <cpaelzer> we need a special solution for the stepping then
09:01 <cpaelzer> which is ok
09:02 <cpaelzer> ok, since the type never existed lets add it with stepping 6 into all our qemu's
09:02 <cpaelzer> what we need to do then is when I merge qemu 4.0 or later (and essentially forever)
09:02 <cpaelzer> to REMOVE the line that declares pre 3.1 cascade lake as stepping 5
09:03 <cpaelzer> rafaeldtinoco: I have added a todo to the trello card for the virt-stack merges
09:05 <cpaelzer> hw/i386/pc.cso for the SRU-backports of b0a19803 we'd want the change to target/i386/cpu.c but not the one to hw/i386/pc.c
09:06 <cpaelzer> you'd need to slightly rework patch #17 for that
09:07 <cpaelzer> keeping the technical dept (in the form of delta to be kept forever) low, can we just not add ecb85fe48cacb2f
09:07 <rafaeldtinoco> cpaelzer: perfect. that is much better
09:07 <cpaelzer> at lesat that seems to follow my thought
09:08 <cpaelzer> by not adding #15 we will NOT switch MPX off
09:08 <cpaelzer> and that means it will still be on
09:08 <rafaeldtinoco> cpaelzer: it makes sense
09:08 <cpaelzer> qemu >=4.0 will consider anything <=3.1 to have MPX on
09:08 <rafaeldtinoco> i thought similar (dragging the change for upper versions)
09:08 <rafaeldtinoco> but not having it and removing the delta
09:08 <rafaeldtinoco> is better to maintain
09:08 <rafaeldtinoco> alright, what about mpx ?
09:08 ...
- da8eec6... by Rafael David Tinoco
-
- 0015-remove-
cpuid-intel_ pt.patch
i386: remove the 'INTEL_PT' CPUID bit from named CPU models
(upstream: 4c257911dcc7, desc: v3.1.0-1496-g4c257911d c)
Rafael David Tinoco (rafaeldtinoco) wrote : | # |
Christian,
Sorry for the small delay here, catching up with new hire tasks and stuff. So, for this one I have pushed a new merge request, forcing a push, and I have uploaded a similar pkg source (differing in changelog only) into the following PPA:
https:/
I'm gonna do some tests now and provide feedback here, and will have a double check if I have covered all that we agreed on.
Thanks a lot for the review.
Rafael David Tinoco (rafaeldtinoco) wrote : | # |
Posted some final comments in the code showing what we agreed on.
Tks again
Rafael David Tinoco (rafaeldtinoco) wrote : | # |
$ sudo lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description: Ubuntu 18.04.2 LTS
Release: 18.04
Codename: bionic
$ dpkg -l | grep -i qemu-kvm
ii qemu-kvm 1:2.11+
$ uname -a
Linux lockheed 4.18.0-23-generic #24~18.04.1-Ubuntu SMP Thu Jun 13 17:08:52 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
----
Alright, playing with a machine supporting CascadeLake I got:
$ sudo /usr/bin/
stdio
[ 0.000000] Linux version 4.18.0-23-generic (buildd@
[ 0.000000] Command line: root=/dev/vda noresume console=tty0 console=
[ 0.000000] KERNEL supported cpus:
[ 0.000000] Intel GenuineIntel
[ 0.000000] AMD AuthenticAMD
[ 0.000000] Centaur CentaurHauls
[ 0.000000] x86/fpu: Supporting XSAVE feature 0x001: 'x87 floating point registers'
and no apparent errors. /proc/cpuinfo inside guest shows:
----
inaddy@guest:~$ cat /proc/cpuinfo
processor : 0
vendor_id : GenuineIntel
cpu family : 6
model : 85
model name : Intel Xeon Processor (Cascadelake)
stepping : 6
microcode : 0x1
cpu MHz : 2095.076
cache size : 16384 KB
physical id : 0
siblings : 1
core id : 0
cpu cores : 1
apicid : 0
initial apicid : 0
fpu : yes
fpu_exception : yes
cpuid level : 13
wp : yes
flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush mmx fxsr sse sse2 syscall nx pdpe1gb rdtscp lm constant_tsc rep_good nopl xtopology cpuid tsc_known_freq pni pclmulqdq ssse3 fma cx16 pcid sse4_1 sse4_2 x2apic movbe popcnt tsc_deadline_timer aes xsave avx f16c rdrand hypervisor lahf_lm abm 3dnowprefetch cpuid_fault invpcid_single pti ssbd ibrs ibpb fsgsbase bmi1 hle avx2 smep bmi2 erms invpcid rtm mpx avx512f avx512dq rdseed adx smap clflushopt clwb avx512cd avx512bw avx512vl xsaveopt xsavec xgetbv1 arat pku ospke avx512_vnni
bugs : cpu_meltdown spectre_v1 s...
Rafael David Tinoco (rafaeldtinoco) wrote : | # |
Alright, so... adding:
-cpu Cascadelake-
as cmdline arg in the new QEMU version
gave me the "arch_capabilities" the cpuinfo file:
----
inaddy@guest:~$ cat /proc/cpuinfo
processor : 0
vendor_id : GenuineIntel
cpu family : 6
model : 85
model name : Intel Xeon Processor (Cascadelake)
stepping : 6
microcode : 0x1
cpu MHz : 2095.076
cache size : 16384 KB
physical id : 0
siblings : 1
core id : 0
cpu cores : 1
apicid : 0
initial apicid : 0
fpu : yes
fpu_exception : yes
cpuid level : 13
wp : yes
flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush mmx fxsr sse sse2 syscall nx pdpe1gb rdtscp lm constant_tsc rep_good nopl xtopology cpuid tsc_known_freq pni pclmulqdq ssse3 fma cx16 pcid sse4_1 sse4_2 x2apic movbe popcnt tsc_deadline_timer aes xsave avx f16c rdrand hypervisor lahf_lm abm 3dnowprefetch cpuid_fault invpcid_single pti ssbd ibrs ibpb fsgsbase bmi1 hle avx2 smep bmi2 erms invpcid rtm mpx avx512f avx512dq rdseed adx smap clflushopt clwb avx512cd avx512bw avx512vl xsaveopt xsavec xgetbv1 arat pku ospke avx512_vnni arch_capabilities
bugs : cpu_meltdown spectre_v1 spectre_v2 spec_store_bypass l1tf mds
bogomips : 4190.15
clflush size : 64
cache_alignment : 64
address sizes : 40 bits physical, 48 bits virtual
power management:
----
I'll finish tests Friday morning but I guess this is a good to go for the kernel team to start the backport and for us to continue our tests.
Christian Ehrhardt (paelzer) wrote : | # |
[...]
> > +++ b/debian/
> > @@ -0,0 +1,53 @@
> > +i386: Update stepping of Cascadelake-Server
>
> This is the #17 re-work. Most important part is to remember that we will have to set stepping as 6 until our released QEMU version does not contain the upstream change (4.0 merge). After that we will have to set COMPAT for cascadelake CPU since 2_11, settings its stepping to 6 since that version.
Almost :-)
No change to 2_11, as this is done indirectly.
qemu 4.0 will (by upstream) have code telling it that any cascade-lake
prior to 4.0 will be stepping 5 in the pc_compat_3.1.
This line will have to be dropped as "our" cascade-lake will have been
stepping 6 from the beginning.
But that is bokeshedding on details, for this merge it LGTM now - thanks.
The only thing is the text in the header to not be misleading, so the
snipped below needs to be adapted.
> > +
> > + * Future Ubuntu merges, containing the upstream backported patch,
> > + will have to modify PC_COMPAT_3_1 and HW_COMPAT_3_1 and say
> > + stepping for the CascadeLake CPU was available since
> > + PC_COMPAT_2_11 and HW_COMPAT_2_11, so compatibility is
> > + guaranteed in migrations between hosts containing different
> > + QEMU versions.
Rafael David Tinoco (rafaeldtinoco) wrote : | # |
Christian,
https:/
HOST MITIGATION FEATURES REPORT:
https:/
OLD QEMU GUEST MIT FEATURES REPORT:
https:/
NEW QEMU GUEST MIT FEATURES REPORT:
https:/
MIT FEATURES REPORT DELTA FROM OLD TO NEW:
https:/
Meaning we basically have enabled INSIDE the GUEST:
* Hardware support (CPU microcode) for mitigation techniques
* Enhanced IBRS (IBRS_ALL) -> ENABLED
* CPU indicates ARCH_CAPABILITIES MSR availability: YES
* ARCH_CAPABILITIES MSR advertises IBRS_ALL capability: YES
* CPU explicitly indicates not being vulnerable to Meltdown/L1TF (RDCL_NO): YES -> ENABLED
* CPU/Hypervisor indicates L1D flushing is not necessary on this system: NO -> ENABLED
and
* CPU vulnerability to the speculative execution attack variants
* Vulnerable to CVE-2017-5754 (Variant 3, Meltdown, rogue data cache load): NO -> ENABLED
* Vulnerable to CVE-2018-3620 (Foreshadow-NG (OS), L1 terminal fault): NO -> ENABLED
* Vulnerable to CVE-2018-3646 (Foreshadow-NG (VMM), L1 terminal fault): NO -> ENABLED
Rafael David Tinoco (rafaeldtinoco) wrote : | # |
Kernel support cherry-
Christian Ehrhardt (paelzer) wrote : | # |
This LGTM, approved +1
Could you next provide MPs for releases C/D/E then we could fuse them with the fixes I have for bug 1832622 for a general regression test?
Also a review on those branches for 1832622 would be nice, you'll find them linked on the bug.
Christian Ehrhardt (paelzer) wrote : | # |
with known constraints +1 - rafel is working on tests and also backports for other releases
- 0fd61f5... by Rafael David Tinoco
-
- 0016-no-
ospke-on- some.patch (LP: #1828495):
i386: Disable OSPKE on CPU model definitions
(upstream: bb4928c7cafe, desc: v4.0.0-rc0-3-gbb4928c7 ca)
Rafael David Tinoco (rafaeldtinoco) wrote : | # |
Okay, I have removed patch changing CPU stepping for CascadeLake CPU types, according to the summary of our discussion that can be read here:
https:/
Could you please review this small change and check if we're good ?
Tks!
Christian Ehrhardt (paelzer) wrote : | # |
Yes I see:
++ .stepping = 5,
in
b/debian/
matching upstream - ok
headers listed as upstream now - ok
This should resolve our upgrade issue in Disco that we were concerned of.
Re-Approved here and dropped the related Delta that I added along our former plan to the qemu 4.0 merge.
Christian Ehrhardt (paelzer) wrote : | # |
FYI - I was merging this with other planned SRUs for testing.
While doing so I found that you by accident committed a change to debian/files.
That is an artifact of buildpkg and is never to be committed.
- c27fa94... by Rafael David Tinoco
-
- 0017-target-
i386-add- MDS-NO- feature. patch (LP: #1828495):
target/i386: add MDS-NO feature
(upstream: 20140a82c674, desc: v4.0.0-721-g20140a82c6 )
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.
@Christian,
This is the Bionic change, which is still in -proposed. Do you want 2 SRUs or a single one with this push ? I'll push this to the PPA also, to ask feedback from Intel.
Rafael David Tinoco (rafaeldtinoco) wrote : | # |
This is the upstream change:
https:/
This is the public comment from Intel:
https:/
And commit the patch is:
- 0017-target-
target/i386: add MDS-NO feature
(upstream: 20140a82c674, desc: v4.0.0-
Rafael David Tinoco (rafaeldtinoco) wrote : | # |
I have added a single patch to the end of this patchset, but this is already uploaded to -proposed and we were in verification time. Do you want me to do this fix on top of what we have in -proposed, this way we can raise the version and migrate, when it comes the time, altogether ? What do you prefer ?
Christian Ehrhardt (paelzer) wrote : | # |
This will be in the same SRU.
So we will:
- not mark it verified
- we will take the addition and add it as a new qemu version upload
- the new changelog will only mention, but not reference the bug again like
"Add missing md-no bit in arch_capabilities (LP: 1828495)"
Mind the intentionally missing #
- upload a new version to -unapproved built with dpkg-buildpackage -v <old released version>
- ping the SRU Team to please replace the one in -proposed with the new one by accepting the new
one.
So on your change for now "needs fixing" I want the old branch as you had it plus two new commits.
1. patch 17 (you have that already)
2. new changelog entries as 1:2.11+
Rafael David Tinoco (rafaeldtinoco) wrote : | # |
Alright, working on it
- 3ff65af... by Rafael David Tinoco
-
changelog
- e6891e7... by Rafael David Tinoco
-
changelog
Rafael David Tinoco (rafaeldtinoco) wrote : | # |
Done. Uploaded the PPA package as well.
Christian Ehrhardt (paelzer) wrote : | # |
Man this MP has grown a lot :-)
Ack to the fixup changes.
For proper importing and tagging I'll need it on top of import/
Tag pushed and sponsored with -v1:2.11+
Preview Diff
1 | diff --git a/debian/changelog b/debian/changelog | |||
2 | index e4fd2cf..50c1ae2 100644 | |||
3 | --- a/debian/changelog | |||
4 | +++ b/debian/changelog | |||
5 | @@ -1,3 +1,4 @@ | |||
6 | 1 | <<<<<<< debian/changelog | ||
7 | 1 | qemu (1:2.11+dfsg-1ubuntu7.16) bionic; urgency=medium | 2 | qemu (1:2.11+dfsg-1ubuntu7.16) bionic; urgency=medium |
8 | 2 | 3 | ||
9 | 3 | [ Christian Ehrhardt ] | 4 | [ Christian Ehrhardt ] |
10 | @@ -5,6 +6,19 @@ qemu (1:2.11+dfsg-1ubuntu7.16) bionic; urgency=medium | |||
11 | 5 | tolerate guests with secure boot loaders (LP: #1830243) | 6 | tolerate guests with secure boot loaders (LP: #1830243) |
12 | 6 | 7 | ||
13 | 7 | [ Rafael David Tinoco ] | 8 | [ Rafael David Tinoco ] |
14 | 9 | ======= | ||
15 | 10 | qemu (1:2.11+dfsg-1ubuntu7.17) bionic; urgency=medium | ||
16 | 11 | |||
17 | 12 | * {Ice,Cascade}Lake IA32_ARCH_CAPABILITIES support (LP: 1828495) | ||
18 | 13 | Needed patch is in d/p/u/lp1828495-: | ||
19 | 14 | - 0017-target-i386-add-MDS-NO-feature.patch: | ||
20 | 15 | target/i386: add MDS-NO feature | ||
21 | 16 | |||
22 | 17 | -- Rafael David Tinoco <rafaeldtinoco@ubuntu.com> Mon, 05 Aug 2019 19:12:08 +0000 | ||
23 | 18 | |||
24 | 19 | qemu (1:2.11+dfsg-1ubuntu7.16) bionic; urgency=medium | ||
25 | 20 | |||
26 | 21 | >>>>>>> debian/changelog | ||
27 | 8 | * {Ice,Cascade}Lake CPUs + IA32_ARCH_CAPABILITIES support (LP: #1828495) | 22 | * {Ice,Cascade}Lake CPUs + IA32_ARCH_CAPABILITIES support (LP: #1828495) |
28 | 9 | Needed patches are in d/p/u/lp1828495-: | 23 | Needed patches are in d/p/u/lp1828495-: |
29 | 10 | - 0001-guidance-cpu-models.patch: | 24 | - 0001-guidance-cpu-models.patch: |
30 | @@ -41,7 +55,11 @@ qemu (1:2.11+dfsg-1ubuntu7.16) bionic; urgency=medium | |||
31 | 41 | - 0016-no-ospke-on-some.patch: | 55 | - 0016-no-ospke-on-some.patch: |
32 | 42 | i386: Disable OSPKE on CPU model definitions | 56 | i386: Disable OSPKE on CPU model definitions |
33 | 43 | 57 | ||
34 | 58 | <<<<<<< debian/changelog | ||
35 | 44 | -- Christian Ehrhardt <christian.ehrhardt@canonical.com> Thu, 04 Jul 2019 14:47:56 +0200 | 59 | -- Christian Ehrhardt <christian.ehrhardt@canonical.com> Thu, 04 Jul 2019 14:47:56 +0200 |
36 | 60 | ======= | ||
37 | 61 | -- Rafael David Tinoco <rafaeldtinoco@ubuntu.com> Wed, 19 Jun 2019 19:48:48 +0000 | ||
38 | 62 | >>>>>>> debian/changelog | ||
39 | 45 | 63 | ||
40 | 46 | qemu (1:2.11+dfsg-1ubuntu7.15) bionic; urgency=medium | 64 | qemu (1:2.11+dfsg-1ubuntu7.15) bionic; urgency=medium |
41 | 47 | 65 | ||
42 | diff --git a/debian/patches/series b/debian/patches/series | |||
43 | index 9fac84c..457769c 100644 | |||
44 | --- a/debian/patches/series | |||
45 | +++ b/debian/patches/series | |||
46 | @@ -87,7 +87,10 @@ ubuntu/enable-md-clear.patch | |||
47 | 87 | CVE-2018-20815.patch | 87 | CVE-2018-20815.patch |
48 | 88 | CVE-2019-9824.patch | 88 | CVE-2019-9824.patch |
49 | 89 | ubuntu/lp-1830704-s390x-cpumodel-ignore-csske-for-expansion.patch | 89 | ubuntu/lp-1830704-s390x-cpumodel-ignore-csske-for-expansion.patch |
50 | 90 | <<<<<<< debian/patches/series | ||
51 | 90 | ubuntu/lp-1830243-s390-bios-Skip-bootmap-signature-entries.patch | 91 | ubuntu/lp-1830243-s390-bios-Skip-bootmap-signature-entries.patch |
52 | 92 | ======= | ||
53 | 93 | >>>>>>> debian/patches/series | ||
54 | 91 | ubuntu/lp1828495-0001-guidance-cpu-models.patch | 94 | ubuntu/lp1828495-0001-guidance-cpu-models.patch |
55 | 92 | ubuntu/lp1828495-0002-msr-new-msr-indices.patch | 95 | ubuntu/lp1828495-0002-msr-new-msr-indices.patch |
56 | 93 | ubuntu/lp1828495-0003-cpuid-feature-ia32-arch-capabilities.patch | 96 | ubuntu/lp1828495-0003-cpuid-feature-ia32-arch-capabilities.patch |
57 | @@ -104,3 +107,7 @@ ubuntu/lp1828495-0013-cascadelake-server.patch | |||
58 | 104 | ubuntu/lp1828495-0014-remove-cpuid-pconfig.patch | 107 | ubuntu/lp1828495-0014-remove-cpuid-pconfig.patch |
59 | 105 | ubuntu/lp1828495-0015-remove-cpuid-intel_pt.patch | 108 | ubuntu/lp1828495-0015-remove-cpuid-intel_pt.patch |
60 | 106 | ubuntu/lp1828495-0016-no-ospke-on-some.patch | 109 | ubuntu/lp1828495-0016-no-ospke-on-some.patch |
61 | 110 | <<<<<<< debian/patches/series | ||
62 | 111 | ======= | ||
63 | 112 | ubuntu/lp1828495-0017-target-i386-add-MDS-NO-feature.patch | ||
64 | 113 | >>>>>>> debian/patches/series | ||
65 | diff --git a/debian/patches/ubuntu/lp1828495-0017-target-i386-add-MDS-NO-feature.patch b/debian/patches/ubuntu/lp1828495-0017-target-i386-add-MDS-NO-feature.patch | |||
66 | 107 | new file mode 100644 | 114 | new file mode 100644 |
67 | index 0000000..949423c | |||
68 | --- /dev/null | |||
69 | +++ b/debian/patches/ubuntu/lp1828495-0017-target-i386-add-MDS-NO-feature.patch | |||
70 | @@ -0,0 +1,31 @@ | |||
71 | 1 | target/i386: add MDS-NO feature | ||
72 | 2 | |||
73 | 3 | Microarchitectural Data Sampling is a hardware vulnerability which allows | ||
74 | 4 | unprivileged speculative access to data which is available in various CPU | ||
75 | 5 | internal buffers. | ||
76 | 6 | |||
77 | 7 | Some Intel processors use the ARCH_CAP_MDS_NO bit in the | ||
78 | 8 | IA32_ARCH_CAPABILITIES | ||
79 | 9 | MSR to report that they are not vulnerable, make it available to guests. | ||
80 | 10 | |||
81 | 11 | Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> | ||
82 | 12 | Message-Id: <20190516185320.28340-1-pbonzini@redhat.com> | ||
83 | 13 | Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> | ||
84 | 14 | |||
85 | 15 | Author: Paolo Bonzini <pbonzini@redhat.com> | ||
86 | 16 | Origin: upstream, https://github.com/qemu/qemu/commit/20140a82c674 | ||
87 | 17 | Bug-Ubuntu: https://bugs.launchpad.net/bugs/1828495 | ||
88 | 18 | Reviewed-By: Rafael David Tinoco <rafaeldtinoco@gmail.com> | ||
89 | 19 | Last-Update: 2019-08-02 | ||
90 | 20 | |||
91 | 21 | --- qemu-2.11+dfsg.orig/target/i386/cpu.c | ||
92 | 22 | +++ qemu-2.11+dfsg/target/i386/cpu.c | ||
93 | 23 | @@ -597,7 +597,7 @@ static FeatureWordInfo feature_word_info | ||
94 | 24 | .type = MSR_FEATURE_WORD, | ||
95 | 25 | .feat_names = { | ||
96 | 26 | "rdctl-no", "ibrs-all", "rsba", "skip-l1dfl-vmentry", | ||
97 | 27 | - "ssb-no", NULL, NULL, NULL, | ||
98 | 28 | + "ssb-no", "mds-no", NULL, NULL, | ||
99 | 29 | NULL, NULL, NULL, NULL, | ||
100 | 30 | NULL, NULL, NULL, NULL, | ||
101 | 31 | NULL, NULL, NULL, NULL, |
Christian,
I have pushed this earlier than later, so I'm still reviewing this. I wanted you to have access soon so we can work 4 hands to make this ready.
Let me know your findings, please..
I'm fixing a missing CPUID_7_ 0_EDX_PCONFIG declaration in patch lp1828495-0005 because I did not include this definition since it was revered in Dec 2018, so I'm fixing this and will push again.