On Thu, Apr 14, 2005 at 01:25:19PM +0100, Michael A Fetterman wrote:
> 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?
Yes, thats easy ;)
> 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.
Ah, ok.
> I'd like to just remove the halt you added there, OK?
Fine with me, I've added your changes instead.
> 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.
Ah, *thats* the point of these beasts. The page manipulations done on
them look like l2 operations (well, they actually are as they really
point to l1 pages), that confused me ;)
> 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.
Yep, I noticed that as well as the PAE versions became a bit more
complex and I tried to make them inline functions instead which didn't
work ...
I can change them to pass-by-reference instead, it's probably a good
idea. Hope gcc is clever enougth to see that it's the same after all
and doesn't generate extra code then.
> 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...
Yes, was intentionally. I think that isn't bad, it makes the code more
readable. And I think it actually is impossible to return structs in C,
you can only return a pointer to a struct, which would't help for the
"building entries as expressions" case.
> 5) I found a couple compilation problems when by compiling with debug=y...
Merged, thanks.
Current patch set is at http://dl.bytesex.org/patches/xen-2/ now
(issue #4 isn't adressed yet).
Gerd
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home