Retry.

1. Most common code are moved from shadow to paging:
* log dirty related fields (dirty_count ...) are moved to paging_domain
* log_dirty_bitmap allocation, free, peek, and clean
* mark_dirty_page becomes a common function too
* a new lock dirty lock is created to guard these code

2. shadow/hap_log_dirty_enable() and shadow/hap_log_dirty_disable()
These four functions were not changed. However, I really want to create 
two common functions (paging_log_dirty_disable() and 
paging_log_dirty_enable()) for them. To do this, it requires two 
function pointers (*log_dirty_enable() and *log_dirty_disable()), which 
point to shadow-specific code or hap-specific code. For example, 
*log_dirty_enable() points to shadow_log_dirty_enable().

Tim, let me know if you like this approach.

3. p2m_set_l1e_flags() is renamed to p2m_set_flags_global() as 
requested. It does NOT walk P2M. Instead, it still relies on 
set_p2m_entry() to walk P2M table.

The reason: I feel uncomfortable to duplicate the code of 
set_p2m_entry() in this method. Most of them will be same as 
set_p2m_entry() and p2m_next_level(). What is your opinion?


Any comments is welcome. I will create a new patch after collecting them.

Thanks,

-Wei


Tim Deegan wrote:
> 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.
> 
> 
>  > +/* 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@xensource.com>, XenSource UK Limited
> Registered office c/o EC2Y 5EB, UK; company number 05334508
> 
>