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.]
swap_page.patch
Description: swap_page.patch
free_page.patch
Description: free_page.patch
writable_p2m.patch
Description: writable_p2m.patch
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home