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

["Jiang, Yunhong" <yunhong.jiang@intel.com>]

Keir Fraser <mailto:keir.fraser@eu.citrix.com> wrote:
> On 19/03/2009 09:32, "Tim Deegan" <Tim.Deegan@eu.citrix.com> 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.

I will update it. In fact, I didn't find a proper name for it. Maybe something like MMU_FOREIGN_PT_UPDATE? But it may still be confused as update pt to point memory belongs to other domain.

> 
> 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.

I turn to the mmu_ext_op because it has only 1 entry left. So do you mean it is ok to be there?

- yhj

> 
> -- Keir