Re: [PATCH v10 16/50] x86/sev: Introduce snp leaked pages list

[Vlastimil Babka]

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?

> Thanks,
> Ashish