Re: [PATCH 2/2] x86/paging: tidy paging_mfn_is_dirty()

[Andrew Cooper <amc96@xxxxxxxx>]

On 01/12/2021 10:35, Jan Beulich wrote:
> The function returning a boolean indicator, make it return bool. Also
> constify its struct domain parameter, albeit requiring to also adjust
> mm_locked_by_me(). Furthermore the function is used by shadow code only.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> --- a/xen/arch/x86/mm/mm-locks.h
> +++ b/xen/arch/x86/mm/mm-locks.h
> @@ -40,7 +40,7 @@ static inline void mm_lock_init(mm_lock_
>      l->unlock_level = 0;
>  }
>  
> -static inline int mm_locked_by_me(mm_lock_t *l)
> +static inline int mm_locked_by_me(const mm_lock_t *l)

bool too?

>  {
>      return (l->lock.recurse_cpu == current->processor);
>  }
> --- a/xen/arch/x86/mm/paging.c
> +++ b/xen/arch/x86/mm/paging.c
> @@ -351,14 +351,14 @@ void paging_mark_dirty(struct domain *d,
>      paging_mark_pfn_dirty(d, pfn);
>  }
>  
> -
> +#ifdef CONFIG_SHADOW_PAGING
>  /* Is this guest page dirty? */
> -int paging_mfn_is_dirty(struct domain *d, mfn_t gmfn)
> +bool paging_mfn_is_dirty(const struct domain *d, mfn_t gmfn)
>  {
>      pfn_t pfn;
>      mfn_t mfn, *l4, *l3, *l2;
>      unsigned long *l1;
> -    int rv;
> +    bool dirty;
>  
>      ASSERT(paging_locked_by_me(d));
>      ASSERT(paging_mode_log_dirty(d));
> @@ -367,36 +367,37 @@ int paging_mfn_is_dirty(struct domain *d
>      pfn = _pfn(get_gpfn_from_mfn(mfn_x(gmfn)));

There's nothing inherently paging.c related about this function. 
Thoughts on moving the implementation across, rather than #ifdef-ing it?

If not, can we at least correct gmfn to mfn here and in the prototype?

Alternatively, do we want to start putting things like this in a real
library so we can have the toolchain figure this out automatically?

>      /* Invalid pages can't be dirty. */
>      if ( unlikely(!VALID_M2P(pfn_x(pfn))) )
> -        return 0;
> +        return false;
>  
>      mfn = d->arch.paging.log_dirty.top;
>      if ( !mfn_valid(mfn) )

These don't need to be mfn_valid().  They can be checks against
MFN_INVALID instead, because the logdirty bitmap is a Xen internal
structure.

In response to your comment in the previous patch, this would
substantially reduce the overhead of paging_mark_pfn_dirty() and
paging_mfn_is_dirty().

~Andrew