Re: [v2 05/11] vmx: add new data structure member to support PML

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

On 04/17/2015 10:31 AM, Kai Huang wrote:
>
>
> On 04/17/2015 06:39 AM, Tian, Kevin wrote:
>>> From: Kai Huang [mailto:kai.huang@linux.intel.com]
>>> Sent: Wednesday, April 15, 2015 3:04 PM
>>>
>>> A new 4K page pointer is added to arch_vmx_struct as PML buffer for 
>>> vcpu.
>>> And a
>>> new 'status' field is added to vmx_domain to indicate whether PML is 
>>> enabled
>>> for
>>> the domain or not. The 'status' field also can be used for further 
>>> similiar
>>> purpose.
>> not sure about the last sentence. what's the similar purpose to 
>> "whether PML
>> is enabled"? :-)
> I mean potentially there might be such feature in the future, and I 
> can't give you an example right now. If you are just commenting the 
> description here but fine with the current code, I can remove that 
> last sentence if you like. Or do you suggest to just use a "bool_t 
> pml_enabled"? I am fine with both, but looks there's no objection from 
> others so I intend to keep it as 'unsigned int status', if you agree.
Hi Kevin,

What's your opinion here? Is 'unsigned int status' OK to you?

>
>>
>>> Note both new members don't have to be initialized to zero 
>>> explicitly as both
>>> vcpu and domain structure are zero-ed when they are created.
>> no initialization in this patch, so why explaining it here?
> OK. Looks it's a common sense to all of you so I'll just remove this 
> sentence.
>
>>
>>> Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
>>> ---
>>>   xen/include/asm-x86/hvm/vmx/vmcs.h | 7 +++++++
>>>   1 file changed, 7 insertions(+)
>>>
>>> diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h
>>> b/xen/include/asm-x86/hvm/vmx/vmcs.h
>>> index f831a78..2c679ac 100644
>>> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
>>> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
>>> @@ -70,8 +70,12 @@ struct ept_data {
>>>       cpumask_var_t synced_mask;
>>>   };
>>>
>>> +#define _VMX_DOMAIN_PML_ENABLED    0
>>> +#define VMX_DOMAIN_PML_ENABLED     (1ul <<
>>> _VMX_DOMAIN_PML_ENABLED)
>>>   struct vmx_domain {
>>>       unsigned long apic_access_mfn;
>>> +    /* VMX_DOMAIN_* */
>>> +    unsigned long status;
>>>   };
>>>
>>>   struct pi_desc {
>>> @@ -142,6 +146,9 @@ struct arch_vmx_struct {
>>>       /* Bitmap to control vmexit policy for Non-root VMREAD/VMWRITE */
>>>       struct page_info     *vmread_bitmap;
>>>       struct page_info     *vmwrite_bitmap;
>>> +
>>> +#define NR_PML_ENTRIES   512
>>> +    struct page_info     *pml_pg;
>> move the macro out of the structure.
> OK. I will move it just above the declaration of struct arch_vmx_struct.
>
>> and is pml_buffer or pml_buf more clear?
>
> To me pml_buffer or pml_buf is more likely a virtual address you can 
> access the buffer directly, while pml_pg indicates it's a pointer of 
> struct page_info. If you you look at patch 6, you can find statements 
> like:
>
>     uint64_t *pml_buf;
>
>     pml_buf = __map_domain_page(v->arch.hvm_vmx.pml_pg);
>
> So I intend to keep it.
And this one? Are you OK with 'pml_pg'?

Thanks,
-Kai
>
> Thanks,
> -Kai
>>
>>>   };
>>>
>>>   int vmx_create_vmcs(struct vcpu *v);
>>> -- 
>>> 2.1.0
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel