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

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

Tim, thanks for review very much. Attached is the new version. See below for 
comments.

Thanks
Yunhong Jiang

Tim Deegan <mailto:Tim.Deegan@xxxxxxxxxx> wrote:
> Hi,
> 
> At 10:24 +0000 on 18 Mar (1237371884), Jiang, Yunhong wrote:
>> Tim, this is the implementation as discussed.
>> The swap_page.patch is for HV side change, basically it call
> the mod_lx_entry to update the table.
> 
> That seems good.  One or two nits with that patch:
> 
> - 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.

> 
> - The "swap" operation takes the physical address of a PTE and a new
>   MFN.  Why not a new PTE?  And if it's going to be called "swap", why
>   not take the old PTE too and do an atomic update?  (or just call it
>   something else and only take the new PTE?)

Yes, I change the name to update_pte, and pass only the new PTE.

> 
> - Why the checks for the validity of the old MFN before the change?
>   What are you guarding against?

Yes, seems it is meaningless.

> 
> And please document the hypercall, specially since its side-effects could
> be quite surprising. 
> 
>> The free_page.patch is the function from user space tools to offlien a
>> page. 
> 
> This is much better than the previous version, thanks.

I missed one thing in previous patch, i.e. the changes to 
xc_core_arch_map_p2m(). 
Originally I change that function to map the p2m table as rw (it is forgoted in 
previous mail). Now I add a new function xc_core_arch_map_p2m_writable() so 
that not break the original API.

But I'm a bit confused why the xc_domain_save.c will not use this function to 
map p2m table also? Instead,  I noticed a lot of duplicate on these two files, 
I can send out a clean patch in future if it is ok.

Thanks
-- Yunhong Jiang

> 
> Cheers,
> 
> Tim.
> 
>> 
>> Thanks
>> Yunhong Jiang
>> 
>> Jiang, Yunhong <> wrote:
>>> Tim Deegan <mailto:Tim.Deegan@xxxxxxxxxx> wrote:
>>>> At 14:37 +0000 on 19 Feb (1235054276), Jiang, Yunhong wrote:
>>>>> 
>>>>>>> It should be possible to have the tools do all the PTE manipulations
>>>>>>> with MMU update hypercalls (I think -- Keir may correct me here). Then
>>>>>>> the final hypercall to surrender the page will fail if the grant
>>>>>>> tables are wrong; if it does, put the PTEs back and fall back to a
>>>>>>> live migration.  Isn't that what your in-xen code does anyway?
>>>>> 
>>>>> Tim, after checking the code more carefully, seems currently
>>>> the MMU update hypercalls (including mod_lx_entry ) assume it
>>>> is for current domain, while in our usage model, it will
>>>> update MMU for other domain, so I will try to do following
>>>> changes: 1) change mod_lx_entry() to get a domain parameter 2)
>>>> Add a new hypercall (or a new command to do_mmu_update ) to
>>>> update the MMU for other domain. I'm not sure if there are
>>>> other usage model for such requirement, and if such changes
>>>> acceptable? Any feedback is welcome.
>>>>> 
>>>> 
>>>> Sorry for the delay -- I was travelling around the summit and this got
>>>> lost.  Yes, I think this is an OK approach.  I doubt there will be many
>>>> other users of such a hypercall since most OSes will get upset by their
>>>> PTEs changing under their feet, but I prefer it to the current patch.
>>> 
>>> Sure, I will do this way.
>>> 
>>> Thanks
>>> Yunhong Jiang
>>> 
>>>> 
>>>> Cheers,
>>>> 
>>>> Tim.
>>>> 
>>>> --
>>>> Tim Deegan <Tim.Deegan@xxxxxxxxxx>
>>>> Principal Software Engineer, Citrix Systems (R&D) Ltd.
>>>> [Company #02300071, SL9 0DZ, UK.]
> 
> 
> 
> --
> Tim Deegan <Tim.Deegan@xxxxxxxxxx>
> Principal Software Engineer, Citrix Systems (R&D) Ltd.
> [Company #02300071, SL9 0DZ, UK.]

Attachment: swap_page.patch
Description: swap_page.patch

Attachment: free_page.patch
Description: free_page.patch

Attachment: writable_p2m.patch
Description: writable_p2m.patch

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