[PATCH 05/11] powerpc/kvm: Split HPT allocation from activation
David Gibson
david at gibson.dropbear.id.au
Mon Dec 19 11:24:58 AEDT 2016
On Fri, Dec 16, 2016 at 12:57:26PM +0100, Thomas Huth wrote:
> On 15.12.2016 06:53, David Gibson wrote:
> > Currently, kvmppc_alloc_hpt() both allocates a new hashed page table (HPT)
> > and sets it up as the active page table for a VM. For the upcoming HPT
> > resize implementation we're going to want to allocate HPTs separately from
> > activating them.
> >
> > So, split the allocation itself out into kvmppc_allocate_hpt() and perform
> > the activation with a new kvmppc_set_hpt() function. Likewise we split
> > kvmppc_free_hpt(), which just frees the HPT, from kvmppc_release_hpt()
> > which unsets it as an active HPT, then frees it.
> >
> > We also move the logic to fall back to smaller HPT sizes if the first try
> > fails into the single caller which used that behaviour,
> > kvmppc_hv_setup_htab_rma(). This introduces a slight semantic change, in
> > that previously if the initial attempt at CMA allocation failed, we would
> > fall back to attempting smaller sizes with the page allocator. Now, we
> > try first CMA, then the page allocator at each size. As far as I can tell
> > this change should be harmless.
> >
> > To match, we make kvmppc_free_hpt() just free the actual HPT itself. The
> > call to kvmppc_free_lpid() that was there, we move to the single caller.
> >
> > Signed-off-by: David Gibson <david at gibson.dropbear.id.au>
> > ---
> > arch/powerpc/include/asm/kvm_book3s_64.h | 3 ++
> > arch/powerpc/include/asm/kvm_ppc.h | 5 +-
> > arch/powerpc/kvm/book3s_64_mmu_hv.c | 90 ++++++++++++++++----------------
> > arch/powerpc/kvm/book3s_hv.c | 18 +++++--
> > 4 files changed, 65 insertions(+), 51 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/kvm_book3s_64.h b/arch/powerpc/include/asm/kvm_book3s_64.h
> > index 8810327..6dc4004 100644
> > --- a/arch/powerpc/include/asm/kvm_book3s_64.h
> > +++ b/arch/powerpc/include/asm/kvm_book3s_64.h
> > @@ -22,6 +22,9 @@
> >
> > #include <asm/book3s/64/mmu-hash.h>
> >
> > +/* Power architecture requires HPT is at least 256kB */
> > +#define PPC_MIN_HPT_ORDER 18
> > +
> > #ifdef CONFIG_KVM_BOOK3S_PR_POSSIBLE
> > static inline struct kvmppc_book3s_shadow_vcpu *svcpu_get(struct kvm_vcpu *vcpu)
> > {
> > diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> > index 3db6be9..41575b8 100644
> > --- a/arch/powerpc/include/asm/kvm_ppc.h
> > +++ b/arch/powerpc/include/asm/kvm_ppc.h
> > @@ -155,9 +155,10 @@ extern void kvmppc_core_destroy_mmu(struct kvm_vcpu *vcpu);
> > extern int kvmppc_kvm_pv(struct kvm_vcpu *vcpu);
> > extern void kvmppc_map_magic(struct kvm_vcpu *vcpu);
> >
> > -extern long kvmppc_alloc_hpt(struct kvm *kvm, u32 *htab_orderp);
> > +extern int kvmppc_allocate_hpt(struct kvm_hpt_info *info, u32 order);
> > +extern void kvmppc_set_hpt(struct kvm *kvm, struct kvm_hpt_info *info);
> > extern long kvmppc_alloc_reset_hpt(struct kvm *kvm, u32 *htab_orderp);
> > -extern void kvmppc_free_hpt(struct kvm *kvm);
> > +extern void kvmppc_free_hpt(struct kvm_hpt_info *info);
> > extern long kvmppc_prepare_vrma(struct kvm *kvm,
> > struct kvm_userspace_memory_region *mem);
> > extern void kvmppc_map_vrma(struct kvm_vcpu *vcpu,
> > diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > index fe88132..68bb228 100644
> > --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > @@ -40,76 +40,70 @@
> >
> > #include "trace_hv.h"
> >
> > -/* Power architecture requires HPT is at least 256kB */
> > -#define PPC_MIN_HPT_ORDER 18
> > -
> > static long kvmppc_virtmode_do_h_enter(struct kvm *kvm, unsigned long flags,
> > long pte_index, unsigned long pteh,
> > unsigned long ptel, unsigned long *pte_idx_ret);
> > static void kvmppc_rmap_reset(struct kvm *kvm);
> >
> > -long kvmppc_alloc_hpt(struct kvm *kvm, u32 *htab_orderp)
> > +int kvmppc_allocate_hpt(struct kvm_hpt_info *info, u32 order)
> > {
> > unsigned long hpt = 0;
> > - struct revmap_entry *rev;
> > + int cma = 0;
> > struct page *page = NULL;
> > - long order = KVM_DEFAULT_HPT_ORDER;
> > -
> > - if (htab_orderp) {
> > - order = *htab_orderp;
> > - if (order < PPC_MIN_HPT_ORDER)
> > - order = PPC_MIN_HPT_ORDER;
> > - }
>
> Not sure, but as far as I can see, *htab_orderp could still be provided
> from userspace via kvm_arch_vm_ioctl_hv() -> kvmppc_alloc_reset_hpt() ?
> In that case, I think you should still check that the order is bigger
> than PPC_MIN_HPT_ORDER, and return an error code otherwise?
> Or if you feel confident that this value can never be supplied by
> userspace anymore, add at least a BUG_ON(order < PPC_MIN_HPT_ORDER) here?
Ah, you're right, I do need to check this.
> > + struct revmap_entry *rev;
> > + unsigned long npte;
> >
> > - kvm->arch.hpt.cma = 0;
> > + hpt = 0;
> > + cma = 0;
>
> Both hpt and cma are initialized to zero in the variable declarations
> already, so the above two lines are redundant.
Oops. I even spotted that one at some point, then forgot about it
again.
> > page = kvm_alloc_hpt_cma(1ul << (order - PAGE_SHIFT));
> > if (page) {
> > hpt = (unsigned long)pfn_to_kaddr(page_to_pfn(page));
> > memset((void *)hpt, 0, (1ul << order));
> > - kvm->arch.hpt.cma = 1;
> > + cma = 1;
> > }
> >
> > - /* Lastly try successively smaller sizes from the page allocator */
> > - /* Only do this if userspace didn't specify a size via ioctl */
> > - while (!hpt && order > PPC_MIN_HPT_ORDER && !htab_orderp) {
> > - hpt = __get_free_pages(GFP_KERNEL|__GFP_ZERO|__GFP_REPEAT|
> > - __GFP_NOWARN, order - PAGE_SHIFT);
> > - if (!hpt)
> > - --order;
> > - }
> > + if (!hpt)
> > + hpt = __get_free_pages(GFP_KERNEL|__GFP_ZERO|__GFP_REPEAT
> > + |__GFP_NOWARN, order - PAGE_SHIFT);
> >
> > if (!hpt)
> > return -ENOMEM;
> >
> > - kvm->arch.hpt.virt = hpt;
> > - kvm->arch.hpt.order = order;
> > -
> > - atomic64_set(&kvm->arch.mmio_update, 0);
> > + /* HPTEs are 2**4 bytes long */
> > + npte = 1ul << (order - 4);
> >
> > /* Allocate reverse map array */
> > - rev = vmalloc(sizeof(struct revmap_entry) * kvmppc_hpt_npte(&kvm->arch.hpt));
> > + rev = vmalloc(sizeof(struct revmap_entry) * npte);
> > if (!rev) {
> > - pr_err("kvmppc_alloc_hpt: Couldn't alloc reverse map array\n");
> > + pr_err("kvmppc_allocate_hpt: Couldn't alloc reverse map array\n");
> > goto out_freehpt;
>
> So here you jump to out_freehpt before setting info->virt ...
>
> > }
> > - kvm->arch.hpt.rev = rev;
> > - kvm->arch.sdr1 = __pa(hpt) | (order - 18);
> >
> > - pr_info("KVM guest htab at %lx (order %ld), LPID %x\n",
> > - hpt, order, kvm->arch.lpid);
> > + info->order = order;
> > + info->virt = hpt;
> > + info->cma = cma;
> > + info->rev = rev;
> >
> > - if (htab_orderp)
> > - *htab_orderp = order;
> > return 0;
> >
> > out_freehpt:
> > - if (kvm->arch.hpt.cma)
> > + if (cma)
> > kvm_free_hpt_cma(page, 1 << (order - PAGE_SHIFT));
> > else
> > - free_pages(hpt, order - PAGE_SHIFT);
> > + free_pages(info->virt, order - PAGE_SHIFT);
>
> ... but here you free info->virt which has not been set in case of the
> "goto" above. That's clearly wrong.
Good catch. Also, there's only one use of that label, so there's not
really any reason to use a goto at all. Adjusted.
> > return -ENOMEM;
> > }
> >
> > +void kvmppc_set_hpt(struct kvm *kvm, struct kvm_hpt_info *info)
> > +{
> > + atomic64_set(&kvm->arch.mmio_update, 0);
> > + kvm->arch.hpt = *info;
> > + kvm->arch.sdr1 = __pa(info->virt) | (info->order - 18);
> > +
> > + pr_info("KVM guest htab at %lx (order %ld), LPID %x\n",
> > + info->virt, (long)info->order, kvm->arch.lpid);
>
> Not directly related to your patch, but these messages pop up in the
> dmesg each time I start a guest ... for the normal user who does not
> have a clue what "htab" means, this can be pretty much annoying. Could
> you maybe turn this into a pr_debug() instead?
Maybe, but that's not really in scope for these patches.
> > +}
> > +
> > long kvmppc_alloc_reset_hpt(struct kvm *kvm, u32 *htab_orderp)
> > {
> > long err = -EBUSY;
> > @@ -138,24 +132,28 @@ long kvmppc_alloc_reset_hpt(struct kvm *kvm, u32 *htab_orderp)
> > *htab_orderp = order;
> > err = 0;
> > } else {
> > - err = kvmppc_alloc_hpt(kvm, htab_orderp);
> > - order = *htab_orderp;
> > + struct kvm_hpt_info info;
> > +
> > + err = kvmppc_allocate_hpt(&info, *htab_orderp);
> > + if (err < 0)
> > + goto out;
> > + kvmppc_set_hpt(kvm, &info);
>
> Just a matter of taste (I dislike gotos if avoidable), but you could
> also do:
>
> err = kvmppc_allocate_hpt(&info, *htab_orderp);
> if (!err)
> kvmppc_set_hpt(kvm, &info);
>
> ... and that's even one line shorter ;-)
Hrm. I'd prefer to keep the goto: when most 1-line if statements are
the exception/failure case, it's very easy to miss that here it's the
success case.
> > }
> > out:
> > mutex_unlock(&kvm->lock);
> > return err;
> > }
> >
> > -void kvmppc_free_hpt(struct kvm *kvm)
> > +void kvmppc_free_hpt(struct kvm_hpt_info *info)
> > {
> > - kvmppc_free_lpid(kvm->arch.lpid);
> > - vfree(kvm->arch.hpt.rev);
> > - if (kvm->arch.hpt.cma)
> > - kvm_free_hpt_cma(virt_to_page(kvm->arch.hpt.virt),
> > - 1 << (kvm->arch.hpt.order - PAGE_SHIFT));
> > + vfree(info->rev);
> > + if (info->cma)
> > + kvm_free_hpt_cma(virt_to_page(info->virt),
> > + 1 << (info->order - PAGE_SHIFT));
> > else
> > - free_pages(kvm->arch.hpt.virt,
> > - kvm->arch.hpt.order - PAGE_SHIFT);
> > + free_pages(info->virt, info->order - PAGE_SHIFT);
> > + info->virt = 0;
> > + info->order = 0;
> > }
> >
> > /* Bits in first HPTE dword for pagesize 4k, 64k or 16M */
> > diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> > index 78baa2b..71c5adb 100644
> > --- a/arch/powerpc/kvm/book3s_hv.c
> > +++ b/arch/powerpc/kvm/book3s_hv.c
> > @@ -3115,11 +3115,22 @@ static int kvmppc_hv_setup_htab_rma(struct kvm_vcpu *vcpu)
> >
> > /* Allocate hashed page table (if not done already) and reset it */
> > if (!kvm->arch.hpt.virt) {
> > - err = kvmppc_alloc_hpt(kvm, NULL);
> > - if (err) {
> > + int order = KVM_DEFAULT_HPT_ORDER;
> > + struct kvm_hpt_info info;
> > +
> > + err = kvmppc_allocate_hpt(&info, order);
> > + /* If we get here, it means userspace didn't specify a
> > + * size explicitly. So, try successively smaller
> > + * sizes if the default failed. */
> > + while (err < 0 && --order >= PPC_MIN_HPT_ORDER)
> > + err = kvmppc_allocate_hpt(&info, order);
>
> Not sure, but maybe replace "err < 0" with "err == -ENOMEM" in the while
> condition? Or should it also loop on other future possible errors types?
No, I think you're right. Looping only on -ENOMEM is a better idea.
> > + if (err < 0) {
> > pr_err("KVM: Couldn't alloc HPT\n");
> > goto out;
> > }
> > +
> > + kvmppc_set_hpt(kvm, &info);
> > }
> >
> > /* Look up the memslot for guest physical address 0 */
> > @@ -3357,7 +3368,8 @@ static void kvmppc_core_destroy_vm_hv(struct kvm *kvm)
> >
> > kvmppc_free_vcores(kvm);
> >
> > - kvmppc_free_hpt(kvm);
> > + kvmppc_free_lpid(kvm->arch.lpid);
> > + kvmppc_free_hpt(&kvm->arch.hpt);
> >
> > kvmppc_free_pimap(kvm);
> > }
> >
>
> Thomas
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/linuxppc-dev/attachments/20161219/49f974ad/attachment-0001.sig>
More information about the Linuxppc-dev
mailing list