Re: nandemulator
On Sun, Feb 24, 2019, 11:33 AM David Holland wrote: > On Sat, Feb 23, 2019 at 02:05:39PM -0700, Warner Losh wrote: > > On Sat, Feb 23, 2019 at 12:40 PM David Holland < > dholland-t...@netbsd.org> > > wrote: > > > > > Do we have docs for the object nandemulator is supposed to be > > > emulating? Some questions have arisen about how complete it is and > > > nobody I've talked to seems to really have answers. > > > > So looking at the code... > >[...] > > I know these aren't definitive answers as I didn't write the code and am > > basing this on briefly studying the code + the knowledge I picked up > about > > NAND while working with planar SLC and MLC NAND in the 34nm to 19nm > > technology nodes for Intel, Micron and Toshiba. So in the absence of > other > > answers, mine may be OK. However, I'd be happy to defer to someone who > > wrote the code and/or did a comparison of commands vs datasheets from > that > > era. > > I think you underestimate how much the rest of us don't know :-) > > Many thanks -- that is definitely enough information to sort things > out, and I'd had no idea even where to begin looking. > I'm happy to fill in more details. I worked at FusionIO for their third and forth generation of cards doing tweaks to thresholds to optimize read performance and reliability... I forget what the baseline for most people is :) I had thought about saying "just a lot of old stuff from the early 2000s," but that seemed to be too vague. But seriously. I'm happy to help in any way I can. Warner >
Re: RFC: New userspace fetch/store API
[thorpej] > My intent was to cover the â??naturalâ??[*] fixed-size types for a > particular arch, and then provide convenience aliases for the common > basic C types: char, short, int, long, pointer. C does not have a `pointer' type; what it has is a `pointer to OTHER-TYPE' for each type OTHER-TYPE. On all NetBSD ports I'm aware of, this is a distinction without a difference, since all memory is the same and all pointers are just memory addresses. Whether this is something to care about depends on how much you are willing to depend on the compiler letting you get away with things that C does not, strictly, promise. [still thorpej, but another message] > If weâ??re concerned about portability of the things using this API, > then we simply specify the alignment to be sizeof(type). That check > is straight-forward in MI C. Not really. There is no MI way to check the alignment of a pointer. Pointers cannot be arguments to & or %, and there is nothing MI about converting a pointer to an integer. Again, on all NetBSD ports I'm aware of, the situation is nicer; it would be reasonable to say that it's straightforward in MI NetBSD code, but it's not really MI C. [dholland, replying to the second of the above quotes] > No, even if you know what the alignment's supposed to be, you can't > legally check it. Well, you can check it, certainly. But such checking works, when it does, only because the compiler happens to implement certain implementation-dependent aspects of C in whichever way the checking code you use expects. [dholland, same message] > Also, these days you can expect the compiler to simply remove such > checks on the grounds that all pointers to types with required > alignment are expected to be already aligned. Such a compiler is not suitable for compiling kernels, because of exactly this sort of thing. Compilers have a lot of latitude according to the C spec, yes, but compilers that take advantage of it (and cannot be told to not do so) are not suitable for building implementation infrastructure, such as even malloc(), never mind a kernel. /~\ The ASCII Mouse \ / Ribbon Campaign X Against HTMLmo...@rodents-montreal.org / \ Email! 7D C8 61 52 5D E7 2D 39 4E F1 31 3E E8 B3 27 4B
re: RFC: New userspace fetch/store API
> > On Feb 23, 2019, at 4:04 PM, matthew green wrote: > > > > i like this. the current API is ... odd. > > Oh any thoughts on have intrsafe or not? if we can get away without it, that would be best. how hard is the single(?) caller to fix? :-) .mrg.
Re: RFC: New userspace fetch/store API
> On Feb 24, 2019, at 1:42 PM, Mindaugas Rasiukevicius wrote:
>
>> 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.
Again, I’m happy to eliminate the _intrsafe variants. The only use is in the
profiling code, and it’s just to avoid taking an AST.
> 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.
There are lots of uses in MD code, and a few in MI code (ureadc() and
mincore(), for example). I figured having the same API when it’s just MD code
would be worthwhile.
>> 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.
I’ll make C wrappers for all of these, and have the MD guts prefixed with an
underscore so I can centralize the assertion logic.
>> 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_*
Even at the expense of being different from atomic_ops(3)?
-- thorpej
Re: RFC: New userspace fetch/store API
On Sun, 24 Feb 2019 at 10:38, Jason Thorpe wrote:
>
>
> > On Feb 24, 2019, at 7:05 AM, Andrew Cagney wrote:
> >
> > Can there be an inefficient default, implemented using something like
> > copy{in,out}() (yes I note the point about alignment). I sometimes
> > wonder of NetBSD's lost its way in terms of portability - it seems to
> > be accumulating an increasing number of redundant yet required
> > primitives.
>
> Honestly, it’s not adding that many — most of the ones specified in my
> proposal were required before (to varying degrees) just with different names
> and return semantics. And most of the “new” ones in my proposal as simple
> aliases. Furthermore, on the two implementations I’ve done so far, a great
> deal of the code is macro-ized to reduce the redundant typing, and even the
> “intrsafe” ones share most of the code by using a different (macro-ized)
> prologue and then jumping into the body of the non-intrsafe variant. It’s
> really not that much code.
Right.. But can it be done once in generic code using information
available while compiling?
> As far as “inefficient default” … depending on how copyin / copyout are
> implemented, it’s also an issue of correctness. At least the 32-bit and
> 64-bit variants would need to be atomic with respect to other loads / stores
> to the same address. This is one property that I need for my own use.
And that's a hard requirement. What of the other way around - copy*()
implemented using fetch_*().
> If there really is a concern about the number of operations here,I would be
> more than happy to drop the _8 and _16 (and thus char and short) widths from
> the formal API… but then other MI code would need to change to lose that
> functionality (in the case of the profiling code that uses fuswintr() and
> suswintr()), or to use heavier means of doing the same thing (e.g. copyin or
> copyout of a single byte).
>
> -- thorpej
>
Re: RFC: New userspace fetch/store API
> On Feb 24, 2019, at 8:26 PM, matthew green wrote: > >>> On Feb 23, 2019, at 4:04 PM, matthew green wrote: >>> >>> i like this. the current API is ... odd. >> >> Oh any thoughts on have intrsafe or not? > > if we can get away without it, that would be best. how hard is > the single(?) caller to fix? :-) Not at all — just make it use the “got a page fault” case always. I’ve done it in my local tree. -- thorpej
Re: RFC: New userspace fetch/store API
Jason Thorpe 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
Re: RFC: New userspace fetch/store API
> On Feb 24, 2019, at 10:28 AM, David Holland wrote: > > No, even if you know what the alignment's supposed to be, you can't > legally check it. Or maybe you can, but it's in any event tangled in a > bunch of language-lawyering. > > Also, these days you can expect the compiler to simply remove such > checks on the grounds that all pointers to types with required > alignment are expected to be already aligned. Grumble, mumble… ok, fair. > The ones I remember looking at (which is not all of them, and probably > not alpha) didn't test onfault until they got to we_re_toast, meaning > any fault is already covered. But in any event it's free at runtime > (the sense of free I meant) because you only get to it when a trap > would otherwise be triggering a panic. I guess I’ll audit them as part of the process. > It's only not free on architectures where there are special > instructions for accessing usermode and they need to be explicitly > checked for failure rather than using on-fault logic; in that case you > might also need an explicit alignment check to avoid allowing bad > pointers to trigger a panic. That check should be in the MD code for > such ports. (If there even are any... I think there exist machines > that work this way, not sure we run on any of them.) m68k uses special instructions to access user space (well, “the other address space”) … but they also don’t have alignment constraints. Hilarious! -- thorpej
Re: nandemulator
On Sat, Feb 23, 2019 at 02:05:39PM -0700, Warner Losh wrote: > On Sat, Feb 23, 2019 at 12:40 PM David Holland > wrote: > > > Do we have docs for the object nandemulator is supposed to be > > emulating? Some questions have arisen about how complete it is and > > nobody I've talked to seems to really have answers. > > So looking at the code... >[...] > I know these aren't definitive answers as I didn't write the code and am > basing this on briefly studying the code + the knowledge I picked up about > NAND while working with planar SLC and MLC NAND in the 34nm to 19nm > technology nodes for Intel, Micron and Toshiba. So in the absence of other > answers, mine may be OK. However, I'd be happy to defer to someone who > wrote the code and/or did a comparison of commands vs datasheets from that > era. I think you underestimate how much the rest of us don't know :-) Many thanks -- that is definitely enough information to sort things out, and I'd had no idea even where to begin looking. -- David A. Holland dholl...@netbsd.org
Re: RFC: New userspace fetch/store API
On Sun, Feb 24, 2019 at 08:47:12AM -0800, Jason Thorpe wrote: > > > On Feb 24, 2019, at 8:42 AM, David Holland > > wrote: > > Given that the alignment check _is_ free on some architectures, and > > that dealing with alignment in machine-independent C is problematic, > > that doesn't seem like the right way? > > If we?re concerned about portability of the things using this API, > then we simply specify the alignment to be sizeof(type). That > check is straight-forward in MI C. No, even if you know what the alignment's supposed to be, you can't legally check it. Or maybe you can, but it's in any event tangled in a bunch of language-lawyering. Also, these days you can expect the compiler to simply remove such checks on the grounds that all pointers to types with required alignment are expected to be already aligned. > The alignment check is not necessary ?free? in MD code ? a > mis-aligned address would likely trigger an alignment fault (SIGBUS > code path) rather than a page fault (SIGSEGV code path) in the trap > handler. Certainly, it didn?t seem like the alpha trap handler > handled a misaligned address in this case at all. This isn?t the > end of the world, but it means putting ?onfault? logic in more than > one code path in the trap handler. The ones I remember looking at (which is not all of them, and probably not alpha) didn't test onfault until they got to we_re_toast, meaning any fault is already covered. But in any event it's free at runtime (the sense of free I meant) because you only get to it when a trap would otherwise be triggering a panic. It's only not free on architectures where there are special instructions for accessing usermode and they need to be explicitly checked for failure rather than using on-fault logic; in that case you might also need an explicit alignment check to avoid allowing bad pointers to trigger a panic. That check should be in the MD code for such ports. (If there even are any... I think there exist machines that work this way, not sure we run on any of them.) -- David A. Holland dholl...@netbsd.org
Re: RFC: New userspace fetch/store API
> On Feb 24, 2019, at 10:07 AM, Brian Buhrow wrote: > > hello. It strikes me that if you've already written the code to fetch > and store 8, 16, 32 and sometimes 64 bit values, it should be committed. I am pretty sure most platforms already have the 8- and 16-bit variants, just under different names. I did have to write new implementations for alpha, as they were there-but-not-fully-implemented. -- thorpej
Re: RFC: New userspace fetch/store API
hello. It strikes me that if you've already written the code to fetch and store 8, 16, 32 and sometimes 64 bit values, it should be committed. If other OS's have this functionality, then it means applications wil use it and subsequent porting of such applications to NetBSD will be easier if the functionality is there. It may seem like I'm arguing we should do it because others did, and perhaps I am to some extent, but having an easy platform to port aplications to makes netBSD much more attractive than it would be if a bunch of functionality that's expected by application authors is missing. Just my 2 cents. -thanks -Brian
Re: RFC: New userspace fetch/store API
> On Feb 24, 2019, at 9:40 AM, Jason Thorpe wrote: > > 32-bit on every platform, and 64-bit on LP64 platforms (for pointers). > > I’m fine with that, so long as we’re willing to “give up” on the other things > that use byte / short / in-interrupt. I will note, however, that some platforms are still going to need the byte / short variants for their own MD purposes (m68k trap handler, for example). If this is the direction we decide to go, I will make them use the same naming pattern, and will protect their prototypes inside a __HAVE_USER_FETCH_STORE_16 or whatever. -- thorpej
Re: RFC: New userspace fetch/store API
> On Feb 24, 2019, at 9:12 AM, Joerg Sonnenberger wrote: > > I'm far more inclined to just want the limited API with as many > constraints as reasonable, e.g. an atomic 32bit fetch or store and > that's it. Maybe add 64bit variants, but no more. No point really in > having a lot of dead weight around. 32-bit on every platform, and 64-bit on LP64 platforms (for pointers). I’m fine with that, so long as we’re willing to “give up” on the other things that use byte / short / in-interrupt. -- thorpej
Re: RFC: New userspace fetch/store API
On Sat, Feb 23, 2019 at 04:40:40PM -0800, Jason Thorpe wrote: > >> i like this. the current API is ... odd. > >> > >> can you add alignment documentation? eg, if only to say that it > >> must support aligned or unaligned accesses for the relevant > >> datatypes. not sure what we need currently, ie, if unaligned is > >> needed because that happens today, or we can define it as wrong > >> in this API. > > > > Yah, I?ll make that clear. It?s implicit in the current API (alpha would > > blow up otherwise), but I will make it explicit. > > I added the following: > > > All addresses MUST be naturally-aligned for their type. If unaligned > access is required, use copy(9). > > > OK? Given that the predominant use of these is to fetch from user-supplied pointers, which can be crap, it would best to include an alignment check; otherwise ~all call sites will require explicit checks, which in addition to being error-prone start running into annoying portability questions. (On at least some platforms the alignment check is free, also, because an alignment fault during the access will be caught by the copyin fault logic.) -- David A. Holland dholl...@netbsd.org
Re: RFC: New userspace fetch/store API
[Sorry, I meant to respond to multiple points in my prior message, but the coffee hasn’t fully kicked in yet…] > On Feb 24, 2019, at 7:05 AM, Andrew Cagney wrote: > > On Sat, 23 Feb 2019 at 18:26, Jason Thorpe wrote: > >> A project I’m working on has a need for a proper “int” size fetch / store on >> all platforms, and it seems best to just tidy the whole mess up. So, I am >> proposing a new replacement user fetch/store API. > > Can you be more specific on why "int" rather than a more abstract type > (we've things like size_t ptrdiff_t et.al. all kicking around) - I'm > curious. In my particular case, it’s the kernel portion of an API that on other platforms uses an “int” in this particular argument slot. For the sake of not only API compatibility as well as COMPAT_* compatibility, “int” is what I decided to go with for my particular use case. My intent was to cover the “natural”[*] fixed-size types for a particular arch, and then provide convenience aliases for the common basic C types: char, short, int, long, pointer. The latter are aliases (and I wanted to place the aliases in a centralized location, subr_copy.c, but the assembler / linker doesn’t like aliasing symbols in a different compilation unit). [*] pre-BWX alpha of course has to emulate 8- and 16-bit accesses. >> int ufetch_8(const uint8_t *uaddr, uint8_t *valp); > > To follow through with Edgar Fuß's point. If I were looking for a > function to transfer a uint8_t I'd expect it to be called > fetch_uint8(). As I mentioned in my reply to Edgar, this is to be consistent with atomic_ops(3), which in turn used the same names as Solaris. > I do find passing pointers as both the addr and the dest confusing; > but you've added a const which should help get the order right. Yes, well, one of the problems with the old fetch(9) API is that “error condition” and “value” are returned in the same place, and thus there are ambiguous cases. -- thorpej
Re: RFC: New userspace fetch/store API
> On Feb 24, 2019, at 7:05 AM, Andrew Cagney wrote:
>
> Can there be an inefficient default, implemented using something like
> copy{in,out}() (yes I note the point about alignment). I sometimes
> wonder of NetBSD's lost its way in terms of portability - it seems to
> be accumulating an increasing number of redundant yet required
> primitives.
Honestly, it’s not adding that many — most of the ones specified in my proposal
were required before (to varying degrees) just with different names and return
semantics. And most of the “new” ones in my proposal as simple aliases.
Furthermore, on the two implementations I’ve done so far, a great deal of the
code is macro-ized to reduce the redundant typing, and even the “intrsafe” ones
share most of the code by using a different (macro-ized) prologue and then
jumping into the body of the non-intrsafe variant. It’s really not that much
code.
As far as “inefficient default” … depending on how copyin / copyout are
implemented, it’s also an issue of correctness. At least the 32-bit and 64-bit
variants would need to be atomic with respect to other loads / stores to the
same address. This is one property that I need for my own use.
If there really is a concern about the number of operations here,I would be
more than happy to drop the _8 and _16 (and thus char and short) widths from
the formal API… but then other MI code would need to change to lose that
functionality (in the case of the profiling code that uses fuswintr() and
suswintr()), or to use heavier means of doing the same thing (e.g. copyin or
copyout of a single byte).
-- thorpej
Re: RFC: New userspace fetch/store API
On Sat, 23 Feb 2019 at 18:26, Jason Thorpe wrote:
> A project I’m working on has a need for a proper “int” size fetch / store on
> all platforms, and it seems best to just tidy the whole mess up. So, I am
> proposing a new replacement user fetch/store API.
Can you be more specific on why "int" rather than a more abstract type
(we've things like size_t ptrdiff_t et.al. all kicking around) - I'm
curious.
> I have implemented the new API for alpha and amd64, and am putting together a
> test harness to exercise all aspects of the new API. Once that is done, I’ll
> tackle the remaining architectures.
> The implementation is entirely in architecture-dependent code. The following
> fetch primitives must be supplied:
As a corollary to KRE's point, why?
Can there be an inefficient default, implemented using something like
copy{in,out}() (yes I note the point about alignment). I sometimes
wonder of NetBSD's lost its way in terms of portability - it seems to
be accumulating an increasing number of redundant yet required
primitives.
> int ufetch_8(const uint8_t *uaddr, uint8_t *valp);
To follow through with Edgar Fuß's point. If I were looking for a
function to transfer a uint8_t I'd expect it to be called
fetch_uint8().
> 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
>
> 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);
I do find passing pointers as both the addr and the dest confusing;
but you've added a const which should help get the order right.
Re: RFC: New userspace fetch/store API
> On Feb 24, 2019, at 2:34 AM, Edgar Fuß wrote: > >> 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); > I would have expected the argument of a funtion called xxx_short etc. > to be a short etc., not an unsigned short etc. > Does it make sense to either make the argument char/short/int/long or > change the name to ufetch_uXXX? My proposed names match the pattern used by atomic_ops(3) (which in turn matches how Solaris named those routines). In general I would favor consistency with atomic_ops(3) for consistency’s sake, but I’m not wedded to the names and will change them if the consensus is that they should change. -- thorpej
Re: RFC: New userspace fetch/store API
> 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); I would have expected the argument of a funtion called xxx_short etc. to be a short etc., not an unsigned short etc. Does it make sense to either make the argument char/short/int/long or change the name to ufetch_uXXX?
