On Friday 09 February 2007 10:20, Rusty Russell wrote:
> This is the core of lguest: both the guest code (always compiled in to
> the image so it can boot under lguest), and the host code (lg.ko).
>
> There is only one config prompt at the moment: lguest is currently
> designed to run exactly the same guest and host kernels so we can
> frob the ABI freely.
>
> Unfortunately, we don't have the build infrastructure for "private"
> asm-offsets.h files, so there's a not-so-neat include in
> arch/i386/kernel/asm-offsets.c.
Ask the kbuild people to fix that?
It indeed looks ugly.
I bet Xen et.al. could make good use of that too.
> +# This links the hypervisor in the right place and turns it into a C array.
> +$(obj)/hypervisor-raw: $(obj)/hypervisor.o
> + @$(LD) -static -Tdata=`printf %#x $$(($(HYPE_ADDR)))` -Ttext=`printf %#x $$(($(HYPE_ADDR)+$(HYPE_DATA_SIZE)))` -o $@ $< && $(OBJCOPY) -O binary $@
> +$(obj)/hypervisor-blob.c: $(obj)/hypervisor-raw
> + @od -tx1 -An -v $< | sed -e 's/^ /0x/' -e 's/$$/,/' -e 's/ /,0x/g' > $@
an .S file with .incbin is more efficient and simpler
(note it has to be an separate .S file, otherwise icecream/distcc break)
It won't allow to show off any sed skills, but I guess we can live with that ;-)
> +static struct vm_struct *hypervisor_vma;
> +static int cpu_had_pge;
> +static struct {
> + unsigned long offset;
> + unsigned short segment;
> +} lguest_entry;
> +struct page *hype_pages; /* Contiguous pages. */
Statics? looks funky. Why only a single hypervisor_vma?
> +struct lguest lguests[MAX_LGUEST_GUESTS];
> +DECLARE_MUTEX(lguest_lock);
> +
> +/* IDT entries are at start of hypervisor. */
> +const unsigned long *__lguest_default_idt_entries(void)
> +{
> + return (void *)HYPE_ADDR;
> +}
> +
> +/* Next is switch_to_guest */
> +static void *__lguest_switch_to_guest(void)
> +{
> + return (void *)HYPE_ADDR + HYPE_DATA_SIZE;
> +}
> +
> +/* Then we use everything else to hold guest state. */
> +struct lguest_state *__lguest_states(void)
> +{
> + return (void *)HYPE_ADDR + sizeof(hypervisor_blob);
This cries for asm_offsets.h too, doesn't it?
> +}
> +
> +static __init int map_hypervisor(void)
> +{
> + unsigned int i;
> + int err;
> + struct page *pages[HYPERVISOR_PAGES], **pagep = pages;
> +
> + hype_pages = alloc_pages(GFP_KERNEL|__GFP_ZERO,
> + get_order(HYPERVISOR_SIZE));
Wasteful because of the rounding. Probably wants reintroduction
of alloc_pages_exact()
> +
> +static __exit void unmap_hypervisor(void)
> +{
> + vunmap(hypervisor_vma->addr);
> + __free_pages(hype_pages, get_order(HYPERVISOR_SIZE));
Shouldn't you clean up the GDTs too?
> +}
> +
> +/* IN/OUT insns: enough to get us past boot-time probing. */
> +static int emulate_insn(struct lguest *lg)
> +{
> + u8 insn;
> + unsigned int insnlen = 0, in = 0, shift = 0;
> + unsigned long physaddr = guest_pa(lg, lg->state->regs.eip);
> +
> + /* This only works for addresses in linear mapping... */
> + if (lg->state->regs.eip < lg->page_offset)
> + return 0;
Shouldn't there be a printk here?
> +/* Saves exporting idt_table from kernel */
> +static struct desc_struct *get_idt_table(void)
> +{
> + struct Xgt_desc_struct idt;
> +
> + asm("sidt %0":"=m" (idt));
Nasty, but ok.
> + return (void *)idt.address;
> +}
> +
> +extern asmlinkage void math_state_restore(void);
No externs in .c files
> +
> +/* Trap page resets this when it reloads gs. */
> +static int new_gfp_eip(struct lguest *lg, struct lguest_regs *regs)
> +{
> + u32 eip;
> + get_user(eip, &lg->lguest_data->gs_gpf_eip);
> + if (eip == regs->eip)
> + return 0;
> + put_user(regs->eip, &lg->lguest_data->gs_gpf_eip);
No fault checking?
lhread/write use probably also needs to be double checked that a malicious
guest can't put the kernel into a loop.
> + return 1;
> +}
> +
> +static void set_ts(unsigned int guest_ts)
> +{
> + u32 cr0;
> + if (guest_ts) {
> + asm("movl %%cr0,%0":"=r" (cr0));
> + if (!(cr0 & 8))
> + asm("movl %0,%%cr0": :"r" (cr0|8));
> + }
We have macros and defines for this in standard headers.
\
> + while (!lg->dead) {
> + unsigned int cr2 = 0; /* Damn gcc */
> +
> + /* Hypercalls first: we might have been out to userspace */
> + if (do_async_hcalls(lg))
> + goto pending_dma;
> +
> + if (regs->trapnum == LGUEST_TRAP_ENTRY) {
> + /* Only do hypercall once. */
> + regs->trapnum = 255;
> + if (hypercall(lg, regs))
> + goto pending_dma;
> + }
> +
> + if (signal_pending(current))
> + return -EINTR
Probably needs freezer checking here somewhere.
> ;
> + maybe_do_interrupt(lg);
> +
> + if (lg->dead)
> + break;
> +
> + if (lg->halted) {
> + set_current_state(TASK_INTERRUPTIBLE);
> + schedule_timeout(1);
1? And what is that good for anyways?
> + /* FIXME: If it's reloading %gs in a loop? */
Yes what then? Have you tried it?
In general i miss printks when things go wrong. Do you expect
all users to have a gdbstub ready? @)
> +pending_dma:
> + put_user(lg->pending_dma, (unsigned long *)user);
> + put_user(lg->pending_addr, (unsigned long *)user+1);
error checking? How do you avoid loops?
> + if (cpu_has_pge) { /* We have a broader idea of "global". */
> + cpu_had_pge = 1;
> + on_each_cpu(adjust_pge, 0, 0, 1);
cpu hotplug?
> + clear_bit(X86_FEATURE_PGE, boot_cpu_data.x86_capability);
> + }
> + return 0;
> +}
>
> + case LHCALL_CRASH: {
> + char msg[128];
> + lhread(lg, msg, regs->edx, sizeof(msg));
> + msg[sizeof(msg)-1] = '\0';
Might be safer to vet for isprint here
> +#define log(...) \
> + do { \
> + mm_segment_t oldfs = get_fs(); \
> + char buf[100]; \
At least older gccs will accumulate the bufs in a function, eventually possibly blowing
the stack. Better use a function.
> + /* If they're halted, we re-enable interrupts. */
> + if (lg->halted) {
> + /* Re-enable interrupts. */
> + put_user(512, &lg->lguest_data->irq_enabled);
interesting magic number
> + /* Ignore NMI, doublefault, hypercall, spurious interrupt. */
> + if (i == 2 || i == 8 || i == 15 || i == LGUEST_TRAP_ENTRY)
> + return;
> + /* FIXME: We should handle debug and int3 */
> + else if (i == 1 || i == 3)
> + return;
> + /* We intercept page fault, general protection fault and fpu missing */
> + else if (i == 13)
> + copy_trap(lg, &lg->gpf_trap, &d);
> + else if (i == 14)
> + copy_trap(lg, &lg->page_trap, &d);
> + else if (i == 7)
> + copy_trap(lg, &lg->fpu_trap, &d);
> + /* Other traps go straight to guest. */
> + else if (i < FIRST_EXTERNAL_VECTOR || i == SYSCALL_VECTOR)
> + setup_idt(lg, i, &d);
> + /* A virtual interrupt */
> + else if (i < FIRST_EXTERNAL_VECTOR + LGUEST_IRQS)
> + copy_trap(lg, &lg->interrupt[i-FIRST_EXTERNAL_VECTOR], &d);\
switch is not cool enough anymore?
>
> + down(&lguest_lock);
i suspect mutexes are the new way to do this
> + down_read(¤t->mm->mmap_sem);
> + if (get_futex_key((u32 __user *)addr, &key) != 0) {
> + kill_guest(lg, "bad dma address %#lx", addr);
> + goto unlock;
Risky? Use probe_kernel_address et.al.?
> +#if 0
> +/* FIXME: Use asm-offsets here... */
Remove?
> +extern int mce_disabled;
tststs
> +
> +/* FIXME: Update iff tsc rate changes. */
It does.
> +static fastcall void lguest_cpuid(unsigned int *eax, unsigned int *ebx,
> + unsigned int *ecx, unsigned int *edx)
> +{
> + int is_feature = (*eax == 1);
> +
> + asm volatile ("cpuid"
> + : "=a" (*eax),
> + "=b" (*ebx),
> + "=c" (*ecx),
> + "=d" (*edx)
> + : "0" (*eax), "2" (*ecx));
What's wrong with the standard cpuid*() macros?
> + extern struct Xgt_desc_struct cpu_gdt_descr;
> + extern struct i386_pda boot_pda;
No externs in .c
> +
> + paravirt_ops.name = "lguest";
Can you just statically initialize this and then copy over?
> + asm volatile ("mov %0, %%gs" : : "r" (__KERNEL_PDA) : "memory");
This will be %fs soon.
... haven't read everything else. the IO driver earlier was also not very closely looked at.
-Andi
On Fri, 2007-02-09 at 11:09 +0100, Andi Kleen wrote:
> On Friday 09 February 2007 10:20, Rusty Russell wrote:
> > Unfortunately, we don't have the build infrastructure for "private"
> > asm-offsets.h files, so there's a not-so-neat include in
> > arch/i386/kernel/asm-offsets.c.
>
> Ask the kbuild people to fix that?
>
> It indeed looks ugly.
>
> I bet Xen et.al. could make good use of that too.
Yes. I originally had the constants #defined in the header and a whole
heap of BUILD_BUG_ON(XYZ_offset != offsetof(xyz)) in my module, which
was even uglier (but at least contained in my code).
> > +# This links the hypervisor in the right place and turns it into a C array.
> > +$(obj)/hypervisor-raw: $(obj)/hypervisor.o
> > + @$(LD) -static -Tdata=`printf %#x $$(($(HYPE_ADDR)))` -Ttext=`printf %#x $$(($(HYPE_ADDR)+$(HYPE_DATA_SIZE)))` -o $@ $< && $(OBJCOPY) -O binary $@
> > +$(obj)/hypervisor-blob.c: $(obj)/hypervisor-raw
> > + @od -tx1 -An -v $< | sed -e 's/^ /0x/' -e 's/$$/,/' -e 's/ /,0x/g' > $@
>
> an .S file with .incbin is more efficient and simpler
> (note it has to be an separate .S file, otherwise icecream/distcc break)
>
> It won't allow to show off any sed skills, but I guess we can live with that ;-)
Good idea, except I currently use sizeof(hypervisor_blob): I'd have to
extract the size separately and hand it in the CFLAGS 8(
> > +static struct vm_struct *hypervisor_vma;
> > +static int cpu_had_pge;
> > +static struct {
> > + unsigned long offset;
> > + unsigned short segment;
> > +} lguest_entry;
> > +struct page *hype_pages; /* Contiguous pages. */
>
> Statics? looks funky. Why only a single hypervisor_vma?
We only have one switcher: it contains an array of "struct
lguest_state"; one for each guest. (This is host code we're looking at
here).
> > +/* IDT entries are at start of hypervisor. */
> > +const unsigned long *__lguest_default_idt_entries(void)
> > +{
> > + return (void *)HYPE_ADDR;
> > +}
> > +
> > +/* Next is switch_to_guest */
> > +static void *__lguest_switch_to_guest(void)
> > +{
> > + return (void *)HYPE_ADDR + HYPE_DATA_SIZE;
> > +}
> > +
> > +/* Then we use everything else to hold guest state. */
> > +struct lguest_state *__lguest_states(void)
> > +{
> > + return (void *)HYPE_ADDR + sizeof(hypervisor_blob);
>
> This cries for asm_offsets.h too, doesn't it?
HYPE_DATA_SIZE is the size of the hypervisor.S data segment, I'm not
sure how I'd get that into asm_offsets....
> > +}
> > +
> > +static __init int map_hypervisor(void)
> > +{
> > + unsigned int i;
> > + int err;
> > + struct page *pages[HYPERVISOR_PAGES], **pagep = pages;
> > +
> > + hype_pages = alloc_pages(GFP_KERNEL|__GFP_ZERO,
> > + get_order(HYPERVISOR_SIZE));
>
> Wasteful because of the rounding. Probably wants reintroduction
> of alloc_pages_exact()
HYPERVISOR_SIZE here is 64k, so it's actually OK (we use the space after
the ~3k text & data from hypervisor.S to hold as many struct
lguest_state's as we can). The name is bad tho; SWITCHER_MAP_SIZE would
probably be better...
> > +
> > +static __exit void unmap_hypervisor(void)
> > +{
> > + vunmap(hypervisor_vma->addr);
> > + __free_pages(hype_pages, get_order(HYPERVISOR_SIZE));
>
> Shouldn't you clean up the GDTs too?
I could, but there doesn't seem a great deal of point. If anyone else
is using them, we're in trouble already...
> > +/* IN/OUT insns: enough to get us past boot-time probing. */
> > +static int emulate_insn(struct lguest *lg)
> > +{
> > + u8 insn;
> > + unsigned int insnlen = 0, in = 0, shift = 0;
> > + unsigned long physaddr = guest_pa(lg, lg->state->regs.eip);
> > +
> > + /* This only works for addresses in linear mapping... */
> > + if (lg->state->regs.eip < lg->page_offset)
> > + return 0;
>
> Shouldn't there be a printk here?
No, the guest should not be able to evoke a printk from the host kernel.
In this case, however, we'll reflect the trap to the kernel, or if the
kernel hasn't installed an IDT yet we'll kill the guest and the lguest
program will get the string "unhandled trap 13 at <EIP> (err=0)".
Basically this emulation code is simply to get us through all that
annoying early boot probing (paravirt_ops does *not* override in/out).
> > +/* Saves exporting idt_table from kernel */
> > +static struct desc_struct *get_idt_table(void)
> > +{
> > + struct Xgt_desc_struct idt;
> > +
> > + asm("sidt %0":"=m" (idt));
>
> Nasty, but ok.
>
> > + return (void *)idt.address;
> > +}
> > +
> > +extern asmlinkage void math_state_restore(void);
>
> No externs in .c files
Agreed, will fix.
> > +
> > +/* Trap page resets this when it reloads gs. */
> > +static int new_gfp_eip(struct lguest *lg, struct lguest_regs *regs)
> > +{
> > + u32 eip;
> > + get_user(eip, &lg->lguest_data->gs_gpf_eip);
> > + if (eip == regs->eip)
> > + return 0;
> > + put_user(regs->eip, &lg->lguest_data->gs_gpf_eip);
>
> No fault checking?
What do we care if the guest process has unmapped the lguest_data page:
it's his funeral. We could put a kill_guest there, but it seems a waste
of code to me.
> lhread/write use probably also needs to be double checked that a malicious
> guest can't put the kernel into a loop.
How?
> > +static void set_ts(unsigned int guest_ts)
> > +{
> > + u32 cr0;
> > + if (guest_ts) {
> > + asm("movl %%cr0,%0":"=r" (cr0));
> > + if (!(cr0 & 8))
> > + asm("movl %0,%%cr0": :"r" (cr0|8));
> > + }
>
> We have macros and defines for this in standard headers.
This was in prep to unexport paravirt_ops, but you threatened the freeze
on me so I pushed this ahead.
Some of this code will get cleaned up along with others when that patch
goes through (my idea is that we'll make sure to expose the native
versions as inlines, so this code can use native_read_cr0 etc. directly.
lguest hosts cannot be paravirtualized anyway 8).
> > + if (signal_pending(current))
> > + return -EINTR
>
> Probably needs freezer checking here somewhere.
Yes, that code always mystified me...
> > + if (lg->halted) {
> > + set_current_state(TASK_INTERRUPTIBLE);
> > + schedule_timeout(1);
>
> 1? And what is that good for anyways?
It's waiting for the dynamic tick code to go in. Again, I was hoping
that that would go in and I could go in behind it, but you made it clear
that waiting longer isn't an option.
> > + /* FIXME: If it's reloading %gs in a loop? */
>
> Yes what then? Have you tried it?
>
> In general i miss printks when things go wrong. Do you expect
> all users to have a gdbstub ready? @)
If this happens then the process inside the guest which is doing this
will get a Segmentation Fault, because we'll deliver a GPF to the kernel
at that EIP. In practice, glibc doesn't do this that I have found. The
note is there so I think harder about it if anyone reports strange segvs
under lguest 8)
> > +pending_dma:
> > + put_user(lg->pending_dma, (unsigned long *)user);
> > + put_user(lg->pending_addr, (unsigned long *)user+1);
>
> error checking? How do you avoid loops?
Why? And I still don't get what loops?
> > + if (cpu_has_pge) { /* We have a broader idea of "global". */
> > + cpu_had_pge = 1;
> > + on_each_cpu(adjust_pge, 0, 0, 1);
>
> cpu hotplug?
Good catch. Should lock out cpu hotplug during this. Once it's done,
we're fine, because we clear the feature bit. Same when putting it
back.
> > + clear_bit(X86_FEATURE_PGE, boot_cpu_data.x86_capability);
> > + }
> > + return 0;
> > +}
> >
> > + case LHCALL_CRASH: {
> > + char msg[128];
> > + lhread(lg, msg, regs->edx, sizeof(msg));
> > + msg[sizeof(msg)-1] = '\0';
>
> Might be safer to vet for isprint here.
Hmm, I hope the kasprintf won't barf... this buffer gets handed straight
to the lguest process on its next read.
> > +#define log(...) \
> > + do { \
> > + mm_segment_t oldfs = get_fs(); \
> > + char buf[100]; \
>
> At least older gccs will accumulate the bufs in a function, eventually possibly blowing
> the stack. Better use a function.
Or I'll kill the macro altogether. It's useful for shotgun debugging a
guest, but it's unused.
> > + /* If they're halted, we re-enable interrupts. */
> > + if (lg->halted) {
> > + /* Re-enable interrupts. */
> > + put_user(512, &lg->lguest_data->irq_enabled);
>
> interesting magic number
Yeah, we don't define it anywhere, and we probably should. From
irqflags.h:
static inline int raw_irqs_disabled_flags(unsigned long flags)
{
return !(flags & (1 << 9));
}
8(
> > + /* Ignore NMI, doublefault, hypercall, spurious interrupt. */
> > + if (i == 2 || i == 8 || i == 15 || i == LGUEST_TRAP_ENTRY)
> > + return;
> > + /* FIXME: We should handle debug and int3 */
> > + else if (i == 1 || i == 3)
> > + return;
> > + /* We intercept page fault, general protection fault and fpu missing */
> > + else if (i == 13)
> > + copy_trap(lg, &lg->gpf_trap, &d);
> > + else if (i == 14)
> > + copy_trap(lg, &lg->page_trap, &d);
> > + else if (i == 7)
> > + copy_trap(lg, &lg->fpu_trap, &d);
> > + /* Other traps go straight to guest. */
> > + else if (i < FIRST_EXTERNAL_VECTOR || i == SYSCALL_VECTOR)
> > + setup_idt(lg, i, &d);
> > + /* A virtual interrupt */
> > + else if (i < FIRST_EXTERNAL_VECTOR + LGUEST_IRQS)
> > + copy_trap(lg, &lg->interrupt[i-FIRST_EXTERNAL_VECTOR], &d);\
>
> switch is not cool enough anymore?
It would have to be a switch then gunk at the bottom, because those last
two tests don't switch-ify. IIRC I changed back from a switch because
of that.
> > + down(&lguest_lock);
>
> i suspect mutexes are the new way to do this
Yep, will replace.
> > + down_read(¤t->mm->mmap_sem);
> > + if (get_futex_key((u32 __user *)addr, &key) != 0) {
> > + kill_guest(lg, "bad dma address %#lx", addr);
> > + goto unlock;
>
> Risky? Use probe_kernel_address et.al.?
No, get_futex_key handles anything; we rely on that in futex.c
> > +#if 0
> > +/* FIXME: Use asm-offsets here... */
>
> Remove?
Yep, done in the split-up version...
> > +extern int mce_disabled;
>
> tststs
I'll get that too 8)
> > +
> > +/* FIXME: Update iff tsc rate changes. */
>
> It does.
Hey, but I have a FIXME for it!
> > +static fastcall void lguest_cpuid(unsigned int *eax, unsigned int *ebx,
> > + unsigned int *ecx, unsigned int *edx)
> > +{
> > + int is_feature = (*eax == 1);
> > +
> > + asm volatile ("cpuid"
> > + : "=a" (*eax),
> > + "=b" (*ebx),
> > + "=c" (*ecx),
> > + "=d" (*edx)
> > + : "0" (*eax), "2" (*ecx));
>
> What's wrong with the standard cpuid*() macros?
Good call... we expose native_cpuid so I should use that (I can't use
cpuid: this *is* cpuid!)
> > + extern struct Xgt_desc_struct cpu_gdt_descr;
> > + extern struct i386_pda boot_pda;
>
> No externs in .c
Yes, ugly code. Usually this stuff is done in asm, so it doesn't exist
in a header. I'll fix it.
> > +
> > + paravirt_ops.name = "lguest";
>
> Can you just statically initialize this and then copy over?
Sure, but then I'll hit all the things I don't want to override, and
it'll be far less clear.
> > + asm volatile ("mov %0, %%gs" : : "r" (__KERNEL_PDA) : "memory");
>
> This will be %fs soon.
I know, I had to change this back to backport from -mm tree 8(
Fortunately, the breakage is *really* obvious when it happens.
> ... haven't read everything else. the IO driver earlier was also not very closely looked at.
OK, I'll resend patches (in 4 parts) based on these comments (and
Andrew's).
Thanks!
Rusty.
On Fri, Feb 09, 2007 at 11:39:31PM +1100, Rusty Russell wrote:
> On Fri, 2007-02-09 at 11:09 +0100, Andi Kleen wrote:
> > > +# This links the hypervisor in the right place and turns it into a C array.
> > > +$(obj)/hypervisor-raw: $(obj)/hypervisor.o
> > > + @$(LD) -static -Tdata=`printf %#x $$(($(HYPE_ADDR)))` -Ttext=`printf %#x $$(($(HYPE_ADDR)+$(HYPE_DATA_SIZE)))` -o $@ $< && $(OBJCOPY) -O binary $@
> > > +$(obj)/hypervisor-blob.c: $(obj)/hypervisor-raw
> > > + @od -tx1 -An -v $< | sed -e 's/^ /0x/' -e 's/$$/,/' -e 's/ /,0x/g' > $@
> >
> > an .S file with .incbin is more efficient and simpler
> > (note it has to be an separate .S file, otherwise icecream/distcc break)
> >
> > It won't allow to show off any sed skills, but I guess we can live with that ;-)
>
> Good idea, except I currently use sizeof(hypervisor_blob): I'd have to
> extract the size separately and hand it in the CFLAGS 8(
hypervisor_start:
.incbin "hypervisor"
hypervisor_end:
...
extern char hypervisor_start[], hypervisor_end[];
size = hypervisor_end - hypervisor_start;
> > > +static int cpu_had_pge;
> > > +static struct {
> > > + unsigned long offset;
> > > + unsigned short segment;
> > > +} lguest_entry;
> > > +struct page *hype_pages; /* Contiguous pages. */
> >
> > Statics? looks funky. Why only a single hypervisor_vma?
>
> We only have one switcher: it contains an array of "struct
> lguest_state"; one for each guest. (This is host code we're looking at
> here).
This means it is not SMP safe?
> No, the guest should not be able to evoke a printk from the host kernel.
This means nobody will know why it failed.
> > > + else if (i < FIRST_EXTERNAL_VECTOR || i == SYSCALL_VECTOR)
> > > + setup_idt(lg, i, &d);
> > > + /* A virtual interrupt */
> > > + else if (i < FIRST_EXTERNAL_VECTOR + LGUEST_IRQS)
> > > + copy_trap(lg, &lg->interrupt[i-FIRST_EXTERNAL_VECTOR], &d);\
> >
> > switch is not cool enough anymore?
>
> It would have to be a switch then gunk at the bottom, because those last
> two tests don't switch-ify. IIRC I changed back from a switch because
> of that.
gcc has a handy extension for this:
case 0...FIRST_EXTERNAL_VECTOR-1:
case SYSCALL_VECTOR:
case FIRST_EXTERNAL_VECTOR...FIRST_EXTERNAL_VECTOR+LGUEST_IRQS:
Re: the loops; e.g. we used to have possible loop cases
when a page fault does read instructions and then causes another
page fault etc.etc. I haven't seen any immediate danger of this,
but it might be worth double checking.
-Andi
On Fri, Feb 09, 2007 at 11:39:31PM +1100, Rusty Russell wrote:
> On Fri, 2007-02-09 at 11:09 +0100, Andi Kleen wrote:
> > On Friday 09 February 2007 10:20, Rusty Russell wrote:
> > > Unfortunately, we don't have the build infrastructure for "private"
> > > asm-offsets.h files, so there's a not-so-neat include in
> > > arch/i386/kernel/asm-offsets.c.
> >
> > Ask the kbuild people to fix that?
> >
> > It indeed looks ugly.
> >
> > I bet Xen et.al. could make good use of that too.
>
> Yes. I originally had the constants #defined in the header and a whole
> heap of BUILD_BUG_ON(XYZ_offset != offsetof(xyz)) in my module, which
> was even uglier (but at least contained in my code).
I do not quite see what you ask for.
Care to try to describe the problem a bit then I may look at it sometime.
[Heading for vacation in a few hours so no prompt reply]
Sam
On Fri, 2007-02-09 at 14:57 +0100, Andi Kleen wrote:
> On Fri, Feb 09, 2007 at 11:39:31PM +1100, Rusty Russell wrote:
> > On Fri, 2007-02-09 at 11:09 +0100, Andi Kleen wrote:
> > > > +# This links the hypervisor in the right place and turns it into a C array.
> > > > +$(obj)/hypervisor-raw: $(obj)/hypervisor.o
> > > > + @$(LD) -static -Tdata=`printf %#x $$(($(HYPE_ADDR)))` -Ttext=`printf %#x $$(($(HYPE_ADDR)+$(HYPE_DATA_SIZE)))` -o $@ $< && $(OBJCOPY) -O binary $@
> > > > +$(obj)/hypervisor-blob.c: $(obj)/hypervisor-raw
> > > > + @od -tx1 -An -v $< | sed -e 's/^ /0x/' -e 's/$$/,/' -e 's/ /,0x/g' > $@
> > >
> > > an .S file with .incbin is more efficient and simpler
> > > (note it has to be an separate .S file, otherwise icecream/distcc break)
> > >
> > > It won't allow to show off any sed skills, but I guess we can live with that ;-)
> >
> > Good idea, except I currently use sizeof(hypervisor_blob): I'd have to
> > extract the size separately and hand it in the CFLAGS 8(
>
> hypervisor_start:
> .incbin "hypervisor"
> hypervisor_end:
>
> ...
> extern char hypervisor_start[], hypervisor_end[];
>
> size = hypervisor_end - hypervisor_start;
#define MAX_LGUEST_GUESTS \
((HYPERVISOR_SIZE-sizeof(hypervisor_blob))/sizeof(struct lguest_state))
struct lguest lguests[MAX_LGUEST_GUESTS];
I could kmalloc that array, of course, but is it worth it to get rid of
one line in a Makefile?
> > > Statics? looks funky. Why only a single hypervisor_vma?
> >
> > We only have one switcher: it contains an array of "struct
> > lguest_state"; one for each guest. (This is host code we're looking at
> > here).
>
> This means it is not SMP safe?
No, it's host-SMP safe. There's no guest SMP support though, which
keeps things nice and simple.
> > No, the guest should not be able to evoke a printk from the host kernel.
>
> This means nobody will know why it failed.
No, that's why the lguest process gets the error string (and prints it
out). Biggest usability improvement I made in a while.
The kernel log is the absolute worst place to report errors; iptables
and module code both do that and the #1 FAQ is "What happened?"
I didn't make the same mistake this (third) time! Here's lguest when
the guest crashes:
# lguest 64m bzImage ...
...
lguest: CRASH: Attempted to kill init!
#
> > It would have to be a switch then gunk at the bottom, because those last
> > two tests don't switch-ify. IIRC I changed back from a switch because
> > of that.
>
> gcc has a handy extension for this:
>
> case 0...FIRST_EXTERNAL_VECTOR-1:
> case SYSCALL_VECTOR:
> case FIRST_EXTERNAL_VECTOR...FIRST_EXTERNAL_VECTOR+LGUEST_IRQS:
Indeed, and I really wanted to use it, but it still doesn't allow
overlapping ranges. The first cases are in the middle of 0 ...
FIRST_EXTERNAL_VECTOR-1, and if LGUEST_IRQS is large enough,
SYSCALL_VECTOR is in the middle of FIRST_EXTERNAL_VECTOR ...
FIRST_EXTERNAL_VECTOR+LGUEST_IRQS 8(
Nonetheless, I switchified what I could in my update (testing now).
> Re: the loops; e.g. we used to have possible loop cases
> when a page fault does read instructions and then causes another
> page fault etc.etc. I haven't seen any immediate danger of this,
> but it might be worth double checking.
OK, I'll run some tests here. There shouldn't be any danger here
though....
Thanks!
Rusty.
On Fri, 2007-02-09 at 15:17 +0100, Sam Ravnborg wrote:
> I do not quite see what you ask for.
> Care to try to describe the problem a bit then I may look at it sometime.
>
> [Heading for vacation in a few hours so no prompt reply]
I'd like my own, private "asm-offsets.h". In this case, in
arch/i386/lguest/. I guess it's a matter of extracting the core of the
asm-offsets.h magic and generalizing it.
Have a good break!
Rusty.
> From: Rusty Russell
> Newsgroups: gmane.linux.kernel,gmane.linux.kernel.virtualization
> Subject: Re: [PATCH 6/10] lguest code: the little linux hypervisor.
> Date: Sat, 10 Feb 2007 02:23:19 +1100
Hallo, Rusty, guys.
> On Fri, 2007-02-09 at 15:17 +0100, Sam Ravnborg wrote:
>> I do not quite see what you ask for.
>> Care to try to describe the problem a bit then I may look at it sometime.
>>
>> [Heading for vacation in a few hours so no prompt reply]
>
> I'd like my own, private "asm-offsets.h". In this case, in
> arch/i386/lguest/. I guess it's a matter of extracting the core of the
> asm-offsets.h magic and generalizing it.
>
> Have a good break!
> Rusty.
If you will have time for newbie, to explain in a few words, what is it need
for (whole idea, or key detail), and, maybe, why it is generated so ... interestingly:
asm-offsets.c -> *.s -> *.h
(but this looks like interconnecting C and assembler, obviously)
I will glad to help providing solution maybe somewhat earlier (well, i'm
trying to understand whole building process, if that matters).
.... And, of course, if this q isn't dumb.
Thanks.
____
> If you will have time for newbie, to explain in a few words, what is it need
> for (whole idea, or key detail), and, maybe, why it is generated so ... interestingly:
>
> asm-offsets.c -> *.s -> *.h
> (but this looks like interconnecting C and assembler, obviously)
>
> I will glad to help providing solution maybe somewhat earlier (well, i'm
> trying to understand whole building process, if that matters).
The problem is trying to figure out what needs to be done to get
multiple asm-offsets.h.
-Andi
>
> If you will have time for newbie, to explain in a few words, what is it need
> for (whole idea, or key detail), and, maybe, why it is generated so ... interestingly:
>
> asm-offsets.c -> *.s -> *.h
> (but this looks like interconnecting C and assembler, obviously)
Correct - asm-offsets is used to transfer constants for .c to assembler,
which us especially convinient for constant that differ when a stuct
is changed.
>
> I will glad to help providing solution maybe somewhat earlier (well, i'm
> trying to understand whole building process, if that matters).
Basically what is requested is to move the generic parts of asm-offsets
generation from top-level Kbuild file to a generic file somewhere.
Since this is not generic Kbuild functionality but more specific to the
kernel it should not be part of Kbuild generic files.
include/asm-generic/asm-offsets seems to be a fair place for it.
It would contain the sed-y parts as well as cmd_offsets definition.
I would be glad if you could give that a spin.
Sam
On Mon, 2007-02-12 at 14:34 +0100, Oleg Verych wrote:
> > I'd like my own, private "asm-offsets.h". In this case, in
> > arch/i386/lguest/. I guess it's a matter of extracting the core of the
> > asm-offsets.h magic and generalizing it.
> >
> > Have a good break!
> > Rusty.
>
> If you will have time for newbie, to explain in a few words, what is it need
> for (whole idea, or key detail), and, maybe, why it is generated so ... interestingly:
>
> asm-offsets.c -> *.s -> *.h
> (but this looks like interconnecting C and assembler, obviously)
Hi Oleg,
Always happy to explain. There's often a need to access constants in
assembler, which can only be derived from C, such as the size of a
structure, or the offset of a certain member within a structure.
Hardcoding the numbers in assembler is fragile leading to breakage when
something changes.
So, asm-offsets.c is the solution: it uses asm() statements to emit
patterns in the assembler, with the compiler computing the actual
numbers, eg:
#define DEFINE(sym, val) \
asm volatile("\n->" #sym " %0 " #val : : "i" (val))
DEFINE(SIZEOF_FOOBAR, sizeof(foobar));
Becomes in asm-offsets.s:
->SIZEOF_FOOBAR $10 sizeof(foobar) #
This gets sed'd back into asm-offsets.h:
#define SIZEOF_FOOBAR 10 /* SIZEOF_FOOBAR # */
This can be included from .S files (which get passed through the
pre-processor).
Hope that helps!
Rusty.
On Tue, Feb 13, 2007 at 10:41:36AM +1100, Rusty Russell wrote:
> Always happy to explain.
Thanks!
[]
> So, asm-offsets.c is the solution: it uses asm() statements to emit
> patterns in the assembler, with the compiler computing the actual
> numbers, eg:
>
> #define DEFINE(sym, val) \
(1) asm volatile("\n->" #sym " %0 " #val : : "i" (val))
> DEFINE(SIZEOF_FOOBAR, sizeof(foobar));
>
(2) Becomes in asm-offsets.s:
> ->SIZEOF_FOOBAR $10 sizeof(foobar) #
>
(3) This gets sed'd back into asm-offsets.h:
> #define SIZEOF_FOOBAR 10 /* SIZEOF_FOOBAR # */
>
> This can be included from .S files (which get passed through the
> pre-processor).
So, to make this parsing job clear once again, (1) is a pattern generator
of (2) for sed (this is not assembler), which is making final (3) then.
And this are actually C-for-asm offsets (asm-offsets.h is confusing to
me).
On Mon, Feb 12, 2007 at 10:41:28PM +0100, Sam Ravnborg wrote:
[]
> > I will glad to help providing solution maybe somewhat earlier (well, i'm
> > trying to understand whole building process, if that matters).
>
> Basically what is requested is to move the generic parts of asm-offsets
> generation from top-level Kbuild file to a generic file somewhere.
>
> Since this is not generic Kbuild functionality but more specific to the
> kernel it should not be part of Kbuild generic files.
> include/asm-generic/asm-offsets seems to be a fair place for it.
Too much "generic" were used. Anyways, simply thing is, that there must be:
-- hardware file(s) for Arch;
-- "private" software file(s), for paravirt and such.
On Mon, Feb 12, 2007 at 06:24:13PM +0100, Andi Kleen wrote:
[]
> The problem is trying to figure out what needs to be done to get
> multiple asm-offsets.h.
Proposition will follow.
Kind regards.
P.S.
While it was fun to run kinds of offtopic operation systems in
virtual machine back in 2000-2001. Now it isn't.
Mainly due to yet another zoo. May be this is good for quick adoption,
commercial benefit, etc. Yet thankfully to Rusty, general useful
infrastructure is available now. Kudos to everyone :)
____
On Tue, Feb 13, 2007 at 04:10:44AM +0100, Oleg Verych wrote:
[]
>
> Proposition will follow.
>
[]
[patch proposition] kbuild: lguest with private asm-offsets
* added some bloat to lguest's Makefile:
- lguest doesn't rebuild, if not changed (due to FORCED implicit %o:%S),
- support of kbuild's queit/nonquiet commands,
- (hopefully) more readable and clear build process,
- private asm-offsets,
* some whitespace was killed,
* needs "asm-offsets magic demystified, generalized".
Done on top of first lkml post of lguest.
pp-by: Oleg Verych
---
arch/i386/kernel/asm-offsets.c | 23 +-----------------
arch/i386/lguest/Makefile | 40 +++++++++++++++++++++++++++++---
arch/i386/lguest/asm-offsets.c | 31 +++++++++++++++++++++++++
arch/i386/lguest/hypervisor.S | 8 +++---
4 files changed, 73 insertions(+), 29 deletions(-)
Index: linux-2.6.20/arch/i386/lguest/Makefile
===================================================================
--- linux-2.6.20.orig/arch/i386/lguest/Makefile 2007-02-16 15:32:51.665149000 +0100
+++ linux-2.6.20/arch/i386/lguest/Makefile 2007-02-16 16:16:48.525942500 +0100
@@ -1,2 +1,6 @@
+#
+# i386/lguest/Makefile
+#
+
# Guest requires the paravirt_ops replacement and the bus driver.
obj-$(CONFIG_LGUEST_GUEST) += lguest.o lguest_bus.o
@@ -7,4 +11,6 @@ lg-objs := core.o hypercalls.o page_tabl
segments.o io.o lguest_user.o
+asm-offsets-lg := $(objtree)/include/asm/asm-offsets-lg.h
+
# We use top 4MB for guest traps page, then hypervisor. */
HYPE_ADDR := (0xFFC00000+4096)
@@ -13,10 +19,36 @@ HYPE_DATA_SIZE := 1024
CFLAGS += -DHYPE_ADDR="$(HYPE_ADDR)" -DHYPE_DATA_SIZE="$(HYPE_DATA_SIZE)"
+LD_HYPE_DATA := $(shell printf %\#x $$(($(HYPE_ADDR))))
+LD_HYPE_TEXT := $(shell printf %\#x $$(($(HYPE_ADDR)+$(HYPE_DATA_SIZE))))
+
+quiet_cmd_ld_hyber = LD [H] $@
+ cmd_ld_hyber = $(LD) -static -Tdata=$(LD_HYPE_DATA) \
+ -Ttext=$(LD_HYPE_TEXT) -o $@ $<
+cmd_objcopy = $(OBJCOPY) -O binary $@
+
+quiet_cmd_blob = BLOB $@
+ cmd_blob = od -tx1 -An -v $< | sed -e 's/^ /0x/' \
+ -e 's/$$/,/' -e 's/ /,0x/g' > $@
+quiet_cmd_offsets = GEN $@
+ cmd_offsets = $(srctree)/scripts/mkCconstants $< $@
+
$(obj)/core.o: $(obj)/hypervisor-blob.c
+
+$(obj)/hypervisor-blob.c: $(obj)/hypervisor-raw
+ $(call cmd,blob)
+
# This links the hypervisor in the right place and turns it into a C array.
$(obj)/hypervisor-raw: $(obj)/hypervisor.o
- @$(LD) -static -Tdata=`printf %#x $$(($(HYPE_ADDR)))` -Ttext=`printf %#x $$(($(HYPE_ADDR)+$(HYPE_DATA_SIZE)))` -o $@ $< && $(OBJCOPY) -O binary $@
-$(obj)/hypervisor-blob.c: $(obj)/hypervisor-raw
- @od -tx1 -An -v $< | sed -e 's/^ /0x/' -e 's/$$/,/' -e 's/ /,0x/g' > $@
+ $(call if_changed,ld_hyber)
+ $(call cmd,objcopy)
+
+$(obj)/hypervisor.o: $(src)/hypervisor.S $(asm-offsets-lg)
+ $(call if_changed_dep,as_o_S)
+
+$(asm-offsets-lg): $(obj)/asm-offsets.s
+ $(call cmd,offsets)
+
+$(obj)/asm-offsets.s: $(src)/asm-offsets.c
+ $(call if_changed_dep,cc_s_c)
-clean-files := hypervisor-blob.c hypervisor-raw
+clean-files := $(asm-offsets-lg) hypervisor-blob.c hypervisor-raw
Index: linux-2.6.20/arch/i386/lguest/asm-offsets.c
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.20/arch/i386/lguest/asm-offsets.c 2007-02-16 16:03:10.410813500 +0100
@@ -0,0 +1,31 @@
+/*
+ * lguest's "private"
+ */
+
+#include <linux/sched.h>
+#include <asm/lguest.h>
+#include "../lguest/lg.h"
+
+#define DEFINE(sym, val) \
+ asm volatile("\n->" #sym " %0 " #val : : "i" (val))
+
+#define BLANK() asm volatile("\n->" : : )
+
+#define OFFSET(sym, str, mem) \
+ DEFINE(sym, offsetof(struct str, mem));
+
+void fell_lguest(void)
+{
+ BLANK();
+ OFFSET(LGUEST_DATA_irq_enabled, lguest_data, irq_enabled);
+ OFFSET(LGUEST_STATE_host_stackptr, lguest_state, host.stackptr);
+ OFFSET(LGUEST_STATE_host_pgdir, lguest_state, host.pgdir);
+ OFFSET(LGUEST_STATE_host_gdt, lguest_state, host.gdt);
+ OFFSET(LGUEST_STATE_host_idt, lguest_state, host.idt);
+ OFFSET(LGUEST_STATE_regs, lguest_state, regs);
+ OFFSET(LGUEST_STATE_gdt, lguest_state, gdt);
+ OFFSET(LGUEST_STATE_idt, lguest_state, idt);
+ OFFSET(LGUEST_STATE_gdt_table, lguest_state, gdt_table);
+ OFFSET(LGUEST_STATE_trapnum, lguest_state, regs.trapnum);
+ OFFSET(LGUEST_STATE_errcode, lguest_state, regs.errcode);
+}
Index: linux-2.6.20/arch/i386/kernel/asm-offsets.c
===================================================================
--- linux-2.6.20.orig/arch/i386/kernel/asm-offsets.c 2007-02-16 16:02:11.155110250 +0100
+++ linux-2.6.20/arch/i386/kernel/asm-offsets.c 2007-02-16 16:03:52.697456250 +0100
@@ -17,11 +17,7 @@
#include <asm/elf.h>
#include <asm/pda.h>
-#ifdef CONFIG_LGUEST_GUEST
-#include <asm/lguest.h>
-#include "../lguest/lg.h"
-#endif
#define DEFINE(sym, val) \
- asm volatile("\n->" #sym " %0 " #val : : "i" (val))
+ asm volatile("\n->" #sym " %0 " #val : : "i" (val))
#define BLANK() asm volatile("\n->" : : )
@@ -104,5 +100,5 @@ void foo(void)
BLANK();
- OFFSET(PDA_cpu, i386_pda, cpu_number);
+ OFFSET(PDA_cpu, i386_pda, cpu_number);
OFFSET(PDA_pcurrent, i386_pda, pcurrent);
@@ -116,18 +112,3 @@ void foo(void)
OFFSET(PARAVIRT_read_cr0, paravirt_ops, read_cr0);
#endif
-
-#ifdef CONFIG_LGUEST_GUEST
- BLANK();
- OFFSET(LGUEST_DATA_irq_enabled, lguest_data, irq_enabled);
- OFFSET(LGUEST_STATE_host_stackptr, lguest_state, host.stackptr);
- OFFSET(LGUEST_STATE_host_pgdir, lguest_state, host.pgdir);
- OFFSET(LGUEST_STATE_host_gdt, lguest_state, host.gdt);
- OFFSET(LGUEST_STATE_host_idt, lguest_state, host.idt);
- OFFSET(LGUEST_STATE_regs, lguest_state, regs);
- OFFSET(LGUEST_STATE_gdt, lguest_state, gdt);
- OFFSET(LGUEST_STATE_idt, lguest_state, idt);
- OFFSET(LGUEST_STATE_gdt_table, lguest_state, gdt_table);
- OFFSET(LGUEST_STATE_trapnum, lguest_state, regs.trapnum);
- OFFSET(LGUEST_STATE_errcode, lguest_state, regs.errcode);
-#endif
}
Index: linux-2.6.20/arch/i386/lguest/hypervisor.S
===================================================================
--- linux-2.6.20.orig/arch/i386/lguest/hypervisor.S 2007-02-16 16:09:02.620825250 +0100
+++ linux-2.6.20/arch/i386/lguest/hypervisor.S 2007-02-16 16:13:19.548882250 +0100
@@ -2,5 +2,5 @@
Layout is: default_idt_entries (1k), then switch_to_guest entry point. */
#include <linux/linkage.h>
-#include <asm/asm-offsets.h>
+#include <asm/asm-offsets-lg.h>
#include "lg.h"
@@ -104,5 +104,5 @@ switch_to_guest:
popl %es; \
popl %ss
-
+
/* Return to run_guest_once. */
return_to_host:
@@ -151,5 +151,5 @@ deliver_to_host_with_errcode:
.endr
.endm
-
+
/* We intercept every interrupt, because we may need to switch back to
* host. Unfortunately we can't tell them apart except by entry
@@ -158,5 +158,5 @@ deliver_to_host_with_errcode:
irq_stubs:
.data
-default_idt_entries:
+default_idt_entries:
.text
IRQ_STUBS 0 1 return_to_host /* First two traps */
> >
> > Proposition will follow.
> >
> []
>
> [patch proposition] kbuild: lguest with private asm-offsets
[]
> * needs "asm-offsets magic demystified, generalized".
[]
[patch proposition] kbuild: asm-offsets generalized
* scripts/mkCconstants:
- asm-offsets magic demystified, generalized,
* (hopefully) more readable sed scripts,
* top Kbuild may be updated...
* file needs `chmod u+x`, i don't know, how it's done in patch(1).
pp-by: Oleg Verych
---
scripts/mkCconstants | 50 +++++++++++++++++++++++++++++++++++++++++
1 files changed, 50 insertions(+), 0 deletions(-)
Index: linux-2.6.20/scripts/mkCconstants
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.20/scripts/mkCconstants 2007-02-16 15:33:51.696900750 +0100
@@ -0,0 +1,50 @@
+#!/bin/sh
+
+# Input file, where values of interest are stored is produced by
+# `cmd_cc_s_c'. It yields calculation of constants, needed in
+# assembler modules. Output is a suitable header file.
+#
+# $1 - input filename;
+# $2 - output filename;
+# $3 - header file format: "normal" (default), "mips".
+
+set -e
+
+[ -z "$1" ] || [ -z "$2" ] && exit 1
+
+case $3 in
+ mips)
+ SED_SCRIPT='
+/^@@@/{
+s/^@@@//;
+s/ \#.*\$//;
+p;
+}'
+ ;;
+ normal | *)
+ SED_SCRIPT='
+/^->/{
+s:^->\([^ ]*\) [\$#]*\([^ ]*\) \(.*\):#define \1 \2 /* \3 */:;
+s:->::;
+p;
+}'
+ ;;
+esac
+
+cat << "EOF" > $2
+#ifndef __ASM_OFFSETS_H__
+#define __ASM_OFFSETS_H__
+
+/*
+ * This file was generated by scripts/mkCconstants
+ */
+
+EOF
+
+sed -ne "$SED_SCRIPT" $1 >> $2
+
+cat << "EOF" >> $2
+
+#endif
+
+EOF
--
-o--=O`C info emacs : not found
#oo'L O info make : not found
<___=E M man gcc : not found
On Fri, Feb 16, 2007 at 04:59:29PM +0100, Oleg Verych wrote:
> > >
> > > Proposition will follow.
> > >
> > []
> >
> > [patch proposition] kbuild: lguest with private asm-offsets
> []
> > * needs "asm-offsets magic demystified, generalized".
> []
To make asm-offset generic I had in mind something like the
following.
It uses the currect functionality, and allows for architecture override
as needed - but default not used. And architecture override is kept in a
architecture specific location.
Sam
commit df95bb04b04ff2f64805dfa8459099ffe469c8a5
Author: Sam Ravnborg <[email protected]>
Date: Fri Feb 16 19:51:29 2007 +0100
kbuild: Make asm-offset generic
Signed-off-by: Sam Ravnborg <[email protected]>
diff --git a/include/asm-generic/asm-offset b/include/asm-generic/asm-offset
new file mode 100644
index 0000000..5e5629e
--- /dev/null
+++ b/include/asm-generic/asm-offset
@@ -0,0 +1,30 @@
+#
+# Support generating constant useable from assembler but defined on C-level
+#
+# See usage in top-level Kbuild file
+
+# Default sed regexp - multiline due to syntax constraints
+define sed-y
+ "/^->/{s:^->\([^ ]*\) [\$$#]*\([^ ]*\) \(.*\):#define \1 \2 /* \3 */:; s:->::; p;}"
+endef
+
+# let architectures override the sed expression as needed
+-include include/asm/asm-offset
+
+quiet_cmd_offsets = GEN $@
+define cmd_offsets
+ (set -e; \
+ echo "#ifndef __ASM_OFFSETS_H__"; \
+ echo "#define __ASM_OFFSETS_H__"; \
+ echo "/*"; \
+ echo " * DO NOT MODIFY."; \
+ echo " *"; \
+ echo " * This file was generated by Kbuild"; \
+ echo " *"; \
+ echo " */"; \
+ echo ""; \
+ sed -ne $(sed-y) $<; \
+ echo ""; \
+ echo "#endif" ) > $@
+endef
+
diff --git a/include/asm-mips/asm-offset b/include/asm-mips/asm-offset
new file mode 100644
index 0000000..b2eb959
--- /dev/null
+++ b/include/asm-mips/asm-offset
@@ -0,0 +1,4 @@
+# Override default sed for MIPS when generating asm-offset
+sed-$(CONFIG_MIPS) := "/^@@@/{s/^@@@//; s/ \#.*\$$//; p;}"
+
+
diff --git a/Kbuild b/Kbuild
index 0451f69..0a32f5f 100644
--- a/Kbuild
+++ b/Kbuild
@@ -1,5 +1,8 @@
#
# Kbuild for top-level directory of the kernel
+
+
+
# This file takes care of the following:
# 1) Generate asm-offsets.h
@@ -7,36 +10,14 @@ #####
# 1) Generate asm-offsets.h
#
+include include/asm-generic/asm-offset
+
offsets-file := include/asm-$(ARCH)/asm-offsets.h
always := $(offsets-file)
targets := $(offsets-file)
targets += arch/$(ARCH)/kernel/asm-offsets.s
-# Default sed regexp - multiline due to syntax constraints
-define sed-y
- "/^->/{s:^->\([^ ]*\) [\$$#]*\([^ ]*\) \(.*\):#define \1 \2 /* \3 */:; s:->::; p;}"
-endef
-# Override default regexp for specific architectures
-sed-$(CONFIG_MIPS) := "/^@@@/{s/^@@@//; s/ \#.*\$$//; p;}"
-
-quiet_cmd_offsets = GEN $@
-define cmd_offsets
- (set -e; \
- echo "#ifndef __ASM_OFFSETS_H__"; \
- echo "#define __ASM_OFFSETS_H__"; \
- echo "/*"; \
- echo " * DO NOT MODIFY."; \
- echo " *"; \
- echo " * This file was generated by Kbuild"; \
- echo " *"; \
- echo " */"; \
- echo ""; \
- sed -ne $(sed-y) $<; \
- echo ""; \
- echo "#endif" ) > $@
-endef
-
# We use internal kbuild rules to avoid the "is up to date" message from make
arch/$(ARCH)/kernel/asm-offsets.s: arch/$(ARCH)/kernel/asm-offsets.c FORCE
$(Q)mkdir -p $(dir $@)
Hallo.
On Fri, Feb 16, 2007 at 07:56:35PM +0100, Sam Ravnborg wrote:
> On Fri, Feb 16, 2007 at 04:59:29PM +0100, Oleg Verych wrote:
> > > >
> > > > Proposition will follow.
> > > >
> > > []
> > >
> > > [patch proposition] kbuild: lguest with private asm-offsets
> > []
> > > * needs "asm-offsets magic demystified, generalized".
> > []
>
> To make asm-offset generic I had in mind something like the
> following.
Then i misunderstood, what you mean, sorry.
> It uses the currect functionality, and allows for architecture override
> as needed - but default not used. And architecture override is kept in a
> architecture specific location.
>
> Sam
>
> commit df95bb04b04ff2f64805dfa8459099ffe469c8a5
> Author: Sam Ravnborg <[email protected]>
> Date: Fri Feb 16 19:51:29 2007 +0100
>
> kbuild: Make asm-offset generic
>
> Signed-off-by: Sam Ravnborg <[email protected]>
>
> diff --git a/include/asm-generic/asm-offset b/include/asm-generic/asm-offset
> new file mode 100644
> index 0000000..5e5629e
> --- /dev/null
> +++ b/include/asm-generic/asm-offset
> @@ -0,0 +1,30 @@
> +#
> +# Support generating constant useable from assembler but defined on C-level
> +#
> +# See usage in top-level Kbuild file
> +
> +# Default sed regexp - multiline due to syntax constraints
> +define sed-y
> + "/^->/{s:^->\([^ ]*\) [\$$#]*\([^ ]*\) \(.*\):#define \1 \2 /* \3 */:; s:->::; p;}"
> +endef
> +
> +# let architectures override the sed expression as needed
> +-include include/asm/asm-offset
> +
> +quiet_cmd_offsets = GEN $@
> +define cmd_offsets
> + (set -e; \
> + echo "#ifndef __ASM_OFFSETS_H__"; \
> + echo "#define __ASM_OFFSETS_H__"; \
> + echo "/*"; \
> + echo " * DO NOT MODIFY."; \
> + echo " *"; \
> + echo " * This file was generated by Kbuild"; \
> + echo " *"; \
> + echo " */"; \
> + echo ""; \
> + sed -ne $(sed-y) $<; \
> + echo ""; \
> + echo "#endif" ) > $@
> +endef
> +
> diff --git a/include/asm-mips/asm-offset b/include/asm-mips/asm-offset
> new file mode 100644
> index 0000000..b2eb959
> --- /dev/null
> +++ b/include/asm-mips/asm-offset
> @@ -0,0 +1,4 @@
> +# Override default sed for MIPS when generating asm-offset
> +sed-$(CONFIG_MIPS) := "/^@@@/{s/^@@@//; s/ \#.*\$$//; p;}"
> +
> +
> diff --git a/Kbuild b/Kbuild
> index 0451f69..0a32f5f 100644
> --- a/Kbuild
> +++ b/Kbuild
> @@ -1,5 +1,8 @@
> #
> # Kbuild for top-level directory of the kernel
> +
> +
> +
> # This file takes care of the following:
> # 1) Generate asm-offsets.h
>
> @@ -7,36 +10,14 @@ #####
> # 1) Generate asm-offsets.h
> #
>
> +include include/asm-generic/asm-offset
> +
> offsets-file := include/asm-$(ARCH)/asm-offsets.h
>
> always := $(offsets-file)
> targets := $(offsets-file)
> targets += arch/$(ARCH)/kernel/asm-offsets.s
>
> -# Default sed regexp - multiline due to syntax constraints
> -define sed-y
> - "/^->/{s:^->\([^ ]*\) [\$$#]*\([^ ]*\) \(.*\):#define \1 \2 /* \3 */:; s:->::; p;}"
> -endef
> -# Override default regexp for specific architectures
> -sed-$(CONFIG_MIPS) := "/^@@@/{s/^@@@//; s/ \#.*\$$//; p;}"
> -
> -quiet_cmd_offsets = GEN $@
> -define cmd_offsets
> - (set -e; \
> - echo "#ifndef __ASM_OFFSETS_H__"; \
> - echo "#define __ASM_OFFSETS_H__"; \
> - echo "/*"; \
> - echo " * DO NOT MODIFY."; \
> - echo " *"; \
> - echo " * This file was generated by Kbuild"; \
> - echo " *"; \
> - echo " */"; \
> - echo ""; \
> - sed -ne $(sed-y) $<; \
> - echo ""; \
> - echo "#endif" ) > $@
> -endef
> -
> # We use internal kbuild rules to avoid the "is up to date" message from make
> arch/$(ARCH)/kernel/asm-offsets.s: arch/$(ARCH)/kernel/asm-offsets.c FORCE
> $(Q)mkdir -p $(dir $@)
Thanks.
____
On Fri, 2007-02-16 at 22:56 +0100, Oleg Verych wrote:
> Hallo.
lguest parts look good though!
Rusty.
On Sat, Feb 17, 2007 at 03:43:49PM +1100, Rusty Russell wrote:
> On Fri, 2007-02-16 at 22:56 +0100, Oleg Verych wrote:
> > Hallo.
>
> lguest parts look good though!
Thanks.
Well, then what about my way of doing generalization?
I.e. one-file shell script-let, rather many indistinguishable GNU make
files throughout source tree?
Sam's approach also offers using of CONGIG* options. But this has two
issues (for me, of course):
* lguest is in i386 arch, *top* Kbuild doing this for specific ARCH
(include/asm/);
* lguest wants to have private for-asm constants generated, unless CONFIG
is set, kbuild never will reach that directory.
Finally (my favorite). While i was told, that i'm doing more obfusticated
solutions, i doubt i did now: script-let makes magic more readable, and
GNU make's complications, like `$$' and various whitespace issues, do not
influence.
One thing is, that
+quiet_cmd_offsets = GEN $@
+ cmd_offsets = $(srctree)/scripts/mkCconstants $< $@
+
can be moved in Kbuild.include with CONFIG* upgrade:
....
sed-$(CONFIG_MIPS) = mips
....
cmd_offsets = $(srctree)/scripts/mkCconstants $< $@ $(sed-y)
Kind regards!
--
-o--=O`C info emacs : not found
#oo'L O info make : not found
<___=E M man gcc : not found