On Fri, Mar 4, 2022 at 1:59 PM David Matlack <dmatlack@xxxxxxxxxx> wrote:
>
> On Thu, Feb 24, 2022 at 11:20 AM David Matlack <dmatlack@xxxxxxxxxx> wrote:
> >
> > On Thu, Feb 24, 2022 at 3:29 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
> > >
> > > On Thu, 03 Feb 2022 01:00:47 +0000,
> > > David Matlack <dmatlack@xxxxxxxxxx> wrote:
> > > >
>
> [...]
>
> > > >
> > > > /* Cache some mmu pages needed inside spinlock regions */
> > > > - struct kvm_mmu_memory_cache mmu_page_cache;
> > > > + DEFINE_KVM_MMU_MEMORY_CACHE(mmu_page_cache);
> > >
> > > I must say I'm really not a fan of the anonymous structure trick. I
> > > can see why you are doing it that way, but it feels pretty brittle.
> >
> > Yeah I don't love it. It's really optimizing for minimizing the patch diff.
> >
> > The alternative I considered was to dynamically allocate the
> > kvm_mmu_memory_cache structs. This would get rid of the anonymous
> > struct and the objects array, and also eliminate the rather gross
> > capacity hack in kvm_mmu_topup_memory_cache().
> >
> > The downsides of this approach is more code and more failure paths if
> > the allocation fails.
>
> I tried changing all kvm_mmu_memory_cache structs to be dynamically
> allocated, but it created a lot of complexity to the setup/teardown
> code paths in x86, arm64, mips, and riscv (the arches that use the
> caches). I don't think this route is worth it, especially since these
> structs don't *need* to be dynamically allocated.
>
> When you said the anonymous struct feels brittle, what did you have in
> mind specifically?
>
> >
> > >
> > > >
> > > > /* Target CPU and feature flags */
> > > > int target;
> > > > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> > > > index bc2aba953299..9c853c529b49 100644
> > > > --- a/arch/arm64/kvm/mmu.c
> > > > +++ b/arch/arm64/kvm/mmu.c
> > > > @@ -765,7 +765,8 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
> > > > {
> > > > phys_addr_t addr;
> > > > int ret = 0;
> > > > - struct kvm_mmu_memory_cache cache = { 0, __GFP_ZERO, NULL, };
> > > > + DEFINE_KVM_MMU_MEMORY_CACHE(cache) page_cache = {};
> > > > + struct kvm_mmu_memory_cache *cache = &page_cache.cache;
> > > > struct kvm_pgtable *pgt = kvm->arch.mmu.pgt;
> > > > enum kvm_pgtable_prot prot = KVM_PGTABLE_PROT_DEVICE |
> > > > KVM_PGTABLE_PROT_R |
> > > > @@ -774,18 +775,17 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
> > > > if (is_protected_kvm_enabled())
> > > > return -EPERM;
> > > >
> > > > + cache->gfp_zero = __GFP_ZERO;
> > >
> > > nit: consider this instead, which preserves the existing flow:
> >
> > Will do.
> >
> > >
> > > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> > > index 26d6c53be083..86a7ebd03a44 100644
> > > --- a/arch/arm64/kvm/mmu.c
> > > +++ b/arch/arm64/kvm/mmu.c
> > > @@ -764,7 +764,9 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
> > > {
> > > phys_addr_t addr;
> > > int ret = 0;
> > > - DEFINE_KVM_MMU_MEMORY_CACHE(cache) page_cache = {};
> > > + DEFINE_KVM_MMU_MEMORY_CACHE(cache) page_cache = {
> > > + .cache = { .gfp_zero = __GFP_ZERO},
> > > + };
> > > struct kvm_mmu_memory_cache *cache = &page_cache.cache;
> > > struct kvm_pgtable *pgt = kvm->arch.mmu.pgt;
> > > enum kvm_pgtable_prot prot = KVM_PGTABLE_PROT_DEVICE |
> > > @@ -774,7 +776,6 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
> > > if (is_protected_kvm_enabled())
> > > return -EPERM;
> > >
> > > - cache->gfp_zero = __GFP_ZERO;
> > > size += offset_in_page(guest_ipa);
> > > guest_ipa &= PAGE_MASK;
> > >
> > > but whole "declare the outer structure and just use the inner one"
> > > hack is... huh... :-/
> >
> > Yeah it's not great. Unfortunately (or maybe fortunately?) anonymous
> > structs cannot be defined in functions. So naming the outer struct is
> > necessary even though we only need to use the inner one.
>
> I see two alternatives to make this cleaner:
>
> 1. Dynamically allocate just this cache. The caches defined in
> vcpu_arch will continue to use DEFINE_KVM_MMU_MEMORY_CACHE(). This
> would get rid of the outer struct but require an extra memory
> allocation.
> 2. Move this cache to struct kvm_arch using
> DEFINE_KVM_MMU_MEMORY_CACHE(). Then we don't need to stack allocate it
> or dynamically allocate it.
>
> Do either of these approaches appeal to you more than the current one?
(There's obvious performance and memory overhead trade-offs with these
different approaches, but I don't know enough about arm64 KVM to
assess which option might be best.)
>
> >
> > >
> > > This hunk also conflicts with what currently sits in -next. Not a big
> > > deal, but just so you know.
> >
> > Ack.
> >
> > >
> > > > diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
> > > > index dceac12c1ce5..9575fb8d333f 100644
> > > > --- a/include/linux/kvm_types.h
> > > > +++ b/include/linux/kvm_types.h
> > > > @@ -78,14 +78,34 @@ struct gfn_to_pfn_cache {
> > > > * MMU flows is problematic, as is triggering reclaim, I/O, etc... while
> > > > * holding MMU locks. Note, these caches act more like prefetch buffers than
> > > > * classical caches, i.e. objects are not returned to the cache on being freed.
> > > > + *
> > > > + * The storage for the cache objects is laid out after the struct to allow
> > > > + * different declarations to choose different capacities. If the capacity field
> > > > + * is 0, the capacity is assumed to be KVM_ARCH_NR_OBJS_PER_MEMORY_CACHE.
> > > > */
> > > > struct kvm_mmu_memory_cache {
> > > > int nobjs;
> > > > + int capacity;
> > > > gfp_t gfp_zero;
> > > > struct kmem_cache *kmem_cache;
> > > > - void *objects[KVM_ARCH_NR_OBJS_PER_MEMORY_CACHE];
> > > > + void *objects[0];
> > >
> > > The VLA police is going to track you down ([0] vs []).
> >
> > Thanks!
> >
> >
> > >
> > > M.
> > >
> > > --
> > > Without deviation from the norm, progress is not possible.
