From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH 2/3] x86/mm: make {cmpxchg,
 write}_guest_entry() hook shadow mode specific
Date: Fri, 5 Feb 2016 14:59:21 +0000
Message-ID: <56B4B8C9.9060703@citrix.com>
References: <56B4628602000078000CED9E@prv-mh.provo.novell.com>
	<56B4650B02000078000CEDBB@prv-mh.provo.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta5.messagelabs.com ([195.245.231.135])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <prvs=8364524b4=Andrew.Cooper3@citrix.com>)
	id 1aRhr3-00072l-Gu
	for xen-devel@lists.xenproject.org; Fri, 05 Feb 2016 14:59:25 +0000
In-Reply-To: <56B4650B02000078000CEDBB@prv-mh.provo.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>, xen-devel <xen-devel@lists.xenproject.org>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>, Tim Deegan <tim@xen.org>, Keir Fraser <keir@xen.org>
List-Id: xen-devel@lists.xenproject.org

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