Re: [PATCH 2/3] x86/mm: make {cmpxchg, write}_guest_entry() hook shadow mode specific

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 05/02/16 08:02, Jan Beulich wrote:
> ... as they're being used for PV guests only, which don't use HAP mode.
> This eliminates another pair of NULL callbacks in HAP as well as in 2-
> and 3-guest-level shadow modes.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, but with two
suggestions.

> @@ -299,14 +299,15 @@ static inline void paging_update_paging_
>  /* Write a new value into the guest pagetable, and update the
>   * paging-assistance state appropriately.  Returns 0 if we page-faulted,
>   * 1 for success. */
> -static inline int paging_write_guest_entry(struct vcpu *v, intpte_t *p,
> -                                           intpte_t new, mfn_t gmfn)
> +static inline bool_t paging_write_guest_entry(struct vcpu *v, intpte_t *p,
> +                                              intpte_t new, mfn_t gmfn)
>  {
> -    if ( unlikely(paging_mode_enabled(v->domain) 
> -                  && v->arch.paging.mode != NULL) )
> -        return paging_get_hostmode(v)->write_guest_entry(v, p, new, gmfn);
> -    else 
> -        return (!__copy_to_user(p, &new, sizeof(new)));
> +#ifdef CONFIG_SHADOW_PAGING
> +    if ( unlikely(paging_mode_shadow(v->domain)) && paging_get_hostmode(v) )
> +        return paging_get_hostmode(v)->shadow.write_guest_entry(v, p, new,
> +                                                                gmfn);

I would be tempted to split the line at the -> in this case, to avoid
squashing all the parameters together on the RHS like that, but I
suppose it doesn't matter too much.

> +#endif
> +    return !__copy_to_user(p, &new, sizeof(new));

I realise that the old function behaved the same, but this really should
ASSERT(v == current) before trampling the wrong virtual address space.

~Andrew