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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

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@lists.xen.org
https://lists.xen.org/xen-devel