Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Received-SPF: Pass (protection.outlook.com: domain of amd.com designates
 165.204.84.17 as permitted sender) receiver=protection.outlook.com;
 client-ip=165.204.84.17; helo=SATLEXMB04.amd.com; pr=C
Date:   Wed, 29 Mar 2023 17:59:58 -0500
From:   Michael Roth <michael.roth@amd.com>
To:     Vlastimil Babka <vbabka@suse.cz>
CC:     <kvm@vger.kernel.org>, <linux-coco@lists.linux.dev>,
        <linux-mm@kvack.org>, <linux-crypto@vger.kernel.org>,
        <x86@kernel.org>, <linux-kernel@vger.kernel.org>,
        <tglx@linutronix.de>, <mingo@redhat.com>, <jroedel@suse.de>,
        <thomas.lendacky@amd.com>, <hpa@zytor.com>, <ardb@kernel.org>,
        <pbonzini@redhat.com>, <seanjc@google.com>, <vkuznets@redhat.com>,
        <jmattson@google.com>, <luto@kernel.org>,
        <dave.hansen@linux.intel.com>, <slp@redhat.com>,
        <pgonda@google.com>, <peterz@infradead.org>,
        <srinivas.pandruvada@linux.intel.com>, <rientjes@google.com>,
        <dovmurik@linux.ibm.com>, <tobin@ibm.com>, <bp@alien8.de>,
        <kirill@shutemov.name>, <ak@linux.intel.com>,
        <tony.luck@intel.com>, <marcorr@google.com>,
        <sathyanarayanan.kuppuswamy@linux.intel.com>,
        <alpergun@google.com>, <dgilbert@redhat.com>, <jarkko@kernel.org>,
        <ashish.kalra@amd.com>, <nikunj.dadhania@amd.com>,
        Brijesh Singh <brijesh.singh@amd.com>
Subject: Re: [PATCH RFC v8 12/56] x86/sev: Add RMP entry lookup helpers
Message-ID: <20230329225958.4s3nwcqyjwzydwsf@amd.com>
References: <20230220183847.59159-1-michael.roth@amd.com>
 <20230220183847.59159-13-michael.roth@amd.com>
 <b48ad925-eff0-8421-730a-6da13bf93ab6@suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <b48ad925-eff0-8421-730a-6da13bf93ab6@suse.cz>
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Mar 2023 23:00:17.4407
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 386e2420-2963-4438-c419-08db30a95d40
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d;Ip=[165.204.84.17];Helo=[SATLEXMB04.amd.com]
X-MS-Exchange-CrossTenant-AuthSource: DM6NAM11FT054.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR12MB8434
X-Spam-Status: No, score=-0.2 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
        DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE autolearn=unavailable
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, Mar 03, 2023 at 04:28:39PM +0100, Vlastimil Babka wrote:
> On 2/20/23 19:38, Michael Roth wrote:
> > From: Brijesh Singh <brijesh.singh@amd.com>
> > 
> > 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://bugzilla.kernel.org/attachment.cgi?id=296015.
> > 
> > Co-developed-by: Ashish Kalra <ashish.kalra@amd.com>
> > Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> > Signed-off-by: Michael Roth <michael.roth@amd.com>
> > ---
> 
> > +/*
> > + * Return 1 if the RMP entry is assigned, 0 if it exists but is not assigned,
> > + * and -errno if there is no corresponding RMP entry.
> > + */
> 
> Hmm IMHO the kernel's idiomatic way is to return 0 on "success" and I'd
> assume the more intuitive expectation of success here if the entry is
> assigned?

In general I'd agree. Here's it's a little awkward though.
snp_lookup_rmpentry() sort of wants to be a bool, where true indicates
an assigned entry was found, false indicates no assigned entry.

But it also has to deal with error values, so the most direct way to
encapsulate that is true == 1, false == 0, and < 0 for errors.

Inverting it to align more with kernel expections of 0 == success/true
gets awkward too, because stuff like:

  if (snp_lookup_rmpentry(...))
    //error

still doesn't work the way most other functions written in this way
would since it could still be "successful" if we were expecting PFN to
be in shared state. So the return value needs special handling there
too.

Would it make sense to define it something like this?:

  /*
   * Query information about the RMP entry corresponding to the given
   * PFN.
   * 
   * Returns 0 on success, and -errno if there was a problem accessing
   * the RMP entry.
   */
  int snp_lookup_rmpentry(u64 pfn, int *level, bool *assigned)

> The various callers seem to differ though so I guess it depends on
> context. Some however don't distinguish their "failure" from an ERR and
> maybe they should, at least for the purposes of the various printks?

Yes, regardless of what we decide above, the call-sites should properly
distinguish between failure/assigned/not-assigned and report the
information accordingly. I'll get those fixed up where needed.

Thanks,

-Mike

> 
> > +int snp_lookup_rmpentry(u64 pfn, int *level)
> > +{
> > +	struct rmpentry *e;
> > +
> > +	e = __snp_lookup_rmpentry(pfn, level);
> > +	if (IS_ERR(e))
> > +		return PTR_ERR(e);
> > +
> > +	return !!rmpentry_assigned(e);
> > +}
> > +EXPORT_SYMBOL_GPL(snp_lookup_rmpentry);
>