From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E5154C6778C for ; Thu, 5 Jul 2018 16:25:41 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 9664624095 for ; Thu, 5 Jul 2018 16:25:41 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 9664624095 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753954AbeGEQZg (ORCPT ); Thu, 5 Jul 2018 12:25:36 -0400 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:52970 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753809AbeGEQYR (ORCPT ); Thu, 5 Jul 2018 12:24:17 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id CAD907A9; Thu, 5 Jul 2018 09:24:16 -0700 (PDT) Received: from [10.1.206.75] (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 9A3673F5BA; Thu, 5 Jul 2018 09:24:15 -0700 (PDT) Subject: Re: [PATCH v4 1/7] KVM: arm/arm64: Share common code in user_mem_abort() To: Punit Agrawal , kvmarm@lists.cs.columbia.edu Cc: linux-arm-kernel@lists.infradead.org, christoffer.dall@arm.com, linux-kernel@vger.kernel.org, suzuki.poulose@arm.com References: <20180705140850.5801-1-punit.agrawal@arm.com> <20180705140850.5801-2-punit.agrawal@arm.com> From: Marc Zyngier Organization: ARM Ltd Message-ID: Date: Thu, 5 Jul 2018 17:24:14 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <20180705140850.5801-2-punit.agrawal@arm.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-GB Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Punit, On 05/07/18 15:08, Punit Agrawal wrote: > The code for operations such as marking the pfn as dirty, and > dcache/icache maintenance during stage 2 fault handling is duplicated > between normal pages and PMD hugepages. > > Instead of creating another copy of the operations when we introduce > PUD hugepages, let's share them across the different pagesizes. > > Signed-off-by: Punit Agrawal > Cc: Christoffer Dall > Cc: Marc Zyngier > --- > virt/kvm/arm/mmu.c | 68 +++++++++++++++++++++++++++------------------- > 1 file changed, 40 insertions(+), 28 deletions(-) > > diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c > index 1d90d79706bd..dd14cc36c51c 100644 > --- a/virt/kvm/arm/mmu.c > +++ b/virt/kvm/arm/mmu.c > @@ -1398,6 +1398,21 @@ static void invalidate_icache_guest_page(kvm_pfn_t pfn, unsigned long size) > __invalidate_icache_guest_page(pfn, size); > } > > +static bool stage2_should_exec(struct kvm *kvm, phys_addr_t addr, > + bool exec_fault, unsigned long fault_status) I find this "should exec" very confusing. > +{ > + /* > + * If we took an execution fault we will have made the > + * icache/dcache coherent and should now let the s2 mapping be > + * executable. > + * > + * Write faults (!exec_fault && FSC_PERM) are orthogonal to > + * execute permissions, and we preserve whatever we have. > + */ > + return exec_fault || > + (fault_status == FSC_PERM && stage2_is_exec(kvm, addr)); > +} > + > static void kvm_send_hwpoison_signal(unsigned long address, > struct vm_area_struct *vma) > { > @@ -1431,7 +1446,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, > kvm_pfn_t pfn; > pgprot_t mem_type = PAGE_S2; > bool logging_active = memslot_is_logging(memslot); > - unsigned long flags = 0; > + unsigned long vma_pagesize, flags = 0; > > write_fault = kvm_is_write_fault(vcpu); > exec_fault = kvm_vcpu_trap_is_iabt(vcpu); > @@ -1451,7 +1466,8 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, > return -EFAULT; > } > > - if (vma_kernel_pagesize(vma) == PMD_SIZE && !logging_active) { > + vma_pagesize = vma_kernel_pagesize(vma); > + if (vma_pagesize == PMD_SIZE && !logging_active) { > hugetlb = true; > gfn = (fault_ipa & PMD_MASK) >> PAGE_SHIFT; > } else { > @@ -1520,28 +1536,34 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, > if (mmu_notifier_retry(kvm, mmu_seq)) > goto out_unlock; > > - if (!hugetlb && !force_pte) > + if (!hugetlb && !force_pte) { > + /* > + * Only PMD_SIZE transparent hugepages(THP) are > + * currently supported. This code will need to be > + * updated to support other THP sizes. > + */ > hugetlb = transparent_hugepage_adjust(&pfn, &fault_ipa); > + if (hugetlb) > + vma_pagesize = PMD_SIZE; > + } > + > + if (writable) > + kvm_set_pfn_dirty(pfn); > > - if (hugetlb) { > + if (fault_status != FSC_PERM) > + clean_dcache_guest_page(pfn, vma_pagesize); > + > + if (exec_fault) > + invalidate_icache_guest_page(pfn, vma_pagesize); > + > + if (hugetlb && vma_pagesize == PMD_SIZE) { > pmd_t new_pmd = pfn_pmd(pfn, mem_type); > new_pmd = pmd_mkhuge(new_pmd); > - if (writable) { > + if (writable) > new_pmd = kvm_s2pmd_mkwrite(new_pmd); > - kvm_set_pfn_dirty(pfn); > - } > > - if (fault_status != FSC_PERM) > - clean_dcache_guest_page(pfn, PMD_SIZE); > - > - if (exec_fault) { > + if (stage2_should_exec(kvm, fault_ipa, exec_fault, fault_status)) > new_pmd = kvm_s2pmd_mkexec(new_pmd); OK, I find this absolutely horrid... ;-) The rest of the function deals with discrete flags, and all of a sudden we have a function call with a bunch of seemingly unrelated parameters. And you are repeating it for each vma_pagesize... How about something like: bool needs_exec; [...] needs_exec = exec_fault || (fault_status == FSC_PERM && stage2_is_exec(kvm, fault_ipa); And then you just check needs_exec to update the pte/pmd. And you drop this helper. > - invalidate_icache_guest_page(pfn, PMD_SIZE); > - } else if (fault_status == FSC_PERM) { > - /* Preserve execute if XN was already cleared */ > - if (stage2_is_exec(kvm, fault_ipa)) > - new_pmd = kvm_s2pmd_mkexec(new_pmd); > - } > > ret = stage2_set_pmd_huge(kvm, memcache, fault_ipa, &new_pmd); > } else { > @@ -1549,21 +1571,11 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, > > if (writable) { > new_pte = kvm_s2pte_mkwrite(new_pte); > - kvm_set_pfn_dirty(pfn); > mark_page_dirty(kvm, gfn); > } > > - if (fault_status != FSC_PERM) > - clean_dcache_guest_page(pfn, PAGE_SIZE); > - > - if (exec_fault) { > + if (stage2_should_exec(kvm, fault_ipa, exec_fault, fault_status)) > new_pte = kvm_s2pte_mkexec(new_pte); > - invalidate_icache_guest_page(pfn, PAGE_SIZE); > - } else if (fault_status == FSC_PERM) { > - /* Preserve execute if XN was already cleared */ > - if (stage2_is_exec(kvm, fault_ipa)) > - new_pte = kvm_s2pte_mkexec(new_pte); > - } > > ret = stage2_set_pte(kvm, memcache, fault_ipa, &new_pte, flags); > } > Thanks, M. -- Jazz is not dead. It just smells funny...