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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <JBeulich@suse.com>,
	xen-devel <xen-devel@lists.xenproject.org>
Cc: Kevin Tian <kevin.tian@intel.com>, Wei Liu <wei.liu2@citrix.com>,
	suravee.suthikulpanit@amd.com, Eddie Dong <eddie.dong@intel.com>,
	Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>,
	Jun Nakajima <jun.nakajima@intel.com>, Keir Fraser <keir@xen.org>,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>
Subject: Re: [PATCH] x86/HVM: honor p2m_ram_ro in hvm_map_guest_frame_rw()
Date: Fri, 24 Jul 2015 13:02:44 +0100	[thread overview]
Message-ID: <55B22964.2030701@citrix.com> (raw)
In-Reply-To: <55B224660200007800095083@prv-mh.provo.novell.com>

On 24/07/15 10:41, Jan Beulich wrote:
> ... and its callers.
>
> While all non-nested users are made fully honor the semantics of that
> type, doing so in the nested case seemed insane (if doable at all,
> considering VMCS shadowing), and hence there the respective operations
> are simply made fail.

Sorry, but I can't parse this sentence.  Surely in the nested case, it 
is the host p2m type which is relevant to whether a mapping should be 
forced read only?

>
> One case not (yet) taken care of is that of a page getting transitioned
> to this type after a mapping got established.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Beyond that log-dirty handling in _hvm_map_guest_frame() looks bogus
> too: What if a XEN_DOMCTL_SHADOW_OP_* gets issued and acted upon
> between the setting of the dirty flag and the actual write happening?
> I.e. shouldn't the flag instead be set in hvm_unmap_guest_frame()?

It does indeed.  (Ideally the dirty bit should probably be held high for 
the duration that a mapping exists, but that is absolutely infeasible to 
do).

This is definitely not the only issue between the p2m and logdirty. At 
some point I need to see about making migration safe to use while the 
guest is ballooning.  Both PV and HVM guests break in different ways if 
they actually perform ballooning or physmap alteration while logdirty is 
active for live migration purposes.

>
> Note that this is conflicting with the altp2m series (adding another
> call to hvm_map_guest_frame_rw(), reviewing of which made me spot the
> issue in the first place).
>
>
> @@ -3797,6 +3805,7 @@ static int hvm_load_segment_selector(
>               break;
>           }
>       } while ( !(desc.b & 0x100) && /* Ensure Accessed flag is set */
> +              writable && /* except if we are to discard writes */
>                 (cmpxchg(&pdesc->b, desc.b, desc.b | 0x100) != desc.b) );

I can't recall where I read it in the manual, but I believe it is a 
faultable error to load a descriptor from RO memory if the accessed bit 
is not already set.  This was to prevent a processor livelock when 
running with gdtr pointing into ROM (which was a considered usecase).

Otherwise, the rest of the patch looks fine.

~Andrew