Re: [Xen-devel] [PATCH 1/2] x86/vmx: dump MSR load area

[Matt Wilson <msw@xxxxxxxx>]

On Wed, Aug 10, 2016 at 04:44:21AM -0600, Jan Beulich wrote:
> >>> On 10.08.16 at 08:59, <msw@xxxxxxxx> wrote:
> > --- a/xen/arch/x86/hvm/vmx/vmcs.c
> > +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> > @@ -1740,6 +1740,20 @@ static void vmx_dump_sel2(char *name, uint32_t lim)
> >      printk("%s:            %08x %016"PRIx64"\n", name, limit, base);
> >  }
> >  
> > +static void vmx_dump_vcpu_msr_area(struct vcpu *v)
> 
> const

Ack.

> > +{
> > +    const struct vmx_msr_entry *msr_area = v->arch.hvm_vmx.msr_area;
> > +    unsigned int i, msr_count = v->arch.hvm_vmx.msr_count;
> > +
> > +    printk("  msr_count = %d\n", v->arch.hvm_vmx.msr_count);
> 
> Please use the local variable you have just latched and %u.

Ack.

> > +    for ( i = 0; i < msr_count; i++ )
> > +    {
> > +        printk("  msr_area[%d].index=0x%08x .data=0x%016lx\n",
> 
> [%u], and I'm not sure the 0x prefixes are really all that helpful
> here. I think it would also be beneficial to shorten the logged text
> a little, as this may consume quite a bit of serial line bandwidth.
> How about "   MSR[%08x]=%016lx\n"?

Everyone isn't running at 115,200 baud these days? ;-)

Ack.

> > @@ -1879,6 +1893,13 @@ void vmcs_dump_vcpu(struct vcpu *v)
> >           (SECONDARY_EXEC_ENABLE_VPID | SECONDARY_EXEC_ENABLE_VM_FUNCTIONS) 
> > )
> >          printk("Virtual processor ID = 0x%04x VMfunc controls = %016lx\n",
> >                 vmr16(VIRTUAL_PROCESSOR_ID), vmr(VM_FUNCTION_CONTROL));
> > +    printk("EXIT MSR load count = 0x%04x\n",
> > +           (uint32_t)vmr(VM_EXIT_MSR_LOAD_COUNT));
> > +    printk("EXIT MSR store count = 0x%04x\n",
> > +           (uint32_t)vmr(VM_EXIT_MSR_STORE_COUNT));
> > +    printk("ENTRY MSR load count = 0x%04x\n",
> > +           (uint32_t)vmr(VM_ENTRY_MSR_LOAD_COUNT));
> 
> First - do you really need to make three log lines out of these? And
> then, please use vmr32(), as the neighboring vmr16() suggests.
> Plus finally - please log all four counts consistently either in hex
> or in dec.

With one line, output might look something like:
(XEN) MSR load/store count ExitLoad=0x0001 ExitStore=0x0023 EntryLoad=0x0023

Spaces around = are inconsistent in the existing output and it seems
that no space is more popular. Does this format seem better to you?

I see three counts here - are you talking about the msr_count above?
For msr_count I was thinking that this is internal Xen state, whereas
the other values are VMCS fields where everything else is dumped in
hex. I think printing msr_count is redundant (one could just count the
lines of output), so I'll just remove it.

--msw

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