Re: [PATCH RFC v8 12/56] x86/sev: Add RMP entry lookup helpers

[Michael Roth]

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@xxxxxxx>
> > 
> > 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@xxxxxxx>
> > Signed-off-by: Ashish Kalra <ashish.kalra@xxxxxxx>
> > Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx>
> > Signed-off-by: Michael Roth <michael.roth@xxxxxxx>
> > ---
> 
> > +/*
> > + * 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);
>