Messages in this thread | | | Subject | Re: [PATCH 1/5] lguest host feedback tidyups | From | Rusty Russell <> | Date | Mon, 14 May 2007 11:44:22 +1000 |
| |
On Sun, 2007-05-13 at 19:39 +0100, Al Viro wrote: > On Fri, May 11, 2007 at 11:19:14AM +1000, Rusty Russell wrote: > > @@ -218,7 +218,7 @@ u32 lgread_u32(struct lguest *lg, u32 ad > > > > /* Don't let them access lguest binary */ > > if (!lguest_address_ok(lg, addr, sizeof(val)) > > - || get_user(val, (u32 __user *)addr) != 0) > > + || get_user(val, (__force u32 __user *)addr) != 0) > > kill_guest(lg, "bad read address %u", addr); > > return val; > > *Ahem* > > What kind of address are we really getting there? IOW, where does it > ultimately come from?
Hi Al,
This patch has been superseded. But to clarify, the address generally comes from a guest register. My confusion came from sparse warnings on the above code like the following:
warning: cast removes address space of expression
I inserted __force, but it's actually caused by the "addr" being a "u32": if it's an "unsigned long" sparse doesn't warn. This is a win anyway: although the code is i386-specific at the moment, that will change. I prefer to use u32 rather than unsigned long to declare registers (eg. if I ever wanted to support 32 bit guests on a 64 bit host), but that's not even a consideration at this stage.
> > lock_cpu_hotplug(); > > if (cpu_has_pge) { /* We have a broader idea of "global". */ > > cpu_had_pge = 1; > > - on_each_cpu(adjust_pge, 0, 0, 1); > > + on_each_cpu(adjust_pge, (void *)0, 0, 1); > > That's called NULL...
Yes, but this is clearer. Here's adjust_pge:
static void adjust_pge(void *on) { if (on) write_cr4(read_cr4() | X86_CR4_PGE); else write_cr4(read_cr4() & ~X86_CR4_PGE); }
And here's the two calls to it:
on_each_cpu(adjust_pge, (void *)0, 0, 1); ... on_each_cpu(adjust_pge, (void *)1, 0, 1);
> > case LHCALL_LOAD_TLS: > > - guest_load_tls(lg, (struct desc_struct __user*)regs->edx); > > + guest_load_tls(lg, > > + (__force struct desc_struct __user*)regs->edx); > > Umm... That's borderline OK, but...
Yeah, gone in replacement patch.
> > static void push_guest_stack(struct lguest *lg, u32 __user **gstack, u32 val) > > { > > - lgwrite_u32(lg, (u32)--(*gstack), val); > > + lgwrite_u32(lg, (__force u32)--(*gstack), val); > > } > > Now, _that_ is just plain dumb. Why not declare that lgwrite_u32() as taking > u32 __user * as argument and kill the casts?
Last I tried, this turns out to create even more casts.
> > - lg->regs->esp = (u32)gstack + lg->page_offset; > > + lg->regs->esp = (__force u32)gstack + lg->page_offset; > > Yuck. Cast to unsigned long (or uintptr_t), please. In this case it is > legitimate.
Indeed, replacement patch uses unsigned long.
Thanks, Rusty.
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |