Re: [Xen-devel] [PATCH 10/17] vmx: nest: VMExit handler in L2

[Tim Deegan <Tim.Deegan@xxxxxxxxxx>]

At 17:06 +0100 on 20 May (1274375166), Qing He wrote:
> On Thu, 2010-05-20 at 19:44 +0800, Tim Deegan wrote:
> > At 10:41 +0100 on 22 Apr (1271932882), Qing He wrote:
> > > +
> > > +    case VMX_EXIT_REASONS_FAILED_VMENTRY:
> > > +    case EXIT_REASON_TRIPLE_FAULT:
> > > +    case EXIT_REASON_TASK_SWITCH:
> > > +    case EXIT_REASON_IO_INSTRUCTION:
> > > +    case EXIT_REASON_CPUID:
> > > +    case EXIT_REASON_MSR_READ:
> > > +    case EXIT_REASON_MSR_WRITE:
> > 
> > Aren't these gated on a control bitmap in the L1 VMCS?
> > 
> 
> cpuid is unconditional
> io and msr are controlled through bitmaps, but they are turned
> off in the capability reporting

OK.

> > > +    case EXIT_REASON_HLT:
> > > +    case EXIT_REASON_RDTSC:
> > > +    case EXIT_REASON_RDPMC:
> > > +    case EXIT_REASON_MWAIT_INSTRUCTION:
> > > +    case EXIT_REASON_PAUSE_INSTRUCTION:
> > > +    case EXIT_REASON_MONITOR_INSTRUCTION:
> > > +    case EXIT_REASON_DR_ACCESS:
> > > +    case EXIT_REASON_INVLPG:
> > > +    {
> > > +        int i;
> > > +
> > > +        /* exit according to guest exec_control */
> > > +        ctrl = __get_vvmcs(nest->vvmcs, CPU_BASED_VM_EXEC_CONTROL);
> > > +
> > > +        for ( i = 0; i < ARRAY_SIZE(control_bit_for_reason); i++ )
> > > +            if ( control_bit_for_reason[i].reason == exit_reason )
> > > +                break;
> > 
> > You've already got a switch statement - why not gate these individually
> > rather than bundling them together and scanning an array?
> > 
> 
> Well, they are the `regular' part of exit handling, a bit in the control
> bitmap corresponds to their behavior

I understand that.  It just seems inefficient to bundle them all
together into one clause of the switch statement and then scan an array
looking for which one you've hit.  Wouldn't it be better to give each
one its own clause and then use goto (!) or similar to jump to the
common code?

> > > +    if ( nest->vmexit_pending )
> > > +        bypass_l0 = 1;
> > 
> > This variable doesn't seem to do anything useful. 
> 
> This is a preparation for those doesn't generate virtual
> vmexit but need to bypass normal L0 exit handler

Yes, I saw that in the later patch. 

Tim.

> > > +    return bypass_l0;
> > > +}
> > 
> > Cheers,
> > 
> > Tim.
> > 
> > -- 
> > Tim Deegan <Tim.Deegan@xxxxxxxxxx>
> > Principal Software Engineer, XenServer Engineering
> > Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

-- 
Tim Deegan <Tim.Deegan@xxxxxxxxxx>
Principal Software Engineer, XenServer Engineering
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel