On 5/10/21 1:23 PM, Peter Gonda wrote:
>> +
>> +static int snp_set_rmptable_state(unsigned long paddr, int npages,
>> + struct rmpupdate *val, bool locked, bool need_reclaim)
>> +{
>> + unsigned long pfn = __sme_clr(paddr) >> PAGE_SHIFT;
>> + unsigned long pfn_end = pfn + npages;
>> + int rc;
>> +
>> + while (pfn < pfn_end) {
>> + if (need_reclaim)
>> + if (snp_reclaim_page(pfn_to_page(pfn), locked))
>> + return -EFAULT;
>> +
>> + rc = rmpupdate(pfn_to_page(pfn), val);
>> + if (rc)
>> + return rc;
> This functional can return an error but have partially converted some
> of the npages requested by the caller. Should this function return the
> number of affected pages or something to allow the caller to know if
> some pages need to be reverted? Or should the function attempt to do
> that itself?
I will look into improving this function to cleanup the partial updates
on the failure. Thanks
>
>> +
>> + pfn++;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void __snp_free_firmware_pages(struct page *page, int order)
>> +{
>> + struct rmpupdate val = {};
>> + unsigned long paddr;
>> +
>> + if (!page)
>> + return;
>> +
>> + paddr = __pa((unsigned long)page_address(page));
>> +
>> + if (snp_set_rmptable_state(paddr, 1 << order, &val, false, true))
>> + return;
> We now have leaked the given pages right? Should some warning be
> logged or should we track these leaked pages and maybe try and free
> them with a kworker?
I will add the log about it. Only reason I can think of this function
failing is if the firmware fails to clear the immutable bit from the
page, If it did then I don't see any reason why a kworker retry will
succeed. Per the SNP firmware spec, the firmware should be able to clear
immutable bit as long as the firmware is in the INIT state.
>
>> +
>> + __free_pages(page, order);
>> +}
>> +
