On Tue, Aug 03, 2021, Lai Jiangshan wrote:
> On Fri, Jul 30, 2021 at 2:06 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > Ha, we can do this without increasing the memory footprint and without co-opting
> > a bit from pgd or hpa. Because of compiler alignment/padding, the u8s and bools
> > between mmu_role and prev_roots already occupy 8 bytes, even though the actual
> > size is 4 bytes. In total, we need room for 4 roots (3 previous + current), i.e.
> > 4 bytes. If a separate array is used, no additional memory is consumed and no
> > masking is needed when reading/writing e.g. pgd.
> >
> > The cost is an extra swap() when updating the prev_roots LRU, but that's peanuts
> > and would likely be offset by masking anyways.
> >
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index 99f37781a6fc..13bb3c3a60b4 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -424,10 +424,12 @@ struct kvm_mmu {
> > hpa_t root_hpa;
> > gpa_t root_pgd;
> > union kvm_mmu_role mmu_role;
> > + bool root_unsync;
> > u8 root_level;
> > u8 shadow_root_level;
> > u8 ept_ad;
> > bool direct_map;
> > + bool unsync_roots[KVM_MMU_NUM_PREV_ROOTS];
> > struct kvm_mmu_root_info prev_roots[KVM_MMU_NUM_PREV_ROOTS];
> >
>
> Hello
>
> I think it is too complicated. And it is hard to accept to put "unsync"
> out of struct kvm_mmu_root_info when they should be bound to each other.
I agree it's a bit ugly to have the separate unsync_roots array, but I don't see
how it's any more complex. It's literally a single swap() call.
> How about this:
> - KVM_MMU_NUM_PREV_ROOTS
> + KVM_MMU_NUM_CACHED_ROOTS
> - mmu->prev_roots[KVM_MMU_NUM_PREV_ROOTS]
> + mmu->cached_roots[KVM_MMU_NUM_CACHED_ROOTS]
I don't have a strong preference on PREV vs CACHED. CACHED is probably more
intuitive, but KVM isn't truly caching the root, it's just tracking the HPA (and
PGD for indirect MMUs), e.g. the root may no longer exist if the backing shadow
page was zapped. On the other hand, the main helper is cached_root_available()...
> - mmu->root_hpa
> + mmu->cached_roots[0].hpa
> - mmu->root_pgd
> + mmu->cached_roots[0].pgd
>
> And using the bit63 in @pgd as the information that it is not requested
FWIW, using bit 0 will likely generate more efficient code.
> to sync since the last sync.
Again, I don't have a super strong preference. I don't hate or love either one :-)
Vitaly, Paolo, any preferences on names and approaches for tracking if a "cached"
root is unsync?
