On Wed, Jan 15, 2020 at 4:32 PM Liran Alon <liran.alon@xxxxxxxxxx> wrote:
>
>
>
> > On 16 Jan 2020, at 2:16, Jim Mattson <jmattson@xxxxxxxxxx> wrote:
> >
> > Beginning with commit 0b9f6c4615c99 ("x86/kvm: Support the vCPU
> > preemption check"), the KVM_VCPU_PREEMPTED flag is set in the guest
> > copy of the kvm_steal_time struct on every call to vcpu_put(). As a
> > result, guest memory is dirtied on every call to vcpu_put(), even when
> > the VM is quiescent.
> >
> > To avoid dirtying guest memory unnecessarily, don't bother setting the
> > flag in the guest copy of the struct if it is already set in the
> > kernel copy of the struct.
>
> I suggest adding this comment to code as-well.
Ping. I don't know if a v2 of this change with the comment in code is
needed for acceptance, but I don't want this to fall through the
cracks and get lost.
>
> >
> > If a different vCPU thread clears the guest copy of the flag, it will
> > no longer get reset on the next call to vcpu_put, but it's not clear
> > that resetting the flag in this case was intentional to begin with.
>
> I agree… I find it hard to believe that guest vCPU is allowed to clear the flag
> and expect host to set it again on the next vcpu_put() call. Doesn’t really make sense.
>
> >
> > Signed-off-by: Jim Mattson <jmattson@xxxxxxxxxx>
> > Tested-by: Kevin Mcgaire <kevinmcgaire@xxxxxxxxxx>
> > Reviewed-by: Ben Gardon <bgardon@xxxxxxxxxx>
> > Reviewed-by: Oliver Upton <oupton@xxxxxxxxxx>
>
> Good catch.
> Reviewed-by: Liran Alon <liran.alon@xxxxxxxxxx>
>
> -Liran
>
> >
> > ---
> > arch/x86/kvm/x86.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index cf917139de6b..3dc17b173f88 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -3504,6 +3504,9 @@ static void kvm_steal_time_set_preempted(struct kvm_vcpu *vcpu)
> > if (!(vcpu->arch.st.msr_val & KVM_MSR_ENABLED))
> > return;
> >
> > + if (vcpu->arch.st.steal.preempted & KVM_VCPU_PREEMPTED)
> > + return;
> > +
> > vcpu->arch.st.steal.preempted = KVM_VCPU_PREEMPTED;
> >
> > kvm_write_guest_offset_cached(vcpu->kvm, &vcpu->arch.st.stime,
> > --
> > 2.25.0.rc1.283.g88dfdc4193-goog
> >
>
