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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Adrian Pop <apop@bitdefender.com>, xen-devel@lists.xenproject.org
Cc: Kevin Tian <kevin.tian@intel.com>,
	Tamas K Lengyel <tamas@tklengyel.com>,
	Wei Liu <wei.liu2@citrix.com>,
	Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
	Razvan Cojocaru <rcojocaru@bitdefender.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	Jan Beulich <jbeulich@suse.com>,
	Jun Nakajima <jun.nakajima@intel.com>,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>
Subject: Re: [PATCH v2] x86/monitor: add support for descriptor access events
Date: Tue, 4 Apr 2017 16:26:24 +0100	[thread overview]
Message-ID: <815d1001-eae5-0ecf-05f1-219e6fc00111@citrix.com> (raw)
In-Reply-To: <20170404095757.9064-1-apop@bitdefender.com>

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

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

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

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