Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 20 Jul 2021 22:06:24 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Brijesh Singh <brijesh.singh@amd.com>
Cc:     x86@kernel.org, linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        linux-efi@vger.kernel.org, platform-driver-x86@vger.kernel.org,
        linux-coco@lists.linux.dev, linux-mm@kvack.org,
        linux-crypto@vger.kernel.org, Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Joerg Roedel <jroedel@suse.de>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        "H. Peter Anvin" <hpa@zytor.com>, Ard Biesheuvel <ardb@kernel.org>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Andy Lutomirski <luto@kernel.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Sergio Lopez <slp@redhat.com>, Peter Gonda <pgonda@google.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
        David Rientjes <rientjes@google.com>,
        Dov Murik <dovmurik@linux.ibm.com>,
        Tobin Feldman-Fitzthum <tobin@ibm.com>,
        Borislav Petkov <bp@alien8.de>,
        Michael Roth <michael.roth@amd.com>,
        Vlastimil Babka <vbabka@suse.cz>, tony.luck@intel.com,
        npmccallum@redhat.com, brijesh.ksingh@gmail.com
Subject: Re: [PATCH Part2 RFC v4 05/40] x86/sev: Add RMP entry lookup helpers
Message-ID: <YPdI4JLrJJdPxy7e@google.com>
References: <20210707183616.5620-1-brijesh.singh@amd.com>
 <20210707183616.5620-6-brijesh.singh@amd.com>
 <YPCAZaROOHNskGlO@google.com>
 <437a5230-64fc-64ab-9378-612c34e1b641@amd.com>
 <39be0f79-e8e4-fd4a-5c4a-47731c61740d@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <39be0f79-e8e4-fd4a-5c4a-47731c61740d@amd.com>
Precedence: bulk

On Fri, Jul 16, 2021, Brijesh Singh wrote:
> 
> On 7/15/21 2:28 PM, Brijesh Singh wrote:
> >> 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()?

As suggested below, move dump_rmpentry() into sev.c so that it can use the
microarchitectural version.  For debug, I'm pretty that's what we'll want anyways,
e.g. dump the raw value along with the meaning of various bits.

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

Why define an architectural "struct rmpentry"?  IIUC, there isn't a true
architectural RMP entry; the APM defines architectural fields but doesn't define
a layout.  Functionally, making up our own struct isn't a problem, I just don't
see the point since all use cases only care about Assigned and Page-Size, and
we can do them a favor by translating Page-Size to X86_PG_LEVEL.

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