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

[Paolo Bonzini]

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).

> 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!

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.

Paolo