On Thu, 26 Jan 2023 12:35:37 -0400
Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
> On Thu, Jan 26, 2023 at 04:39:02PM +0100, Claudio Imbrenda wrote:
>
> > I can tell you that the original goal of that function is to make sure
> > that there are no extra references. in particular, we want to prevent
> > I/O of any kind to be ongoing while the page becomes secure. (the I/O
> > will fail and, depending on which device it was, the whole system might
> > end up in a rather unhappy state)
>
> Sure, but if there is concurrent IO you just try again right? It
> doesn't wait for refs to drop for instance.
>
> So make the lock_page work the same way:
the more I look at this, the less I understand why I wrote the code
like that. I do have a comment, though
>
> diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
> index 9f18a4af9c1319..847ee50b8672c6 100644
> --- a/arch/s390/kernel/uv.c
> +++ b/arch/s390/kernel/uv.c
> @@ -193,20 +193,11 @@ static int expected_page_refs(struct page *page)
> }
>
> static int make_secure_pte(pte_t *ptep, unsigned long addr,
> - struct page *exp_page, struct uv_cb_header *uvcb)
> + struct page *page, struct uv_cb_header *uvcb)
> {
> pte_t entry = READ_ONCE(*ptep);
> - struct page *page;
> int expected, cc = 0;
>
> - if (!pte_present(entry))
> - return -ENXIO;
> - if (pte_val(entry) & _PAGE_INVALID)
> - return -ENXIO;
> -
> - page = pte_page(entry);
> - if (page != exp_page)
> - return -ENXIO;
> if (PageWriteback(page))
> return -EAGAIN;
> expected = expected_page_refs(page);
> @@ -304,17 +295,25 @@ int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb)
> goto out;
>
> rc = -ENXIO;
> - page = follow_page(vma, uaddr, FOLL_WRITE);
> - if (IS_ERR_OR_NULL(page))
> - goto out;
> -
> - lock_page(page);
> ptep = get_locked_pte(gmap->mm, uaddr, &ptelock);
> +
> + if (!pte_present(entry))
> + goto out_unlock_pte;
> + if (pte_val(entry) & _PAGE_INVALID)
> + goto out_unlock_pte;
I guess we also need to make sure the page was writable?
FOLL_WRITE made sure of that
so I guess something like:
if (!pte_write(entry))
goto out_unlock_pte;
> + page = pte_page(entry);
> +
> + if (!trylock_page(page)) {
> + rc = -EAGAIN;
> + goto out_unlock_pte;
> + }
> +
> if (should_export_before_import(uvcb, gmap->mm))
> uv_convert_from_secure(page_to_phys(page));
> rc = make_secure_pte(ptep, uaddr, page, uvcb);
> - pte_unmap_unlock(ptep, ptelock);
> unlock_page(page);
> +out_unlock_pte:
> + pte_unmap_unlock(ptep, ptelock);
> out:
> mmap_read_unlock(gmap->mm);
>
