| Message ID | 1457974391-28456-5-git-send-email-suzuki.poulose@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 14, 2016 at 04:53:03PM +0000, Suzuki K Poulose wrote: > kvm_pmd_huge doesn't have any dependency on the page table > where the pmd lives (i.e, hyp vs. stage2). So, rename it to > huge_pmd() to make it explicit. > > kvm_p.d_* wrappers will be used for helpers which differ > across hyp vs stage2. > > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> > --- > arch/arm/kvm/mmu.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c > index a16631c..3b038bb 100644 > --- a/arch/arm/kvm/mmu.c > +++ b/arch/arm/kvm/mmu.c > @@ -44,7 +44,7 @@ static phys_addr_t hyp_idmap_vector; > > #define hyp_pgd_order get_order(PTRS_PER_PGD * sizeof(pgd_t)) > > -#define kvm_pmd_huge(_x) (pmd_huge(_x) || pmd_trans_huge(_x)) > +#define huge_pmd(_x) (pmd_huge(_x) || pmd_trans_huge(_x)) I note that in arch/arm we have pmd_thp_or_huge() for this in arch/arm/include/asm/pgtable-{2,3}level.h. If we're going to rename this, it's probably best to align on that name, which will also avoid and confusion as to the difference between pmd_huge and huge_pmd. Similarly, it might best live in pgtable.h if it isn't KVM-specific. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 14/03/16 17:06, Mark Rutland wrote: > On Mon, Mar 14, 2016 at 04:53:03PM +0000, Suzuki K Poulose wrote: >> kvm_pmd_huge doesn't have any dependency on the page table >> where the pmd lives (i.e, hyp vs. stage2). So, rename it to >> huge_pmd() to make it explicit. >> #define hyp_pgd_order get_order(PTRS_PER_PGD * sizeof(pgd_t)) >> >> -#define kvm_pmd_huge(_x) (pmd_huge(_x) || pmd_trans_huge(_x)) >> +#define huge_pmd(_x) (pmd_huge(_x) || pmd_trans_huge(_x)) > > I note that in arch/arm we have pmd_thp_or_huge() for this in > arch/arm/include/asm/pgtable-{2,3}level.h. > > If we're going to rename this, it's probably best to align on that name, > which will also avoid and confusion as to the difference between > pmd_huge and huge_pmd. > > Similarly, it might best live in pgtable.h if it isn't KVM-specific. Thanks for that pointer, will define one for arm64 and use that in kvm. Cheers Suzuki -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 14, 2016 at 04:53:03PM +0000, Suzuki K Poulose wrote: > kvm_pmd_huge doesn't have any dependency on the page table > where the pmd lives (i.e, hyp vs. stage2). So, rename it to > huge_pmd() to make it explicit. > > kvm_p.d_* wrappers will be used for helpers which differ > across hyp vs stage2. I don't understand this commit message. Do you associate the kvm_ prefix specifically with one of hyp or stage2? I remember reviewers in the past specifically asked to name anything relating to pgtable macros in the kvm code with a kvm_ prefix to distinguish them from logic used elsewhere in the kernel. I specifically do not like having huge_pmd() be significantly different in logic from pmd_huge(), so defining pmd_thp_or_huge() for arm64 is a much better option. Thanks, -Christoffer > > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> > --- > arch/arm/kvm/mmu.c | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c > index a16631c..3b038bb 100644 > --- a/arch/arm/kvm/mmu.c > +++ b/arch/arm/kvm/mmu.c > @@ -44,7 +44,7 @@ static phys_addr_t hyp_idmap_vector; > > #define hyp_pgd_order get_order(PTRS_PER_PGD * sizeof(pgd_t)) > > -#define kvm_pmd_huge(_x) (pmd_huge(_x) || pmd_trans_huge(_x)) > +#define huge_pmd(_x) (pmd_huge(_x) || pmd_trans_huge(_x)) > #define kvm_pud_huge(_x) pud_huge(_x) > > #define KVM_S2PTE_FLAG_IS_IOMAP (1UL << 0) > @@ -114,7 +114,7 @@ static bool kvm_is_device_pfn(unsigned long pfn) > */ > static void stage2_dissolve_pmd(struct kvm *kvm, phys_addr_t addr, pmd_t *pmd) > { > - if (!kvm_pmd_huge(*pmd)) > + if (!huge_pmd(*pmd)) > return; > > pmd_clear(pmd); > @@ -176,7 +176,7 @@ static void clear_pud_entry(struct kvm *kvm, pud_t *pud, phys_addr_t addr) > static void clear_pmd_entry(struct kvm *kvm, pmd_t *pmd, phys_addr_t addr) > { > pte_t *pte_table = pte_offset_kernel(pmd, 0); > - VM_BUG_ON(kvm_pmd_huge(*pmd)); > + VM_BUG_ON(huge_pmd(*pmd)); > pmd_clear(pmd); > kvm_tlb_flush_vmid_ipa(kvm, addr); > pte_free_kernel(NULL, pte_table); > @@ -239,7 +239,7 @@ static void unmap_pmds(struct kvm *kvm, pud_t *pud, > do { > next = kvm_pmd_addr_end(addr, end); > if (!pmd_none(*pmd)) { > - if (kvm_pmd_huge(*pmd)) { > + if (huge_pmd(*pmd)) { > pmd_t old_pmd = *pmd; > > pmd_clear(pmd); > @@ -325,7 +325,7 @@ static void stage2_flush_pmds(struct kvm *kvm, pud_t *pud, > do { > next = kvm_pmd_addr_end(addr, end); > if (!pmd_none(*pmd)) { > - if (kvm_pmd_huge(*pmd)) > + if (huge_pmd(*pmd)) > kvm_flush_dcache_pmd(*pmd); > else > stage2_flush_ptes(kvm, pmd, addr, next); > @@ -1043,7 +1043,7 @@ static void stage2_wp_pmds(pud_t *pud, phys_addr_t addr, phys_addr_t end) > do { > next = kvm_pmd_addr_end(addr, end); > if (!pmd_none(*pmd)) { > - if (kvm_pmd_huge(*pmd)) { > + if (huge_pmd(*pmd)) { > if (!kvm_s2pmd_readonly(pmd)) > kvm_set_s2pmd_readonly(pmd); > } else { > @@ -1324,7 +1324,7 @@ static void handle_access_fault(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa) > if (!pmd || pmd_none(*pmd)) /* Nothing there */ > goto out; > > - if (kvm_pmd_huge(*pmd)) { /* THP, HugeTLB */ > + if (huge_pmd(*pmd)) { /* THP, HugeTLB */ > *pmd = pmd_mkyoung(*pmd); > pfn = pmd_pfn(*pmd); > pfn_valid = true; > @@ -1532,7 +1532,7 @@ static int kvm_age_hva_handler(struct kvm *kvm, gpa_t gpa, void *data) > if (!pmd || pmd_none(*pmd)) /* Nothing there */ > return 0; > > - if (kvm_pmd_huge(*pmd)) { /* THP, HugeTLB */ > + if (huge_pmd(*pmd)) { /* THP, HugeTLB */ > if (pmd_young(*pmd)) { > *pmd = pmd_mkold(*pmd); > return 1; > @@ -1562,7 +1562,7 @@ static int kvm_test_age_hva_handler(struct kvm *kvm, gpa_t gpa, void *data) > if (!pmd || pmd_none(*pmd)) /* Nothing there */ > return 0; > > - if (kvm_pmd_huge(*pmd)) /* THP, HugeTLB */ > + if (huge_pmd(*pmd)) /* THP, HugeTLB */ > return pmd_young(*pmd); > > pte = pte_offset_kernel(pmd, gpa); > -- > 1.7.9.5 > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22/03/16 08:55, Christoffer Dall wrote: > On Mon, Mar 14, 2016 at 04:53:03PM +0000, Suzuki K Poulose wrote: >> kvm_pmd_huge doesn't have any dependency on the page table >> where the pmd lives (i.e, hyp vs. stage2). So, rename it to >> huge_pmd() to make it explicit. >> >> kvm_p.d_* wrappers will be used for helpers which differ >> across hyp vs stage2. > > I don't understand this commit message. Do you associate the kvm_ > prefix specifically with one of hyp or stage2? So the idea is kvm_ prefix will be used for handling either hyp or stage2 depending on what we are dealing with (i.e, kvm parameter to the helpers). So here, we just want to know if a given pmd represents a huge page, either via thp or via hugetlb and that doesn't have anything to do with hyp or stage2. Hence the change. As > > I remember reviewers in the past specifically asked to name anything > relating to pgtable macros in the kvm code with a kvm_ prefix to > distinguish them from logic used elsewhere in the kernel. Correct. In this case it doesn't apply to kvm_pmd_huge(). > > I specifically do not like having huge_pmd() be significantly different > in logic from pmd_huge(), so defining pmd_thp_or_huge() for arm64 is a > much better option. Yes, I have switched to that in the next version. Thanks Suzuki -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c index a16631c..3b038bb 100644 --- a/arch/arm/kvm/mmu.c +++ b/arch/arm/kvm/mmu.c @@ -44,7 +44,7 @@ static phys_addr_t hyp_idmap_vector; #define hyp_pgd_order get_order(PTRS_PER_PGD * sizeof(pgd_t)) -#define kvm_pmd_huge(_x) (pmd_huge(_x) || pmd_trans_huge(_x)) +#define huge_pmd(_x) (pmd_huge(_x) || pmd_trans_huge(_x)) #define kvm_pud_huge(_x) pud_huge(_x) #define KVM_S2PTE_FLAG_IS_IOMAP (1UL << 0) @@ -114,7 +114,7 @@ static bool kvm_is_device_pfn(unsigned long pfn) */ static void stage2_dissolve_pmd(struct kvm *kvm, phys_addr_t addr, pmd_t *pmd) { - if (!kvm_pmd_huge(*pmd)) + if (!huge_pmd(*pmd)) return; pmd_clear(pmd); @@ -176,7 +176,7 @@ static void clear_pud_entry(struct kvm *kvm, pud_t *pud, phys_addr_t addr) static void clear_pmd_entry(struct kvm *kvm, pmd_t *pmd, phys_addr_t addr) { pte_t *pte_table = pte_offset_kernel(pmd, 0); - VM_BUG_ON(kvm_pmd_huge(*pmd)); + VM_BUG_ON(huge_pmd(*pmd)); pmd_clear(pmd); kvm_tlb_flush_vmid_ipa(kvm, addr); pte_free_kernel(NULL, pte_table); @@ -239,7 +239,7 @@ static void unmap_pmds(struct kvm *kvm, pud_t *pud, do { next = kvm_pmd_addr_end(addr, end); if (!pmd_none(*pmd)) { - if (kvm_pmd_huge(*pmd)) { + if (huge_pmd(*pmd)) { pmd_t old_pmd = *pmd; pmd_clear(pmd); @@ -325,7 +325,7 @@ static void stage2_flush_pmds(struct kvm *kvm, pud_t *pud, do { next = kvm_pmd_addr_end(addr, end); if (!pmd_none(*pmd)) { - if (kvm_pmd_huge(*pmd)) + if (huge_pmd(*pmd)) kvm_flush_dcache_pmd(*pmd); else stage2_flush_ptes(kvm, pmd, addr, next); @@ -1043,7 +1043,7 @@ static void stage2_wp_pmds(pud_t *pud, phys_addr_t addr, phys_addr_t end) do { next = kvm_pmd_addr_end(addr, end); if (!pmd_none(*pmd)) { - if (kvm_pmd_huge(*pmd)) { + if (huge_pmd(*pmd)) { if (!kvm_s2pmd_readonly(pmd)) kvm_set_s2pmd_readonly(pmd); } else { @@ -1324,7 +1324,7 @@ static void handle_access_fault(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa) if (!pmd || pmd_none(*pmd)) /* Nothing there */ goto out; - if (kvm_pmd_huge(*pmd)) { /* THP, HugeTLB */ + if (huge_pmd(*pmd)) { /* THP, HugeTLB */ *pmd = pmd_mkyoung(*pmd); pfn = pmd_pfn(*pmd); pfn_valid = true; @@ -1532,7 +1532,7 @@ static int kvm_age_hva_handler(struct kvm *kvm, gpa_t gpa, void *data) if (!pmd || pmd_none(*pmd)) /* Nothing there */ return 0; - if (kvm_pmd_huge(*pmd)) { /* THP, HugeTLB */ + if (huge_pmd(*pmd)) { /* THP, HugeTLB */ if (pmd_young(*pmd)) { *pmd = pmd_mkold(*pmd); return 1; @@ -1562,7 +1562,7 @@ static int kvm_test_age_hva_handler(struct kvm *kvm, gpa_t gpa, void *data) if (!pmd || pmd_none(*pmd)) /* Nothing there */ return 0; - if (kvm_pmd_huge(*pmd)) /* THP, HugeTLB */ + if (huge_pmd(*pmd)) /* THP, HugeTLB */ return pmd_young(*pmd); pte = pte_offset_kernel(pmd, gpa);
kvm_pmd_huge doesn't have any dependency on the page table where the pmd lives (i.e, hyp vs. stage2). So, rename it to huge_pmd() to make it explicit. kvm_p.d_* wrappers will be used for helpers which differ across hyp vs stage2. Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> --- arch/arm/kvm/mmu.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)