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

[Vlad-Ioan TOPAN <itopan@xxxxxxxxxxxxxxx>]

On Tue, 14 Mar 2017 09:15:04 -0400
Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:

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

That's the "roughly same amount of code, but less readable" option, but
it works for me if that's the consensus.

> 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

Okay then, I'll post v2 of the patch with the two suggested changes.

--
Vlad-Ioan TOPAN
Linux Kernel Development Lead
Bitdefender

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