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" <dgilb...@redhat.com> > > > > 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 <dgilb...@redhat.com> > > Suggested-by: Paolo Bonzini <pbonz...@redhat.com> > > 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
