On Thu, Oct 31, 2013 at 09:04:42PM +0100, Alexander van Heukelum wrote:
> On Tue, Oct 22, 2013, at 3:35, Josh Triplett wrote:
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -976,6 +976,16 @@ config VM86
> > XFree86 to initialize some video cards via BIOS. Disabling this
> > option saves about 6k.
> >
> > +config X86_IOPORT
> > + bool "iopl and ioperm system calls"
> > + default y
> > + ---help---
> > + This option enables the iopl and ioperm system calls, which allow
> > + privileged userspace processes to directly access I/O ports. This
> > + is used by some legacy software to drive hardware directly from
> > + userspace rather than via a proper kernel driver. Unless you intend
> > + to run such software, you can safely say N here.
> > +
>
> I think this entry should be under General setup / Configure standard kernel
> features (expert users).
It's entirely x86-specific, and it's similar to VM86 just above it; it
belongs on the x86-specific menu.
> Remove references to "legacy" and "proper driver".
Hardware drivers belong in the kernel, and anything using iopl or ioperm
won't run on x86-64, which argues rather strongly for its obsolescence.
There's also /dev/port for cleaner access to ports from userspace.
However, I've rephrased this for v2.
> > --- a/arch/x86/kernel/cpu/common.c
> > +++ b/arch/x86/kernel/cpu/common.c
> > @@ -1223,7 +1223,6 @@ void cpu_init(void)
> > struct tss_struct *t;
> > unsigned long v;
> > int cpu;
> > - int i;
> >
> > /*
> > * Load microcode on this cpu if a valid microcode is available.
> > @@ -1285,14 +1284,7 @@ void cpu_init(void)
> > }
> > }
> >
> > - t->x86_tss.io_bitmap_base = offsetof(struct tss_struct, io_bitmap);
> > -
> > - /*
> > - * <= is required because the CPU will access up to
> > - * 8 bits beyond the end of the IO permission bitmap.
> > - */
> > - for (i = 0; i <= IO_BITMAP_LONGS; i++)
> > - t->io_bitmap[i] = ~0UL;
> > + init_tss_io(t);
> >
> > atomic_inc(&init_mm.mm_count);
> > me->active_mm = &init_mm;
> > @@ -1351,7 +1343,7 @@ void cpu_init(void)
> > load_TR_desc();
> > load_LDT(&init_mm.context);
> >
> > - t->x86_tss.io_bitmap_base = offsetof(struct tss_struct, io_bitmap);
> > + init_tss_io(t);
>
> This patch is too big. I think it would all look nicer if you added ioport.c in
> one patch, and then convert the users in a separate patch?
I'm not sure what you mean by "added ioport.c"; ioport.c already exists,
and this patch just makes it optional.
> > --- a/arch/x86/kernel/process-io.h
> > +++ b/arch/x86/kernel/process-io.h
> > @@ -1,9 +1,17 @@
> > #ifndef _X86_KERNEL_PROCESS_IO_H
> > #define _X86_KERNEL_PROCESS_IO_H
> >
> > +static inline void clear_thread_io_bitmap(struct task_struct *p)
> > +{
> > +#ifdef CONFIG_X86_IOPORT
> > + p->thread.io_bitmap_ptr = NULL;
> > +#endif /* CONFIG_X86_IOPORT */
> > +}
> > +
>
> This is thought of as ugly... Instead, do something like
>
> #ifndef CONFIG_X86_IOPORT
>
> static inline void clear_thread_io_bitmap(struct task_struct *p) {}
> static inline int copy_io_bitmap(struct task_struct *me, struct task_struct *p) {return 0}
> ... etc...
>
> #else
>
> static inline void clear_thread_io_bitmap(struct task_struct *p)
> {
> p->thread.io_bitmap_ptr = NULL;
> }
> ... etc...
>
> #endif
In .c files, any ifdefs at all are ugly; in .h files, both styles are
quite common, and in particular the style I used is common when omitting
the entire body of the function. It has the advantage of keeping a
common function signature rather than duplicating it.
> > --- a/arch/x86/kernel/process_32.c
> > +++ b/arch/x86/kernel/process_32.c
> > @@ -155,7 +155,7 @@ int copy_thread(unsigned long clone_flags, unsigned long sp,
> > childregs->cs = __KERNEL_CS | get_kernel_rpl();
> > childregs->flags = X86_EFLAGS_IF | X86_EFLAGS_FIXED;
> > p->fpu_counter = 0;
> > - p->thread.io_bitmap_ptr = NULL;
> > + clear_thread_io_bitmap(p);
> > memset(p->thread.ptrace_bps, 0, sizeof(p->thread.ptrace_bps));
> > return 0;
> > }
> > @@ -269,14 +269,7 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
> > */
> > load_TLS(next, cpu);
> >
> > - /*
> > - * Restore IOPL if needed. In normal use, the flags restore
> > - * in the switch assembly will handle this. But if the kernel
> > - * is running virtualized at a non-zero CPL, the popf will
> > - * not restore flags, so it must be done in a separate step.
> > - */
> > - if (get_kernel_rpl() && unlikely(prev->iopl != next->iopl))
> > - set_iopl_mask(next->iopl);
> > + switch_iopl_mask(prev, next);
> >
> > /*
> > * Now maybe handle debug registers and/or IO bitmaps
>
> If copy_thread would be in process.c instead, the .h-file would be unnecessary,
> right?
No, there are calls to the new functions in the .h file in several other
places.
- Josh Triplett
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
