Re: [Xen-devel] [v2 06/11] vmx: add help functions to support PML

[Tim Deegan <tim@xxxxxxx>]

At 11:32 +0800 on 17 Apr (1429270332), Kai Huang wrote:
> 
> 
> On 04/17/2015 08:10 AM, Tim Deegan wrote:
> > At 22:57 +0000 on 16 Apr (1429225024), Tian, Kevin wrote:
> >
> >>> From: Kai Huang [mailto:kai.huang@xxxxxxxxxxxxxxx]
> >>> +        if ( !p2m_change_type_one(v->domain, gfn, p2m_ram_logdirty,
> >>> +                    p2m_ram_rw) )
> >>> +            paging_mark_gfn_dirty(v->domain, gfn);
> >> Should we handle error from p2m_change_type_one and consequently
> >> making this flush function non-void?
> > I don't think we need to return an error, but we should probably
> > call mark_dirty here for anything except -EBUSY.
> Hi Kevin, Tim,
> 
> My intention here is to rule out the GFN with original type that is not 
> p2m_ram_logdirty, though with patch 11 it's not likely have such GFN 
> logged.
> 
> Looks -EBUSY returns exactly when original type is not p2m_ram_logdirty, 
> so I think it might be OK to do as Tim suggested.
> 
> But given the same thing has already been done in hap_track_dirty_vram 
> (hap_track_dirty_vram->paging_log_dirty_range, in which gfn is set in 
> bitmap with !p2m_change_type_one is true), and in EPT violation 
> (p2m_change_type_one is called unconditionally without checking return 
> value), I think it should be safe to do the current code here.

The paging_log_dirty_range case is doing something quite different:
it is making pages read-only so they can be tracked, and it needs to
mark any page that couldn't be made read-only (because the guest can
write to them).  Its three cases are:
 - change succeeded: no mark, we will trap any new writes
 - EBUSY: mark, since we can't be sure we'll trap new writes
 - other error: mark, since we can't be sure we'll trap new writes

In this case we _know_ the guest has written to the page (because it's
in the PML log), so our only reason for not calling mark_dirty() is
if we see that someone else has changed the p2m (EBUSY) and that
someone else ought to already have DTRT.

Now that I've written it out, and since we expect these races to be
very rare, I've changed my mind: we should _always_ call mark_dirty
here.  The extra cost should be negligible, and a missing mark is
extremely difficult to debug.

Cheers,

Tim.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel