Re: [PATCH 02/02] Handles broken page occurred during migration

[George Dunlap <george.dunlap@eu.citrix.com>]

On 31/10/12 12:21, Liu, Jinsong wrote:
> Handles broken page occurred during migration
>
> This patch handles broken page which occurred during migration.
>
> It monitors the critical area of live migration (from vMCE point of view,
> the copypages stage of migration is the critical area while other areas are not).
>
> If a vMCE occur at the critical area of live migration, it marks the broken page
> to dirty map, so that at copypages stage of migration, its pfn_type
> and pfn number would transfer to target and then take appropriate action.
>
> At target, it would set p2m as p2m_ram_broken for broken page, so that if
> guest access the broken page again, it would kill itself as expected.
>
> Suggested-by: George Dunlap <george.dunlap@eu.citrix.com>
> Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>

[snip]

> diff -r 3313ee9f6142 xen/arch/x86/cpu/mcheck/mce_intel.c
> --- a/xen/arch/x86/cpu/mcheck/mce_intel.c	Thu Oct 25 05:49:11 2012 +0800
> +++ b/xen/arch/x86/cpu/mcheck/mce_intel.c	Tue Oct 30 06:07:05 2012 +0800
> @@ -342,6 +342,22 @@
>                       goto vmce_failed;
>                   }
>   
> +                if ( unlikely(d->arch.vmce_monitor) )
> +                {
> +                    /*
> +                     * vMCE occur during migration
> +                     *
> +                     * mark broken page to dirty bitmap, so that at copypages
> +                     * stage of migration, its pfn_type and pfn number would
> +                     * transfer to target and then take appropriate action
> +                     *
> +                     * At target, it would set p2m as p2m_ram_broken for broken
> +                     * page, so that if guest access the broken page again, it
> +                     * would kill itself as expected.
> +                     */
> +                    paging_mark_dirty(d, mfn);
> +                }
> +

Given that logdirty is only enabled when we're doing some kind of 
migration (Remus or otherwise), wouldn't it make sense to just 
unconditionally call paging_mark_dirty(), rather than doing all this 
stuff with vmce_monitor?  paging_mark_dirty() is already a no-op if 
live-migration is not enabled; and if it is enabled, you probably want 
whoever is using logdirty to go through and deal with the broken page 
anyway.

So it seems like you could just add paging_mark_dirty() to the previous 
patch, and get rid of the rest of this patch entirely.

  -George