Re: [Xen-devel] [RFC] Nested Paging Live Migration

[Tim Deegan <Tim.Deegan@xxxxxxxxxxxxx>]

Hi, 

Thanks for this patch.

At 10:05 -0500 on 01 Jun (1180692316), Huang2, Wei wrote:
> The attached file supports AMD-V nested paging live migration. Please
> comment. I will create an updated version after collecting feedbacks.

Can a lot more log-dirty code (bitmap allocation, clearing, reporting)
be made common?  E.g.: hap_mark_dirty() is virtually identical to
sh_mark_dirty() -- including some recursive locking and associated
comments that are not true in HAP modes.  Maybe give it its own lock to
cover bit-setting?  Probably only the code for clearing the bitmap
(i.e., resetting the trap that will cause us to mark pages dirty) needs
to be split out.

> The following areas require special attention:
> 1. paging_mark_dirty()
> Currently, paging_mark_dirty() dispatches to sh_mark_dirty() or
> hap_mark_dirty() based on paging support. I personally prefer a function
> pointer. However, current paging interface only provides a function
> pointer for vcpu-level functions, not for domain-level functions. This
> is a bit annoying. 

Make it a common function and that should go away. 
  
> 2. locking in p2m_set_l1e_flags()
> p2m_set_l1e_flags(), which is invoked by hap.c, calls
> hap_write_p2m_entry(). hap_lock() is called twice. I currently remove
> hap_lock() in hap_write_p2m_entry(). A better solution is needed here.

Hmm.  Since you don't ever change the monitor table of a HAP domain, it
might be possible to make hap_write_p2m_entry (and
hap.c:p2m_install_entry_in_monitors()) safe without locking.

It is worth noting that this would be a different locking discipline
from the one used in shadow code -- code paths that take both the p2m
lock and the shadow lock always take the p2m lock first (there are some
convolutions in shadow init routines etc to make sure this is true).
If the hap lock is to be taken before the p2m lock that will need some
care and attention in the rest of the code.

> +int p2m_set_l1e_flags(struct domain *d, u32 l1e_flags)
> +{
[...]
> +    for ( entry = d->page_list.next;
> +          entry != &d->page_list;
> +          entry = entry->next )
> +    {

Why not just walk the p2m?  It shouldn't be very sparse.

> +/* This function handles P2M page faults by fixing l1e flags with correct 
> + * values.  It also calls paging_mark_dirty() function to record the dirty
> + * pages.
> + */
> +int p2m_fix_table(struct domain *d, paddr_t gpa)

Can this have a better name?  It's not really fixing anything. Maybe
have this be p2m_set_flags() and the previous function be
p2m_set_flags_global()?

Also maybe the call to mark_dirty could be made from the SVM code, which
is where we're really handling the write?

Cheers,

Tim.

-- 
Tim Deegan <Tim.Deegan@xxxxxxxxxxxxx>, XenSource UK Limited
Registered office c/o EC2Y 5EB, UK; company number 05334508

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