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

[Vlastimil Babka]

On 3/30/23 00:59, Michael Roth wrote:
> 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)

Yeah that looks fine to me. Hope you find out it makes it easier to work
with in the callers too.

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

Great, thanks!

> 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);
>>