On Tue, Apr 26, 2022 at 12:26:10PM +0200, Jan Beulich wrote:
> Just like for PV guests MMU_MACHPHYS_UPDATE implies marking of the
> respective page as dirty, additions to a HVM guest's P2M should do so.
>
> For HVM the opposite is also true: Pages being removed from the P2M are
> no longer dirty at their prior GFN; there's no point in telling the tool
> stack to try and copy that page, when this will fail anyway (until
> perhaps a new page gets placed there). Introduce paging_mark_pfn_clean()
> (intentionally without a paging_mark_clean() counterpart) to handle
> this. Note that while there is an earlier call to set_gpfn_from_mfn() in
> guest_physmap_add_entry(), but there's little reason to mark the page
> clean there when later in the function it'll be marked dirty. This is
> even more so given that at this point it's only the M2P that gets
> updated, with the P2M still left unchanged.
>
> Signed-off-by: Jan Beulich <[email protected]>
> ---
> p2m_add_page()'s error handling looks bogus in this regard anyway: If an
> error occurs before an MFN actually is assciated with the new GFN, the
> M2P entry ought to be restored imo. But of course a guest is still hosed
> if the operation succeeds partially.
>
> Note that I've not even checked mem-paging and mem-sharing code for
> whether they may need similar adjustment. At least the latters is, aiui,
> incompatible with log-dirty mode anyway.
> ---
> v3: Re-base.
>
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -549,7 +549,10 @@ p2m_remove_entry(struct p2m_domain *p2m,
> {
> p2m->get_entry(p2m, gfn_add(gfn, i), &t, &a, 0, NULL, NULL);
> if ( !p2m_is_special(t) && !p2m_is_shared(t) )
> + {
> set_gpfn_from_mfn(mfn_x(mfn) + i, INVALID_M2P_ENTRY);
> + paging_mark_pfn_clean(p2m->domain, _pfn(gfn_x(gfn) + i));
> + }
> }
> }
>
> @@ -737,8 +740,11 @@ p2m_add_page(struct domain *d, gfn_t gfn
> if ( !p2m_is_grant(t) )
> {
> for ( i = 0; i < (1UL << page_order); i++ )
> + {
> set_gpfn_from_mfn(mfn_x(mfn_add(mfn, i)),
> gfn_x(gfn_add(gfn, i)));
> + paging_mark_pfn_dirty(d, _pfn(gfn_x(gfn) + i));
Have you considered placing the respective
paging_mark_pfn_{clean,dirty}() calls in p2m_entry_modify()?
There's a lot of repetition here with regard to handling the side
effects of p2m changes that are forced into the callers, that could
likely be contained inside of p2m_entry_modify() at first sight.
> --- a/xen/arch/x86/include/asm/paging.h
> +++ b/xen/arch/x86/include/asm/paging.h
> @@ -165,8 +165,9 @@ void paging_log_dirty_init(struct domain
>
> /* mark a page as dirty */
> void paging_mark_dirty(struct domain *d, mfn_t gmfn);
> -/* mark a page as dirty with taking guest pfn as parameter */
> +/* mark a page as dirty/clean with taking guest pfn as parameter */
I think it would be clearer to use gfn here rather than "guest pfn",
and the function parameter should be "gfn_t gfn".
Thanks, Roger.
