From: Thomas Huth <thuth@redhat.com>
To: David Gibson <david@gibson.dropbear.id.au>, paulus@samba.org
Cc: michael@ellerman.id.au, benh@kernel.crashing.org,
sjitindarsingh@gmail.com, lvivier@redhat.com,
linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
kvm@vger.kernel.org
Subject: Re: [PATCH 04/11] powerpc/kvm: Don't store values derivable from HPT order
Date: Fri, 16 Dec 2016 11:39:26 +0100 [thread overview]
Message-ID: <db1ccf7b-7355-e0d4-ac36-26cc9c833105@redhat.com> (raw)
In-Reply-To: <20161215055404.29351-5-david@gibson.dropbear.id.au>
On 15.12.2016 06:53, David Gibson wrote:
> Currently the kvm_hpt_info structure stores the hashed page table's order,
> and also the number of HPTEs it contains and a mask for its size. The
> last two can be easily derived from the order, so remove them and just
> calculate them as necessary with a couple of helper inlines.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
[...]
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index b5799d1..fe88132 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -83,15 +83,11 @@ long kvmppc_alloc_hpt(struct kvm *kvm, u32 *htab_orderp)
>
> kvm->arch.hpt.virt = hpt;
> kvm->arch.hpt.order = order;
> - /* HPTEs are 2**4 bytes long */
> - kvm->arch.hpt.npte = 1ul << (order - 4);
> - /* 128 (2**7) bytes in each HPTEG */
> - kvm->arch.hpt.mask = (1ul << (order - 7)) - 1;
>
> atomic64_set(&kvm->arch.mmio_update, 0);
>
> /* Allocate reverse map array */
> - rev = vmalloc(sizeof(struct revmap_entry) * kvm->arch.hpt.npte);
> + rev = vmalloc(sizeof(struct revmap_entry) * kvmppc_hpt_npte(&kvm->arch.hpt));
> if (!rev) {
> pr_err("kvmppc_alloc_hpt: Couldn't alloc reverse map array\n");
> goto out_freehpt;
> @@ -194,8 +190,8 @@ void kvmppc_map_vrma(struct kvm_vcpu *vcpu, struct kvm_memory_slot *memslot,
> if (npages > 1ul << (40 - porder))
> npages = 1ul << (40 - porder);
> /* Can't use more than 1 HPTE per HPTEG */
> - if (npages > kvm->arch.hpt.mask + 1)
> - npages = kvm->arch.hpt.mask + 1;
> + if (npages > kvmppc_hpt_mask(&kvm->arch.hpt) + 1)
> + npages = kvmppc_hpt_mask(&kvm->arch.hpt) + 1;
>
> hp0 = HPTE_V_1TB_SEG | (VRMA_VSID << (40 - 16)) |
> HPTE_V_BOLTED | hpte0_pgsize_encoding(psize);
> @@ -205,7 +201,8 @@ void kvmppc_map_vrma(struct kvm_vcpu *vcpu, struct kvm_memory_slot *memslot,
> for (i = 0; i < npages; ++i) {
> addr = i << porder;
> /* can't use hpt_hash since va > 64 bits */
> - hash = (i ^ (VRMA_VSID ^ (VRMA_VSID << 25))) & kvm->arch.hpt.mask;
> + hash = (i ^ (VRMA_VSID ^ (VRMA_VSID << 25)))
> + & kvmppc_hpt_mask(&kvm->arch.hpt);
> /*
> * We assume that the hash table is empty and no
> * vcpus are using it at this stage. Since we create
kvmppc_hpt_mask() is now called three times in kvmppc_map_vrma() ... you
could use a local variable to store the value so that the calculation
has only be done once here.
> @@ -1306,7 +1303,7 @@ static ssize_t kvm_htab_read(struct file *file, char __user *buf,
>
> /* Skip uninteresting entries, i.e. clean on not-first pass */
> if (!first_pass) {
> - while (i < kvm->arch.hpt.npte &&
> + while (i < kvmppc_hpt_npte(&kvm->arch.hpt) &&
> !hpte_dirty(revp, hptp)) {
> ++i;
> hptp += 2;
> @@ -1316,7 +1313,7 @@ static ssize_t kvm_htab_read(struct file *file, char __user *buf,
> hdr.index = i;
>
> /* Grab a series of valid entries */
> - while (i < kvm->arch.hpt.npte &&
> + while (i < kvmppc_hpt_npte(&kvm->arch.hpt) &&
> hdr.n_valid < 0xffff &&
> nb + HPTE_SIZE < count &&
> record_hpte(flags, hptp, hpte, revp, 1, first_pass)) {
> @@ -1332,7 +1329,7 @@ static ssize_t kvm_htab_read(struct file *file, char __user *buf,
> ++revp;
> }
> /* Now skip invalid entries while we can */
> - while (i < kvm->arch.hpt.npte &&
> + while (i < kvmppc_hpt_npte(&kvm->arch.hpt) &&
> hdr.n_invalid < 0xffff &&
> record_hpte(flags, hptp, hpte, revp, 0, first_pass)) {
> /* found an invalid entry */
Dito, you could use a local variable to store the value from
kvmppc_hpt_npte()
Anyway, apart from these nits, patch looks fine to me, so:
Reviewed-by: Thomas Huth <thuth@redhat.com>
next prev parent reply other threads:[~2016-12-16 10:39 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-15 5:53 [PATCH 00/11] KVM implementation of PAPR HPT resizing extension David Gibson
2016-12-15 5:53 ` [PATCH 01/11] powerpc/kvm: Reserve capabilities and ioctls for HPT resizing David Gibson
2016-12-16 9:25 ` Thomas Huth
2016-12-19 6:36 ` David Gibson
2016-12-16 13:15 ` Thomas Huth
2016-12-19 6:37 ` David Gibson
2016-12-15 5:53 ` [PATCH 02/11] powerpc/kvm: Rename kvm_alloc_hpt() for clarity David Gibson
2016-12-16 9:03 ` Thomas Huth
2016-12-15 5:53 ` [PATCH 03/11] powerpc/kvm: Gather HPT related variables into sub-structure David Gibson
2016-12-16 9:24 ` Thomas Huth
2016-12-19 0:03 ` David Gibson
2016-12-15 5:53 ` [PATCH 04/11] powerpc/kvm: Don't store values derivable from HPT order David Gibson
2016-12-16 10:39 ` Thomas Huth [this message]
2016-12-19 0:04 ` David Gibson
2016-12-15 5:53 ` [PATCH 05/11] powerpc/kvm: Split HPT allocation from activation David Gibson
2016-12-16 11:57 ` Thomas Huth
2016-12-19 0:24 ` David Gibson
2016-12-15 5:53 ` [PATCH 06/11] powerpc/kvm: Allow KVM_PPC_ALLOCATE_HTAB ioctl() to change HPT size David Gibson
2016-12-16 12:44 ` Thomas Huth
2016-12-19 0:48 ` David Gibson
2016-12-19 7:49 ` Thomas Huth
2016-12-19 23:16 ` David Gibson
2016-12-15 5:54 ` [PATCH 07/11] powerpc/kvm: Create kvmppc_unmap_hpte_helper() David Gibson
2016-12-16 12:48 ` Thomas Huth
2016-12-15 5:54 ` [PATCH 08/11] powerpc/kvm: KVM-HV HPT resizing stub implementation David Gibson
2016-12-16 15:35 ` Thomas Huth
2016-12-15 5:54 ` [PATCH 09/11] powerpc/kvm: Outline of KVM-HV HPT resizing implementation David Gibson
2016-12-15 5:54 ` [PATCH 10/11] powerpc/kvm: " David Gibson
2016-12-15 5:54 ` [PATCH 11/11] powerpc/kvm: Advertise availablity of HPT resizing on KVM HV David Gibson
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=db1ccf7b-7355-e0d4-ac36-26cc9c833105@redhat.com \
--to=thuth@redhat.com \
--cc=benh@kernel.crashing.org \
--cc=david@gibson.dropbear.id.au \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=lvivier@redhat.com \
--cc=michael@ellerman.id.au \
--cc=paulus@samba.org \
--cc=sjitindarsingh@gmail.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).