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

[Adrian Pop <apop@bitdefender.com>]

On Tue, Apr 04, 2017 at 04:26:24PM +0100, Andrew Cooper wrote:
> On 04/04/17 10:57, Adrian Pop wrote:
> > diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
> > index f5cd245771..d60e4afd0c 100644
> > --- a/xen/arch/x86/hvm/monitor.c
> > +++ b/xen/arch/x86/hvm/monitor.c
> > @@ -72,6 +72,28 @@ 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;
> > +    vm_event_request_t req = {
> > +        .reason = VM_EVENT_REASON_DESCRIPTOR_ACCESS,
> > +        .u.desc_access.descriptor = descriptor,
> > +        .u.desc_access.is_write = is_write,
> > +    };
> 
> Newline here
 
Ok.

> > +    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;
> > +    }
> 
> And here please.

Ok.

> > +    monitor_traps(curr, 1, &req);
> > +}
> > +
> >  static inline unsigned long gfn_of_rip(unsigned long rip)
> >  {
> >      struct vcpu *curr = current;
> > diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
> > index 2b781ab120..b00ba52443 100644
> > --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> > +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> > @@ -628,4 +628,46 @@ 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 */
> 
> I think you need a :32 /* undefined */ in each of these, to avoid
> breaking the Clang build, which cares that each half of the union have
> the same bit size.

All right.

> All of these can be fixed on commit if there are no other issues. 
> Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks!

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