Re: [PATCH 05/10] VMX: add help functions to support PML

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

On 30/03/15 07:43, Kai Huang wrote:
>
>
> On 03/28/2015 05:09 AM, Andrew Cooper wrote:
>> On 27/03/15 02:35, Kai Huang wrote:
>>
>>> +}
>>> +
>>> +int vmx_vcpu_enable_pml(struct vcpu *v)
>>> +{
>>> +    struct domain *d = v->domain;
>>> +
>>> +    ASSERT(!vmx_vcpu_pml_enabled(v));
>>> +
>>> +    v->arch.hvm_vmx.pml_pg = d->arch.paging.alloc_page(d);
>>> +    if ( !v->arch.hvm_vmx.pml_pg )
>>> +        return -ENOMEM;
>>> +
>>> +    vmx_vmcs_enter(v);
>>> +
>>> +    __vmwrite(PML_ADDRESS, page_to_mfn(v->arch.hvm_vmx.pml_pg) <<
>>> PAGE_SHIFT);
>>> +    __vmwrite(GUEST_PML_INDEX, PML_ENTITY_NUM - 1);
>>> +
>>> +    v->arch.hvm_vmx.secondary_exec_control |=
>>> SECONDARY_EXEC_ENABLE_PML;
>>> +
>>> +    __vmwrite(SECONDARY_VM_EXEC_CONTROL,
>>> +            v->arch.hvm_vmx.secondary_exec_control);
>> Alignment.
> Do you mean to put 'v->arch.hvm_vmx.secondary_exec_control' to the
> same line with '__vmwrite(SECONDARY_VM_EXEC_CONTROL,'? In this case
> the number of characters will be 81.

Splitting the line is fine.  The v should be vertically in line with S
from SECONDARY

>
>>
>>> +        unsigned long gfn;
>>> +        mfn_t mfn;
>>> +        p2m_type_t t;
>>> +        p2m_access_t a;
>>> +
>>> +        gfn = pml_buf[pml_idx] >> PAGE_SHIFT;
>>> +        mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL);
>>> +        if ( mfn_x(mfn) == INVALID_MFN )
>>> +        {
>>> +            /*
>>> +             * Either EPT table entry for mapping the GFN has been
>>> destroyed, or
>>> +             * there's something wrong with hardware behavior, in
>>> both cases we
>>> +             * should report a warning.
>>> +             */
>>> +            dprintk(XENLOG_WARNING, "PML: vcpu %d: invalid GPA
>>> 0x%lx logged\n",
>>> +                    v->vcpu_id, pml_buf[pml_idx]);
>> It would be shorter to log gfn rather than gpa.
> Will do. And I'd also like to add the domain ID in the warning info.

Ah of course - dprintk() doesn't identify current().  Use %pv with v.

>
>>
>>> +{
>>> +    return (d->arch.hvm_domain.vmx.status & VMX_DOMAIN_PML_ENABLED)
>>> ? 1 : 0;
>>> +}
>>> +
>>> +/*
>>> + * This function enables PML for particular domain. It should be
>>> called when
>>> + * domain is paused.
>> In which case assert that the domain is paused, or call domain_pause()
>> yourself to take an extra pause refcount.
> Which function should I use to assert domain is paused? I didn't find
> a function like "domain_paused". Is below good enough?
>
> ASSERT(atomic_read(&d->pause_count));

Hmm - we indeed don't have an appropriate helper.  That ASSERT() will do
for now.

~Andrew