Kevin P. Lawton wrote:
>
> ftp://bochs.com/pub/freemware/prescan-2000_0222a.tar.gz
Kevin,
Great work !!!
I'm just browsing through the code, here are some initial comments.
First, the useless little details:
------
* Copyright (C) 2000 Free Software Foundation, Inc.
* 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA
FreeMWare isn't (C) FSF because we're not a GNU project.
We could become a GNU project, but right now we arent ;)
------
* Authors:
* Kevin P. Lawton (initial implementation)
I think we're better off leaving this off the comments
in the files, but have an AUTHORS file in stead, like
linux does (or was it called CONTRIBUTORS ?). That way
it's probably easier to see who's worked on a certain
part (i.e. is expected to fix it if it's broken ;))
------
Now we have a total of *three* instruction decoders
in FreeMWare --- Ulrich's decoder, BOCHS decoder,
and the custom decoder in kernel/emulation.c.
Isn't this a bit overkill ? Probably, one instruction
decoder should be more than enough. We should take
one decoder (perhaps the BOCHS one) and strip all
the unneccessary code to make it as fast as possible,
and consequently use it everywhere.
------
PS do you mind if I relayout the prescan code like what
most other FreeMWare code looks like ?
------
Diving into the code:
------
void prescan(u8 *modified, u8 *unmodified, u32 offset, unsigned seg32,
opcode_vmap_t *op_vmap, u8 *, unsigned depth);
Do we need to forward all of these as parameters ?
Like op_vmap, I'd assume it's the same for all invocations
of prescan()...
------
// Some special cases I've noted for whatever reasons.
// push esp, pop esp? 54, 5c
Hmm. Why does this need to be virtualised ?
// cmpxchg_xbts/ibts 0f a6/a7
// cmpxchg_e.g. 0f b0/b1
Not sure about this, but I think these only need
to be virtualised if
- the host processor has the F00F bug, and
- the instruction is F00F (LOCK CMPXCHG8B EAX)
Or am I missing something ?
// bswap_e.x 0f c8..cf
Hm. Why ?
I don't really know the other instructions on
that list, perhaps it would be a good idea to
elaborate on why they need to be caught.
------
// If fetch-decode ran up against a boundary and could not
// fetch enough bytes, or if the opcode is invalid, then mark
// the instruction as needing virtualization.
Looking at the earlier code, by "boundary" you mean
page boundary. Do we always need to trap the last
instruction on the page, if it doesn't fit ? In
principle it should be needed only once, when the
next page hasn't been scanned yet. I guess this
means that we need to "remember" that an instruction
was marked on a page boundary -- when it is executed,
the exception handler code needs to
(1) replace the original code
(2) scan the next page
There are more of these things (like branching outside
of the page). I guess we should write a prototype
exception handling code as well... perhaps I can work
on this in the weekend (if I have the time... always less
than I'd like :-/).
------
// We need to make sure that this instruction fits within the
// page boundary, otherwise we have to virtualize it.
I'm probably being stupid here, but how can this possibly
*not* be the case ? After all, we gave fetchDecode()
the max_len attribute above, so if it hits a page boundary
we should never get to this code anyway.
------
Now a general worry: using this algorithm, we need
*three* pages of memory per code page. Isn't this
going to be a terrible memory hog ? Actually, I guess
we can throw away the meta page when we're done
processing a page (probably easiest to allocate a page
permanently per VM, and to reuse it on every invocation
of prescan()) but then we're still using two times
as much memory as we actually need --- we can assume
there are way fewer modifications in a page than
"good" code. Can't we, in stead of using a whole
separate page, make the modifications directly in
the code page and copy the original code into a
hash table, or something similar ? What do you think?
-- Ramon
