From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 0339128E1 for ; Thu, 9 Feb 2023 22:48:25 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1675982904; h=from:from:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=WZaDBzx58JAMjEn7EDEgD2XWnnJnFhK/yUPI3hFd8g8=; b=M1sqeiCjC/xRdQo2hJgn4O+CneCzC2aJqkkyAWiRaonceq8gp5k2lymKDF9p11xQ8Uu9qD 9oKG9cW41Fw+2LaBvwvMav7wbd3SaglpXXxiJIIoUEhz55eTzE5OZe9OYFQpxEn5xsFXYd tw1mzQ3+Dt1NDLBkc2j2hNkmfKPa3fQ= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-137-vfsIj0iqMe-BmbPY66kaMw-1; Thu, 09 Feb 2023 17:48:22 -0500 X-MC-Unique: vfsIj0iqMe-BmbPY66kaMw-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id D490585A588; Thu, 9 Feb 2023 22:48:20 +0000 (UTC) Received: from [10.64.54.63] (vpn2-54-63.bne.redhat.com [10.64.54.63]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 563582166B29; Thu, 9 Feb 2023 22:48:14 +0000 (UTC) Reply-To: Gavin Shan Subject: Re: [PATCH v2 04/12] KVM: arm64: Add kvm_pgtable_stage2_split() To: Ricardo Koller Cc: pbonzini@redhat.com, maz@kernel.org, oupton@google.com, yuzenghui@huawei.com, dmatlack@google.com, kvm@vger.kernel.org, kvmarm@lists.linux.dev, qperret@google.com, catalin.marinas@arm.com, andrew.jones@linux.dev, seanjc@google.com, alexandru.elisei@arm.com, suzuki.poulose@arm.com, eric.auger@redhat.com, reijiw@google.com, rananta@google.com, bgardon@google.com, ricarkol@gmail.com References: <20230206165851.3106338-1-ricarkol@google.com> <20230206165851.3106338-5-ricarkol@google.com> From: Gavin Shan Message-ID: Date: Fri, 10 Feb 2023 09:48:11 +1100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.0 Precedence: bulk X-Mailing-List: kvmarm@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 3.1 on 10.11.54.6 On 2/10/23 3:17 AM, Ricardo Koller wrote: > "(> > > + if (data->mc_capacity < nr_pages) >>>> + return -ENOMEM; >>>> + >>>> + phys = kvm_pte_to_phys(pte); >>>> + prot = kvm_pgtable_stage2_pte_prot(pte); >>>> + >>>> + ret = kvm_pgtable_stage2_create_unlinked(data->mmu->pgt, &new, phys, >>>> + level, prot, mc, force_pte); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + if (!stage2_try_break_pte(ctx, data->mmu)) { >>>> + childp = kvm_pte_follow(new, mm_ops); >>>> + kvm_pgtable_stage2_free_unlinked(mm_ops, childp, level); >>>> + mm_ops->put_page(childp); >>>> + return -EAGAIN; >>>> + } >>>> + >>>> + /* >>>> + * Note, the contents of the page table are guaranteed to be made >>>> + * visible before the new PTE is assigned because stage2_make_pte() >>>> + * writes the PTE using smp_store_release(). >>>> + */ >>>> + stage2_make_pte(ctx, new); >>>> + dsb(ishst); >>>> + data->mc_capacity -= nr_pages; >>>> + return 0; >>>> +} >>>> + >>> >>> I think it's possible 'data->mc_capability' to be replaced by 'mc->nobjs' >>> because they're same thing. With this, we needn't to maintain a duplicate >>> 'data->mc_capability' since 'data->mc' has been existing. >> >> Ah, nice, yes. That would be simpler. >> > > Actually, there's a complication. The memcache details are hidden > inside of pgtable.c, > so different types of memcaches (for vhe and nvhe) can be used for allocations. > Specifically, the memcache objects are passed as an opaque pointer ("void *") > and can be used with "struct hyp_pool" and "struct kvm_mmu_memory_cache". > > So, here are all the options that I can think of: > > 1. stage2_split_walker() is just used on the VHE case with the > "struct kvm_mmu_memory_cache" memcache, so we could just use it > instead of a "void *": > > kvm_pgtable_stage2_split(..., struct kvm_mmu_memory_cache *mc); > > However, it could be used for the NVHE case as well, plus > this would go against the overall design of pgtable.c which tries > to use opaque objects for most things. > > 2. add a "get_nobjs()" method to both memcaches. This is tricky > because "struct hyp_pool" doesn't directly track its capacity. I > would rather not mess with it. > > 3. This whole accounting of available pages in the memcache is > needed because of the way I implemented stage2_split_walker() and > the memcache interface. stage2_split_walker() tries to allocate > as many pages for the new table as allowed by the capacity of the > memcache. The issue with blindingly trying until the allocation > fails is that kvm_mmu_memory_cache_alloc() WARNs and tries to > allocate using GFP_ATOMIC when !nobjs. We don't want to do that, > so we could extend kvm_pgtable_mm_ops.zalloc_page() with a > NO_GFP_ATOMIC_ON_EMPTY (or similar). This flag would have to be > ignored on the hyp side. > > 4. what this patch is currently doing: tracking the capacity by > hand. > > I prefer options 4 and 3. WDYT? > Yeah, stage2_split_walker() is currently only used by VHE and it calls to stage2_map_walker() to create the unlinked page table, which is shared by VHE and nVHE. I think option 3 would be better than 4 because we generally just want to fetch the pre-allocated page table, instead of allocating a new page table with GFP_ATOMIC. However, I'm also fine with option 4. I think this can be improved in the future if you agree. >> >>> >>>> +int kvm_pgtable_stage2_split(struct kvm_pgtable *pgt, u64 addr, u64 size, >>>> + void *mc, u64 mc_capacity) >>>> +{ >>>> + struct stage2_split_data split_data = { >>>> + .mmu = pgt->mmu, >>>> + .memcache = mc, >>>> + .mc_capacity = mc_capacity, >>>> + }; >>>> + >>>> + struct kvm_pgtable_walker walker = { >>>> + .cb = stage2_split_walker, >>>> + .flags = KVM_PGTABLE_WALK_LEAF, >>>> + .arg = &split_data, >>>> + }; >>>> + >>>> + return kvm_pgtable_walk(pgt, addr, size, &walker); >>>> +} >>>> + >>>> int __kvm_pgtable_stage2_init(struct kvm_pgtable *pgt, struct kvm_s2_mmu *mmu, >>>> struct kvm_pgtable_mm_ops *mm_ops, >>>> enum kvm_pgtable_stage2_flags flags, >>>> >>> Thanks, Gavin