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