Re: RFC: New userspace fetch/store API

[Mindaugas Rasiukevicius <rmind%netbsd.org@localhost>]

Jason Thorpe <thorpej%me.com@localhost> wrote:
> The existing fetch(9) / store(9) APIs have some problems.  Specifically:
> 
> ==> Their return semantics mean that fuword() cannot disambiguate between
> an error condition (-1) and a legitimate fetch of -1 from user space.
> 
> ==> “word” is poorly defined.  For all practical purposes, it means
> “long”, and there is thus no way to fetch/store an “int” value on LP64
> platforms.
> 
> ==> There are lots of legacy aliases lying about that are no longer
> meaningful (I-space and D-space variants, for example).

Thanks for working on this.  Actually, I started the same revamp with a
similar API (suggested by matt@ at that time) a bunch of years ago.. but
lost interest and didn't finish.

> Outstanding question before I go too far down the rabbit hole: should I
> bother with the “intrsafe” variants?  The only application for it in the
> tree right now is in the profiling code, as an optimization to avoid
> taking an AST when it’s time to bump the counters.

Please do not over-engineer it.  IIRC, there are not that many use cases
of this API, the need for more variants didn't really increase over the
decade(s) and there is no need to provide a vast spectrum of functions.
They can be added later, if there will be a need.

Also, are there any uses of byte and short operations?  If not, then I
think these variants should be eliminated.  Nowadays, I would argue that
generally there should be no good reason to choose sub-word operations;
stick with the word size (it helps with the alignment too) or more.

> 
> These functions may block (to service a page fault), and are NOT safe to
> call from any interrupt context.
> 

Please make sure you add the asserts.

> The implementation is entirely in architecture-dependent code.  The
> following fetch primitives must be supplied:
> 
> int     ufetch_8(const uint8_t *uaddr, uint8_t *valp);
> int     ufetch_16(const uint16_t *uaddr, uint16_t *valp);
> int     ufetch_32(const uint32_t *uaddr, uint32_t *valp);
> #ifdef _LP64
> int     ufetch_64(const uint64_t *uaddr, uint64_t *valp);
> #endif

Please prefix the size with 'u' if you operate on unsigned types:

ufetch_u{8,16,32,64} and the same for ustore_*

> The following aliases must also be provided, mapped to the appropriate
> fixed-size primitives:
> 
> int     ufetch_char(const unsigned char *uaddr, unsigned char *valp);
> int     ufetch_short(const unsigned short *uaddr, unsigned short *valp);
> int     ufetch_int(const unsigned int *uaddr, unsigned int *valp);
> int     ufetch_long(const unsigned long *uaddr, unsigned long *valp);
> int     ufetch_ptr(const void **uaddr, void **valp);
> 
> The following store primitives must be suppled:
> 
> int     ustore_8(uint8_t *uaddr, uint8_t val);
> int     ustore_16(uint16_t *uaddr, uint16_t val);
> int     ustore_32(uint32_t *uaddr, uint32_t val);
> #ifdef _LP64
> int     ustore_64(uint64_t *uaddr, uint64_t val);
> #endif
> 
> The following aliases must also be provided, mapped to the appropriate
> fixed-size primitives:
> 
> int     ustore_char(unsigned char *uaddr, unsigned char val);
> int     ustore_short(unsigned short *uaddr, unsigned short val);
> int     ustore_int(unsigned int *uaddr, unsigned int val);
> int     ustore_long(unsigned long *uaddr, unsigned long val);
> int     ustore_ptr(void **uaddr, void *val);

The same applies here (e.g. ustore_int becomes ustore_uint).

-- 
Mindaugas