[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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@xxxxxxxx>
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(
      } 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.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.