lkml.org 
[lkml]   [2023]   [Dec]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v10 16/50] x86/sev: Introduce snp leaked pages list
From
Hello Vlastimil,

On 12/11/2023 7:08 AM, Vlastimil Babka wrote:
>
>
> On 12/8/23 23:10, Kalra, Ashish wrote:
>> Hello Vlastimil,
>>
>> On 12/7/2023 10:20 AM, Vlastimil Babka wrote:
>>
>>>> +
>>>> +void snp_leak_pages(u64 pfn, unsigned int npages)
>>>> +{
>>>> +    struct page *page = pfn_to_page(pfn);
>>>> +
>>>> +    pr_debug("%s: leaking PFN range 0x%llx-0x%llx\n", __func__, pfn,
>>>> pfn + npages);
>>>> +
>>>> +    spin_lock(&snp_leaked_pages_list_lock);
>>>> +    while (npages--) {
>>>> +        /*
>>>> +         * Reuse the page's buddy list for chaining into the leaked
>>>> +         * pages list. This page should not be on a free list currently
>>>> +         * and is also unsafe to be added to a free list.
>>>> +         */
>>>> +        list_add_tail(&page->buddy_list, &snp_leaked_pages_list);
>>>> +        sev_dump_rmpentry(pfn);
>>>> +        pfn++;
>>>
>>> You increment pfn, but not page, which is always pointing to the page
>>> of the
>>> initial pfn, so need to do page++ too.
>>
>> Yes, that is a bug and needs to be fixed.
>>
>>> But that assumes it's all order-0 pages (hard to tell for me whether
>>> that's
>>> true as we start with a pfn), if there can be compound pages, it would be
>>> best to only add the head page and skip the tail pages - it's not
>>> expected
>>> to use page->buddy_list of tail pages.
>>
>> Can't we use PageCompound() to check if the page is a compound page and
>> then use page->compound_head to get and add the head page to leaked
>> pages list. I understand the tail pages for compound pages are really
>> limited for usage.
>
> Yeah that should work. Need to be careful though, should probably only
> process head pages and check if the whole compound_order() is within the
> range we are to leak, and then leak the head page and advance the loop
> by compound_order(). And if we encounter a tail page, it should probably
> be just skipped. I'm looking at snp_reclaim_pages() which seems to
> process a number of pages with SEV_CMD_SNP_PAGE_RECLAIM and once any
> fails, call snp_leak_pages() on the rest. Could that invoke
> snp_leak_pages with the first pfn being a tail page?

Yes i don't think we can assume that the first pfn will not be a tail
page. But then this becomes complex as we might have already reclaimed
the head page and one or more tail pages successfully or probably never
transitioned head page to FW state as alloc_page()/alloc_pages() would
have returned subpage(s) of a largepage.

But then we really can't use the buddy_list of a tail page to insert it
in the snp leaked pages list, right ?

These non-reclaimed pages are not usable anymore anyway, any access to
them will cause fatal RMP #PF, so don't know if i can use the buddy_list
to insert tail pages as that will corrupt the page metadata ?

We initially used to invoke memory_failure() here to try to gracefully
handle failure of these non-reclaimed pages and that used to handle
hugepages, etc., but as pointed in previous review feedback that is not
a logical approach for this as that's meant more for the RAS stuff.

Maybe it is a simpler approach to have our own container object on top
and have this page pointer and list_head in it and use that list_head to
insert into the snp leaked list instead of re-using the buddy_list for
chaining into the leaked pages list ?

Thanks,
Ashish

\
 
 \ /
  Last update: 2023-12-13 00:27    [W:0.185 / U:0.216 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site