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

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





On 04/17/2015 04:36 PM, Tim Deegan wrote:
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).
Thanks for comprehensive reply. However, looks I can't agree on some points.

paging_log_dirty_range currently is only used by hap_track_dirty_vram for video ram tracking, and it doesn't call paging_mark_dirty in any case. Basically, paging_log_dirty_range only does below thing but nothing else:

    for ( i = 0, pfn = begin_pfn; pfn < begin_pfn + nr; i++, pfn++ )
        if ( !p2m_change_type_one(d, pfn, p2m_ram_rw, p2m_ram_logdirty) )
            dirty_bitmap[i >> 3] |= (1 << (i & 7));

From which we can see the purpose of this function (and let us put PML away for now):

- change GFN's type from p2m_ram_rw back to p2m_ram_logdirty, in order to be able to log the GFN again (more precisely, to track the GFN again in EPT violation), with the fact that the dirty page's p2m type has been changed from p2m_log_dirty to p2m_ram_rw, in EPT violation. - mark the dirty GFN to the bitmap only when above changing p2m_ram_logdirty to p2m_ram_rw is done successfully. It is reasonable, as only a successful changing from p2m_ram_rw to p2m_ram_dirty means the dirty page has been changed from p2m_ram_logdirty to p2m_ram_rw in EPT violation.

Btw, from which we can also note that currently video ram tracking is not via log-dirty radix tree but depends on p2m type change, this is the reason we must call p2m_type_change_one in vmx_vcpu_flush_pml_buffer.

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
Regarding to the above three cases, I assume you are referring to changing p2m_ram_rw back to p2m_ram_logdirty in paging_log_dirty_range , in which case the paging_mark_dirty is not called at all, as I mentioned above.


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.
I agree on this, given you said we can't be sure for the unsuccessful p2m type change.


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.
Which is also good to me, and in this case we should also call p2m_change_type_one(.., p2m_ram_logdirty, p2m_ram_rw) unconditionally, as this is required for video ram tracking.

Thanks,
-Kai

Cheers,

Tim.


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


 


Rackspace

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