RE: [PATCH Part2 v6 09/49] x86/fault: Add support to handle the RMP fault for user address

[Kalra, Ashish]

[AMD Official Use Only - General]

Hello Boris,

>> When SEV-SNP is enabled globally, a write from the host goes through 
>> the

>globally?

>Can SNP be even enabled any other way?

>I see the APM talks about it being enabled globally, I guess this means the RMP represents *all* system memory?

Actually SNP feature can be enabled globally, but SNP is activated on a per VM basis.

>From the APM:
The term SNP-enabled indicates that SEV-SNP is globally enabled in the SYSCFG 
MSR. The term SNP-active indicates that SEV-SNP is enabled for a specific VM in the 
SEV_FEATURES field of its VMSA

>> +/*
>> + * Return 1 if the caller need to retry, 0 if it the address need to be split
>> + * in order to resolve the fault.
>> + */

>Magic numbers.

>Pls do instead:

>enum rmp_pf_ret {
>	RMP_PF_SPLIT	= 0,
>	RMP_PF_RETRY	= 1,
>};

>and use those instead.
Ok.

>> +static int handle_user_rmp_page_fault(struct pt_regs *regs, unsigned long error_code,
>> +				      unsigned long address)
>> +{
>> +	int rmp_level, level;
>> +	pte_t *pte;
>> +	u64 pfn;
>> +
>> +	pte = lookup_address_in_mm(current->mm, address, &level);
>> +
>> +	/*
>> +	 * It can happen if there was a race between an unmap event and
>> +	 * the RMP fault delivery.
>> +	 */

>You need to elaborate more here: a RMP fault can happen and then the
>page can get unmapped? What is the exact scenario here?

Yes, if the page gets unmapped while the RMP fault was being handled,
will add more explanation here.

>> +	if (!pte || !pte_present(*pte))
>> +		return 1;
>> +
>> +	pfn = pte_pfn(*pte);
>> +
>> +	/* If its large page then calculte the fault pfn */
>> +	if (level > PG_LEVEL_4K) {
>> +		unsigned long mask;
>> +
>> +		mask = pages_per_hpage(level) - pages_per_hpage(level - 1);
>> +		pfn |= (address >> PAGE_SHIFT) & mask;

>Oh boy, this is unnecessarily complicated. Isn't this

>	pfn |= pud_index(address);

>or
>	pfn |= pmd_index(address);

>depending on the level?

Actually, the above computes an index into the RMP table. It is basically an index into
the 4K page within the hugepage mapped in the RMP table or in other words an index
into the RMP table entry for 4K page(s) corresponding to a hugepage.

So, pud_index()/pmd_index() can't be used for the same.

>I think it is but it needs more explaining.

>In any case, those are two static masks exactly and they don't need to
>be computed for each #PF.

>> diff --git a/mm/memory.c b/mm/memory.c
>> index 7274f2b52bca..c2187ffcbb8e 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -4945,6 +4945,15 @@ static vm_fault_t handle_pte_fault(struct vm_fault *vmf)
>>  	return 0;
>>  }
>> 
>>. +static int handle_split_page_fault(struct vm_fault *vmf)
>> +{
> >+	if (!IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT))
>> +		return VM_FAULT_SIGBUS;

>Yah, this looks weird: generic code implies that page splitting after a
>#PF makes sense only when SEV is present and none otherwise.

It is mainly a wrapper around__split_huge_pmd() for SNP use case
where the host hugepage is split to be in sync with the RMP table. 

Thanks,
Ashish