Message-ID: <b8de7f7b-7aa1-d98b-74be-62d7c055542b@redhat.com>
Date:   Tue, 10 Nov 2020 11:40:34 +0100
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     "Luck, Tony" <tony.luck@...el.com>,
        Jim Mattson <jmattson@...gle.com>, Qian Cai <cai@...hat.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-tip-commits@...r.kernel.org" 
        <linux-tip-commits@...r.kernel.org>, x86 <x86@...nel.org>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>
Subject: Re: [PATCH] x86/mce: Check for hypervisor before enabling additional
 error logging

On 10/11/20 10:56, Borislav Petkov wrote:
> On Tue, Nov 10, 2020 at 09:50:43AM +0100, Paolo Bonzini wrote:
>> 1) ignore_msrs _cannot_ be on by default.  You cannot know in advance that
>> for all non-architectural MSRs it's okay for them to read as zero and eat
>> writes.  For some non-architectural MSR which never reads as zero on real
>> hardware, who knows that there isn't some code using the contents of the MSR
>> as a divisor, and causing a division by zero exception with ignore_msrs=1?
> 
> So if you're emulating a certain type of hardware - say a certain CPU
> model - then what are you saying? That you're emulating it but not
> really all of it, just some bits?

We try to emulate all that is described in the SDM as architectural, as 
long as we expose the corresponding CPUID leaves.

However, f/m/s mean nothing when running virtualized.  First, trying to 
derive any non-architectural property from the f/m/s is going to fail. 
Second, even the host can be anything as long as it's newer than the 
f/m/s that the VM reports (i.e. you can get a Sandy Bridge model and 
model name even if running on Skylake).

Also, X86_FEATURE_HYPERVISOR might be clear even if running virtualized. 
  (Thank you nVidia for using it to play market segmentation games).

> Because this is what happens - the kernel checks that it runs on a
> certain CPU type and this tells it that those MSRs are there. But then
> comes virt and throws all assumptions out.
> 
> So if it emulates a CPU model and the kernel tries to access those MSRs,
> then the HV should ignore those MSR accesses if it doesn't know about
> them. Why should the kernel change everytime some tool or virtualization
> has shortcomings?

See above: how can the hypervisor know a safe value for all MSRs, 
possibly including the undocumented ones?

>> 3) because of (1) and (2), the solution is very simple.  If the MSR is
>> architectural, its absence is a KVM bug and we'll fix it in all stable
>> versions.  If the MSR is not architectural (and 17Fh isn't; not only it's
>> not mentioned in the SDM,
> 
> It is mentioned in the SDM.

Oh right they moved the MSRs to a separate manual; found it now.  Still, 
it's not architectural.

> But maybe we should have a choice and maybe qemu/kvm should have a way
> to ignore certain MSRs for certain CPU types, regardless of them being
> architectural or not.

If it makes sense to emulate certain non-architectural MSRs we can add 
them.  Supporting the error control MSR wouldn't even be hard, but I'm 
not sure it makes sense:

1) that MSR has not been there on current processors for several years 
(and therefore Intel has clearly no intention of making architectural). 
  For what we know, even current processors might not provide any of 
that extended information at all (and still the VM could present Sandy 
Bridge f/m/s).

2) it would only present extended error info if the host itself enables 
the bit, so one might question the wisdom of backporting that support 
this to stable kernels

3) It's unclear whether the guest would be able to use the extended 
error information at all (and in some cases the description in the 
manual is not even proper English: "allows the iMC to log first device 
error when corrected error is detected during normal read"?).

4) other hypervisors, including older distros, would likely have the 
same issue.

Paolo

Powered by blists - more mailing lists