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

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

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.

What's your comments?

Thanks,
-Kai

>
>>> +    d->arch.hvm_domain.vmx.status |= VMX_DOMAIN_PML_ENABLED;
>> I didn't see how this domain-wise flag is useful. Or if we really
>> want to go this way, you also need to clear this flag if vcpu pml
>> enable is failed in vcpu hotplug phase, since the flag itself means
>> all vcpus of the domain so we must keep this intention in all
>> places.
> IIUC we need this flag so that we know whether to enable PML on any
> new vcpus.  If we fail to enable PML on hotplug, we fail the hotplug
> (like for other vcpu bringup errors) so the invariant still holds.
>
> Cheers,
>
> Tim.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel