On 7/15/21 2:28 PM, Brijesh Singh wrote:

On 7/15/21 1:37 PM, Sean Christopherson wrote:

quoted

On Wed, Jul 07, 2021, Brijesh Singh wrote:

quoted

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.

quoted

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 ?

quoted

So, what about hiding "struct rmpentry" and possibly renaming it to
something
scary/microarchitectural, e.g. something like

Yes, it will work fine.

quoted

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