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