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

[Adrian Pop <apop@bitdefender.com>]

On Fri, Apr 07, 2017 at 07:18:26AM -0600, Jan Beulich wrote:
> >>> On 07.04.17 at 12:17, <apop@bitdefender.com> 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.

Indeed.

> > --- 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.
 
That's right.  Using current directly would be fine.

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

Nothing 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@suse.com>

Thanks!

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