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

["Tian, Kevin" <kevin.tian@xxxxxxxxx>]

> From: Kai Huang [mailto:kai.huang@xxxxxxxxxxxxxxx]
> Sent: Tuesday, April 21, 2015 2:05 PM
> 
> 
> 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@xxxxxxxxxxxxxxx]
> >>> 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?

yes

> 
> >
> >>
> >>> 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@xxxxxxxxxxxxxxx>
> >>> ---
> >>>   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'?
> 

good to me too.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel