Messages in this thread | | | Subject | Re: [PATCH Part2 RFC v4 09/40] x86/fault: Add support to dump RMP entry on fault | From | Brijesh Singh <> | Date | Thu, 8 Jul 2021 11:48:26 -0500 |
| |
On 7/8/21 10:30 AM, Dave Hansen wrote: > I was even thinking that you could use the pmd/pte entries that come > from the walk in dump_pagetable(). > > BTW, I think the snp_lookup_page_in_rmptable() interface is probably > wrong. It takes a 'struct page': >
In some cases the caller already have 'struct page' so it was easier on them. I can change it to snp_lookup_pfn_in_rmptable() to simplify the things. In the cases where the caller already have 'struct page' will simply do page_to_pfn().
> +struct rmpentry *snp_lookup_page_in_rmptable(struct page *page, int *level) > > but then immediately converts it to a paddr: > >> + unsigned long phys = page_to_pfn(page) << PAGE_SHIFT; > > If you just had it take a paddr, you wouldn't have to mess with all of > this pfn_valid() and phys_to_page() error checking.
Noted.
> > Or fix the snp_lookup_page_in_rmptable() interface, please.
Yes.
> > Let's capitalize "RMP" here, please.
Noted.
> >> PGD 304b201067 P4D 304b201067 PUD 20c7f06063 PMD 20c8976063 PTE >> 80000020cee00163 >> RMPEntry paddr 0x20cee00000 [assigned=1 immutable=1 pagesize=0 gpa=0x0 ^^^^^^^^^^
>> asid=0 vmsa=0 validated=0] >> RMPEntry paddr 0x20cee00000 000000000000000f 8000000000000ffd > > That's a good example, thanks! > > But, it does make me think that we shouldn't be spitting out > "immutable". Should we call it "readonly" or something so that folks > have a better chance of figuring out what's wrong? Even better, should > we be looking specifically for X86_PF_RMP *and* immutable=1 and spitting > out something in english about it? >
A write to an assigned page will cause the RMP violation. In this case, the page happen to be firmware page hence the immutable bit was also set. I am trying to use the field name as documented in the APM and SEV-SNP firmware spec.
> This also *looks* to be spitting out the same "RMPEntry paddr > 0x20cee00000" more than once. Maybe we should just indent the extra > entries instead of repeating things. The high/low are missing a "0x" > prefix, they also don't have any kind of text label. > Noted, I will fix it.
> > I actually really like processing the fields. I think it's a good > investment to make the error messages as self-documenting as possible > and not require the poor souls who are decoding oopses to also keep each > vendor's architecture manuals at hand. > Sounds good, I will keep it as-is.
>> >> The reason for iterating through 2MB region is; if the faulting address >> is not assigned in the RMP table, and page table walk level is 2MB then >> one of entry within the large page is the root cause of the fault. Since >> we don't know which entry hence I dump all the non-zero entries. > > Logically you can figure this out though, right? Why throw 511 entries > at the console when we *know* they're useless?
Logically its going to be tricky to figure out which exact entry caused the fault, hence I dump any non-zero entry. I understand it may dump some useless.
>> There are two cases which we need to consider: >> >> 1) the faulting page is a guest private (aka assigned) >> 2) the faulting page is a hypervisor (aka shared) >> >> We will be primarily seeing #1. In this case, we know its a assigned >> page, and we can decode the fields. >> >> The #2 will happen in rare conditions, > > What rare conditions? >
One such condition is RMP "in-use" bit is set; see the patch 20/40. After applying the patch we should not see "in-use" bit set. If we run into similar issues, a full RMP dump will greatly help debug.
>> if it happens, one of the undocumented bit in the RMP entry can >> provide us some useful information hence we dump the raw values. > You're saying that there are things that can cause RMP faults that > aren't documented? That's rather nasty for your users, don't you think? >
The "in-use" bit in the RMP entry caught me off guard. The AMD APM does says that hardware sets in-use bit but it *never* explained in the detail on how to check if the fault was due to in-use bit in the RMP table. As I said, the documentation folks will be updating the RMP entry to document the in-use bit. I hope we will not see any other undocumented surprises, I am keeping my finger cross :)
> I'd be fine if you want to define a mask of unknown bits and spit out to > the users that some unknown bits are set. >
| |