Re: CVS commit: src/lib/librumpuser
On Mon, 16 Jun 2014, Alexander Nasonov wrote: Log Message: Add __RCSID. +#if !defined(lint) +__RCSID($NetBSD: rumpuser_bio.c,v 1.8 2014/06/16 21:07:28 alnsn Exp $); +#endif /* !lint */ Some historical uses of __RCSID have an unnecessary #if/#endif wrapper around them, but for new uses, please just write __RCSID(...); without any #if/#endif wrapper. --apb (Alan Barrett)
Re: CVS commit: src/lib/librumpuser
Alan Barrett wrote: Some historical uses of __RCSID have an unnecessary #if/#endif wrapper around them, but for new uses, please just write __RCSID(...); without any #if/#endif wrapper. I copy/pasted this block from another file from the same directory. Alex
Re: CVS commit: src/lib/librumpuser
On Tue, Jun 17, 2014 at 08:42:35AM +, Alexander Nasonov wrote: Module Name: src Committed By: alnsn Date: Tue Jun 17 08:42:35 UTC 2014 Modified Files: src/lib/librumpuser: Makefile Log Message: Antti objected to including rumpuser_sync_icache. Exclude it from the build. To be clear: the objection was to modifying a stable interface without coordination. The hypercall interface is implemented in multiple places outside of the NetBSD tree, including by 3rd parties. Think of it like changing the libc ABI: no matter the merits of the change itself (which they too remain to be discussed), it will be objected to.
Re: CVS commit: src/lib/librumpuser
Antti Kantee wrote: To be clear: the objection was to modifying a stable interface without coordination. The hypercall interface is implemented in multiple places outside of the NetBSD tree, including by 3rd parties. Stable interface in -current? How are you supposed to make any significant changes then? As I stated in the private email to Antti, we need to support two versions: stable for users and current for developers. Otherwise, it will block a development of NetBSD if every time you need to make a change in NetBSD-current, you have to wait for a rumpuser bump for stable users even though many of them are outside of NetBSD. In other words, don't rely on NetBSD-current being 100% stable, especially for users outside of NetBSD. I don't know enough details about rump to be certain but would splitting only rumpuser into two versions work? If there were two versions of rumpuser I could tweak the makefile to build cpufunc.c only if MKSLJIT=yes and it would solve the problem because MKSLJIT is no by default on arm. Another reason for splitting rumpuser is because some hypercalls can't be implemented with POSIX API. I don't think we need to support Linux or Solaris in NetBSD tree, especailly if it depends on OS-specific code. Think of it like changing the libc ABI: no matter the merits of the change itself (which they too remain to be discussed), it will be objected to. I got an impression from your private email yesterday that the approach is correct (you didn't say that librumpuser code must be POSIX while the code I sent to you wasn't as it used NetBSD specific sysarch syscall). And because it was a pure addition of a new function and it didn't break NetBSD build, I assumed that it's safe to add it. The only thing I didn't do was rumpuser ABI version bump. Alex
Re: CVS commit: src/lib/librumpuser
On 17/06/14 09:46, Alexander Nasonov wrote: Antti Kantee wrote: To be clear: the objection was to modifying a stable interface without coordination. The hypercall interface is implemented in multiple places outside of the NetBSD tree, including by 3rd parties. Stable interface in -current? How are you supposed to make any significant changes then? As I stated in the private email to Antti, we need to support two versions: stable for users and current for developers. Otherwise, it will block a development of NetBSD if every time you need to make a change in NetBSD-current, you have to wait for a rumpuser bump for stable users even though many of them are outside of NetBSD. In other words, don't rely on NetBSD-current being 100% stable, especially for users outside of NetBSD. I don't know enough details about rump to be certain but would splitting only rumpuser into two versions work? If there were two versions of rumpuser I could tweak the makefile to build cpufunc.c only if MKSLJIT=yes and it would solve the problem because MKSLJIT is no by default on arm. I think that will be overengineering it, but if you want to come up with a concrete proposal patch, please. I'd simply use discussion and not rushing commits to avoid issues here. If you don't have time to wait for discussion or coordination, do everything in the privacy of the sljit component. In either case, there's no need for the blocking development drama card. Another reason for splitting rumpuser is because some hypercalls can't be implemented with POSIX API. I don't think we need to support Linux or Solaris in NetBSD tree, especailly if it depends on OS-specific code. I don't think we need to support Linux or Solaris in the NetBSD tree. Similarly, I don't think we need to make changes which _unnecessarily_ makes supporting them hard. Think of it like changing the libc ABI: no matter the merits of the change itself (which they too remain to be discussed), it will be objected to. I got an impression from your private email yesterday that the approach is correct (you didn't say that librumpuser code must be POSIX while the code I sent to you wasn't as it used NetBSD specific sysarch syscall). For reference, here's what I wrote: === snip === If the problem is syncing icache, the approach is correct. However, I'd argue that the problem is dynamically loading code into a rump kernel, and with that the interface falls somewhat short. What if someone wants W^X? === snip === If you understood that as go ahead, sorry for not being clear, falls short was the comment that I was trying to ease in. I'm not saying that librumpuser must be POSIX. I'm not sure where you're getting that from. What I _am_ saying is that there must be some critical thought going in to the interfaces and their implications. We're several years past the oh I'll just add this because I need it today stage of discovery. And because it was a pure addition of a new function and it didn't break NetBSD build, I assumed that it's safe to add it. The only thing I didn't do was rumpuser ABI version bump. It's a pure addition in the same sense as if you made a pure addition to NetBSD's Xen hypercall interface and made guests unconditionally use it. It would not break the NetBSD build. Would you assume that's a safe change to make?
Re: CVS commit: src/lib/librumpuser
Antti Kantee wrote: I think that will be overengineering it, but if you want to come up with a concrete proposal patch, please. I'd simply use discussion and not rushing commits to avoid issues here. The code is in the tree already. I don't need anything else for sljit. The sljit library doesn't support W^X protection. Rumpuser has a way to create an executable page via rumpuser_anonmap(... exec=true). I understand it's not perfect but it's not my area to change memory management in rumpuser. If you don't have time to wait for discussion or coordination, do everything in the privacy of the sljit component. Please teach me how to create a private component. In either case, there's no need for the blocking development drama card. It's not a drama, it's a technical issue. ... For reference, here's what I wrote: === snip === If the problem is syncing icache, the approach is correct. However, I'd argue that the problem is dynamically loading code into a rump kernel, and with that the interface falls somewhat short. What if someone wants W^X? === snip === If you understood that as go ahead, sorry for not being clear, falls short was the comment that I was trying to ease in. My problem is syncing icache, therefore, the approach is correct. I'm not saying that librumpuser must be POSIX. I'm not sure where you're getting that from. rumpuser(3): DESCRIPTION The rumpuser hypercall interfaces allow a rump kernel to access host resources. A hypervisor implementation must implement the routines described in this document to allow a rump kernel to run on the host. The implementation included in NetBSD is for POSIX hosts. ^ ^ and it indeed broke buildrump.sh builds on Linux because sysarch stuff isn't available. There is no way I can make this interface POSIX-compatible because POSIX doesn't specify icache sync as far as I know. What I _am_ saying is that there must be some critical thought going in to the interfaces and their implications. We're several years past the oh I'll just add this because I need it today stage of discovery. Ok, I'll leave it to you to think about it. All I need now is a private component for my change. ... It's a pure addition in the same sense as if you made a pure addition to NetBSD's Xen hypercall interface and made guests unconditionally use it. It would not break the NetBSD build. Would you assume that's a safe change to make? In my case, I could easily build icache in rumpkern conditionally with MKSLJIT if librumpuser didn't break on non NetBSD hosts. Alex
Re: CVS commit: src/lib/librumpuser
Antti Kantee wrote: Use RUMPCOMP_USER_SRCS, several examples under src/sys/rump Thanks for the pointer, will do. That's one more indication that sync icache is the wrong level of problem to represent at the interface level. Existence of __clear_cache is an indication of the opposite. I'm not saying that POSIX should have icache sync routine, they could specify that mprotect syscall that turns on PROT_EXEC should sync icache. But that's no guaranteed by POSIX. If it were a high-level, holistic interface, both the caller and callee would what needs to be done, and the caller could perhaps implement the same with a alternative method. A low-level interface like sync icache for this memory range leaves no room for interpretation, even if it will ever be used for only one purpose. Not quite sure what you're saying because no room for interpretation is a good thing in general and using something for one purpose is even better ;-) -- Alex
Re: CVS commit: src/lib/librumpuser
On 17/06/14 11:23, Alexander Nasonov wrote: If you don't have time to wait for discussion or coordination, do everything in the privacy of the sljit component. Please teach me how to create a private component. Use RUMPCOMP_USER_SRCS, several examples under src/sys/rump I'm not saying that librumpuser must be POSIX. I'm not sure where you're getting that from. rumpuser(3): DESCRIPTION The rumpuser hypercall interfaces allow a rump kernel to access host resources. A hypervisor implementation must implement the routines described in this document to allow a rump kernel to run on the host. The implementation included in NetBSD is for POSIX hosts. ^ ^ ic. I'd change that to POSIX-like, but currently I cannot access nbcvs. and it indeed broke buildrump.sh builds on Linux because sysarch stuff isn't available. There is no way I can make this interface POSIX-compatible because POSIX doesn't specify icache sync as far as I know. That's one more indication that sync icache is the wrong level of problem to represent at the interface level. If it were a high-level, holistic interface, both the caller and callee would what needs to be done, and the caller could perhaps implement the same with a alternative method. A low-level interface like sync icache for this memory range leaves no room for interpretation, even if it will ever be used for only one purpose.
Re: CVS commit: src/lib/librumpuser
On 17/06/14 11:52, Alexander Nasonov wrote: That's one more indication that sync icache is the wrong level of problem to represent at the interface level. Existence of __clear_cache is an indication of the opposite. Let's be thankful we're not discussing implementing a compiler. If it were a high-level, holistic interface, both the caller and callee would what needs to be done, and the caller could perhaps implement the same with a alternative method. A low-level interface like sync icache for this memory range leaves no room for interpretation, even if it will ever be used for only one purpose. Not quite sure what you're saying because no room for interpretation is a good thing in general and using something for one purpose is even better ;-) Ok, one more try, this time with an example: * fact: interface will be used to say I have loaded code here, please arrange things so that it can be executed * fact: you want to call the interface sync icache and possess those semantics the example: Let's assume some fictional platform where you need to sign newly loaded code before it can execute. Should the implementation of sync icache on that platform sign code? If not, you can't execute the code, which was the original purpose. If yes, you've failed to implement the interface correctly, because that might not be what the caller wants at all. Will such a platform be supported? Who knows. Is it cause to leave known problems into the interface? Definitely not! If you don't want to solve anything except your immediate problem, that's more than fair enough (we've all been there), but it needs to be done without breaking things for everyone else or knowingly introducing suboptimal interfaces that everyone else will suffer from.
Re: CVS commit: src/lib/librumpuser
Antti Kantee wrote: Ok, one more try, this time with an example: * fact: interface will be used to say I have loaded code here, please arrange things so that it can be executed * fact: you want to call the interface sync icache and possess those semantics the example: Let's assume some fictional platform where you need to sign newly loaded code before it can execute. Should the implementation of sync icache on that platform sign code? If not, you can't execute the code, which was the original purpose. If yes, you've failed to implement the interface correctly, because that might not be what the caller wants at all. Will such a platform be supported? Who knows. Is it cause to leave known problems into the interface? Definitely not! You're over generalizing. You can't generate and sign your own code without posing a security risk. This case deserves a special interface, you can't just bring it under umbrella of a single hypercall that does everything to turn memory into code. I'm following a common practice of calling __clear_cache for JIT code except that I wrap it as a hypercall. If you don't want to solve anything except your immediate problem, that's more than fair enough (we've all been there), We've seen many examples of over generalizing too. My need is driven by existing sljit code which follows a common practice of calling __clear_cache() gcc extension. If you want to generalize it, we will need to work with sljit upstream to design a new interface. Alternatively, we can disable jit code on arm and mips and give intel a favor. but it needs to be done without breaking things for everyone else or knowingly introducing suboptimal interfaces that everyone else will suffer from. Everyone else is a multi-dimentional term. Are you referring to users of other operating systems or users of arches where sync_icache is noop or a hypotetical arch where you need to sign code before running it? (well, it's probably not hypotetical anymore, I vaguely remember reading about a similar feature few months ago). As I stated in the earlier email, I can build my rumpkern stuff conditionally if you split librumpuser into NetBSD and non-NetBSD versions. I'm going to check how it will work with a private component. Alex
Re: CVS commit: src/lib/librumpuser
On Tue, Jun 17, 2014 at 16:20:57 +0200, Joerg Sonnenberger wrote: On Tue, Jun 17, 2014 at 06:43:21AM +, Alexander Nasonov wrote: Module Name:src Committed By: alnsn Date: Tue Jun 17 06:43:21 UTC 2014 Modified Files: src/lib/librumpuser: rumpuser_pth_dummy.c Log Message: For consistency with other files in the same directory don't include sys/cdefs.h before __RCSID. This one is actually wrong, the include should be the first thing in the file, followed by __RCSID without conditional. In that case a local rump header plays that role (either including cdefs.h or providing the necessary definition). The conditional is a separate issue and yes it shoud be gc'ed. -uwe
Re: CVS commit: src/lib/librumpuser
Valery Ushakov wrote: In that case a local rump header plays that role (either including cdefs.h or providing the necessary definition). Yes, I believe that rumpuser_port.h includes sys/cdefs.h. The conditional is a separate issue and yes it shoud be gc'ed. I agree but I wanted to be consistent with other files in the same directory. Alex
