Re: [PATCH v4 06/14] KVM: s390: pv: properly handle page flags for protected guests

[Christian Borntraeger]

On 06.09.21 17:56, Claudio Imbrenda wrote:
> On Mon, 6 Sep 2021 17:46:40 +0200
> Christian Borntraeger <borntraeger@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > > Acked-by: Janosch Frank <frankja@xxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>