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

[Eduardo Habkost]

On Mon, Jul 04, 2016 at 11:03:59PM +0300, Michael S. Tsirkin wrote:
> On Mon, Jul 04, 2016 at 08:16:06PM +0100, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <address@hidden>
> > 
> > Fill the bits between 51..number-of-physical-address-bits in the
> > MTRR_PHYSMASKn variable range mtrr masks so that they're consistent
> > in the migration stream irrespective of the physical address space
> > of the source VM in a migration.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> > Suggested-by: Paolo Bonzini <address@hidden>
> 
> Could a bit more explanation be added here?
> Why don't we migrate exactly what guest programmed?

Because "exactly what the guest programmed" stops making sense if
phys_bits seen by the guest change when migrating (see Paolo's
explanation below).

> With previous patch we mask on destination, do we not?

The previous patch is necessary when phys_bits is smaller on
migration. This part is necessary when phys_bits increase on
migration.

> 
> If it's for compat with old PC types, then why not
> limit it to that?

It's not just for compat with old PC types. It is necessary when
phys_bits increase on migration, which may happen because it is
the default for a machine-type, or in case it was configured this
way by management.


I asked similar questions on v1, see
http://article.gmane.org/gmane.comp.emulators.qemu/419712

Paolo's reply is below.

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

-- 
Eduardo