Re: [PATCH v3 7/8] x86/mm: update log-dirty bitmap when manipulating P2M

["Roger Pau Monné" <roger.pau@citrix.com>]

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 <jbeulich@suse.com>
> ---
> 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.