From mboxrd@z Thu Jan 1 00:00:00 1970 From: Keir Fraser Subject: Re: [PATCH] Support swap a page from user space tools -- Was RE: [RFC][PATCH] Basic support for page offline Date: Thu, 19 Mar 2009 09:45:54 +0000 Message-ID: References: <20090319093245.GG11733@york.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20090319093245.GG11733@york.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Tim Deegan , "Jiang, Yunhong" Cc: "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org On 19/03/2009 09:32, "Tim Deegan" wrote: >>> - You're passing a physical address (of the PTE to update) in an MFN >>> field. That's not going to be big enough on all platforms. Also it's >>> pretty confusing. >> >> Yes, fixed and now named pte_addr as a uint64. > > You made it an unsigned long, which is still smaller than a paddr_t on > PAE builds. And you can't just make it 64 bits in that union without > breaking the ABI; you'll need to add a new interface somewhere. Maybe > Keir can suggest a better place. Firstly, the comment added to the header file is pretty rubbish. The description fits existing update methods such as MMU_NORMAL_PT_UPDATE, so based on that comment I could quite reasonably reject your patch on grounds that it is redundant. Secondly, the patch name says swap_page and a printk the patch adds refers to 'swap page'. What's being swapped? That's not the name of the operation, nor is swapping referred to in the description comment I mention above. Thirdly, perhaps this makes more sense as a MMU_* op hanging off mmu_update()? That call takes pairs of u64 values, which could give you the space you require. Then you can add a nice comment explaining how your new command differs from MMU_NORMAL_PT_UPDATE. -- Keir