| Message ID | 1306149553-26793-5-git-send-email-levinsasha928@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Sasha Levin <levinsasha928@gmail.com> wrote: > @@ -511,7 +515,13 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix) > kvm->nrcpus = nrcpus; > > memset(real_cmdline, 0, sizeof(real_cmdline)); > - strcpy(real_cmdline, "notsc noapic noacpi pci=conf1 console=ttyS0 earlyprintk=serial"); > + strcpy(real_cmdline, "notsc noapic noacpi pci=conf1"); > + if (vnc) { > + strcat(real_cmdline, " video=vesafb console=tty0"); > + vidmode = 0x312; > + } else { > + strcat(real_cmdline, " console=ttyS0 earlyprintk=serial"); > + } Hm, i think all the kernel parameter handling code wants to move into driver specific routines as well. Something like: serial_init(kvm, real_cmdline); where serial_init() would append to real_cmdline if needed. This removes a bit of serial-driver specific knowledge from kvm-run.c. Same goes for the VESA driver and the above video mode flag logic. > @@ -597,6 +607,9 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix) > > kvm__init_ram(kvm); > > + if (vnc) > + vesa__init(kvm); Shouldnt vesa__init() itself know about whether it's active (i.e. the 'vnc' flag is set) and return early if it's not set? That way this could become more encapsulated and self-sufficient: vesa__init(kvm); With no VESA driver specific state exposed to the generic kvm_cmd_run() function. Ideally kvm_cmd_run() hould just be a series of: serial_init(kvm, real_cmdline); vesa_init(kvm, real_cmdline); ... initialization routines. Later on even this could be removed: using section tricks we can put init functions into a section and drivers could register their init function like initcall(func) functions are registered within the kernel. kvm_cmd_run() could thus iterate over that (build time constructed) section like this: extern initcall_t __initcall_start[], __initcall_end[], __early_initcall_end[]; static void __init do_initcalls(void) { initcall_t *fn; for (fn = __early_initcall_end; fn < __initcall_end; fn++) do_one_initcall(*fn); } and would not actually have *any* knowledge about what drivers were built in. Currently it's fine to initialize everything explicitly - but this would be the long term model to work towards ... Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/23/11 2:38 PM, Ingo Molnar wrote: > * Sasha Levin<levinsasha928@gmail.com> wrote: > >> @@ -511,7 +515,13 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix) >> kvm->nrcpus = nrcpus; >> >> memset(real_cmdline, 0, sizeof(real_cmdline)); >> - strcpy(real_cmdline, "notsc noapic noacpi pci=conf1 console=ttyS0 earlyprintk=serial"); >> + strcpy(real_cmdline, "notsc noapic noacpi pci=conf1"); >> + if (vnc) { >> + strcat(real_cmdline, " video=vesafb console=tty0"); >> + vidmode = 0x312; >> + } else { >> + strcat(real_cmdline, " console=ttyS0 earlyprintk=serial"); >> + } > Hm, i think all the kernel parameter handling code wants to move into driver > specific routines as well. Something like: > > serial_init(kvm, real_cmdline); > > where serial_init() would append to real_cmdline if needed. > > This removes a bit of serial-driver specific knowledge from kvm-run.c. > > Same goes for the VESA driver and the above video mode flag logic. > >> @@ -597,6 +607,9 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix) >> >> kvm__init_ram(kvm); >> >> + if (vnc) >> + vesa__init(kvm); > Shouldnt vesa__init() itself know about whether it's active (i.e. the 'vnc' > flag is set) and return early if it's not set? > > That way this could become more encapsulated and self-sufficient: > > vesa__init(kvm); > > With no VESA driver specific state exposed to the generic kvm_cmd_run() > function. > > Ideally kvm_cmd_run() hould just be a series of: > > serial_init(kvm, real_cmdline); > vesa_init(kvm, real_cmdline); > ... > > initialization routines. Later on even this could be removed: using section > tricks we can put init functions into a section and drivers could register > their init function like initcall(func) functions are registered within the > kernel. kvm_cmd_run() could thus iterate over that (build time constructed) > section like this: > > extern initcall_t __initcall_start[], __initcall_end[], __early_initcall_end[]; > > static void __init do_initcalls(void) > { > initcall_t *fn; > > for (fn = __early_initcall_end; fn< __initcall_end; fn++) > do_one_initcall(*fn); > } > > and would not actually have *any* knowledge about what drivers were built in. > > Currently it's fine to initialize everything explicitly - but this would be the > long term model to work towards ... Prasad, didn't you have patches to do exactly that? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 23, 2011 at 2:19 PM, Sasha Levin <levinsasha928@gmail.com> wrote: > Requirements - Kernel compiled with: > CONFIG_FB_BOOT_VESA_SUPPORT=y > CONFIG_FB_VESA=y > CONFIG_FRAMEBUFFER_CONSOLE=y Dunno if it's possible but it would be nice to have a more readable error message if you don't have that compiled in: penberg@tiger:~/linux/tools/kvm$ ./kvm run --vnc -d ~/images/debian_squeeze_amd64_standard.img # kvm run -k ../../arch/x86/boot/bzImage -m 320 -c 2 Warning: Config tap device error. Are you root? 23/05/2011 17:08:19 Listening for VNC connections on TCP port 5900 Undefined video mode number: 312 Press <ENTER> to see video modes available, <SPACE> to continue, or wait 30 sec Killed This obviously isn't an issue for merging this patch. Pekka -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/23/2011 01:38 PM, Ingo Molnar wrote: > Later on even this could be removed: using section > tricks we can put init functions into a section This is not kernel space, the C library provides a way to do that with __attribute__((constructor))... Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Paolo Bonzini <pbonzini@redhat.com> wrote: > On 05/23/2011 01:38 PM, Ingo Molnar wrote: > > Later on even this could be removed: using section tricks we can put init > > functions into a section > > This is not kernel space, the C library provides a way to do that > with __attribute__((constructor))... Yeah, that would certainly work for simple things but there's several reasons why explicit control over initcalls is preferred in a tool like tools/kvm/ over __attribute__((constructor)): - C constructors run before main() so any generic environment that tools/kvm/ might want to set up is not available yet - including but not limited to the command line arguments. - C constructors have random limitations like apparently not being executed if the constructor is linked into a .a static library. - It has advantages to have explicit control over initcalls - that way debugging and other instrumentation can be added freely. For example the kernel has a debug_initcall boot parameter to print the initcalls as they are executed. They can also be traced. - The kernel has several classes of initcalls with different call priority, i'm not aware of an equivalent __attribute__((constructor)) capability. We could also do more sophisticated layers of initcalls or an explicit initcall dependency tree, should the need arise. Using .section tricks directly isnt particularly complex and the result is rather flexible, well-defined and visible. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/24/2011 12:37 PM, Paolo Bonzini wrote: > On 05/23/2011 01:38 PM, Ingo Molnar wrote: >> Later on even this could be removed: using section >> tricks we can put init functions into a section > > This is not kernel space, the C library provides a way to do that with __attribute__((constructor))... > > Paolo Ingo meant to use the same trick as we do in kernel space for late initcalls.
On 05/24/2011 10:50 AM, Ingo Molnar wrote: > Yeah, that would certainly work for simple things but there's several reasons > why explicit control over initcalls is preferred in a tool like tools/kvm/ over > __attribute__((constructor)): Some advantages you mention are real indeed, but they are general; there's no reason why they apply only to tools/kvm. You can achieve the same by doing only minimal work (registering your subsystems/devices/whatever in a linked list) in the constructors. Then you iterate on the list and call function pointers. I know portability is not relevant to tools/kvm/, but using unportable tricks for the sake of using them is a direct way to NIH. But oh well all of tools/kvm/ is NIH after all. :) > - The kernel has several classes of initcalls with different call priority, > i'm not aware of an equivalent __attribute__((constructor)) capability. > We could also do more sophisticated layers of initcalls or an explicit > initcall dependency tree, should the need arise. Constructors and destructors can have a priority (low runs first for constructors, low runs last for destructors). Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/24/2011 10:50 AM, Ingo Molnar wrote: > - C constructors have random limitations like apparently not being executed if > the constructor is linked into a .a static library. Ah, forgot about this. Given _why_ this happens (for static libraries, the linker omits object modules that are not required to fulfill undefined references in previous objects), I'd be surprised if explicit section tricks do not have the same limitation. After all the constructor attribute is only syntactic sugar for section tricks. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 24, 2011 at 12:10 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > I know portability is not relevant to tools/kvm/, but using unportable > tricks for the sake of using them is a direct way to NIH. But oh well all > of tools/kvm/ is NIH after all. :) Hmm? The point is to follow Linux kernel conventions and idioms (and share code) as much as possible so it's familiar to devs who are already working on the kernel. That's why section tricks seem more appropriate than using constructor to me. Or is there some technical advantage to using constructors? Pekka -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/24/2011 12:55 PM, Pekka Enberg wrote: > On Tue, May 24, 2011 at 12:10 PM, Paolo Bonzini<pbonzini@redhat.com> wrote: > > I know portability is not relevant to tools/kvm/, but using unportable > > tricks for the sake of using them is a direct way to NIH. But oh well all > > of tools/kvm/ is NIH after all. :) > > Hmm? > > The point is to follow Linux kernel conventions and idioms (and share > code) as much as possible so it's familiar to devs who are already > working on the kernel. That's why section tricks seem more appropriate > than using constructor to me. Or is there some technical advantage to > using constructors? You get to reuse infrastructure that's already there. Things like using sections and s/uint64_t/u64/ look anti-reuse to me. Userspace isn't the kernel, for better or for worse.
Hi Avi, On Tue, May 24, 2011 at 2:22 PM, Avi Kivity <avi@redhat.com> wrote: >> The point is to follow Linux kernel conventions and idioms (and share >> code) as much as possible so it's familiar to devs who are already >> working on the kernel. That's why section tricks seem more appropriate >> than using constructor to me. Or is there some technical advantage to >> using constructors? > > You get to reuse infrastructure that's already there. > > Things like using sections and s/uint64_t/u64/ look anti-reuse to me. > Userspace isn't the kernel, for better or for worse. Not really. The type thing is pretty much required once you start using kernel code (as we learned the hard way). Btw, constructor attribute doesn't really seem like a good fit for "late_initcall" type of thing: The constructor attribute causes the function to be called automatically before execution enters main () What am I missing here? Pekka -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2011-05-24 at 14:26 +0300, Pekka Enberg wrote: > Hi Avi, > > On Tue, May 24, 2011 at 2:22 PM, Avi Kivity <avi@redhat.com> wrote: > >> The point is to follow Linux kernel conventions and idioms (and share > >> code) as much as possible so it's familiar to devs who are already > >> working on the kernel. That's why section tricks seem more appropriate > >> than using constructor to me. Or is there some technical advantage to > >> using constructors? > > > > You get to reuse infrastructure that's already there. > > > > Things like using sections and s/uint64_t/u64/ look anti-reuse to me. > > Userspace isn't the kernel, for better or for worse. > > Not really. The type thing is pretty much required once you start > using kernel code (as we learned the hard way). > > Btw, constructor attribute doesn't really seem like a good fit for > "late_initcall" type of thing: > > The constructor attribute causes the function to be called > automatically before execution enters main () You could add a small constructor function that'll add a pointer to the real initialization function to a list which will get called after we get everything initialized and ready.
On 05/24/2011 02:26 PM, Pekka Enberg wrote: > Hi Avi, > > On Tue, May 24, 2011 at 2:22 PM, Avi Kivity<avi@redhat.com> wrote: > >> The point is to follow Linux kernel conventions and idioms (and share > >> code) as much as possible so it's familiar to devs who are already > >> working on the kernel. That's why section tricks seem more appropriate > >> than using constructor to me. Or is there some technical advantage to > >> using constructors? > > > > You get to reuse infrastructure that's already there. > > > > Things like using sections and s/uint64_t/u64/ look anti-reuse to me. > > Userspace isn't the kernel, for better or for worse. > > Not really. The type thing is pretty much required once you start > using kernel code (as we learned the hard way). What happens when you start using userspace libraries? Eventually you'll have a lot more of that than kernel code. > Btw, constructor attribute doesn't really seem like a good fit for > "late_initcall" type of thing: > > The constructor attribute causes the function to be called > automatically before execution enters main () > > What am I missing here? Like Paolo said, you can have the constructor register a function or structure to be called any time you like.
Hi Avi, On Tue, May 24, 2011 at 2:30 PM, Avi Kivity <avi@redhat.com> wrote: > What happens when you start using userspace libraries? > > Eventually you'll have a lot more of that than kernel code. I don't quite understand what problems you think we might have. We're already using userspace libraries (libvnc) and most of us code is non-kernel code. We switched to u64 and friends for two reasons: (1) using uint*_t turned out to be painful when using kernel headers (e.g., mptables, e820, etc.) and (2) we want to be as close as possible to the coding style of tools/perf to be able to reuse their code in the future. On Tue, May 24, 2011 at 2:30 PM, Avi Kivity <avi@redhat.com> wrote: > Like Paolo said, you can have the constructor register a function or > structure to be called any time you like. Oh, okay. We should probably start out with that and switch to section tricks if we have to. Pekka -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/24/2011 02:38 PM, Pekka Enberg wrote: > Hi Avi, > > On Tue, May 24, 2011 at 2:30 PM, Avi Kivity<avi@redhat.com> wrote: > > What happens when you start using userspace libraries? > > > > Eventually you'll have a lot more of that than kernel code. > > I don't quite understand what problems you think we might have. We're > already using userspace libraries (libvnc) and most of us code is > non-kernel code. If uint64_t is defined differently than u64, you won't be able to pass it by reference if an external library expects it. > We switched to u64 and friends for two reasons: (1) using uint*_t > turned out to be painful when using kernel headers (e.g., mptables, > e820, etc.) and (2) we want to be as close as possible to the coding > style of tools/perf to be able to reuse their code in the future. > Regarding this reuse, I see it's done by copy/paste. Won't it be better to have tools/lib and have tools/perf and tools/kvm use that?
Hi Avi, On Tue, May 24, 2011 at 2:41 PM, Avi Kivity <avi@redhat.com> wrote: >> I don't quite understand what problems you think we might have. We're >> already using userspace libraries (libvnc) and most of us code is >> non-kernel code. > > If uint64_t is defined differently than u64, you won't be able to pass it by > reference if an external library expects it. Oh, the sizes must be the same for obvious reasons so that shouldn't be a problem and it's better to do the casting in the external API calls than in core code. On Tue, May 24, 2011 at 2:41 PM, Avi Kivity <avi@redhat.com> wrote: > Regarding this reuse, I see it's done by copy/paste. Won't it be better to > have tools/lib and have tools/perf and tools/kvm use that? Yes, absolutely. We're keeping things well-contained under tools/kvm with copy-paste until we hit Linus' tree in the future, though, for merge reasons. Pekka -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/24/2011 01:56 PM, Pekka Enberg wrote: >> > If uint64_t is defined differently than u64, you won't be able to pass it by >> > reference if an external library expects it. > > Oh, the sizes must be the same for obvious reasons so that shouldn't > be a problem and it's better to do the casting in the external API > calls than in core code. Be careful about unsigned long vs. unsigned long long. If you want to use the u64 spelling that's fine, but I would use a custom header that defines it from uint64_t. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/24/2011 02:41 PM, Avi Kivity wrote: > >> We switched to u64 and friends for two reasons: (1) using uint*_t >> turned out to be painful when using kernel headers (e.g., mptables, >> e820, etc.) and (2) we want to be as close as possible to the coding >> style of tools/perf to be able to reuse their code in the future. >> > > Regarding this reuse, I see it's done by copy/paste. Won't it be > better to have tools/lib and have tools/perf and tools/kvm use that? > Another thing, I believe reuse of actual kernel code has limited utility. The kernel has vast infrastructure and porting all of it to userspace would be a huge undertaking. It's pretty common for code to use printk(), kmalloc(), slab caches, rcu, various locks, per-cpu variables. Are you going to port all of it? and maintain it when it changes? It's a lot easier to use the native userspace stacks.
On 05/24/2011 03:27 PM, Paolo Bonzini wrote: > On 05/24/2011 01:56 PM, Pekka Enberg wrote: >>> > If uint64_t is defined differently than u64, you won't be able to >>> pass it by >>> > reference if an external library expects it. >> >> Oh, the sizes must be the same for obvious reasons so that shouldn't >> be a problem and it's better to do the casting in the external API >> calls than in core code. > > Be careful about unsigned long vs. unsigned long long. Yes, that's the issue I had in mind. > If you want to use the u64 spelling that's fine, but I would use a > custom header that defines it from uint64_t. And now you'll have printf/printk format mismatches if they happen to be different.
Hi Avi, On Tue, May 24, 2011 at 5:37 PM, Avi Kivity <avi@redhat.com> wrote: > Another thing, I believe reuse of actual kernel code has limited utility. OK, I don't agree with that. On Tue, May 24, 2011 at 5:37 PM, Avi Kivity <avi@redhat.com> wrote: > The kernel has vast infrastructure and porting all of it to userspace would > be a huge undertaking. It's pretty common for code to use printk(), > kmalloc(), slab caches, rcu, various locks, per-cpu variables. Are you > going to port all of it? and maintain it when it changes? We will port whatever is useful and makes sense. Nothing new here - we're following perf's lead, that's all. As for the specific issues you mention, only RCU and locking mechanisms seem like something that are not trivial to port (although userspace RCU already exists). [ Porting lockdep to userspace would be one cool feature, though. ] On Tue, May 24, 2011 at 5:37 PM, Avi Kivity <avi@redhat.com> wrote: > It's a lot easier to use the native userspace stacks. We will be using userspace libs where appropriate (like with libvnc). Pekka -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Pekka Enberg <penberg@kernel.org> wrote: > We switched to u64 and friends for two reasons: (1) using uint*_t turned out > to be painful when using kernel headers (e.g., mptables, e820, etc.) and (2) > we want to be as close as possible to the coding style of tools/perf to be > able to reuse their code in the future. 3) uint*_t caused frequent type mismatches and breakages with format strings if accidentally an uint*_t was stuck into a regular printf format - it would trigger a warning only on 64-bit or only on 32-bit systems, causing frequent flux. This is not a very good data type model. 4) uint*_t also causes absolute brain-damaged printf format hacks like: fprintf(stderr, "%s: %" PRIu64 " %" PRIu64 " %" PRIu64 "\n", event_name(counter), count[0], count[1], count[2]); instead of the much cleaner: fprintf(stderr, "%s: %Lu %Lu %Lu\n", event_name(counter), count[0], count[1], count[2]); So we can use uint*_t where absolutely necessary due to external ABI constraints, but otherwise it's discouraged for tools/kvm/ internal code. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Pekka Enberg <penberg@kernel.org> wrote: > As for the specific issues you mention, only RCU and locking mechanisms seem > like something that are not trivial to port (although userspace RCU already > exists). Indeed. > [ Porting lockdep to userspace would be one cool feature, though. ] Agreed - we already ran into a few locking bugs that were IMO needlessly difficult to debug - our expectations got spoiled by lockdep i guess! :-) Porting RCU would also have its distinct advantages in terms of scalability. While tools/perf/ probably wont need it (all its scalability-relevant code runs in the kernel), tools/kvm/ definitely looks like a candidate for good user-space RCU primitives. > On Tue, May 24, 2011 at 5:37 PM, Avi Kivity <avi@redhat.com> wrote: > > It's a lot easier to use the native userspace stacks. > > We will be using userspace libs where appropriate (like with libvnc). We are using a fair amount of userspace libraries in perf too: -ldw, -lelf, -lnewt, -lslang, we used -laudit too for some time. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Paolo Bonzini <pbonzini@redhat.com> wrote: > On 05/24/2011 10:50 AM, Ingo Molnar wrote: > >Yeah, that would certainly work for simple things but there's several reasons > >why explicit control over initcalls is preferred in a tool like tools/kvm/ over > >__attribute__((constructor)): > > Some advantages you mention are real indeed, but they are general; > there's no reason why they apply only to tools/kvm. You can achieve > the same by doing only minimal work (registering your > subsystems/devices/whatever in a linked list) in the constructors. > Then you iterate on the list and call function pointers. Well, the plain fact that __attribute__((constructor)) gets called on binary and shared library startup before main() is called is a show-stopper for us as i described it in my previous mail, so why are we even arguing about it? ((constructor)) has showstopper properties: - We don't have access to the program arguments - stdio is probably not set up yet (this is undefined AFAICS) - Also, over the years i have grown to be suspicious of GCC defined extensions. More often than not the GCC project is fixing regressions not by fixing the compiler but by changing the documentation ;-) We got bitten by regressions in asm() behavior in the kernel rather often. In that sense ((section)) is way more robust: there's not really that many ways to screw that up. Fiddling with the ((constructor)) environment on the other hand ... Note that in terms of explicit iterations we do that with __attribute__((section)) as well: except that *we* define where and how the functions get called. > I know portability is not relevant to tools/kvm/, but using unportable tricks > for the sake of using them is a direct way to NIH. But oh well all of > tools/kvm/ is NIH after all. :) __attribute__((constructor)) is not particularly portable to begin with: does the MSVC compiler support it for example? So __attribute__ ((section)), which is used by the initcall() machinery is similarly portable: GCC and LLVM/Clang support it. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Paolo Bonzini <pbonzini@redhat.com> wrote: > On 05/24/2011 10:50 AM, Ingo Molnar wrote: > > - C constructors have random limitations like apparently not being executed if > > the constructor is linked into a .a static library. > > Ah, forgot about this. Given _why_ this happens (for static libraries, the > linker omits object modules that are not required to fulfill undefined > references in previous objects), I'd be surprised if explicit section tricks > do not have the same limitation. [...] Since we create an actual array (data) and iterate it the worst i can imagine is the linker dropping those sections - in which case the build will fail and we are alerted to the problem. With ((constructor)) the program will misbehave silently - not good. > [...] After all the constructor attribute is only syntactic sugar for section > tricks. Yeah but not just syntactic sugar but also code built into glibc to execute them at some point before main(). And this last bit really matters as well. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/24/2011 09:40 PM, Ingo Molnar wrote: > > Ah, forgot about this. Given_why_ this happens (for static libraries, the > > linker omits object modules that are not required to fulfill undefined > > references in previous objects), I'd be surprised if explicit section tricks > > do not have the same limitation. [...] > > Since we create an actual array (data) Are you? I figured it was like this example from Linux: .x86_cpu_dev.init : AT(ADDR(.x86_cpu_dev.init) - LOAD_OFFSET) { __x86_cpu_dev_start = .; *(.x86_cpu_dev.init) __x86_cpu_dev_end = .; } extern const struct cpu_dev *const __x86_cpu_dev_start[], *const __x86_cpu_dev_end[]; for (cdev = __x86_cpu_dev_start; cdev < __x86_cpu_dev_end; cdev++) { This is pretty much the same as the linker script for ELF: .init_array : { __init_array_start = .; *(.init_array) *(SORT(.init_array$*)) __init_array_end = .; } extern void (*__init_array_start []) (int, char **, char **) attribute_hidden; extern void (*__init_array_end []) (int, char **, char **) attribute_hidden; const size_t size = __init_array_end - __init_array_start; for (size_t i = 0; i < size; i++) (*__init_array_start [i]) (argc, argv, envp); > ((constructor)) has showstopper properties: > > - We don't have access to the program arguments > > - stdio is probably not set up yet (this is undefined AFAICS) As I said, you can do this even better by doing only minimal work in the constructor. Create a struct with several callbacks (pre_init, late_init, destroy, reset, whatever) and possibly other information (a human-readable device name and command-line argument to access it, for example). In the constructor you just build a linked list of said structs, and then you can walk it whenever you see fit: do comparisons, call a function, whatever. This is similar to the cpudev example from the kernel above. A simple example from QEMU: static void virtio_pci_register_devices(void) { pci_qdev_register_many(virtio_info); } /* This macro wraps ((constructor)). */ device_init(virtio_pci_register_devices) > In that sense ((section)) is way more robust: there's not really that many > ways to screw that up. Fiddling with the ((constructor)) environment on the > other hand ... Sorry, this is textbook FUD. > __attribute__((constructor)) is not particularly portable to begin with: does > the MSVC compiler support it for example? No, but GCC supports it on non-ELF platforms, where you would need a similar but different linker script. (Also, the differences between MSVC and GCC can easily be abstracted with a macro). More practically, is your linker script supported by gold? Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Paolo Bonzini <pbonzini@redhat.com> wrote: > > In that sense ((section)) is way more robust: there's not really that many > > ways to screw that up. Fiddling with the ((constructor)) environment on the > > other hand ... > > Sorry, this is textbook FUD. I specifically cited the problem of static libraries. They *do not work* with ((constructor)). That's not FUD but justified scepticism backed by hard evidence. > > __attribute__((constructor)) is not particularly portable to begin with: > > does the MSVC compiler support it for example? > > No, but GCC supports it on non-ELF platforms, [...] So you claim that ((section)) is 'not portable' but concede that it does in fact not port to one of the most widely used compilers. Instead you clarify that under 'not portable' you really meant to say that ((section)) does not work on non-ELF binary platforms. That is a pretty inconsistent position and has nothing to do with 'portability' in itself but with being portable to what *you* consider important. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Paolo Bonzini <pbonzini@redhat.com> wrote: > > ((constructor)) has showstopper properties: > > > > - We don't have access to the program arguments > > > > - stdio is probably not set up yet (this is undefined AFAICS) > > As I said, you can do this even better by doing only minimal work in > the constructor. [...] The costs of doing that: - extra per init entry data structures - extra code being run during the constructor phase - the risk of not getting the ((constructor)) init run at all, such as on static libraries The only benefit is: - ((constructor)) is a non-portable GCC extension so the only portability win is to possibly support non-ELF targets. Which ones do we care about? At this point it is a rather legitimate technical decision for us to say that we do not want to bear those cost for that limited benefit, wouldn't you agree? If someone adds non-ELF support then it can all be switched over to the bit more bloated ((constructor)) approach rather easily - so this all is a non-issue really. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/25/2011 10:32 AM, Ingo Molnar wrote: > > * Paolo Bonzini<pbonzini@redhat.com> wrote: > >>> In that sense ((section)) is way more robust: there's not really that many >>> ways to screw that up. Fiddling with the ((constructor)) environment on the >>> other hand ... >> >> Sorry, this is textbook FUD. > > I specifically cited the problem of static libraries. They *do not work* with > ((constructor)). ((constructor)) has easily explained semantics: it's the same semantics as C++ global constructors. If you don't like those, that's another story. But anyway, I specifically cited the reason why ((section)) is not any better, AFAIU how you would use it. Now I didn't trust myself and tried it. f.ld: SECTIONS { .paolo : { PROVIDE(__paolo_start = .); *(.paolo) PROVIDE(__paolo_end = .); } } f.c: int x __attribute__((section(".paolo"))) = 0x12345678; int main() { extern int __paolo_start[]; extern int __paolo_end[]; int *p; for (p = __paolo_start; p < __paolo_end; p++) { printf ("%x\n", *p); } } f1.c: int y __attribute__((section(".paolo"))) = 0x76543210; compilation with objects: $ gcc f.c -o f.o -c $ gcc f1.c -o f1.o -c $ ld -r f.ld f.o f1.o -o f2.o $ gcc f2.o -o a.out $ ./a.out 12345678 76543210 compilation with static libraries: $ gcc f.c -o f.o -c $ gcc f1.c -o f1.o -c $ ar cr f1.a f1.o $ ld -r f.ld f.o f1.a -o f2.o $ gcc f2.o -o a.out $ ./a.out 12345678 Since this is userspace, you do not have complete control on the build and you still need to use gcc to drive the final link. So you get exactly the same semantics as ((constructor)), plus one extra build step that spits out a warning ("ld: warning: f.ld contains output sections; did you forget -T?"). >>> __attribute__((constructor)) is not particularly portable to begin with: >>> does the MSVC compiler support it for example? >> >> No, but GCC supports it on non-ELF platforms, [...] > > So you claim that ((section)) is 'not portable' but concede that it does in > fact not port to one of the most widely used compilers. Can you please avoid trimming messages? I said "the differences between MSVC and GCC can easily be abstracted with a macro". This is not unlike other differences between the two compilers (e.g. __forceinline vs. the ((always_inline)) attribute), and it is not true of linker scripts. > Instead you clarify that under 'not portable' you really meant to say that > ((section)) does not work on non-ELF binary platforms. > > That is a pretty inconsistent position and has nothing to do with 'portability' > in itself but with being portable to what *you* consider important. My definition of "(reasonably) portable" includes having to write a 5-line macro that works across _all_ GCC targets and _all_ MSVC targets. Having to change the build process for every compiler target (see the above "ld -r" step) is a bit more unportable, though not much. Potentially having to modify the custom linker script for every targeted architecture (the above doesn't work for mingw32, it needs an extra _ in front of the start/end symbols) is pretty much my definition of "unportable". Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Paolo Bonzini <pbonzini@redhat.com> wrote: > > So you claim that ((section)) is 'not portable' but concede that > > it does in fact not port to one of the most widely used > > compilers. > > Can you please avoid trimming messages? I said "the differences > between MSVC and GCC can easily be abstracted with a macro". This > is not unlike other differences between the two compilers (e.g. > __forceinline vs. the ((always_inline)) attribute), and it is not > true of linker scripts. So ((constructor)) is unportable but it can be wrapped. So can other portability problems be solved, such as linker scripts can be written for non-ELF targets (should that ever become necessary), so what's your point? We want to use ELF sections in the future *anyway* to make tools/kvm/ scale even better, for example to use alternative instruction fixups (for which no GCC extension exists) so a linker script will be there anyway. ( We might also want to use sections to speed up exception handling in tools/kvm/. ) Fact is that neither ((section)) *NOR* ((constructor)) is portable: *both* are GCC extensions - while you falsely claimed that ((constructor)) was somehow portable and that ((section)) was an 'unportable trick'. Let me quote your initial claim: >>>>> I know portability is not relevant to tools/kvm/, but using >>>>> unportable tricks for the sake of using them is a direct way to >>>>> NIH. But oh well all of tools/kvm/ is NIH after all. :) Btw., that NIH claim was rather unfair and uncalled for as well. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Paolo Bonzini <pbonzini@redhat.com> wrote: > On 05/25/2011 10:32 AM, Ingo Molnar wrote: > > > >* Paolo Bonzini<pbonzini@redhat.com> wrote: > > > >>> In that sense ((section)) is way more robust: there's not really that many > >>> ways to screw that up. Fiddling with the ((constructor)) environment on the > >>> other hand ... > >> > >> Sorry, this is textbook FUD. > > > > I specifically cited the problem of static libraries. They *do > > not work* with ((constructor)). > > ((constructor)) has easily explained semantics: it's the same > semantics as C++ global constructors. If you don't like those, > that's another story. It is a plain *bug* if device initialization is not being executed if kvm is linked into a static library using the regular way of how libraries are created ... No amount of arguing about 'semantics' will change that simple fact: it breaks code so it's a bug. We don't want to rely on a facility that handles boundary conditions in such a poor way. See my prior point: - Also, over the years i have grown to be suspicious of GCC defined extensions. More often than not the GCC project is fixing regressions not by fixing the compiler but by changing the documentation ;-) We got bitten by regressions in asm() behavior in the kernel rather often. In that sense ((section)) is way more robust: there's not really that many ways to screw that up. Fiddling with the ((constructor)) environment on the other hand ... You are demonstrating this phenomenon rather well. You argue against plain old bugs with 'but these are well-defined semantics'. That's not how we deal with bugs in tools/kvm/ really. And then you argue that the bug can be worked around by writing a linker script: > compilation with static libraries: > $ gcc f.c -o f.o -c > $ gcc f1.c -o f1.o -c > $ ar cr f1.a f1.o > $ ld -r f.ld f.o f1.a -o f2.o > $ gcc f2.o -o a.out > $ ./a.out > 12345678 I will rather use linker scripts and less erratic facilities straight away. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/25/2011 11:36 AM, Ingo Molnar wrote: > So can other portability problems be solved, such as linker scripts > can be written for non-ELF targets (should that ever become > necessary) I know that's not going to be the case, and I mentioned that in my first message---portability is not relevant to tools/kvm/ just like it is not relevant say to systemd. > so what's your point? Using this kind of trick makes it harder to share code with other libraries that may require a higher standard of portability (not "better" or "worse", just "higher"). QEMU _does_ have this problem BTW, we have our own event loop and AIO, our own JSON parser, our own thread abstractions, and so on... > We want to use ELF sections in the future *anyway* to make > tools/kvm/ scale even better, for example to use alternative > instruction fixups (for which no GCC extension exists) so a linker > script will be there anyway. Since you're not writing the hypervisor, but only the device model, I doubt that this is needed (and you'd likely run into troubles with SELinux). QEMU scales to dozens of VCPUs with a big lock around all userspace device model code. But it's premature to say that, and I'd be happy if I were proven wrong. > Fact is that neither ((section)) *NOR* ((constructor)) is portable: > *both* are GCC extensions - while you falsely claimed that > ((constructor)) was somehow portable and that ((section)) was an > 'unportable trick'. Fair enough. I agree that ((constructor)) has limited scope and is compiler-unportable. ((section)) is more powerful and has the cost of using also target-unportable linker scripts, and requiring changes to the build system. Both have the same problem with static libraries (please reread my message). >>>>>> I know portability is not relevant to tools/kvm/, but >>>>>> using unportable tricks for the sake of using them is a >>>>>> direct way to NIH. But oh well all of tools/kvm/ is NIH >>>>>> after all. :) > > Btw., that NIH claim was rather unfair and uncalled for as well. Hey hey I put a smiley for a reason! Anyway I think we both agree that this debate is pointless. I learnt something (I wasn't aware of interaction between ((constructor)) and static libraries), you learnt something (it's the same with ((section)), and it's intrinsic in how static libraries work). Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Paolo Bonzini <pbonzini@redhat.com> wrote: > > so what's your point? > > Using this kind of trick makes it harder to share code with other > libraries that may require a higher standard of portability (not > "better" or "worse", just "higher"). [...] That's an complication but should be fixable, should it ever happen. As things stand today we: - Are *already* using an ELF linker script, see tools/kvm/bios/rom.ld.S - Have multiple valid reasons not to use ((constructor)) - Want to use sections to implement other useful features as well If the *only* linker script use would be the init facility then you'd probably have a valid point - although the possible code flow fragility with ((constructor)) is still a problem: we still would want to know when no constructors were executed. Also it's not clear why ((constructor)) was written in the way it was: why apparently no access is given to the array of init functions and why it's not possible to turn the auto-execution off but still have the array generated, for legitimate cases that want to use data driven constructor execution. > >>>>>> I know portability is not relevant to tools/kvm/, but using > >>>>>> unportable tricks for the sake of using them is a direct way > >>>>>> to NIH. But oh well all of tools/kvm/ is NIH after all. :) > > > > Btw., that NIH claim was rather unfair and uncalled for as well. > > Hey hey I put a smiley for a reason! Well after two insults in a single paragraph you need to put in at least two smileys! Or not write the insults in a technical discssion to begin with, especially if you are criticising a patch rather forcefully. It will be easily misunderstood as a real insult, despite the smiley ;-) > Anyway I think we both agree that this debate is pointless. I > learnt something (I wasn't aware of interaction between > ((constructor)) and static libraries), you learnt something (it's > the same with ((section)), and it's intrinsic in how static > libraries work). While i did not know whether static libraries would work with a linker script (never tried it - and your experiment suggests that they wont), the ((section)) approach we could create a clear runtime BUG_ON() assert for a zero-sized array of init function pointers, while ((constructor)) will silently not execute initialization functions. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/25/2011 12:17 PM, Ingo Molnar wrote: > Also it's not clear why ((constructor)) was written in the way it > was: why apparently no access is given to the array of init functions It is accessible---glibc uses it. > and why it's not possible to turn the auto-execution off but still > have the array generated, for legitimate cases that want to use data > driven constructor execution. The compiler doesn't care about what is done with the data. It simply provides the table for the runtime library to use it. ((constructor)) is a veneer over the same infrastructure used for C++ global constructors; that explains its design pretty well. > the ((section)) approach we could create a clear runtime > BUG_ON() assert for a zero-sized array of init function pointers, Not really. The problem is that f1.o is not pulled from the static library _because the linker thinks it is not necessary_. If f1.o defines another symbol, and f.o uses it, then the constructor is pulled in together with the rest of f.o. As soon as _one_ constructor is pulled in (even from outside the static library), your BUG() would not detect silently missing imports anymore. It turns out a way to get the semantics you want for ((section)) is to use --whole-archive (and possibly --no-whole-archive) on the "ld -r" command line. You can do the same on the link line for ((constructor)) or C++ constructors too. However, since you're effectively using a C++ feature, static libraries are probably a bad idea just like C++ static libraries are. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Paolo Bonzini <pbonzini@redhat.com> wrote: > On 05/25/2011 12:17 PM, Ingo Molnar wrote: > > > Also it's not clear why ((constructor)) was written in the way it > > was: why apparently no access is given to the array of init > > functions > > It is accessible---glibc uses it. > > > and why it's not possible to turn the auto-execution off but > > still have the array generated, for legitimate cases that want to > > use data driven constructor execution. > > The compiler doesn't care about what is done with the data. It > simply provides the table for the runtime library to use it. > ((constructor)) is a veneer over the same infrastructure used for > C++ global constructors; that explains its design pretty well. Obviously the compiler did not provide this feature into a vacuum, it expected the C library to execute constructors, right? So the above dodges my question of why there is no method (in glibc) to turn auto-execution off and let the app do it. If all that was possible then ((constructor)) could certainly be used as a generalized shortcut for certain ((section)) uses. > > the ((section)) approach we could create a clear runtime BUG_ON() > > assert for a zero-sized array of init function pointers, > > Not really. The problem is that f1.o is not pulled from the static > library _because the linker thinks it is not necessary_. If f1.o > defines another symbol, and f.o uses it, then the constructor is > pulled in together with the rest of f.o. As soon as _one_ > constructor is pulled in (even from outside the static library), > your BUG() would not detect silently missing imports anymore. Yeah, good point. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/25/2011 02:53 PM, Ingo Molnar wrote: > Obviously the compiler did not provide this feature into a vacuum, it > expected the C library to execute constructors, right? > > So the above dodges my question of why there is no method (in glibc) > to turn auto-execution off and let the app do it. Well, I wasn't there. :) I suppose they simply chose those semantics for C++ constructors which were easiest to implement (i.e. execute before main). ((constructor)) then followed suit. Any further extension in the direction you suggest would have to be designed very carefully in order to coexist with C++ constructors, or it would have to be something completely separate... in the latter case, ((constructor)) already provides the ingredients to do this, even if at a small cost in efficiency. So there is no big incentive to add this further extension to the compiler and/or the C library. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/tools/kvm/kvm-run.c b/tools/kvm/kvm-run.c index 288e1fb..adbb25b 100644 --- a/tools/kvm/kvm-run.c +++ b/tools/kvm/kvm-run.c @@ -28,6 +28,7 @@ #include <kvm/barrier.h> #include <kvm/symbol.h> #include <kvm/virtio-9p.h> +#include <kvm/vesa.h> /* header files for gitish interface */ #include <kvm/kvm-run.h> @@ -66,6 +67,7 @@ static const char *virtio_9p_dir; static bool single_step; static bool readonly_image[MAX_DISK_IMAGES]; static bool virtio_rng; +static bool vnc; extern bool ioport_debug; extern int active_console; @@ -110,6 +112,7 @@ static const struct option options[] = { OPT_STRING('\0', "kvm-dev", &kvm_dev, "kvm-dev", "KVM device file"), OPT_STRING('\0', "virtio-9p", &virtio_9p_dir, "root dir", "Enable 9p over virtio"), + OPT_BOOLEAN('\0', "vnc", &vnc, "Enable VNC framebuffer"), OPT_GROUP("Kernel options:"), OPT_STRING('k', "kernel", &kernel_filename, "kernel", @@ -413,6 +416,7 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix) char *hi; int i; void *ret; + u16 vidmode = 0; signal(SIGALRM, handle_sigalrm); signal(SIGQUIT, handle_sigquit); @@ -511,7 +515,13 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix) kvm->nrcpus = nrcpus; memset(real_cmdline, 0, sizeof(real_cmdline)); - strcpy(real_cmdline, "notsc noapic noacpi pci=conf1 console=ttyS0 earlyprintk=serial"); + strcpy(real_cmdline, "notsc noapic noacpi pci=conf1"); + if (vnc) { + strcat(real_cmdline, " video=vesafb console=tty0"); + vidmode = 0x312; + } else { + strcat(real_cmdline, " console=ttyS0 earlyprintk=serial"); + } strcat(real_cmdline, " "); if (kernel_cmdline) strlcat(real_cmdline, kernel_cmdline, sizeof(real_cmdline)); @@ -543,7 +553,7 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix) printf(" # kvm run -k %s -m %Lu -c %d\n", kernel_filename, ram_size / 1024 / 1024, nrcpus); if (!kvm__load_kernel(kvm, kernel_filename, initrd_filename, - real_cmdline)) + real_cmdline, vidmode)) die("unable to load kernel %s", kernel_filename); kvm->vmlinux = vmlinux_filename; @@ -597,6 +607,9 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix) kvm__init_ram(kvm); + if (vnc) + vesa__init(kvm); + thread_pool__init(nr_online_cpus); for (i = 0; i < nrcpus; i++) {