On Wed, Jul 28, 2021 at 09:23:53AM -0700, Ben Gardon wrote: > On Wed, Jul 28, 2021 at 12:40 AM Yan Zhao <yan.y.zhao@xxxxxxxxx> wrote: > > > > On Wed, Jul 28, 2021 at 02:56:05PM +0800, Yu Zhang wrote: > > > Thanks a lot for your reply, Sean. > > > > > > On Tue, Jul 27, 2021 at 06:07:35PM +0000, Sean Christopherson wrote: > > > > On Wed, Jul 28, 2021, Yu Zhang wrote: > > > > > Hi all, > > > > > > > > > > I'd like to ask a question about kvm_reset_context(): is there any > > > > > reason that we must alway unload TDP root in kvm_mmu_reset_context()? > > I just realized I sent my response to Yu yesterday without reply-all. > Sending it again here for posterity. I'll add comments on the > discussion inline below too. Thanks Ben, I copied my reply here. With some gramar fixes. :) > > Hi Yu, > > I think the short answer here is no, there's no reason we can't keep > the root around for later. > > When developing the TDP MMU, we were primarily concerned about > performance post-boot, especially during migration or when > re-populating memory for demand paging. In these scenarios the guest > role doesn't really change and so the TDP MMU's shadow page tables > aren't torn down. In my initial testing, I thought I only ever > observed two TDP MMU roots be allocated over the life of the VM, but I > could be wrong. Well I observed more, may be because I am using OVMF? Note, the vCPU number is just one in my test. > > For the TDP MMU root to be torn down, there also has to be no vCPU > using it. This probably happens in transitions to SMM and guest root > level changes, but I suspected that there would usually be at least > one vCPU in some "normal" mode, post boot. That may have been an > incorrect assumption. > > I think the easiest solution to this would be to just have the TDP MMU > roots track the life of the VM by adding an extra increment to their > reference count on allocation and an extra decrement when the VM is > torn down. However this introduces a problem because it increases the > amount of memory the TDP MMU is likely to be using for its page > tables. (It could use the memory either way but it would require some > surprising guest behavior.) So your suggestion is, once allocated, do not free the root page until the VM is destroyed? > > I have a few questions about these unnecessary tear-downs during boot: > 1. How many teardowns did you observe, and how many different roles > did they represent? Just thrashing between two roles, or 12 different > roles? I saw 106 reloadings of the root TDP. Among them, 14 are caused by memslot changes. Remaining ones are caused by the context reset from CR0/CR4/EFER changes(85 for CR0 changes). And I believe most are using the same roles, because in legacy TDP, only 4 different TDP roots are allocated due to the context reset(and several more are caused by memslot updating). But in TDP MMU, that means 106 times of TDP root being torn down and reallocated. > 2. When the TDP MMU's page tables got torn down, how much memory did > they map / how big were they? I did not collect this in TDP MMU, but I once tried with legacy TDP. IIRC, there are only several SPs allocated in one TDP table when the context resets. > 3. If you hacked in the extra refcount increment I suggested above, > how much of a difference in boot time does it make? I have not tried this, but I think that proposal is let TDP MMU try to reuse previous root page with same mmu role with current context, just like the legacy TDP does? Actually I am curious, why would the root needs to be unloaded at all(even in the legacy TDP code)? Sean's reply mentioned that change of the mmu role is the reason, but I do not understand yet. > > For 2 and 3 I ask because if the guest hasn't accessed much of it's > memory early in boot, the paging structure won't be very large and > tearing it down / rebuilding it is pretty cheap. Agree. But I am a bit surprised to see so many CR0 changes in the boot time. > > We may find that we need some kind of page quota for the TDP MMU after > all, if we want to have a bunch of roots at the same time. If that's > the case, perhaps we should spawn another email thread to discuss how > that should work. Could we find a way to obviate the requirement of unloading(if unnecessary)? > > Thanks for raising this issue! > Ben > > > > > > > > > The short answer is that mmu_role is changing, thus a new root shadow page is > > > > needed. > > > > > > I saw the mmu_role is recalculated, but I have not figured out how this > > > change would affect TDP. May I ask a favor to give an example? Thanks! > > One really simple example is if the guest started using SMM. In that > case it's a totally different address space, so we need a new EPT. Yes. I admit unloading the MMU is necessary for SMM. But what about the other scenarios? :) > > > > > > > I realized that if we only recalculate the mmu role, but do not unload > > > the TDP root(e.g., when guest efer.nx flips), base role of the SPs will > > > be inconsistent with the mmu context. But I do not understand why this > > > shall affect TDP. > > It might not always cause problems since TDP is less sensitive to this > kind of thing than shadow paging, but getting all the details right is > hard so we just took the conservative approach of handling all role > changes with a new root. I have the same feeling, but I doubt if it will *never* cause any problem. :) Another impact I can think of is: without unloading, the root_hpa will not be set to INVALID_PAGE, hence the kvm_mmu_load() will not be called before vcpu entry(which may reload guest CR3/PDPTRs as well). But I have no idea if this could cause any trouble or not. B.R. Yu
