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

[Kai Huang <kaih.linux@xxxxxxxxx>]

On Mon, Apr 20, 2015 at 4:29 PM, Tim Deegan <tim@xxxxxxx> wrote:
> 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.

Reasonable. But as so far there's nothing wrong behavior observed with
current code, I intend to leave it unchanged. Is it OK to you?

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

Agreed.

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

True.

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

Agreed. So this time for the PML patches, I'll always call both
mark_dirty and p2m_change_type_one (and ignore return value) for all
logged GPA.

But I intend not to change current video ram tracking code
(paging_log_dirty_range), as explained above. Is this good to you?

Thanks,
-Kai

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



-- 
Thanks,
-Kai

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