Re: [PATCH] x86/HVM: honor p2m_ram_ro in hvm_map_guest_frame_rw()

From: Wei Liu <wei.liu2@citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: KevinTian <kevin.tian@intel.com>, Wei Liu <wei.liu2@citrix.com>,
	suravee.suthikulpanit@amd.com,
	AndrewCooper <andrew.cooper3@citrix.com>,
	EddieDong <eddie.dong@intel.com>,
	Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>,
	Jun Nakajima <jun.nakajima@intel.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>,
	Keir Fraser <keir@xen.org>
Subject: Re: [PATCH] x86/HVM: honor p2m_ram_ro in hvm_map_guest_frame_rw()
Date: Fri, 24 Jul 2015 11:41:09 +0100	[thread overview]
Message-ID: <20150724104109.GC30594@zion.uk.xensource.com> (raw)
In-Reply-To: <55B231850200007800095110@prv-mh.provo.novell.com>

On Fri, Jul 24, 2015 at 04:37:25AM -0600, Jan Beulich wrote:
> >>> On 24.07.15 at 12:26, <wei.liu2@citrix.com> wrote:
> > On Fri, Jul 24, 2015 at 03:41:26AM -0600, Jan Beulich wrote:
> >> @@ -3618,7 +3618,12 @@ static void *__hvm_map_guest_frame(unsig
> >>      }
> >>  
> >>      if ( writable )
> > 
> > I don't claim I know this piece of code, but checking the pointer but
> > not the content looks suspicious.
> > 
> >> -        paging_mark_dirty(d, page_to_mfn(page));
> >> +    {
> >> +        if ( !p2m_is_discard_write(p2mt) )
> >> +            paging_mark_dirty(d, page_to_mfn(page));
> >> +        else
> >> +            *writable = 0;
> > 
> > You then set *writable here, which makes it even more suspicious.
> 
> Why? A caller _wanting_ a writable mapping passes non-NULL as
> the pointer argument (pre-initialized to point to a variable holding
> TRUE aka 1). Upon return the variable will have got set to FALSE
> aka 0 if the page shouldn't be written to.
> 

If this is the convention for using this function then the code you
write is of course fine.

Wei.

> Jan