Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v4 06/14] KVM: s390: pv: properly handle page flags for
 protected guests
To:     Claudio Imbrenda <imbrenda@linux.ibm.com>
Cc:     kvm@vger.kernel.org, cohuck@redhat.com, frankja@linux.ibm.com,
        thuth@redhat.com, pasic@linux.ibm.com, david@redhat.com,
        linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org,
        Ulrich.Weigand@de.ibm.com,
        Gerald Schaefer <gerald.schaefer@linux.ibm.com>
References: <20210818132620.46770-1-imbrenda@linux.ibm.com>
 <20210818132620.46770-7-imbrenda@linux.ibm.com>
 <1a44ff5c-f59f-2f37-2585-084294ed5e11@de.ibm.com>
 <20210906175618.4ce0323f@p-imbrenda>
From:   Christian Borntraeger <borntraeger@de.ibm.com>
Message-ID: <35e4b7a3-42d8-6b8f-e2e7-5b6a81dfcfa3@de.ibm.com>
Date:   Mon, 6 Sep 2021 18:16:10 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.13.0
MIME-Version: 1.0
In-Reply-To: <20210906175618.4ce0323f@p-imbrenda>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 06.09.21 17:56, Claudio Imbrenda wrote:
> On Mon, 6 Sep 2021 17:46:40 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 18.08.21 15:26, Claudio Imbrenda wrote:
>>> Introduce variants of the convert and destroy page functions that also
>>> clear the PG_arch_1 bit used to mark them as secure pages.
>>>
>>> These new functions can only be called on pages for which a reference
>>> is already being held.
>>>
>>> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
>>> Acked-by: Janosch Frank <frankja@linux.ibm.com>
>>
>> Can you refresh my mind? We do have over-indication of PG_arch_1 and this
>> might result in spending some unneeded cycles but in the end this will be
>> correct. Right?
>> And this patch will fix some unnecessary places that add overindication.
> 
> correct, PG_arch_1 will still overindicate, but with this patch it will
> happen less.
> 
> And PG_arch_1 overindication is perfectly fine from a correctness point
> of view.

Maybe add something like this to the patch description then.

>>> +/*
>>> + * The caller must already hold a reference to the page
>>> + */
>>> +int uv_destroy_owned_page(unsigned long paddr)
>>> +{
>>> +	struct page *page = phys_to_page(paddr);

Do we have to protect against weird mappings without struct page here? I have not
followed the discussion about this topic. Maybe Gerald knows if we can have memory
without struct pages.

>>> +	int rc;
>>> +
>>> +	get_page(page);
>>> +	rc = uv_destroy_page(paddr);
>>> +	if (!rc)
>>> +		clear_bit(PG_arch_1, &page->flags);
>>> +	put_page(page);
>>> +	return rc;
>>> +}
>>> +
>>>    /*
>>>     * Requests the Ultravisor to encrypt a guest page and make it
>>>     * accessible to the host for paging (export).
>>> @@ -154,6 +170,22 @@ int uv_convert_from_secure(unsigned long paddr)
>>>    	return 0;
>>>    }
>>>    
>>> +/*
>>> + * The caller must already hold a reference to the page
>>> + */
>>> +int uv_convert_owned_from_secure(unsigned long paddr)
>>> +{
>>> +	struct page *page = phys_to_page(paddr);

Same here. If this is not an issue (and you add something to the patch description as
outlined above)

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>