lkml.org 
[lkml]   [2021]   [Jul]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH Part2 RFC v4 09/40] x86/fault: Add support to dump RMP entry on fault
From
Date


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.
>

\
 
 \ /
  Last update: 2021-07-08 18:51    [W:0.262 / U:0.820 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site