From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] x86/HVM: honor p2m_ram_ro in hvm_map_guest_frame_rw() Date: Fri, 24 Jul 2015 13:02:44 +0100 Message-ID: <55B22964.2030701@citrix.com> References: <55B224660200007800095083@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZIbgg-0003F5-8b for xen-devel@lists.xenproject.org; Fri, 24 Jul 2015 12:02:50 +0000 In-Reply-To: <55B224660200007800095083@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , xen-devel Cc: Kevin Tian , Wei Liu , suravee.suthikulpanit@amd.com, Eddie Dong , Aravind Gopalakrishnan , Jun Nakajima , Keir Fraser , Boris Ostrovsky List-Id: xen-devel@lists.xenproject.org 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 > --- > 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