On 3/24/21 10:04 AM, Brijesh Singh wrote:
> When SEV-SNP is enabled globally in the system, a write from the hypervisor
> can raise an RMP violation. We can resolve the RMP violation by splitting
> the virtual address to a lower page level.
>
> e.g
> - guest made a page shared in the RMP entry so that the hypervisor
> can write to it.
> - the hypervisor has mapped the pfn as a large page. A write access
> will cause an RMP violation if one of the pages within the 2MB region
> is a guest private page.
>
> The above RMP violation can be resolved by simply splitting the large
> page.
What if the large page is provided by hugetlbfs?
What if the kernel uses the direct map to access the page instead of the
userspace mapping?
> The architecture specific code will read the RMP entry to determine
> if the fault can be resolved by splitting and propagating the request
> to split the page by setting newly introduced fault flag
> (FAULT_FLAG_PAGE_SPLIT). If the fault cannot be resolved by splitting,
> then a SIGBUS signal is sent to terminate the process.
Are users just supposed to know what memory types are compatible with
SEV-SNP? Basically, don't use anything that might map a guest using
non-4k entries, except THP?
This does seem like a rather nasty aspect of the hardware. For
everything else, if the virtualization page tables and the x86 tables
disagree, the TLB just sees the smallest page size.
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 7605e06a6dd9..f6571563f433 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -1305,6 +1305,70 @@ do_kern_addr_fault(struct pt_regs *regs, unsigned long
> hw_error_code,
> }
> NOKPROBE_SYMBOL(do_kern_addr_fault);
>
> +#define RMP_FAULT_RETRY 0
> +#define RMP_FAULT_KILL 1
> +#define RMP_FAULT_PAGE_SPLIT 2
> +
> +static inline size_t pages_per_hpage(int level)
> +{
> + return page_level_size(level) / PAGE_SIZE;
> +}
> +
> +/*
> + * The RMP fault can happen when a hypervisor attempts to write to:
> + * 1. a guest owned page or
> + * 2. any pages in the large page is a guest owned page.
> + *
> + * #1 will happen only when a process or VMM is attempting to modify the
> guest page
> + * without the guests cooperation. If a guest wants a VMM to be able to
> write to its memory
> + * then it should make the page shared. If we detect #1, kill the process
> because we can not
> + * resolve the fault.
> + *
> + * #2 can happen when the page level does not match between the RMP entry
> and x86
> + * page table walk, e.g the page is mapped as a large page in the x86 page
> table but its
> + * added as a 4K shared page in the RMP entry. This can be resolved by
> splitting the address
> + * into a smaller page level.
> + */
These comments need to get wrapped a bit sooner. Could you try to match
some of the others in the file?
> +static int handle_rmp_page_fault(unsigned long hw_error_code, unsigned long
> address)
> +{
> + unsigned long pfn, mask;
> + int rmp_level, level;
> + rmpentry_t *e;
> + pte_t *pte;
> +
> + /* Get the native page level */
> + pte = lookup_address_in_mm(current->mm, address, &level);
> + if (unlikely(!pte))
> + return RMP_FAULT_KILL;
> +
> + pfn = pte_pfn(*pte);
> + if (level > PG_LEVEL_4K) {
> + mask = pages_per_hpage(level) - pages_per_hpage(level - 1);
> + pfn |= (address >> PAGE_SHIFT) & mask;
> + }
What is this trying to do, exactly?
> + /* Get the page level from the RMP entry. */
> + e = lookup_page_in_rmptable(pfn_to_page(pfn), &rmp_level);
> + if (!e) {
> + pr_alert("SEV-SNP: failed to lookup RMP entry for address 0x%lx
> pfn 0x%lx\n",
> + address, pfn);
> + return RMP_FAULT_KILL;
> + }
> +
> + /* Its a guest owned page */
> + if (rmpentry_assigned(e))
> + return RMP_FAULT_KILL;
> +
> + /*
> + * Its a shared page but the page level does not match between the
> native walk
> + * and RMP entry.
> + */
For these two-line comments, please try to split the text fairly evenly
between the lines.
> + if (level > rmp_level)
> + return RMP_FAULT_PAGE_SPLIT;
> +
> + return RMP_FAULT_RETRY;
> +}
> +
> /* Handle faults in the user portion of the address space */
> static inline
> void do_user_addr_fault(struct pt_regs *regs,
> @@ -1315,6 +1379,7 @@ void do_user_addr_fault(struct pt_regs *regs,
> struct task_struct *tsk;
> struct mm_struct *mm;
> vm_fault_t fault;
> + int ret;
> unsigned int flags = FAULT_FLAG_DEFAULT;
>
> tsk = current;
> @@ -1377,6 +1442,22 @@ void do_user_addr_fault(struct pt_regs *regs,
> if (hw_error_code & X86_PF_INSTR)
> flags |= FAULT_FLAG_INSTRUCTION;
>
> + /*
> + * If its an RMP violation, see if we can resolve it.
> + */
> + if ((hw_error_code & X86_PF_RMP)) {
> + ret = handle_rmp_page_fault(hw_error_code, address);
> + if (ret == RMP_FAULT_PAGE_SPLIT) {
> + flags |= FAULT_FLAG_PAGE_SPLIT;
> + } else if (ret == RMP_FAULT_KILL) {
> + fault |= VM_FAULT_SIGBUS;
> + mm_fault_error(regs, hw_error_code, address, fault);
> + return;
> + } else {
> + return;
> + }
> + }
> +
> #ifdef CONFIG_X86_64
> /*
> * Faults in the vsyscall page might need emulation. The
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index ecdf8a8cd6ae..1be3218f3738 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -434,6 +434,8 @@ extern pgprot_t protection_map[16];
> * @FAULT_FLAG_REMOTE: The fault is not for current task/mm.
> * @FAULT_FLAG_INSTRUCTION: The fault was during an instruction fetch.
> * @FAULT_FLAG_INTERRUPTIBLE: The fault can be interrupted by non-fatal
> signals.
> + * @FAULT_FLAG_PAGE_SPLIT: The fault was due page size mismatch, split the
> region to smaller
> + * page size and retry.
> *
> * About @FAULT_FLAG_ALLOW_RETRY and @FAULT_FLAG_TRIED: we can specify
> * whether we would allow page faults to retry by specifying these two
> @@ -464,6 +466,7 @@ extern pgprot_t protection_map[16];
> #define FAULT_FLAG_REMOTE 0x80
> #define FAULT_FLAG_INSTRUCTION 0x100
> #define FAULT_FLAG_INTERRUPTIBLE 0x200
> +#define FAULT_FLAG_PAGE_SPLIT 0x400
>
> /*
> * The default fault flags that should be used by most of the
> @@ -501,7 +504,8 @@ static inline bool fault_flag_allow_retry_first(unsigned
> int flags)
> { FAULT_FLAG_USER, "USER" }, \
> { FAULT_FLAG_REMOTE, "REMOTE" }, \
> { FAULT_FLAG_INSTRUCTION, "INSTRUCTION" }, \
> - { FAULT_FLAG_INTERRUPTIBLE, "INTERRUPTIBLE" }
> + { FAULT_FLAG_INTERRUPTIBLE, "INTERRUPTIBLE" }, \
> + { FAULT_FLAG_PAGE_SPLIT, "PAGESPLIT" }
>
> /*
> * vm_fault is filled by the pagefault handler and passed to the vma's
> diff --git a/mm/memory.c b/mm/memory.c
> index feff48e1465a..c9dcf9b30719 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -4427,6 +4427,12 @@ static vm_fault_t handle_pte_fault(struct vm_fault
> *vmf)
> return 0;
> }
>
> +static int handle_split_page_fault(struct vm_fault *vmf)
> +{
> + __split_huge_pmd(vmf->vma, vmf->pmd, vmf->address, false, NULL);
> + return 0;
> +}
Wait a sec, I thought this could fail. Where's the "failed to split"
path? Why does this even return an error code if it's always 0?
> /*
> * By the time we get here, we already hold the mm semaphore
> *
> @@ -4448,6 +4454,7 @@ static vm_fault_t __handle_mm_fault(struct
> vm_area_struct *vma,
> pgd_t *pgd;
> p4d_t *p4d;
> vm_fault_t ret;
> + int split_page = flags & FAULT_FLAG_PAGE_SPLIT;
>
> pgd = pgd_offset(mm, address);
> p4d = p4d_alloc(mm, pgd, address);
> @@ -4504,6 +4511,10 @@ static vm_fault_t __handle_mm_fault(struct
> vm_area_struct *vma,
> pmd_migration_entry_wait(mm, vmf.pmd);
> return 0;
> }
> +
> + if (split_page)
> + return handle_split_page_fault(&vmf);
> +
> if (pmd_trans_huge(orig_pmd) || pmd_devmap(orig_pmd)) {
> if (pmd_protnone(orig_pmd) && vma_is_accessible(vma))
> return do_huge_pmd_numa_page(&vmf, orig_pmd);
Is there a reason for the 'split_page' variable? It seems like a waste
of space.
