Re: [PATCH 2/2] x86/paging: Rename paging_mark_pfn_dirty() and use pfn_t

["Jan Beulich" <JBeulich@suse.com>]

>>> On 14.12.16 at 15:26, <andrew.cooper3@citrix.com> wrote:
> paging_mark_gfn_dirty() actually takes a pfn, even by paramter name.  Rename
> the function and alter the type to pfn_t to match.
> 
> Push pfn_t into the LOGDIRTY_IDX() macros, and clean up a couple of local
> variable types in paging_mark_pfn_dirty().
> 
> Leave an explicit comment in vmx_vcpu_flush_pml_buffer() when we intentally
> perform a straight conversion from gfn to pfn.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with two remarks:

> @@ -331,8 +331,8 @@ void paging_mark_gfn_dirty(struct domain *d, unsigned long pfn)
>      if ( changed )
>      {
>          PAGING_DEBUG(LOGDIRTY,
> -                     "marked mfn %" PRI_mfn " (pfn=%lx), dom %d\n",
> -                     mfn_x(mfn), pfn, d->domain_id);
> +                     "marked mfn %" PRI_mfn " (pfn %" PRI_pfn "), dom %d\n",
> +                     mfn_x(mfn), pfn_x(pfn), d->domain_id);

Mind making the domain part canonical (i.e. d%d), and perhaps
even moving it to the front ("d%d: ...\n")?

> @@ -345,23 +345,23 @@ void paging_mark_gfn_dirty(struct domain *d, unsigned long pfn)
>  /* Mark a page as dirty */
>  void paging_mark_dirty(struct domain *d, mfn_t gmfn)
>  {
> -    unsigned long pfn;
> +    pfn_t pfn;
>  
>      if ( !paging_mode_log_dirty(d) || !mfn_valid(gmfn) ||
>           page_get_owner(mfn_to_page(gmfn)) != d )
>          return;
>  
>      /* We /really/ mean PFN here, even for non-translated guests. */
> -    pfn = get_gpfn_from_mfn(mfn_x(gmfn));
> +    pfn = _pfn(get_gpfn_from_mfn(mfn_x(gmfn)));
>  
> -    paging_mark_gfn_dirty(d, pfn);
> +    paging_mark_pfn_dirty(d, pfn);
>  }

Looking at all of this, could patch 1 perhaps rename gmfn to mfn
in this function?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel