Messages in this thread | | | Subject | Re: [PATCH v4 06/14] KVM: s390: pv: properly handle page flags for protected guests | From | Christian Borntraeger <> | Date | Mon, 6 Sep 2021 18:16:10 +0200 |
| |
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>
| |