Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 1 Apr 2022 17:03:20 +0200
From:   Claudio Imbrenda <imbrenda@linux.ibm.com>
To:     Janosch Frank <frankja@linux.ibm.com>
Cc:     kvm@vger.kernel.org, borntraeger@de.ibm.com, thuth@redhat.com,
        pasic@linux.ibm.com, david@redhat.com, linux-s390@vger.kernel.org,
        linux-kernel@vger.kernel.org, scgl@linux.ibm.com,
        mimu@linux.ibm.com, nrb@linux.ibm.com
Subject: Re: [PATCH v9 11/18] s390/mm: KVM: pv: when tearing down, try to
 destroy protected pages
Message-ID: <20220401170320.4b985bc9@p-imbrenda>
In-Reply-To: <a61d614f-df0a-d0a8-c1f1-45a915e26b23@linux.ibm.com>
References: <20220330122605.247613-1-imbrenda@linux.ibm.com>
        <20220330122605.247613-12-imbrenda@linux.ibm.com>
        <a61d614f-df0a-d0a8-c1f1-45a915e26b23@linux.ibm.com>
Organization: IBM
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 31 Mar 2022 15:34:42 +0200
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 3/30/22 14:25, Claudio Imbrenda wrote:
> > When ptep_get_and_clear_full is called for a mm teardown, we will now
> > attempt to destroy the secure pages. This will be faster than export.
> > 
> > In case it was not a teardown, or if for some reason the destroy page
> > UVC failed, we try with an export page, like before.
> > 
> > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> > Acked-by: Janosch Frank <frankja@linux.ibm.com>
> > ---
> >   arch/s390/include/asm/pgtable.h | 18 +++++++++++++++---
> >   1 file changed, 15 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h
> > index 23ca0d8e058a..72544a1b4a68 100644
> > --- a/arch/s390/include/asm/pgtable.h
> > +++ b/arch/s390/include/asm/pgtable.h
> > @@ -1118,9 +1118,21 @@ static inline pte_t ptep_get_and_clear_full(struct mm_struct *mm,
> >   	} else {
> >   		res = ptep_xchg_lazy(mm, addr, ptep, __pte(_PAGE_INVALID));
> >   	}
> > -	/* At this point the reference through the mapping is still present */
> > -	if (mm_is_protected(mm) && pte_present(res))
> > -		uv_convert_owned_from_secure(pte_val(res) & PAGE_MASK);
> > +	/* Nothing to do */
> > +	if (!mm_is_protected(mm) || !pte_present(res))
> > +		return res;
> > +	/*
> > +	 * At this point the reference through the mapping is still present.  
> 
> That's the case because we zap ptes within a mm that's still existing, 
> right? The mm will be deleted after we have unmapped the memory.

not exactly, the mm can exist without pages.

the reference is there because when we enter the function,
there is still a pointer to the page (the PTE itself), the page is still
reachable, and therefore its reference count cannot be less than 1.

when we leave the function, there is no more mapping for the page
(that's the point of the function after all), and only then the counter
can be decremented.

if you look at zap_pte_range in mm/memory.c, you see that we call
ptep_get_and_clear_full first, and then a few lines below we call
__tlb_remove_page which in the end calls put_page on that page.

> 
> 
> > +	 * The notifier should have destroyed all protected vCPUs at this
> > +	 * point, so the destroy should be successful.
> > +	 */
> > +	if (full && !uv_destroy_owned_page(pte_val(res) & PAGE_MASK))
> > +		return res;
> > +	/*
> > +	 * But if something went wrong and the pages could not be destroyed,
> > +	 * the slower export is used as fallback instead.
> > +	 */
> > +	uv_convert_owned_from_secure(pte_val(res) & PAGE_MASK);
> >   	return res;
> >   }
> >     
>