Re: [PATCH Part2 RFC v4 05/40] x86/sev: Add RMP entry lookup helpers

[Brijesh Singh <brijesh.singh@amd.com>]

On 7/15/21 2:28 PM, Brijesh Singh wrote:
>
>
> On 7/15/21 1:37 PM, Sean Christopherson wrote:
>> On Wed, Jul 07, 2021, Brijesh Singh wrote:
>>> The snp_lookup_page_in_rmptable() can be used by the host to read
>>> the RMP
>>> entry for a given page. The RMP entry format is documented in AMD
>>> PPR, see
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fattachment.cgi%3Fid%3D296015&data=04%7C01%7Cbrijesh.singh%40amd.com%7C2140214b3fbd4a71617008d947bf9ae7%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637619710568694335%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=AkCyolw0P%2BrRFF%2FAnRozld4GkegQ0hR%2F523DI48jB4g%3D&reserved=0.
>>>
>>
>> Ewwwwww, the RMP format isn't architectural!?
>>
>>    Architecturally the format of RMP entries are not specified in
>> APM. In order
>>    to assist software, the following table specifies select portions
>> of the RMP
>>    entry format for this specific product.
>>
>
> Unfortunately yes.
>
> But the documented fields in the RMP entry is architectural. The entry
> fields are documented in the APM section 15.36. So, in future we are
> guaranteed to have those fields available. If we are reading the RMP
> table directly, then architecture should provide some other means to
> get to fields from the RMP entry.
>
>
>> I know we generally don't want to add infrastructure without good
>> reason, but on
>> the other hand exposing a microarchitectural data structure to the
>> kernel at large
>> is going to be a disaster if the format does change on a future
>> processor.
>>
>> Looking at the future patches, dump_rmpentry() is the only power
>> user, e.g.
>> everything else mostly looks at "assigned" and "level" (and one
>> ratelimited warn
>> on "validated" in snp_make_page_shared(), but I suspect that
>> particular check
>> can and should be dropped).
>>
>
> Yes, we need "assigned" and "level" and other entries are mainly for
> the debug purposes.
>
For the debug purposes, we would like to dump additional RMP entries. If
we go with your proposed function then how do we get those information
in the dump_rmpentry()? How about if we provide two functions; the first
function provides architectural format and second provides the raw
values which can be used by the dump_rmpentry() helper.

struct rmpentry *snp_lookup_rmpentry(unsigned long paddr, int *level);

The 'struct rmpentry' uses the format defined in APM Table 15-36.

struct _rmpentry *_snp_lookup_rmpentry(unsigned long paddr, int *level);

The 'struct _rmpentry' will use include the PPR definition (basically
what we have today in this patch).

Thoughts ?


>> So, what about hiding "struct rmpentry" and possibly renaming it to
>> something
>> scary/microarchitectural, e.g. something like
>>
>
> Yes, it will work fine.
>
>> /*
>>   * Returns 1 if the RMP entry is assigned, 0 if it exists but is not
>> assigned,
>>   * and -errno if there is no corresponding RMP entry.
>>   */
>> int snp_lookup_rmpentry(struct page *page, int *level)
>> {
>>     unsigned long phys = page_to_pfn(page) << PAGE_SHIFT;
>>     struct rmpentry *entry, *large_entry;
>>     unsigned long vaddr;
>>
>>     if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP))
>>         return -ENXIO;
>>
>>     vaddr = rmptable_start + rmptable_page_offset(phys);
>>     if (unlikely(vaddr > rmptable_end))
>>         return -EXNIO;
>>
>>     entry = (struct rmpentry *)vaddr;
>>
>>     /* Read a large RMP entry to get the correct page level used in
>> RMP entry. */
>>     vaddr = rmptable_start + rmptable_page_offset(phys & PMD_MASK);
>>     large_entry = (struct rmpentry *)vaddr;
>>     *level = RMP_TO_X86_PG_LEVEL(rmpentry_pagesize(large_entry));
>>
>>     return !!entry->assigned;
>> }
>>
>>
>> And then move dump_rmpentry() (or add a helper) in sev.c so that
>> "struct rmpentry"
>> can be declared in sev.c.
>>
>