Re: [PATCH Part2 RFC v4 09/40] x86/fault: Add support to dump RMP entry on fault

[Dave Hansen <dave.hansen@intel.com>]

> @@ -502,6 +503,81 @@ static void show_ldttss(const struct desc_ptr *gdt, const char *name, u16 index)
>  		 name, index, addr, (desc.limit0 | (desc.limit1 << 16)));
>  }
>  
> +static void dump_rmpentry(unsigned long address)
> +{

A comment on this sucker would be nice.  I *think* this must be a kernel
virtual address.  Reflecting that into the naming or a comment would be
nice.

> +	struct rmpentry *e;
> +	unsigned long pfn;
> +	pgd_t *pgd;
> +	pte_t *pte;
> +	int level;
> +
> +	pgd = __va(read_cr3_pa());
> +	pgd += pgd_index(address);
> +
> +	pte = lookup_address_in_pgd(pgd, address, &level);
> +	if (unlikely(!pte))
> +		return;

It's a little annoying this is doing *another* separate page walk.
Don't we already do this for dumping the page tables themselves at oops
time?

Also, please get rid of all of the likely/unlikely()s in your patches.
They are pure noise unless you have specific knowledge of the compiler
getting something so horribly wrong that it affects real-world performance.

> +	switch (level) {
> +	case PG_LEVEL_4K: {
> +		pfn = pte_pfn(*pte);
> +		break;
> +	}

These superfluous brackets are really strange looking.  Could you remove
them, please?

> +	case PG_LEVEL_2M: {
> +		pfn = pmd_pfn(*(pmd_t *)pte);
> +		break;
> +	}
> +	case PG_LEVEL_1G: {
> +		pfn = pud_pfn(*(pud_t *)pte);
> +		break;
> +	}
> +	case PG_LEVEL_512G: {
> +		pfn = p4d_pfn(*(p4d_t *)pte);
> +		break;
> +	}
> +	default:
> +		return;
> +	}
> +
> +	e = snp_lookup_page_in_rmptable(pfn_to_page(pfn), &level);

So, lookup_address_in_pgd() looks to me like it will return pretty
random page table entries as long as the entry isn't
p{gd,4d,ud,md,te}_none().  It can certainly return !p*_present()
entries.  Those are *NOT* safe to call pfn_to_page() on.

> +	if (unlikely(!e))
> +		return;
> +
> +	/*
> +	 * If the RMP entry at the faulting address was not assigned, then
> +	 * dump may not provide any useful debug information. Iterate
> +	 * through the entire 2MB region, and dump the RMP entries if one
> +	 * of the bit in the RMP entry is set.
> +	 */

Some of this comment should be moved down to the loop itself.

> +	if (rmpentry_assigned(e)) {
> +		pr_alert("RMPEntry paddr 0x%lx [assigned=%d immutable=%d pagesize=%d gpa=0x%lx"
> +			" asid=%d vmsa=%d validated=%d]\n", pfn << PAGE_SHIFT,
> +			rmpentry_assigned(e), rmpentry_immutable(e), rmpentry_pagesize(e),
> +			rmpentry_gpa(e), rmpentry_asid(e), rmpentry_vmsa(e),
> +			rmpentry_validated(e));
> +
> +		pr_alert("RMPEntry paddr 0x%lx %016llx %016llx\n", pfn << PAGE_SHIFT,
> +			e->high, e->low);

Could you please include an entire oops in the changelog that also
includes this information?  It would be really nice if this was at least
consistent in style to the stuff around it.

Also, how much of this stuff like rmpentry_asid() is duplicated in the
"raw" dump of e->high and e->low?

> +	} else {
> +		unsigned long pfn_end;
> +
> +		pfn = pfn & ~0x1ff;

There's a nice magic number.  Why not:

	pfn = pfn & ~(PTRS_PER_PMD-1);

?

This also needs a comment about *WHY* this case is looking at a 2MB region.

> +		pfn_end = pfn + PTRS_PER_PMD;
> +
> +		while (pfn < pfn_end) {
> +			e = snp_lookup_page_in_rmptable(pfn_to_page(pfn), &level);
> +
> +			if (unlikely(!e))
> +				return;
> +
> +			if (e->low || e->high)
> +				pr_alert("RMPEntry paddr 0x%lx: %016llx %016llx\n",
> +					pfn << PAGE_SHIFT, e->high, e->low);

Why does this dump "raw" RMP entries while the above stuff filters them
through a bunch of helper macros?

> +			pfn++;
> +		}
> +	}
> +}
> +
>  static void
>  show_fault_oops(struct pt_regs *regs, unsigned long error_code, unsigned long address)
>  {
> @@ -578,6 +654,9 @@ show_fault_oops(struct pt_regs *regs, unsigned long error_code, unsigned long ad
>  	}
>  
>  	dump_pagetable(address);
> +
> +	if (error_code & X86_PF_RMP)
> +		dump_rmpentry(address);
>  }
>  
>  static noinline void
>