[Xen-devel] Re: [PATCH] Support swap a page from user space tools -- Was RE: [RFC][PATCH] Basic support for page offline

[Keir Fraser <keir.fraser@xxxxxxxxxxxxx>]

On 19/03/2009 09:32, "Tim Deegan" <Tim.Deegan@xxxxxxxxxxxxx> 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



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel