Re: [Qemu-devel] [PATCH 3/5] x86: fill high bits of mtrr mask

[Eduardo Habkost]

On Fri, Jun 17, 2016 at 03:01:55PM +0200, Paolo Bonzini wrote:
> 
> 
> On 17/06/2016 14:46, Eduardo Habkost wrote:
> >> > 
> >> > The bits are reserved anyway, so we can do whatever we want with them.
> >> > In fact I think it's weird for the architecture to make them
> >> > must-be-zero, it might even make more sense to make them must-be-one...
> >> > It's a mask after all, and there's no way to access out-of-range
> >> > physical addresses.
> > 
> > If we always fill the bits on the source, the destination can't
> > differentiate between a 40-bit source that set the MTRR to
> > 0xffffffffff from a 36-bit source that set the MTRR to
> > 0xfffffffff.
> 
> That's not a bug, it's a feature.  MTRRs work by comparing (address &
> mtrr_mask) with mtrr_base.
> 
> A 40-bit source that sets the MTRR to 0xffffffffff must set higher bits
> of mtrr_base to zero, but when you migrate to another destination, you
> want the comparison to hold.  There are two ways for it to hold:
> 
> - if the physical address space becomes shorter, the base bits must be
> zero or writing mtrr_base fails, and the address bits must be zero or
> the processor fails to walk EPT page tables.  So it's fine to strip the
> top four bits of the mask.
> 
> - if the physical address space becomes larger, the base bits must be
> zero but the mask bits must become one.  So why not migrate them this
> way directly.
> 
> (For what it's worth, KVM also stores the mask extended to all ones, and
> strips the unnecessary high bits when reading the MSRs).

So the MSR is changing but the semantics are preserved. That's
good, but we may still have other problems if phys_addr changes
on migration, so:

> 
> > I really want to print a warning if the MTRR value or the
> > phys-bits value is being changed during migration, just in case
> > this has unintended consequences in the future. We can send the
> > current phys_bits value in the migration stream, so the
> > destination can decide how to handle it (and which warnings to
> > print).
> 
> The problem with this is that it must be a warning only, because usually
> things will work (evidence: they have worked on RHEL6 and RHEL7 since
> 2010).  No one will read it until it's too late and they have lost a VM.
> 
> Note that the failure mode is pretty brutal since KVM reports an
> internal error right after restarting on the destination, and who knows
> what used to happen before the assertion was introduced.  All MSRs after
> MTRRs would be ignored by KVM!
> 

It's probably too late for the user to act on it, but I want to
see the warning in the logs of a bug report in case something
else breaks. The MTRR changes may be 100% safe (and may not
deserve a warning), but other things can break when phys_bits
change on migration.

> Migrating and checking the configuration complicates the code and,
> because it's only for a warning, doesn't save you from massaging the
> values to make things work.

I would agree with you if we didn't have the existing
host-phys-bits mode already. In this case, phys_bits is
initialized to a different value depending on the host. You can
argue it is not machine state, but it is not user-specified
configuration either.

In theory, we should never initialize anything on the machine
based on the host we are running. In practice we sometimes do
that, and we know it's unsafe. Sending the value on the migration
stream is a solution to detect when this breaks something. We did
that before, for TSC frequency.

I prefer to add a little extra code, than to waste time debugging
when that stuff breaks.

-- 
Eduardo