Received-SPF: pass (google.com: domain of linux-kernel+bounces-50207-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Fri, 2 Feb 2024 09:07:13 -0800
In-Reply-To: <9098e8bb-cbe4-432c-98d6-ce96a4f7094f@linux.intel.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240123221220.3911317-1-mizhang@google.com> <ZbpqoU49k44xR4zB@google.com>
 <368248d0-d379-23c8-dedf-af7e1e8d23c7@oracle.com> <CAL715WJDesggP0S0M0SWX2QaFfjBNdqD1j1tDU10Qxk6h7O0pA@mail.gmail.com>
 <ZbvUyaEypRmb2s73@google.com> <ZbvjKtsVjpuQmKE2@google.com>
 <ZbvyrvvZM-Tocza2@google.com> <9098e8bb-cbe4-432c-98d6-ce96a4f7094f@linux.intel.com>
Message-ID: <Zb0hQfZX89gJOtRX@google.com>
Subject: Re: [PATCH] KVM: x86/pmu: Fix type length error when reading pmu->fixed_ctr_ctrl
From: Sean Christopherson <seanjc@google.com>
To: Xiong Y Zhang <xiong.y.zhang@linux.intel.com>
Cc: Mingwei Zhang <mizhang@google.com>, Dongli Zhang <dongli.zhang@oracle.com>, 
	Paolo Bonzini <pbonzini@redhat.com>, "H. Peter Anvin" <hpa@zytor.com>, kvm@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="us-ascii"

On Fri, Feb 02, 2024, Xiong Y Zhang wrote:
> 
> 
> On 2/2/2024 3:36 AM, Sean Christopherson wrote:
> > On Thu, Feb 01, 2024, Mingwei Zhang wrote:
> >> On Thu, Feb 01, 2024, Sean Christopherson wrote:
> >>> On Wed, Jan 31, 2024, Mingwei Zhang wrote:
> >>>>> The PMC is still active while the VM side handle_pmi_common() is not going to handle it?
> >>>>
> >>>> hmm, so the new value is '0', but the old value is non-zero, KVM is
> >>>> supposed to zero out (stop) the fix counter), but it skips it. This
> >>>> leads to the counter continuously increasing until it overflows, but
> >>>> guest PMU thought it had disabled it. That's why you got this warning?
> >>>
> >>> No, that can't happen, and KVM would have a massive bug if that were the case.
> >>> The truncation can _only_ cause bits to disappear, it can't magically make bits
> >>> appear, i.e. the _only_ way this can cause a problem is for KVM to incorrectly
> >>> think a PMC is being disabled.
> >>
> >> The reason why the bug does not happen is because there is global
> >> control. So disabling a counter will be effectively done in the global
> >> disable part, ie., when guest PMU writes to MSR 0x38f.
> > 
> > 
> >>> fixed PMC is disabled. KVM will pause the counter in reprogram_counter(), and
> >>> then leave the perf event paused counter as pmc_event_is_allowed() will return
> >>> %false due to the PMC being locally disabled.
> >>>
> >>> But in this case, _if_ the counter is actually enabled, KVM will simply reprogram
> >>> the PMC.  Reprogramming is unnecessary and wasteful, but it's not broken.
> >>
> >> no, if the counter is actually enabled, but then it is assigned to
> >> old_fixed_ctr_ctrl, the value is truncated. When control goes to the
> >> check at the time of disabling the counter, KVM thinks it is disabled,
> >> since the value is already truncated to 0. So KVM will skip by saying
> >> "oh, the counter is already disabled, why reprogram? No need!".
> > 
> > Ooh, I had them backwards.  KVM can miss 1=>0, but not 0=>1.  I'll apply this
> > for 6.8; does this changelog work for you?
> > 
> >   Use a u64 instead of a u8 when taking a snapshot of pmu->fixed_ctr_ctrl
> >   when reprogramming fixed counters, as truncating the value results in KVM
> >   thinking all fixed counters, except counter 0, 
> each counter has four bits in fixed_ctr_ctrl, here u8 could cover counter 0
> and counter 1, so "except counter 0" can be modified to "except counter 0 and
> 1" 

Ugh, math.  I'll adjust it to:

  Use a u64 instead of a u8 when taking a snapshot of pmu->fixed_ctr_ctrl
  when reprogramming fixed counters, as truncating the value results in KVM
  thinking fixed counter 2 is already disabled (the bug also affects fixed
  counters 3+, but KVM doesn't yet support those).  As a result, if the
  guest disables fixed counter 2, KVM will get a false negative and fail to
  reprogram/disable emulation of the counter, which can leads to incorrect
  counts and spurious PMIs in the guest.

Thanks!