Re: [Xen-devel] [PATCH] x86/monitor: add support for descriptor access events

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 03/14/2017 08:50 AM, Razvan Cojocaru wrote:
> On 03/14/2017 02:15 PM, Vlad-Ioan TOPAN wrote:
> > > > @@ -2642,6 +2660,38 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
> > > >      case VMEXIT_PAUSE:
> > > >          svm_vmexit_do_pause(regs);
> > > >          break;
> > > > +
> > > > +    case VMEXIT_IDTR_READ:
> > > > +        hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0, 
> > > > VM_EVENT_DESC_IDTR, 0);
> > > > +        break;
> > > > +
> > > > +    case VMEXIT_GDTR_READ:
> > > > +        hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0, 
> > > > VM_EVENT_DESC_GDTR, 0);
> > > > +        break;
> > > > +
> > > > +    case VMEXIT_LDTR_READ:
> > > > +        hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0, 
> > > > VM_EVENT_DESC_LDTR, 0);
> > > > +        break;
> > > > +
> > > > +    case VMEXIT_TR_READ:
> > > > +        hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0, 
> > > > VM_EVENT_DESC_TR, 0);
> > > > +        break;
> > > > +
> > > > +    case VMEXIT_IDTR_WRITE:
> > > > +        hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0, 
> > > > VM_EVENT_DESC_IDTR, 1);
> > > > +        break;
> > > > +
> > > > +    case VMEXIT_GDTR_WRITE:
> > > > +        hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0, 
> > > > VM_EVENT_DESC_GDTR, 1);
> > > > +        break;
> > > > +
> > > > +    case VMEXIT_LDTR_WRITE:
> > > > +        hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0, 
> > > > VM_EVENT_DESC_LDTR, 1);
> > > > +        break;
> > > > +
> > > > +    case VMEXIT_TR_WRITE:
> > > > +        hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0, 
> > > > VM_EVENT_DESC_TR, 1);
> > > > +        break;
> > > I think this can be halved in size by having
> > > 'hvm_descriptor_access_intercept(..., exit_reason_write?1:0)'
> > >
> > > And maybe even collapse completely by having a lookup table mapping exit
> > > reason to event.
> > The problem with both ideas is that they depend on assumptions about the
> > values of the VMEXIT_* constants to make the code shorter and still
> > keep it readable, which in my opinion would be bad. Although they will
> > most likely stay sequential and keep their current numeric values, it's
> > not something I'd hardcode. Without those assumptions, it's either
> > another switch or a very long if, which would mean roughly the same
> > amount of code, but less readable (it's the way I've written it
> > initally before coming to this version).
> I'm reading Boris' suggestion to mean:
>
> case VMEXIT_IDTR_READ:
> case VMEXIT_IDTR_WRITE:
>     hvm_descriptor_access_intercept(vmcb->exitintinfo.bytes, 0,
>         VM_EVENT_DESC_IDTR, exit_reason == VMEXIT_IDTR_WRITE);
>     break;
>
> I could be wrong.

Right, that's exactly what I meant, thanks.

As for getting rid of all but one cases --- yes, it may be a a bit 
tricky to do it in a reasonably compact manner.
-boris

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