On Mon, Mar 18, 2024, David Woodhouse wrote: > On Mon, 2024-03-18 at 14:34 -0700, Sean Christopherson wrote: > > On Mon, Mar 18, 2024, David Woodhouse wrote: > > > > > > /* Either gpa or uhva must be valid, but not both */ > > > if (WARN_ON_ONCE(kvm_is_error_gpa(gpa) == kvm_is_error_hva(uhva))) > > > return -EINVAL; > > > > > > Hm, that comment doesn't match the code. It says "not both", but the > > > code also catches the "neither" case. I think the gpa is in %rbx and > > > uhva is in %r12, so this is indeed the 'neither' case. > > > > > > Is it expected that we can end up with a cache marked active, but with > > > the address not valid? Maybe through a race condition with deactive? or > > > more likely than that? > > > > It's the darn PV system time MSR, which allows the guest to triggering activation > > with any GPA value. That results in the cache being marked active without KVM > > ever setting the GPA (or any other fields). The fix I'm testing is to move the > > offset+len check up into activate() and refresh(). > > Not sure I even want a gpc of length 1 to work at INVALID_GPA; I don't > think it's the offset+length check we want to be looking at? > > If we've activated the gpc with gpa==INVALID_GPA, surely the right This particular issue isn't due to activating with gpa==INVALID_GPA, it's due to marking the gpc as active without actually activating it. The offset+length check is simply what causes KVM to prematurely bail from activation. > thing to do is just let it fail (perhaps with an explicit check or just > letting the memslot lookup fail). After fixing that WARN_ON be > > if (WARN_ON_ONCE(!kvm_is_error_gpa(gpa) && !kvm_is_error_hva(uhva))) I really don't want to relax the sanity check, as I feel strongly that KVM needs an invariant that an active cache is either GPA-based or HVA-based, i.e. that at least one of GPA or HVA is "valid". In quotes because the GPA doesn't need to be fully validated, just something that doesn't trip kvm_is_error_gpa().
