Re: [Qemu-devel] [PATCH] x86: Reset MTRR on vCPU reset

[Alex Williamson <alex.williamson@redhat.com>]

On Thu, 2014-08-14 at 01:44 +0200, Laszlo Ersek wrote:
> On 08/14/14 01:17, Laszlo Ersek wrote:
> 
> > - With KVM, the lack of loading MTRR state from KVM, combined with the
> >   (partial) storing of MTRR state to KVM, has two consequences:
> >   - migration invalidates (loses) MTRR state,
> 
> I'll concede that migration *already* loses MTRR state (on KVM), even
> before your patch. On the incoming host, the difference is that
> pre-patch, the guest continues running (after migration) with MTRRs in
> the "initial" KVM state, while post-patch, the guest continues running
> after an explicit zeroing of the variable MTRR masks and the deftype.
> 
> I admit that it wouldn't be right to say that the patch "causes" MTRR
> state loss.
> 
> With that, I think I've actually convinced myself that your patch is
> correct:
> 
> The x86_cpu_reset() hunk is correct in any case, independently of KVM
> vs. TCG. (On TCG it even improves MTRR conformance.) Splitting that hunk
> into a separate patch might be worthwhile, but not overly important.
> 
> The kvm_put_msrs() hunk forces a zero write to the variable MTRR
> PhysMasks and the DefType, on both reset and on incoming migration. For
> reset, this is correct behavior. For incoming migration, it is not, but
> it certainly shouldn't qualify as a regression, relative to the current
> status (where MTRR state is simply lost and replaced with initial MTRR
> state on the incoming host).
> 
> I think the above "end results" could be expressed more clearly in the
> code, but I'm already wondering if you'll ever talk to me again, so I'm
> willing to give my R-b if you think that's useful... :)

Heh, I think you've highlighted an important point, perhaps several.  I
was assuming my kvm_put_msrs() was only for reset, but it's clearly not.
So I agree that we need both get and put support.  It probably makes
sense to create one patch cleaning up the hardcoded variable register
array vs guest advertised, another implementing the reset path, and a
final one adding KVM get/put.  I'll get started.  Thanks for the review.

Alex