linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Sean Christopherson <seanjc@google.com>
To: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: LKML <linux-kernel@vger.kernel.org>,
	kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
	Lai Jiangshan <laijs@linux.alibaba.com>,
	Vitaly Kuznetsov <vkuznets@redhat.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Jim Mattson <jmattson@google.com>
Subject: Re: [RFC PATCH] kvm/x86: Keep root hpa in prev_roots as much as possible
Date: Tue, 3 Aug 2021 15:57:18 +0000	[thread overview]
Message-ID: <YQlnXlmt9JvzRn+f@google.com> (raw)
In-Reply-To: <CAJhGHyCU-Om3NWLVg-kbUE7FZD1nNZft8+KeCDH3cr_FDaitXQ@mail.gmail.com>

On Tue, Aug 03, 2021, Lai Jiangshan wrote:
> On Fri, Jul 30, 2021 at 2:06 AM Sean Christopherson <seanjc@google.com> 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?

      reply	other threads:[~2021-08-03 15:57 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-25 21:39 Lai Jiangshan
2021-07-29 18:05 ` Sean Christopherson
2021-08-03  1:19   ` Lai Jiangshan
2021-08-03 15:57     ` Sean Christopherson [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YQlnXlmt9JvzRn+f@google.com \
    --to=seanjc@google.com \
    --cc=jiangshanlai@gmail.com \
    --cc=jmattson@google.com \
    --cc=kvm@vger.kernel.org \
    --cc=laijs@linux.alibaba.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=vkuznets@redhat.com \
    --cc=wanpengli@tencent.com \
    --subject='Re: [RFC PATCH] kvm/x86: Keep root hpa in prev_roots as much as possible' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).