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

[Kai Huang <kai.huang@linux.intel.com>]

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@linux.intel.com]
>>>>> +        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.