From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gerd Knorr Subject: Re: [patch] pagetable cleanups Date: Thu, 14 Apr 2005 17:01:50 +0200 Message-ID: <20050414150150.GA28071@bytesex> References: <20050412185856.GA5832@bytesex> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Michael A Fetterman Cc: xen-devel@lists.xensource.com List-Id: xen-devel@lists.xenproject.org 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