Overall, I think the patch looks pretty good...
A couple of comments:
1) There's no Signed-Off-By comment attached to this. Could you please
provide one?
2) About your question at the bottom of construct_dom0:
The current code there is intended to allow booting of dom0's with
translate mode
enabled. As such, it probably won't stay in the code base forever, but
it was and
is a useful hack. The bringup process for this new shadow code was to
get dom0's
working first, and we're now working on (cleaner) domU support. I'm not
too
worried about x86_64 or pae support for this dom0 translate mode support.
I'd
like to just remove the halt you added there, OK?
3) HL2 tables are not tables of L2 entries. They contain L1 entries.
They are essentially shadows of guest L2 pages, which will be used by Xen
to get
a linear-pagetable-like mapping of the guest's L1 pages.
A normal guest L2 has guest-physical pages in it.
A normal shadow L2 points at shadows of the guest's L1s.
An HL2 has machine addresses of the guest's L1 pages in it, and is *used*
as an
L1 table by Xen. So the things like "l1_pgentry_t *hl2_vtable" in
domain.h were
not typos, and should stay the way they were...
The HL2 is a concept that works well for the simple 2 level page tables,
and is a
clever solution to avoid doing lots of extra map_domain_mem() calls to
access the
guest's L1 tables, but it both falls apart and is fortunately unnecessary
for
64-bit mode. I haven't thought much about PAE yet, but HL2 are probably
still useful there because of the cost of map_domain_mem()...
4) There was probably a bunch of debate about this somewhere before, but I
missed it.
The macros which set/clear page table types don't obey C's pass-by-value
calling
semantics. That means that they can't be replaced with simple functions,
if
desired -- there would always have to be a macro layer. There's also no
macros
for creating L1E or L2E as expressions -- only statements which assign
them.
Perhaps this was intentional? It means that you end up declaring extra
variables to hold essentially temporary values in a few places...
Comments?
5) I found a couple compilation problems when by compiling with debug=y...
I've dealt with issues #2, 3, and 5 in a slightly modified version of your
patch,
available at http://www.cl.cam.ac.uk/~maf46/kraxel.patch.v2
Take a look, and let me know what you think.
Michael
-----Original Message-----
From: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
[mailto:xen-devel-bounces@xxxxxxxxxxxxxxxxxxx] On Behalf Of Gerd Knorr
Sent: Tuesday, April 12, 2005 7:59 PM
To: xen-devel@xxxxxxxxxxxxxxxxxxx
Subject: [Xen-devel] [patch] pagetable cleanups
Hi,
Next version of the pagetable cleanup patch. Builds and boots
domain 0 on x86_32. Changes from the last version:
* macro names are changed.
* adapted to the new shadow code checked in last week.
* new macro: l1e_has_changed() to compare page table
entries.
Open issues:
* I'm not sure how to handle the debug printk's best. These use
the l1e_get_value() macro at the moment to get the raw bits and
print them as unsigned long hex value. I'd like to get rid of
the l1e_get_value() macro altogether though ...
* x86_64 build needs fixing, will look into this tomorrow.
Enjoy,
Gerd
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home