On Mon, Jul 18, 2022, Yu Zhang wrote:
> On Fri, Jul 15, 2022 at 03:56:31PM +0000, Sean Christopherson wrote:
> > On Fri, Jul 15, 2022, Yu Zhang wrote:
> > Oof! I was going to respond with a variety of nits (about the existing comment),
> > and even suggest that we address the TODO just out of sight, but looking at all
> > of this made me realize there's a bug here! vmx_update_exception_bitmap() doesn't
> > update MASK and MATCH!
> >
> > Hitting the bug is extremely unlikely, as it would require changing the guest's
> > MAXPHYADDR via KVM_SET_CPUID2 _after_ KVM_SET_NESTED_STATE, but before KVM_RUN
> > (because KVM now disallows changin CPUID after KVM_RUN).
> >
> > During KVM_SET_CPUID2, KVM will invoke vmx_update_exception_bitmap() to refresh
> > the exception bitmap to handle the ept=1 && allow_smaller_maxphyaddr=1 scenario.
> > But when L2 is active, vmx_update_exception_bitmap() assumes vmcs02 already has
> > the correct MASK+MATCH because of the "clear both if KVM and L1 both want #PF"
> > behavior. But if KVM's desire to intercept #PF changes from 0=>1, then KVM will
> > run L2 with the MASK+MATCH from vmcs12 because vmx_need_pf_intercept() would have
> > returned false at the time of prepare_vmcs02_rare().
>
> And then the #PF could be missed in L0 because previously both L1 and L0 has no
> desire to intercept it, meanwhile KVM fails to update this after migration(I guess
> the only scenario for this to happen is migration?). Is this understanding correct?
Yes, I think that will be the scenario (I hadn't thought too much about the actual
result).
> > Fixing the bug is fairly straightforward, and presents a good opportunity to
> > clean up the code (and this comment) and address the TODO.
> >
> > Unless someone objects to my suggestion for patch 01, can you send a new version
> > of patch 01? I'll send a separate series to fix this theoretical bug, avoid
> > writing MASK+MATCH when vmcs0x.EXCEPTION_BITMAP.PF+0, and to address the TODO.
>
> Sure, I will send another version of patch 01.
>
> >
> > E.g. I believe this is what we want to end up with:
> >
> > if (vmcs12)
> > eb |= vmcs12->exception_bitmap;
> >
> > /*
> > * #PF is conditionally intercepted based on the #PF error code (PFEC)
> > * combined with the exception bitmap. #PF is intercept if:
> > *
> > * EXCEPTION_BITMAP.PF=1 && ((PFEC & MASK) == MATCH).
> > *
> > * If any #PF is being intercepted, update MASK+MATCH, otherwise leave
> > * them alone they do not affect interception (EXCEPTION_BITMAP.PF=0).
> > */
> > if (eb & (1u << PF_VECTOR)) {
> > /*
> > * If EPT is enabled, #PF is only intercepted if MAXPHYADDR is
> > * smaller on the guest than on the host. In that case, KVM
> > * only needs to intercept present, non-reserved #PF. If EPT
> > * is disabled, i.e. KVM is using shadow paging, KVM needs to
> > * intercept all #PF. Note, whether or not KVM wants to
> > * intercept _any_ #PF is handled below.
> > */
> > if (enable_ept) {
> > pfec_mask = PFERR_PRESENT_MASK | PFERR_RSVD_MASK;
> > pfec_match = PFERR_PRESENT_MASK;
> > } else {
> > pfec_mask = 0;
> > pfec_match = 0;
> > }
> >
> > if (!(vmcs12->exception_bitmap & (1u << PF_VECTOR))) {
> > /* L1 doesn't want to intercept #PF, use KVM's MASK+MATCH. */
> > } else if (!kvm_needs_pf_intercept) {
> > /* KVM doesn't want to intercept #PF, use L1's MASK+MATCH. */
> > pfec_mask = vmcs12->page_fault_error_code_mask;
> > pfec_match = vmcs12->page_fault_error_code_match;
> > } else if (pfec_mask != vmcs12->page_fault_error_code_mask ||
> > pfec_match != vmcs12->page_fault_error_code_mask) {
> > /*
> > * KVM and L1 want to intercept #PF with different MASK
> > * and/or MATCH. For simplicity, intercept all #PF by
> > * clearing MASK+MATCH. Merging KVM's and L1's desires
> > * is quite complex, while the odds of meaningfully
> > * reducing what #PFs are intercept are low.
> > */
> > pfec_mask = 0;
> > pfec_match = 0;
> > } else {
> > /* KVM and L1 have identical MASK+MATCH. */
> > }
> > vmcs_write32(PAGE_FAULT_ERROR_CODE_MASK, pfec_mask);
> > vmcs_write32(PAGE_FAULT_ERROR_CODE_MATCH, pfec_match);
> > }
>
> And we do not need to update the PFEC_MASK & PFEC_MATCH in prepare_vmcs02_rare()
> anymore, right? Thanks!
Yep!
Also, IIRC I have a goof or two in the above, i.e. don't waste any time trying to test it.
