Re: [Xen-devel] [patch] pagetable cleanups

[Gerd Knorr <kraxel@xxxxxxxxxxx>]

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