On Fri, 2022-07-29 at 15:07 +0000, Sean Christopherson wrote: > On Fri, Jul 29, 2022, Kai Huang wrote: > > On Thu, 2022-07-28 at 22:17 +0000, Sean Christopherson wrote: > > > Fully re-evaluate whether or not MMIO caching can be enabled when SPTE > > > masks change; simply clearing enable_mmio_caching when a configuration > > > isn't compatible with caching fails to handle the scenario where the > > > masks are updated, e.g. by VMX for EPT or by SVM to account for the C-bit > > > location, and toggle compatibility from false=>true. > > > > > > Snapshot the original module param so that re-evaluating MMIO caching > > > preserves userspace's desire to allow caching. Use a snapshot approach > > > so that enable_mmio_caching still reflects KVM's actual behavior. > > > > > .. > > > > @@ -340,6 +353,12 @@ void kvm_mmu_set_mmio_spte_mask(u64 mmio_value, u64 mmio_mask, u64 access_mask) > > > BUG_ON((u64)(unsigned)access_mask != access_mask); > > > WARN_ON(mmio_value & shadow_nonpresent_or_rsvd_lower_gfn_mask); > > > > > > + /* > > > + * Reset to the original module param value to honor userspace's desire > > > + * to (dis)allow MMIO caching. Update the param itself so that > > > + * userspace can see whether or not KVM is actually using MMIO caching. > > > + */ > > > + enable_mmio_caching = allow_mmio_caching; > > > > I think the problem comes from MMIO caching mask/value are firstly set in > > kvm_mmu_reset_all_pte_masks() (which calls kvm_mmu_set_mmio_spte_mask() and may > > change enable_mmio_caching), and later vendor specific code _may_ or _may_not_ > > call kvm_mmu_set_mmio_spte_mask() again to adjust the mask/value. And when it > > does, the second call from vendor specific code shouldn't depend on the > > 'enable_mmio_caching' value calculated in the first call in > > kvm_mmu_reset_all_pte_masks(). > > Correct. > > > Instead of using 'allow_mmio_caching', should we just remove > > kvm_mmu_set_mmio_spte_mask() in kvm_mmu_reset_all_pte_masks() and enforce vendor > > specific code to always call kvm_mmu_set_mmio_spte_mask() depending on whatever > > hardware feature the vendor uses? > > Hmm, I'd rather not force vendor code to duplicate the "basic" functionality. > It's somewhat silly to preserve the common path since both SVM and VMX need to > override it, but on the other hand those overrides are conditional. OK. > > Case in point, if I'm reading the below patch correctly, svm_shadow_mmio_mask will > be left '0' if the platform doesn't support memory encryption (svm_adjust_mmio_mask() > will bail early). That's a solvable problem, but then I think KVM just ends up > punting this issue to SVM to some extent. That patch is not supposed to have any functional change to AMD. Yes this needs to be fixed if we go with solution. > > Another flaw in the below patch is that enable_mmio_caching doesn't need to be > tracked on a per-VM basis. VMX with EPT can have different masks, but barring a > massive change in KVM or hardware, there will never be a scenario where caching is > enabled for one VM but not another. Yeah it looks so, if we always treat TDX guest must have enable_mmio_caching enabled (reading your reply to patch 4). > > And isn't the below patch also broken for TDX? For TDX, unless things have changed, > the mask+value is supposed to be SUPPRES_VE==0 && RWX==0. So either KVM is generating > the wrong mask (MAXPHYADDR < 51), or KVM is incorrectly marking MMIO caching as disabled > in the TDX case. The MMIO mask/value set for TDX will come in another patch. My suggestion is this patch is some kinda infrastructural patch which doesn't bring any functional change. > > Lastly, in prepration for TDX, enable_mmio_caching should be changed to key off > of the _mask_, not the value. E.g. for TDX, the value will be '0', but the mask > should be SUPPRESS_VE | RWX. Agreed. But perhaps in another patch. We need to re-define what does mask/value mean to enable_mmio_caching. -- Thanks, -Kai
