From: Rusty Russell <rusty@rustcorp.com.au>
To: Chris Malley <mail@chrismalley.co.uk>
Cc: lguest@ozlabs.org, Jes Sorensen <jes@sgi.com>,
virtualization@lists.linux-foundation.org
Subject: Re: [Lguest] [patch 43/43] lguest: generalize lgread_u32/lgwrite_u32.
Date: Sat, 29 Sep 2007 23:29:51 +1000 [thread overview]
Message-ID: <1191072591.26950.37.camel@localhost.localdomain> (raw)
In-Reply-To: <1190898264.6648.12.camel@feisty>
On Thu, 2007-09-27 at 14:04 +0100, Chris Malley wrote:
> Hi Rusty, Jes et al
>
> This last patch causes page_tables.c to fail compilation on my system thus:
Erk, thanks. That one wasn't cooked yet.
This should be better:
Subject: generalize lgread_u32/lgwrite_u32.
Jes complains that page table code still uses lgread_u32 even though
it now uses general kernel pte types. The best thing to do is to
generalize lgread_u32 and lgwrite_u32.
This means we lose the efficiency of getuser(). We could potentially
regain it if we used __copy_from_user instead of copy_from_user, but
I'm not certain that our range check is equivalent to access_ok() on
all platforms.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Acked-by: Jes Sorensen <jes@sgi.com>
---
drivers/lguest/core.c | 39 ++++++---------------------------
drivers/lguest/hypercalls.c | 2 -
drivers/lguest/i386_core.c | 4 +--
drivers/lguest/interrupts_and_traps.c | 2 -
drivers/lguest/lg.h | 23 ++++++++++++++++---
drivers/lguest/page_tables.c | 10 ++++----
drivers/lguest/segments.c | 4 +--
7 files changed, 38 insertions(+), 46 deletions(-)
===================================================================
--- a/drivers/lguest/core.c
+++ b/drivers/lguest/core.c
@@ -145,33 +145,10 @@ int lguest_address_ok(const struct lgues
return (addr+len) / PAGE_SIZE < lg->pfn_limit && (addr+len >= addr);
}
-/* This is a convenient routine to get a 32-bit value from the Guest (a very
- * common operation). Here we can see how useful the kill_lguest() routine we
- * met in the Launcher can be: we return a random value (0) instead of needing
- * to return an error. */
-u32 lgread_u32(struct lguest *lg, unsigned long addr)
-{
- u32 val = 0;
-
- /* Don't let them access lguest binary. */
- if (!lguest_address_ok(lg, addr, sizeof(val))
- || get_user(val, (u32 *)(lg->mem_base + addr)) != 0)
- kill_guest(lg, "bad read address %#lx: pfn_limit=%u membase=%p", addr, lg->pfn_limit, lg->mem_base);
- return val;
-}
-
-/* Same thing for writing a value. */
-void lgwrite_u32(struct lguest *lg, unsigned long addr, u32 val)
-{
- if (!lguest_address_ok(lg, addr, sizeof(val))
- || put_user(val, (u32 *)(lg->mem_base + addr)) != 0)
- kill_guest(lg, "bad write address %#lx", addr);
-}
-
-/* This routine is more generic, and copies a range of Guest bytes into a
- * buffer. If the copy_from_user() fails, we fill the buffer with zeroes, so
- * the caller doesn't end up using uninitialized kernel memory. */
-void lgread(struct lguest *lg, void *b, unsigned long addr, unsigned bytes)
+/* This routine copies memory from the Guest. Here we can see how useful the
+ * kill_lguest() routine we met in the Launcher can be: we return a random
+ * value (all zeroes) instead of needing to return an error. */
+void __lgread(struct lguest *lg, void *b, unsigned long addr, unsigned bytes)
{
if (!lguest_address_ok(lg, addr, bytes)
|| copy_from_user(b, lg->mem_base + addr, bytes) != 0) {
@@ -181,15 +158,15 @@ void lgread(struct lguest *lg, void *b,
}
}
-/* Similarly, our generic routine to copy into a range of Guest bytes. */
-void lgwrite(struct lguest *lg, unsigned long addr, const void *b,
- unsigned bytes)
+/* This is the write (copy into guest) version. */
+void __lgwrite(struct lguest *lg, unsigned long addr, const void *b,
+ unsigned bytes)
{
if (!lguest_address_ok(lg, addr, bytes)
|| copy_to_user(lg->mem_base + addr, b, bytes) != 0)
kill_guest(lg, "bad write address %#lx len %u", addr, bytes);
}
-/* (end of memory access helper routines) :*/
+/*:*/
/*H:030 Let's jump straight to the the main loop which runs the Guest.
* Remember, this is called by the Launcher reading /dev/lguest, and we keep
===================================================================
--- a/drivers/lguest/hypercalls.c
+++ b/drivers/lguest/hypercalls.c
@@ -47,7 +47,7 @@ static void do_hcall(struct lguest *lg,
char msg[128];
/* If the lgread fails, it will call kill_guest() itself; the
* kill_guest() with the message will be ignored. */
- lgread(lg, msg, args->arg1, sizeof(msg));
+ __lgread(lg, msg, args->arg1, sizeof(msg));
msg[sizeof(msg)-1] = '\0';
kill_guest(lg, "CRASH: %s", msg);
break;
===================================================================
--- a/drivers/lguest/i386_core.c
+++ b/drivers/lguest/i386_core.c
@@ -222,7 +222,7 @@ static int emulate_insn(struct lguest *l
return 0;
/* Decoding x86 instructions is icky. */
- lgread(lg, &insn, physaddr, 1);
+ insn = lgread(lg, physaddr, u8);
/* 0x66 is an "operand prefix". It means it's using the upper 16 bits
of the eax register. */
@@ -230,7 +230,7 @@ static int emulate_insn(struct lguest *l
shift = 16;
/* The instruction is 1 byte so far, read the next byte. */
insnlen = 1;
- lgread(lg, &insn, physaddr + insnlen, 1);
+ insn = lgread(lg, physaddr + insnlen, u8);
}
/* We can ignore the lower bit for the moment and decode the 4 opcodes
===================================================================
--- a/drivers/lguest/interrupts_and_traps.c
+++ b/drivers/lguest/interrupts_and_traps.c
@@ -45,7 +45,7 @@ static void push_guest_stack(struct lgue
{
/* Stack grows upwards: move stack then write value. */
*gstack -= 4;
- lgwrite_u32(lg, *gstack, val);
+ lgwrite(lg, *gstack, u32, val);
}
/*H:210 The set_guest_interrupt() routine actually delivers the interrupt or
===================================================================
--- a/drivers/lguest/lg.h
+++ b/drivers/lguest/lg.h
@@ -99,12 +99,27 @@ extern struct mutex lguest_lock;
extern struct mutex lguest_lock;
/* core.c: */
-u32 lgread_u32(struct lguest *lg, unsigned long addr);
-void lgwrite_u32(struct lguest *lg, unsigned long addr, u32 val);
-void lgread(struct lguest *lg, void *buf, unsigned long addr, unsigned len);
-void lgwrite(struct lguest *lg, unsigned long, const void *buf, unsigned len);
int lguest_address_ok(const struct lguest *lg,
unsigned long addr, unsigned long len);
+void __lgread(struct lguest *, void *, unsigned long, unsigned);
+void __lgwrite(struct lguest *, unsigned long, const void *, unsigned);
+
+/*L:306 Using memory-copy operations like that is usually inconvient, so we
+ * have the following helper macros which read and write a specific type (often
+ * an unsigned long).
+ *
+ * This reads into a variable of the given type then returns that. */
+#define lgread(lg, addr, type) \
+ ({ type _v; __lgread((lg), &_v, (addr), sizeof(_v)); _v; })
+
+/* This checks that the variable is of the given type, then writes it out. */
+#define lgwrite(lg, addr, type, val) \
+ do { \
+ typecheck(type, val); \
+ __lgwrite((lg), (addr), &(val), sizeof(val)); \
+ } while(0)
+/* (end of memory access helper routines) :*/
+
int run_guest(struct lguest *lg, unsigned long __user *user);
/* Helper macros to obtain the first 12 or the last 20 bits, this is only the
===================================================================
--- a/drivers/lguest/page_tables.c
+++ b/drivers/lguest/page_tables.c
@@ -209,7 +209,7 @@ int demand_page(struct lguest *lg, unsig
pte_t *spte;
/* First step: get the top-level Guest page table entry. */
- gpgd = __pgd(lgread_u32(lg, gpgd_addr(lg, vaddr)));
+ gpgd = lgread(lg, gpgd_addr(lg, vaddr), pgd_t);
/* Toplevel not present? We can't map it in. */
if (!(pgd_flags(gpgd) & _PAGE_PRESENT))
return 0;
@@ -235,7 +235,7 @@ int demand_page(struct lguest *lg, unsig
/* OK, now we look at the lower level in the Guest page table: keep its
* address, because we might update it later. */
gpte_ptr = gpte_addr(lg, gpgd, vaddr);
- gpte = __pte(lgread_u32(lg, gpte_ptr));
+ gpte = lgread(lg, gpte_ptr, pte_t);
/* If this page isn't in the Guest page tables, we can't page it in. */
if (!(pte_flags(gpte) & _PAGE_PRESENT))
@@ -281,7 +281,7 @@ int demand_page(struct lguest *lg, unsig
/* Finally, we write the Guest PTE entry back: we've set the
* _PAGE_ACCESSED and maybe the _PAGE_DIRTY flags. */
- lgwrite_u32(lg, gpte_ptr, pte_val(gpte));
+ lgwrite(lg, gpte_ptr, pte_t, gpte);
/* We succeeded in mapping the page! */
return 1;
@@ -369,12 +369,12 @@ unsigned long guest_pa(struct lguest *lg
pte_t gpte;
/* First step: get the top-level Guest page table entry. */
- gpgd = __pgd(lgread_u32(lg, gpgd_addr(lg, vaddr)));
+ gpgd = lgread(lg, gpgd_addr(lg, vaddr), pgd_t);
/* Toplevel not present? We can't map it in. */
if (!(pgd_flags(gpgd) & _PAGE_PRESENT))
kill_guest(lg, "Bad address %#lx", vaddr);
- gpte = __pte(lgread_u32(lg, gpte_addr(lg, gpgd, vaddr)));
+ gpte = lgread(lg, gpte_addr(lg, gpgd, vaddr), pte_t);
if (!(pte_flags(gpte) & _PAGE_PRESENT))
kill_guest(lg, "Bad address %#lx", vaddr);
===================================================================
--- a/drivers/lguest/segments.c
+++ b/drivers/lguest/segments.c
@@ -150,7 +150,7 @@ void load_guest_gdt(struct lguest *lg, u
kill_guest(lg, "too many gdt entries %i", num);
/* We read the whole thing in, then fix it up. */
- lgread(lg, lg->arch.gdt, table, num * sizeof(lg->arch.gdt[0]));
+ __lgread(lg, lg->arch.gdt, table, num * sizeof(lg->arch.gdt[0]));
fixup_gdt_table(lg, 0, ARRAY_SIZE(lg->arch.gdt));
/* Mark that the GDT changed so the core knows it has to copy it again,
* even if the Guest is run on the same CPU. */
@@ -161,7 +161,7 @@ void guest_load_tls(struct lguest *lg, u
{
struct desc_struct *tls = &lg->arch.gdt[GDT_ENTRY_TLS_MIN];
- lgread(lg, tls, gtls, sizeof(*tls)*GDT_ENTRY_TLS_ENTRIES);
+ __lgread(lg, tls, gtls, sizeof(*tls)*GDT_ENTRY_TLS_ENTRIES);
fixup_gdt_table(lg, GDT_ENTRY_TLS_MIN, GDT_ENTRY_TLS_MAX+1);
lg->changed |= CHANGED_GDT_TLS;
}
next prev parent reply other threads:[~2007-09-29 13:29 UTC|newest]
Thread overview: 65+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-09-26 6:36 [patch 00/43] lguest: Patches for 2.6.24 (and patchbomb test) rusty
2007-09-26 6:36 ` [patch 01/43] lguest: lguest example launcher truncates block device file to 0 length on problems rusty
2007-09-26 6:36 ` [patch 02/43] lguest: fix modules oopsing in lguest guests rusty
2007-09-26 6:36 ` [patch 03/43] lguest: Normalize config options for guest support rusty
2007-09-26 6:36 ` [patch 04/43] lguest: Consolidate host virtualization support under Virtualization menu rusty
2007-09-26 6:36 ` [patch 05/43] lguest: Example launcher should include asm/e820.h instead of asm-i386/ rusty
2007-09-26 6:36 ` [patch 06/43] lguest: turn err into errx in lguest call sites rusty
2007-09-26 6:36 ` [patch 07/43] lguest: Use copy_to_user() not put_user for struct timespec rusty
2007-09-26 6:36 ` [patch 08/43] lguest: Lguest currently depends on 32-bit x86, not just x86 rusty
2007-09-26 6:36 ` [patch 09/43] lguest: lguest.txt update rusty
2007-09-26 6:36 ` [patch 10/43] lguest: Make lguest_launcher.h types userspace-friendly rusty
2007-09-26 6:36 ` [patch 11/43] lguest: lguest_devices belongs in lguest_bus.c: its not i386-specific rusty
2007-09-26 6:36 ` [patch 12/43] lguest: Only start khvcd when someone uses hvc_console driver rusty
2007-09-26 6:36 ` [patch 13/43] lguest: Move lguest hcalls to arch-specific header rusty
2007-09-26 6:36 ` [patch 14/43] lguest: Move lguest guest support to arch/i386 where it logically belongs rusty
2007-09-26 6:36 ` [patch 15/43] lguest: Rename switcher.S to i386_switcher.S, since its very i386-specific rusty
2007-09-26 6:36 ` [patch 16/43] lguest: Accept elf files that are valid but have sections that can not be mmaped for some reason rusty
2007-09-26 6:36 ` [patch 17/43] lguest: Introduce guest mem offset, static link example launcher rusty
2007-09-26 6:36 ` [patch 18/43] lguest: Remove fixed limit on number of guests, and lguests array rusty
2007-09-26 6:36 ` [patch 19/43] lguest: Make shadow IDT a complete IDT with 256 entries rusty
2007-09-26 6:36 ` [patch 20/43] lguest: Move i386 part of core.c to i386_core.c rusty
2007-09-26 6:36 ` [patch 21/43] lguest: Reorder guest saved regs to match hyperall order rusty
2007-09-26 6:36 ` [patch 22/43] lguest: Introduce "hcall" pointer to indicate pending hypercall rusty
2007-09-26 6:36 ` [patch 23/43] lguest: Make hypercalls arch-independent rusty
2007-09-26 6:36 ` [patch 24/43] lguest: Change example launcher to use unsigned long not u32 rusty
2007-09-26 6:36 ` [patch 25/43] lguest: Move register setup into i386_core.c rusty
2007-09-26 6:36 ` [patch 26/43] lguest: guest.h declares a struct timespec, make it include linux/time.h rusty
2007-09-26 6:36 ` [patch 27/43] lguest: Pagetables to use normal kernel types rusty
2007-09-26 6:36 ` [patch 28/43] lguest: Rename "cr3" to "gpgdir" to avoid x86-specific naming rusty
2007-09-26 6:36 ` [patch 29/43] lguest: Introduce "used_vectors" bitmap which can be used to reserve vectors rusty
2007-09-26 6:36 ` [patch 30/43] lguest: Allow guest to specify syscall vector to use rusty
2007-09-26 6:36 ` [patch 31/43] lguest: Boot with virtual == physical to get closer to native Linux rusty
2007-09-27 0:12 ` Jeremy Fitzhardinge
2007-09-27 0:53 ` [Lguest] " ron minnich
2007-09-29 13:02 ` Rusty Russell
2007-09-26 6:36 ` [patch 32/43] lguest: Virtio interface rusty
2007-10-02 9:03 ` Christian Borntraeger
2007-10-02 12:00 ` Rusty Russell
2007-10-10 8:50 ` Christian Borntraeger
2007-10-10 13:43 ` Glauber de Oliveira Costa
2007-10-10 14:24 ` Arnd Bergmann
2007-10-10 15:31 ` Eric Van Hensbergen
2007-10-10 16:00 ` Arnd Bergmann
2007-10-11 14:17 ` Rusty Russell
2007-09-26 6:36 ` [patch 33/43] lguest: Net driver using virtio rusty
2007-09-26 6:36 ` [patch 34/43] lguest: Block " rusty
2007-09-28 11:32 ` [Lguest] " Chris Malley
2007-09-29 13:26 ` Rusty Russell
2007-09-26 6:36 ` [patch 35/43] lguest: Virtio console driver rusty
2007-09-26 6:36 ` [patch 36/43] lguest: Module autoprobing support for virtio drivers rusty
2007-09-26 6:36 ` [patch 37/43] lguest: Virtio helper routines for a descriptor ringbuffer implementation rusty
2007-09-30 17:03 ` Avi Kivity
2007-10-01 12:03 ` Rusty Russell
2007-10-01 12:13 ` Avi Kivity
2007-10-02 4:21 ` Rusty Russell
2007-10-02 6:02 ` Avi Kivity
2007-09-26 6:36 ` [patch 38/43] lguest: This gets rid of the lguest bus, drivers and DMA mechanism, to make way for a generic virtio mechanism rusty
2007-09-26 6:36 ` [patch 39/43] lguest: This patch gets rid of the old lguest host I/O infrastructure and replaces it with a single hypercall "LHCALL_NOTIFY" which takes an address rusty
2007-09-26 6:36 ` [patch 40/43] lguest: Lguest support for Virtio rusty
2007-09-26 6:36 ` [patch 41/43] lguest: Update example launcher for virtio rusty
2007-09-26 6:37 ` [patch 42/43] lguest: Example launcher handle guests not being ready for input rusty
2007-09-26 6:37 ` [patch 43/43] lguest: generalize lgread_u32/lgwrite_u32 rusty
2007-09-27 13:04 ` [Lguest] " Chris Malley
2007-09-29 13:29 ` Rusty Russell [this message]
2007-10-09 20:25 ` [Lguest] [patch 00/43] lguest: Patches for 2.6.24 (and patchbomb test) Eric Van Hensbergen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1191072591.26950.37.camel@localhost.localdomain \
--to=rusty@rustcorp.com.au \
--cc=jes@sgi.com \
--cc=lguest@ozlabs.org \
--cc=mail@chrismalley.co.uk \
--cc=virtualization@lists.linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).