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

[Kai Huang <kai.huang@xxxxxxxxxxxxxxx>]

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.
> > 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.

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

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