From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Han, Huaitong" <huaitong.han@intel.com>
Subject: Re: [PATCH V6 4/5] xen/mm: Clean up pfec handling in
 gva_to_gfn
Date: Wed, 27 Jan 2016 07:22:24 +0000
Message-ID: <1453879352.4038.12.camel@intel.com>
References: <1453188659-8908-1-git-send-email-huaitong.han@intel.com>
	<1453188659-8908-5-git-send-email-huaitong.han@intel.com>
	<20160126143002.GB24854@deinos.phlegethon.org>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <20160126143002.GB24854@deinos.phlegethon.org>
Content-Language: en-US
Content-ID: <FBB4AB9F2E7CCE46A9F7434964489D86@intel.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: "tjd@phlegethon.org" <tjd@phlegethon.org>
Cc: "wei.liu2@citrix.com" <wei.liu2@citrix.com>, "george.dunlap@eu.citrix.com" <george.dunlap@eu.citrix.com>, "andrew.cooper3@citrix.com" <andrew.cooper3@citrix.com>, "george.dunlap@citrix.com" <george.dunlap@citrix.com>, "xen-devel@lists.xen.org" <xen-devel@lists.xen.org>, "jbeulich@suse.com" <jbeulich@suse.com>, "keir@xen.org" <keir@xen.org>
List-Id: xen-devel@lists.xenproject.org

On Tue, 2016-01-26 at 14:30 +0000, Tim Deegan wrote:
> Hi,
> 
> At 15:30 +0800 on 19 Jan (1453217458), Huaitong Han wrote:
> > At the moment, the pfec argument to gva_to_gfn has two functions:
> > 
> > * To inform guest_walk what kind of access is happenind
> > 
> > * As a value to pass back into the guest in the event of a fault.
> > 
> > Unfortunately this is not quite treated consistently: the
> > hvm_fetch_*
> > function will "pre-clear" the PFEC_insn_fetch flag before calling
> > gva_to_gfn; meaning guest_walk doesn't actually know whether a
> > given
> > access is an instruction fetch or not.  This works now, but will
> > cause
> > issues when pkeys are introduced, since guest_walk will need to
> > know
> > whether an access is an instruction fetch even if it doesn't return
> > PFEC_insn_fetch.
> > 
> > Fix this by making a clean separation for in and out
> > functionalities
> > of the pfec argument:
> > 
> > 1. Always pass in the access type to gva_to_gfn
> > 
> > 2. Filter out inappropriate access flags before returning from
> > gva_to_gfn.
> 
> This seems OK.  But can you please:
>  - Add this new adjustment once, in paging_gva_to_gfn(), instead of
>    adding it to each implementation; and
>  - Adjust the comment above the declaration of paging_gva_to_gfn() in
>    paging.h to describe this new behaviour.
Although adding adjustment in paging_gva_to_gfn can reduce code
duplication, adding it to each implementation is more readable, becasue
other sections of pfec are handled in each implementation.

> 
> Cheers,
> 
> Tim.