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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.04.17 at 12:17, <apop@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3589,6 +3589,41 @@ gp_fault:
>      return X86EMUL_EXCEPTION;
>  }
>  
> +int hvm_descriptor_access_intercept(uint64_t exit_info,
> +                                    uint64_t vmx_exit_qualification,
> +                                    unsigned int descriptor, bool is_write)
> +{
> +    struct vcpu *curr = current;
> +    struct domain *currd = curr->domain;
> +
> +    if ( currd->arch.monitor.descriptor_access_enabled )
> +    {
> +        ASSERT(curr->arch.vm_event);
> +        hvm_monitor_descriptor_access(exit_info, vmx_exit_qualification,
> +                                      descriptor, is_write);
> +    }
> +    else
> +    {
> +        struct hvm_emulate_ctxt ctxt = {};

Pointless initializer - this function ...

> +        hvm_emulate_init_once(&ctxt, NULL, guest_cpu_user_regs());

... memset()s the whole structure.

> --- a/xen/arch/x86/hvm/monitor.c
> +++ b/xen/arch/x86/hvm/monitor.c
> @@ -72,6 +72,30 @@ void hvm_monitor_msr(unsigned int msr, uint64_t value)
>      }
>  }
>  
> +void hvm_monitor_descriptor_access(uint64_t exit_info,
> +                                   uint64_t vmx_exit_qualification,
> +                                   uint8_t descriptor, bool is_write)
> +{
> +    struct vcpu *curr = current;

Pointless local variable, it is being use just once ...

> +    vm_event_request_t req = {
> +        .reason = VM_EVENT_REASON_DESCRIPTOR_ACCESS,
> +        .u.desc_access.descriptor = descriptor,
> +        .u.desc_access.is_write = is_write,
> +    };
> +
> +    if ( cpu_has_vmx )
> +    {
> +        req.u.desc_access.arch.vmx.instr_info = exit_info;
> +        req.u.desc_access.arch.vmx.exit_qualification = 
> vmx_exit_qualification;
> +    }
> +    else
> +    {
> +        req.u.desc_access.arch.svm.exitinfo = exit_info;
> +    }
> +
> +    monitor_traps(curr, true, &req);

... here afaics.

> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> @@ -628,4 +628,48 @@ typedef struct {
>      u16 eptp_index;
>  } ve_info_t;
>  
> +/* VM-Exit instruction info for LIDT, LGDT, SIDT, SGDT */
> +typedef union idt_or_gdt_instr_info {
> +    unsigned long raw;
> +    struct {
> +        unsigned long scaling   :2,  /* bits 0:1 - Scaling */
> +                                :5,  /* bits 6:2 - Undefined */
> +        addr_size               :3,  /* bits 9:7 - Address size */
> +                                :1,  /* bit 10 - Cleared to 0 */
> +        operand_size            :1,  /* bit 11 - Operand size */
> +                                :3,  /* bits 14:12 - Undefined */
> +        segment_reg             :3,  /* bits 17:15 - Segment register */
> +        index_reg               :4,  /* bits 21:18 - Index register */
> +        index_reg_invalid       :1,  /* bit 22 - Index register invalid */
> +        base_reg                :4,  /* bits 26:23 - Base register */
> +        base_reg_invalid        :1,  /* bit 27 - Base register invalid */
> +        instr_identity          :1,  /* bit 28 - 0:GDT, 1:IDT */
> +        instr_write             :1,  /* bit 29 - 0:store, 1:load */
> +                                :2,  /* bits 30:31 - Undefined */
> +                                :32; /* bits 32:63 - Undefined */

Is there anything wrong with :34?

With these cosmetic issues addressed (which I guess I'll take the
liberty of doing while committing)
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan


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