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

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

On 04/16/2015 11:42 PM, Jan Beulich wrote:
>>>> On 15.04.15 at 09:03, <kai.huang@linux.intel.com> wrote:
>> +int vmx_vcpu_enable_pml(struct vcpu *v)
>> +{
>> +    struct domain *d = v->domain;
>> +
>> +    if ( vmx_vcpu_pml_enabled(v) )
>> +        return 0;
>> +
>> +    v->arch.hvm_vmx.pml_pg = d->arch.paging.alloc_page(d);
> So you latch v->domain into d for this invocation, ...
>
>> +void vmx_vcpu_disable_pml(struct vcpu *v)
>> +{
>> +    if ( !vmx_vcpu_pml_enabled(v) )
>> +        return;
>> +
>> +    /* Make sure we don't lose any logged GPAs */
>> +    vmx_vcpu_flush_pml_buffer(v);
>> +
>> +    vmx_vmcs_enter(v);
>> +
>> +    v->arch.hvm_vmx.secondary_exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
>> +    __vmwrite(SECONDARY_VM_EXEC_CONTROL,
>> +              v->arch.hvm_vmx.secondary_exec_control);
>> +
>> +    vmx_vmcs_exit(v);
>> +
>> +    v->domain->arch.paging.free_page(v->domain, v->arch.hvm_vmx.pml_pg);
> ... but not for this one. Please be consistent.
Hmm. My bad. I'll use v->domain in both function.

>
>> +void vmx_vcpu_flush_pml_buffer(struct vcpu *v)
>> +{
>> +    uint64_t *pml_buf;
>> +    unsigned long pml_idx;
>> +
>> +    ASSERT(vmx_vcpu_pml_enabled(v));
>> +
>> +    vmx_vmcs_enter(v);
>> +
>> +    __vmread(GUEST_PML_INDEX, &pml_idx);
> Don't you require the vCPU to be non-running or current when you
> get here? If so, perhaps add a respective ASSERT()?
Yes an ASSERT would be better.

v->pause_count will be increased if vcpu is kicked out by domain_pause 
explicitly, but looks the same thing won't be done if vcpu is kicked out 
by PML buffer full VMEXIT. So should the ASSERT be done like below?

ASSERT(atomic_read(&v->pause_count) || (v == current));

>
>> +
>> +    /* Do nothing if PML buffer is empty */
>> +    if ( pml_idx == (NR_PML_ENTRIES - 1) )
>> +        goto out;
>> +
>> +    pml_buf = __map_domain_page(v->arch.hvm_vmx.pml_pg);
>> +
>> +    /*
>> +     * PML index can be either 2^16-1 (buffer is full), or 0~511 (buffer is not
>> +     * full), and in latter case PML index always points to next available
>> +     * entity.
>> +     */
>> +    if (pml_idx >= NR_PML_ENTRIES)
>> +        pml_idx = 0;
>> +    else
>> +        pml_idx++;
>> +
>> +    for ( ; pml_idx < NR_PML_ENTRIES; pml_idx++ )
>> +    {
>> +        unsigned long gfn = pml_buf[pml_idx] >> PAGE_SHIFT;
> Blank line here please.
Will do.

>
>> +        /*
>> +         * Need to change type from log-dirty to normal memory for logged GFN.
>> +         * hap_track_dirty_vram depends on it to work. And we really only need
>> +         * to mark GFNs which hve been successfully changed from log-dirty to
>> +         * normal memory to be dirty.
>> +         */
>> +        if ( !p2m_change_type_one(v->domain, gfn, p2m_ram_logdirty,
>> +                    p2m_ram_rw) )
> Indentation.
To be where exactly? Sorry I didn't find an example to refer in such case.

>
>> +            paging_mark_gfn_dirty(v->domain, gfn);
>> +    }
>> +
>> +    unmap_domain_page(pml_buf);
>> +
>> +    /* Reset PML index */
>> +    __vmwrite(GUEST_PML_INDEX, NR_PML_ENTRIES - 1);
>> +
>> +out:
> Labels indented by at least one space please.
OK. I'll put one space before the "out:" label.

Thanks,
-Kai
>
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel