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

[Tim Deegan <tim@xxxxxxx>]

At 17:29 +0800 on 17 Apr (1429291763), Kai Huang wrote:
> 
> 
> 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.

Sure, it doesn't call paging_mark_dirty(), but instead it puts the
marks into a bitmap directly.

> 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.

Right; so this code should probably also be setting the mark if
p2m_change_type_one() returns anything except EBUSY.

But in this vram code we can't just set the mark in all cases, because
we need to detect the case where the type is still p2m_ram_logdirty --
i.e. the page hasn't been written to.

> 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.

I think we need to do that anyway, to make sure that next time we clear
the bitmap, the change back from _rw to _logdirty clears the D bit.

But it does suggest that we might want to flush the PML buffers in the
vram function.

> > 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.

Yep. 

Tim.

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