Re: [PATCH 07/10] VMX: handle PML enabling in vmx_vcpu_initialise

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 30/03/15 08:03, Kai Huang wrote:
>
>
> On 03/28/2015 05:12 AM, Andrew Cooper wrote:
>> On 27/03/15 02:35, Kai Huang wrote:
>>> It's possible domain has already been in log-dirty mode when
>>> creating vcpu, in
>>> which case we should enable PML for this vcpu if PML has been
>>> enabled for the
>>> domain.
>>>
>>> Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
>>> ---
>>>   xen/arch/x86/hvm/vmx/vmx.c | 24 ++++++++++++++++++++++++
>>>   1 file changed, 24 insertions(+)
>>>
>>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>>> index 453bcc5..fce3aa2 100644
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -116,6 +116,30 @@ static int vmx_vcpu_initialise(struct vcpu *v)
>>>           return rc;
>>>       }
>>>   +    /*
>>> +     * It's rare but still possible that domain has already been in
>>> log-dirty
>>> +     * mode when vcpu is being created (commented by Tim), in which
>>> case we
>>> +     * should enable PML for this vcpu if PML has been enabled for
>>> the domain,
>>> +     * and failure to enable results in failure of creating this vcpu.
>>> +     *
>>> +     * Note even there's no vcpu created for the domain,
>>> vmx_domain_enable_pml
>>> +     * will return successful in which case vmx_domain_pml_enabled
>>> will also
>>> +     * return true. And even this is the first vcpu to be created with
>>> +     * vmx_domain_pml_enabled being true, failure of enabling PML
>>> still results
>>> +     * in failure of creating vcpu, to avoid complicated logic to
>>> revert PML
>>> +     * style EPT table to non-PML style EPT table.
>>> +     */
>>> +    if ( vmx_domain_pml_enabled(v->domain) )
>>> +    {
>>> +        if ( (rc = vmx_vcpu_enable_pml(v)) != 0 )
>> Given the comment here, is the assertion in the top of
>> vmx_vcpu_enable_pml() liable to trip?
> Do you mean below assertion at beginning of vmx_vcpu_enable_pml might
> not work here?
>
>     ASSERT(!vmx_vcpu_pml_enabled(v));
>
> To me it asserts for this particular vcpu, not the domain, so even in
> this case the assertion is reasonable and should work fine, shouldn't it?

You are correct.  I was getting vcpus and domains mixed up.  Sorry for
the noise.

~Andrew