Message ID | 20210818132620.46770-7-imbrenda@linux.ibm.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | KVM: s390: pv: implement lazy destroy for reboot | expand |
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. > --- > arch/s390/include/asm/pgtable.h | 9 ++++++--- > arch/s390/include/asm/uv.h | 10 ++++++++-- > arch/s390/kernel/uv.c | 34 ++++++++++++++++++++++++++++++++- > arch/s390/mm/gmap.c | 4 +++- > 4 files changed, 50 insertions(+), 7 deletions(-) > > diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h > index dcac7b2df72c..0f1af2232ebe 100644 > --- a/arch/s390/include/asm/pgtable.h > +++ b/arch/s390/include/asm/pgtable.h > @@ -1074,8 +1074,9 @@ static inline pte_t ptep_get_and_clear(struct mm_struct *mm, > pte_t res; > > 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_from_secure(pte_val(res) & PAGE_MASK); > + uv_convert_owned_from_secure(pte_val(res) & PAGE_MASK); > return res; > } > > @@ -1091,8 +1092,9 @@ static inline pte_t ptep_clear_flush(struct vm_area_struct *vma, > pte_t res; > > res = ptep_xchg_direct(vma->vm_mm, addr, ptep, __pte(_PAGE_INVALID)); > + /* At this point the reference through the mapping is still present */ > if (mm_is_protected(vma->vm_mm) && pte_present(res)) > - uv_convert_from_secure(pte_val(res) & PAGE_MASK); > + uv_convert_owned_from_secure(pte_val(res) & PAGE_MASK); > return res; > } > > @@ -1116,8 +1118,9 @@ 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_from_secure(pte_val(res) & PAGE_MASK); > + uv_convert_owned_from_secure(pte_val(res) & PAGE_MASK); > return res; > } > > diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h > index b35add51b967..3236293d5a31 100644 > --- a/arch/s390/include/asm/uv.h > +++ b/arch/s390/include/asm/uv.h > @@ -356,8 +356,9 @@ static inline int is_prot_virt_host(void) > } > > int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb); > -int uv_destroy_page(unsigned long paddr); > +int uv_destroy_owned_page(unsigned long paddr); > int uv_convert_from_secure(unsigned long paddr); > +int uv_convert_owned_from_secure(unsigned long paddr); > int gmap_convert_to_secure(struct gmap *gmap, unsigned long gaddr); > > void setup_uv(void); > @@ -367,7 +368,7 @@ void adjust_to_uv_max(unsigned long *vmax); > static inline void setup_uv(void) {} > static inline void adjust_to_uv_max(unsigned long *vmax) {} > > -static inline int uv_destroy_page(unsigned long paddr) > +static inline int uv_destroy_owned_page(unsigned long paddr) > { > return 0; > } > @@ -376,6 +377,11 @@ static inline int uv_convert_from_secure(unsigned long paddr) > { > return 0; > } > + > +static inline int uv_convert_owned_from_secure(unsigned long paddr) > +{ > + return 0; > +} > #endif > > #if defined(CONFIG_PROTECTED_VIRTUALIZATION_GUEST) || IS_ENABLED(CONFIG_KVM) > diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c > index 68a8fbafcb9c..05f8bf61d20a 100644 > --- a/arch/s390/kernel/uv.c > +++ b/arch/s390/kernel/uv.c > @@ -115,7 +115,7 @@ static int uv_pin_shared(unsigned long paddr) > * > * @paddr: Absolute host address of page to be destroyed > */ > -int uv_destroy_page(unsigned long paddr) > +static int uv_destroy_page(unsigned long paddr) > { > struct uv_cb_cfs uvcb = { > .header.cmd = UVC_CMD_DESTR_SEC_STOR, > @@ -135,6 +135,22 @@ int uv_destroy_page(unsigned long paddr) > return 0; > } > > +/* > + * 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); > + 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); > + int rc; > + > + get_page(page); > + rc = uv_convert_from_secure(paddr); > + if (!rc) > + clear_bit(PG_arch_1, &page->flags); > + put_page(page); > + return rc; > +} > + > /* > * Calculate the expected ref_count for a page that would otherwise have no > * further pins. This was cribbed from similar functions in other places in > diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c > index 5a138f6220c4..38b792ab57f7 100644 > --- a/arch/s390/mm/gmap.c > +++ b/arch/s390/mm/gmap.c > @@ -2678,8 +2678,10 @@ static int __s390_reset_acc(pte_t *ptep, unsigned long addr, > { > pte_t pte = READ_ONCE(*ptep); > > + /* There is a reference through the mapping */ > if (pte_present(pte)) > - WARN_ON_ONCE(uv_destroy_page(pte_val(pte) & PAGE_MASK)); > + WARN_ON_ONCE(uv_destroy_owned_page(pte_val(pte) & PAGE_MASK)); > + > return 0; > } > >
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. > > --- > > arch/s390/include/asm/pgtable.h | 9 ++++++--- > > arch/s390/include/asm/uv.h | 10 ++++++++-- > > arch/s390/kernel/uv.c | 34 ++++++++++++++++++++++++++++++++- > > arch/s390/mm/gmap.c | 4 +++- > > 4 files changed, 50 insertions(+), 7 deletions(-) > > > > diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h > > index dcac7b2df72c..0f1af2232ebe 100644 > > --- a/arch/s390/include/asm/pgtable.h > > +++ b/arch/s390/include/asm/pgtable.h > > @@ -1074,8 +1074,9 @@ static inline pte_t ptep_get_and_clear(struct mm_struct *mm, > > pte_t res; > > > > 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_from_secure(pte_val(res) & PAGE_MASK); > > + uv_convert_owned_from_secure(pte_val(res) & PAGE_MASK); > > return res; > > } > > > > @@ -1091,8 +1092,9 @@ static inline pte_t ptep_clear_flush(struct vm_area_struct *vma, > > pte_t res; > > > > res = ptep_xchg_direct(vma->vm_mm, addr, ptep, __pte(_PAGE_INVALID)); > > + /* At this point the reference through the mapping is still present */ > > if (mm_is_protected(vma->vm_mm) && pte_present(res)) > > - uv_convert_from_secure(pte_val(res) & PAGE_MASK); > > + uv_convert_owned_from_secure(pte_val(res) & PAGE_MASK); > > return res; > > } > > > > @@ -1116,8 +1118,9 @@ 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_from_secure(pte_val(res) & PAGE_MASK); > > + uv_convert_owned_from_secure(pte_val(res) & PAGE_MASK); > > return res; > > } > > > > diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h > > index b35add51b967..3236293d5a31 100644 > > --- a/arch/s390/include/asm/uv.h > > +++ b/arch/s390/include/asm/uv.h > > @@ -356,8 +356,9 @@ static inline int is_prot_virt_host(void) > > } > > > > int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb); > > -int uv_destroy_page(unsigned long paddr); > > +int uv_destroy_owned_page(unsigned long paddr); > > int uv_convert_from_secure(unsigned long paddr); > > +int uv_convert_owned_from_secure(unsigned long paddr); > > int gmap_convert_to_secure(struct gmap *gmap, unsigned long gaddr); > > > > void setup_uv(void); > > @@ -367,7 +368,7 @@ void adjust_to_uv_max(unsigned long *vmax); > > static inline void setup_uv(void) {} > > static inline void adjust_to_uv_max(unsigned long *vmax) {} > > > > -static inline int uv_destroy_page(unsigned long paddr) > > +static inline int uv_destroy_owned_page(unsigned long paddr) > > { > > return 0; > > } > > @@ -376,6 +377,11 @@ static inline int uv_convert_from_secure(unsigned long paddr) > > { > > return 0; > > } > > + > > +static inline int uv_convert_owned_from_secure(unsigned long paddr) > > +{ > > + return 0; > > +} > > #endif > > > > #if defined(CONFIG_PROTECTED_VIRTUALIZATION_GUEST) || IS_ENABLED(CONFIG_KVM) > > diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c > > index 68a8fbafcb9c..05f8bf61d20a 100644 > > --- a/arch/s390/kernel/uv.c > > +++ b/arch/s390/kernel/uv.c > > @@ -115,7 +115,7 @@ static int uv_pin_shared(unsigned long paddr) > > * > > * @paddr: Absolute host address of page to be destroyed > > */ > > -int uv_destroy_page(unsigned long paddr) > > +static int uv_destroy_page(unsigned long paddr) > > { > > struct uv_cb_cfs uvcb = { > > .header.cmd = UVC_CMD_DESTR_SEC_STOR, > > @@ -135,6 +135,22 @@ int uv_destroy_page(unsigned long paddr) > > return 0; > > } > > > > +/* > > + * 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); > > + 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); > > + int rc; > > + > > + get_page(page); > > + rc = uv_convert_from_secure(paddr); > > + if (!rc) > > + clear_bit(PG_arch_1, &page->flags); > > + put_page(page); > > + return rc; > > +} > > + > > /* > > * Calculate the expected ref_count for a page that would otherwise have no > > * further pins. This was cribbed from similar functions in other places in > > diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c > > index 5a138f6220c4..38b792ab57f7 100644 > > --- a/arch/s390/mm/gmap.c > > +++ b/arch/s390/mm/gmap.c > > @@ -2678,8 +2678,10 @@ static int __s390_reset_acc(pte_t *ptep, unsigned long addr, > > { > > pte_t pte = READ_ONCE(*ptep); > > > > + /* There is a reference through the mapping */ > > if (pte_present(pte)) > > - WARN_ON_ONCE(uv_destroy_page(pte_val(pte) & PAGE_MASK)); > > + WARN_ON_ONCE(uv_destroy_owned_page(pte_val(pte) & PAGE_MASK)); > > + > > return 0; > > } > > > >
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>
On Mon, 6 Sep 2021 18:16:10 +0200 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > 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. at first glance, it seems we can't have mappings without a struct page > > >>> + 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> > >
diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h index dcac7b2df72c..0f1af2232ebe 100644 --- a/arch/s390/include/asm/pgtable.h +++ b/arch/s390/include/asm/pgtable.h @@ -1074,8 +1074,9 @@ static inline pte_t ptep_get_and_clear(struct mm_struct *mm, pte_t res; 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_from_secure(pte_val(res) & PAGE_MASK); + uv_convert_owned_from_secure(pte_val(res) & PAGE_MASK); return res; } @@ -1091,8 +1092,9 @@ static inline pte_t ptep_clear_flush(struct vm_area_struct *vma, pte_t res; res = ptep_xchg_direct(vma->vm_mm, addr, ptep, __pte(_PAGE_INVALID)); + /* At this point the reference through the mapping is still present */ if (mm_is_protected(vma->vm_mm) && pte_present(res)) - uv_convert_from_secure(pte_val(res) & PAGE_MASK); + uv_convert_owned_from_secure(pte_val(res) & PAGE_MASK); return res; } @@ -1116,8 +1118,9 @@ 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_from_secure(pte_val(res) & PAGE_MASK); + uv_convert_owned_from_secure(pte_val(res) & PAGE_MASK); return res; } diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h index b35add51b967..3236293d5a31 100644 --- a/arch/s390/include/asm/uv.h +++ b/arch/s390/include/asm/uv.h @@ -356,8 +356,9 @@ static inline int is_prot_virt_host(void) } int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb); -int uv_destroy_page(unsigned long paddr); +int uv_destroy_owned_page(unsigned long paddr); int uv_convert_from_secure(unsigned long paddr); +int uv_convert_owned_from_secure(unsigned long paddr); int gmap_convert_to_secure(struct gmap *gmap, unsigned long gaddr); void setup_uv(void); @@ -367,7 +368,7 @@ void adjust_to_uv_max(unsigned long *vmax); static inline void setup_uv(void) {} static inline void adjust_to_uv_max(unsigned long *vmax) {} -static inline int uv_destroy_page(unsigned long paddr) +static inline int uv_destroy_owned_page(unsigned long paddr) { return 0; } @@ -376,6 +377,11 @@ static inline int uv_convert_from_secure(unsigned long paddr) { return 0; } + +static inline int uv_convert_owned_from_secure(unsigned long paddr) +{ + return 0; +} #endif #if defined(CONFIG_PROTECTED_VIRTUALIZATION_GUEST) || IS_ENABLED(CONFIG_KVM) diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c index 68a8fbafcb9c..05f8bf61d20a 100644 --- a/arch/s390/kernel/uv.c +++ b/arch/s390/kernel/uv.c @@ -115,7 +115,7 @@ static int uv_pin_shared(unsigned long paddr) * * @paddr: Absolute host address of page to be destroyed */ -int uv_destroy_page(unsigned long paddr) +static int uv_destroy_page(unsigned long paddr) { struct uv_cb_cfs uvcb = { .header.cmd = UVC_CMD_DESTR_SEC_STOR, @@ -135,6 +135,22 @@ int uv_destroy_page(unsigned long paddr) return 0; } +/* + * 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); + 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); + int rc; + + get_page(page); + rc = uv_convert_from_secure(paddr); + if (!rc) + clear_bit(PG_arch_1, &page->flags); + put_page(page); + return rc; +} + /* * Calculate the expected ref_count for a page that would otherwise have no * further pins. This was cribbed from similar functions in other places in diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c index 5a138f6220c4..38b792ab57f7 100644 --- a/arch/s390/mm/gmap.c +++ b/arch/s390/mm/gmap.c @@ -2678,8 +2678,10 @@ static int __s390_reset_acc(pte_t *ptep, unsigned long addr, { pte_t pte = READ_ONCE(*ptep); + /* There is a reference through the mapping */ if (pte_present(pte)) - WARN_ON_ONCE(uv_destroy_page(pte_val(pte) & PAGE_MASK)); + WARN_ON_ONCE(uv_destroy_owned_page(pte_val(pte) & PAGE_MASK)); + return 0; }