| Message ID | 20240404163642.1125529-4-david@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | s390: page_mapcount(), page_has_private() and PG_arch_1 | expand |
On Thu, Apr 04, 2024 at 06:36:40PM +0200, David Hildenbrand wrote: > Now that make_folio_secure() may only set PG_arch_1 for small folios, > let's convert relevant remaining UV code to only work on (small) folios > and simply reject large folios early. This way, we'll never end up > touching PG_arch_1 on tail pages of a large folio in UV code. > > The folio_get()/folio_put() for functions that are documented to already > hold a folio reference look weird and it should probably be removed. > Similarly, uv_destroy_owned_page() and uv_convert_owned_from_secure() > should really consume a folio reference instead. But these are cleanups for > another day. Yes, and we should convert arch_make_page_accessible() to arch_make_folio_accessible() ... one of the two callers already has the folio (and page-writeback already calls arch_make_folio_accessible()
On Thu, 4 Apr 2024 18:36:40 +0200 David Hildenbrand <david@redhat.com> wrote: > Now that make_folio_secure() may only set PG_arch_1 for small folios, > let's convert relevant remaining UV code to only work on (small) folios > and simply reject large folios early. This way, we'll never end up > touching PG_arch_1 on tail pages of a large folio in UV code. > > The folio_get()/folio_put() for functions that are documented to already > hold a folio reference look weird and it should probably be removed. > Similarly, uv_destroy_owned_page() and uv_convert_owned_from_secure() > should really consume a folio reference instead. But these are cleanups for > another day. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > arch/s390/include/asm/page.h | 1 + > arch/s390/kernel/uv.c | 39 +++++++++++++++++++++--------------- > 2 files changed, 24 insertions(+), 16 deletions(-) > > diff --git a/arch/s390/include/asm/page.h b/arch/s390/include/asm/page.h > index 54d015bcd8e3..b64384872c0f 100644 > --- a/arch/s390/include/asm/page.h > +++ b/arch/s390/include/asm/page.h > @@ -214,6 +214,7 @@ static inline unsigned long __phys_addr(unsigned long x, bool is_31bit) > #define pfn_to_phys(pfn) ((pfn) << PAGE_SHIFT) > > #define phys_to_page(phys) pfn_to_page(phys_to_pfn(phys)) > +#define phys_to_folio(phys) page_folio(phys_to_page(phys)) > #define page_to_phys(page) pfn_to_phys(page_to_pfn(page)) > #define folio_to_phys(page) pfn_to_phys(folio_pfn(folio)) > > diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c > index adcbd4b13035..9c0113b26735 100644 > --- a/arch/s390/kernel/uv.c > +++ b/arch/s390/kernel/uv.c > @@ -134,14 +134,17 @@ static int uv_destroy_page(unsigned long paddr) > */ > int uv_destroy_owned_page(unsigned long paddr) > { > - struct page *page = phys_to_page(paddr); > + struct folio *folio = phys_to_folio(paddr); > int rc; > > - get_page(page); > + if (unlikely(folio_test_large(folio))) > + return 0; please add a comment here to explain why it's ok to just return 0 here... > + > + folio_get(folio); > rc = uv_destroy_page(paddr); > if (!rc) > - clear_bit(PG_arch_1, &page->flags); > - put_page(page); > + clear_bit(PG_arch_1, &folio->flags); > + folio_put(folio); > return rc; > } > > @@ -169,14 +172,17 @@ int uv_convert_from_secure(unsigned long paddr) > */ > int uv_convert_owned_from_secure(unsigned long paddr) > { > - struct page *page = phys_to_page(paddr); > + struct folio *folio = phys_to_folio(paddr); > int rc; > > - get_page(page); > + if (unlikely(folio_test_large(folio))) > + return 0; ... and here > + > + folio_get(folio); > rc = uv_convert_from_secure(paddr); > if (!rc) > - clear_bit(PG_arch_1, &page->flags); > - put_page(page); > + clear_bit(PG_arch_1, &folio->flags); > + folio_put(folio); > return rc; > } > > @@ -457,33 +463,34 @@ EXPORT_SYMBOL_GPL(gmap_destroy_page); > */ > int arch_make_page_accessible(struct page *page) > { > + struct folio *folio = page_folio(page); > int rc = 0; > > - /* Hugepage cannot be protected, so nothing to do */ > - if (PageHuge(page)) > + /* Large folios cannot be protected, so nothing to do */ > + if (unlikely(folio_test_large(folio))) > return 0; > > /* > * PG_arch_1 is used in 3 places: > * 1. for kernel page tables during early boot > * 2. for storage keys of huge pages and KVM > - * 3. As an indication that this page might be secure. This can > + * 3. As an indication that this small folio might be secure. This can > * overindicate, e.g. we set the bit before calling > * convert_to_secure. > * As secure pages are never huge, all 3 variants can co-exists. > */ > - if (!test_bit(PG_arch_1, &page->flags)) > + if (!test_bit(PG_arch_1, &folio->flags)) > return 0; > > - rc = uv_pin_shared(page_to_phys(page)); > + rc = uv_pin_shared(folio_to_phys(folio)); > if (!rc) { > - clear_bit(PG_arch_1, &page->flags); > + clear_bit(PG_arch_1, &folio->flags); > return 0; > } > > - rc = uv_convert_from_secure(page_to_phys(page)); > + rc = uv_convert_from_secure(folio_to_phys(folio)); > if (!rc) { > - clear_bit(PG_arch_1, &page->flags); > + clear_bit(PG_arch_1, &folio->flags); > return 0; > } >
On 10.04.24 19:42, Claudio Imbrenda wrote: > On Thu, 4 Apr 2024 18:36:40 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> Now that make_folio_secure() may only set PG_arch_1 for small folios, >> let's convert relevant remaining UV code to only work on (small) folios >> and simply reject large folios early. This way, we'll never end up >> touching PG_arch_1 on tail pages of a large folio in UV code. >> >> The folio_get()/folio_put() for functions that are documented to already >> hold a folio reference look weird and it should probably be removed. >> Similarly, uv_destroy_owned_page() and uv_convert_owned_from_secure() >> should really consume a folio reference instead. But these are cleanups for >> another day. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> arch/s390/include/asm/page.h | 1 + >> arch/s390/kernel/uv.c | 39 +++++++++++++++++++++--------------- >> 2 files changed, 24 insertions(+), 16 deletions(-) >> >> diff --git a/arch/s390/include/asm/page.h b/arch/s390/include/asm/page.h >> index 54d015bcd8e3..b64384872c0f 100644 >> --- a/arch/s390/include/asm/page.h >> +++ b/arch/s390/include/asm/page.h >> @@ -214,6 +214,7 @@ static inline unsigned long __phys_addr(unsigned long x, bool is_31bit) >> #define pfn_to_phys(pfn) ((pfn) << PAGE_SHIFT) >> >> #define phys_to_page(phys) pfn_to_page(phys_to_pfn(phys)) >> +#define phys_to_folio(phys) page_folio(phys_to_page(phys)) >> #define page_to_phys(page) pfn_to_phys(page_to_pfn(page)) >> #define folio_to_phys(page) pfn_to_phys(folio_pfn(folio)) >> >> diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c >> index adcbd4b13035..9c0113b26735 100644 >> --- a/arch/s390/kernel/uv.c >> +++ b/arch/s390/kernel/uv.c >> @@ -134,14 +134,17 @@ static int uv_destroy_page(unsigned long paddr) >> */ >> int uv_destroy_owned_page(unsigned long paddr) >> { >> - struct page *page = phys_to_page(paddr); >> + struct folio *folio = phys_to_folio(paddr); >> int rc; >> >> - get_page(page); >> + if (unlikely(folio_test_large(folio))) >> + return 0; > > please add a comment here to explain why it's ok to just return 0 > here... Will use "/* See gmap_make_secure(): large folios cannot be protected */" > >> + >> + folio_get(folio); >> rc = uv_destroy_page(paddr); >> if (!rc) >> - clear_bit(PG_arch_1, &page->flags); >> - put_page(page); >> + clear_bit(PG_arch_1, &folio->flags); >> + folio_put(folio); >> return rc; >> } >> >> @@ -169,14 +172,17 @@ int uv_convert_from_secure(unsigned long paddr) >> */ >> int uv_convert_owned_from_secure(unsigned long paddr) >> { >> - struct page *page = phys_to_page(paddr); >> + struct folio *folio = phys_to_folio(paddr); >> int rc; >> >> - get_page(page); >> + if (unlikely(folio_test_large(folio))) >> + return 0; > > ... and here here as well and ... > >> + >> + folio_get(folio); >> rc = uv_convert_from_secure(paddr); >> if (!rc) >> - clear_bit(PG_arch_1, &page->flags); >> - put_page(page); >> + clear_bit(PG_arch_1, &folio->flags); >> + folio_put(folio); >> return rc; >> } >> >> @@ -457,33 +463,34 @@ EXPORT_SYMBOL_GPL(gmap_destroy_page); >> */ >> int arch_make_page_accessible(struct page *page) >> { >> + struct folio *folio = page_folio(page); >> int rc = 0; >> >> - /* Hugepage cannot be protected, so nothing to do */ >> - if (PageHuge(page)) >> + /* Large folios cannot be protected, so nothing to do */ ^ here as well. >> + if (unlikely(folio_test_large(folio))) >> return 0; >>
On 05.04.24 05:36, Matthew Wilcox wrote: > On Thu, Apr 04, 2024 at 06:36:40PM +0200, David Hildenbrand wrote: >> Now that make_folio_secure() may only set PG_arch_1 for small folios, >> let's convert relevant remaining UV code to only work on (small) folios >> and simply reject large folios early. This way, we'll never end up >> touching PG_arch_1 on tail pages of a large folio in UV code. >> >> The folio_get()/folio_put() for functions that are documented to already >> hold a folio reference look weird and it should probably be removed. >> Similarly, uv_destroy_owned_page() and uv_convert_owned_from_secure() >> should really consume a folio reference instead. But these are cleanups for >> another day. > > Yes, and we should convert arch_make_page_accessible() to > arch_make_folio_accessible() ... one of the two callers already has the > folio (and page-writeback already calls arch_make_folio_accessible() > Yes. We should then get rid of the arch_make_folio_accessible() loop over pages. Arch code can handle that, if required. Will include that in this series.
diff --git a/arch/s390/include/asm/page.h b/arch/s390/include/asm/page.h index 54d015bcd8e3..b64384872c0f 100644 --- a/arch/s390/include/asm/page.h +++ b/arch/s390/include/asm/page.h @@ -214,6 +214,7 @@ static inline unsigned long __phys_addr(unsigned long x, bool is_31bit) #define pfn_to_phys(pfn) ((pfn) << PAGE_SHIFT) #define phys_to_page(phys) pfn_to_page(phys_to_pfn(phys)) +#define phys_to_folio(phys) page_folio(phys_to_page(phys)) #define page_to_phys(page) pfn_to_phys(page_to_pfn(page)) #define folio_to_phys(page) pfn_to_phys(folio_pfn(folio)) diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c index adcbd4b13035..9c0113b26735 100644 --- a/arch/s390/kernel/uv.c +++ b/arch/s390/kernel/uv.c @@ -134,14 +134,17 @@ static int uv_destroy_page(unsigned long paddr) */ int uv_destroy_owned_page(unsigned long paddr) { - struct page *page = phys_to_page(paddr); + struct folio *folio = phys_to_folio(paddr); int rc; - get_page(page); + if (unlikely(folio_test_large(folio))) + return 0; + + folio_get(folio); rc = uv_destroy_page(paddr); if (!rc) - clear_bit(PG_arch_1, &page->flags); - put_page(page); + clear_bit(PG_arch_1, &folio->flags); + folio_put(folio); return rc; } @@ -169,14 +172,17 @@ int uv_convert_from_secure(unsigned long paddr) */ int uv_convert_owned_from_secure(unsigned long paddr) { - struct page *page = phys_to_page(paddr); + struct folio *folio = phys_to_folio(paddr); int rc; - get_page(page); + if (unlikely(folio_test_large(folio))) + return 0; + + folio_get(folio); rc = uv_convert_from_secure(paddr); if (!rc) - clear_bit(PG_arch_1, &page->flags); - put_page(page); + clear_bit(PG_arch_1, &folio->flags); + folio_put(folio); return rc; } @@ -457,33 +463,34 @@ EXPORT_SYMBOL_GPL(gmap_destroy_page); */ int arch_make_page_accessible(struct page *page) { + struct folio *folio = page_folio(page); int rc = 0; - /* Hugepage cannot be protected, so nothing to do */ - if (PageHuge(page)) + /* Large folios cannot be protected, so nothing to do */ + if (unlikely(folio_test_large(folio))) return 0; /* * PG_arch_1 is used in 3 places: * 1. for kernel page tables during early boot * 2. for storage keys of huge pages and KVM - * 3. As an indication that this page might be secure. This can + * 3. As an indication that this small folio might be secure. This can * overindicate, e.g. we set the bit before calling * convert_to_secure. * As secure pages are never huge, all 3 variants can co-exists. */ - if (!test_bit(PG_arch_1, &page->flags)) + if (!test_bit(PG_arch_1, &folio->flags)) return 0; - rc = uv_pin_shared(page_to_phys(page)); + rc = uv_pin_shared(folio_to_phys(folio)); if (!rc) { - clear_bit(PG_arch_1, &page->flags); + clear_bit(PG_arch_1, &folio->flags); return 0; } - rc = uv_convert_from_secure(page_to_phys(page)); + rc = uv_convert_from_secure(folio_to_phys(folio)); if (!rc) { - clear_bit(PG_arch_1, &page->flags); + clear_bit(PG_arch_1, &folio->flags); return 0; }
Now that make_folio_secure() may only set PG_arch_1 for small folios, let's convert relevant remaining UV code to only work on (small) folios and simply reject large folios early. This way, we'll never end up touching PG_arch_1 on tail pages of a large folio in UV code. The folio_get()/folio_put() for functions that are documented to already hold a folio reference look weird and it should probably be removed. Similarly, uv_destroy_owned_page() and uv_convert_owned_from_secure() should really consume a folio reference instead. But these are cleanups for another day. Signed-off-by: David Hildenbrand <david@redhat.com> --- arch/s390/include/asm/page.h | 1 + arch/s390/kernel/uv.c | 39 +++++++++++++++++++++--------------- 2 files changed, 24 insertions(+), 16 deletions(-)