Re: [PATCH 03/15] x86/emul: Rename hvm_trap to x86_event and move it into the emulation infrastructure

From: "Jan Beulich" <JBeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Kevin Tian <kevin.tian@intel.com>,
	Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
	Xen-devel <xen-devel@lists.xen.org>,
	Paul Durrant <paul.durrant@citrix.com>,
	Jun Nakajima <jun.nakajima@intel.com>,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>
Subject: Re: [PATCH 03/15] x86/emul: Rename hvm_trap to x86_event and move it into the emulation infrastructure
Date: Thu, 24 Nov 2016 06:56:23 -0700	[thread overview]
Message-ID: <5836FF970200007800121B7E@prv-mh.provo.novell.com> (raw)
In-Reply-To: <1479915538-15282-4-git-send-email-andrew.cooper3@citrix.com>

>>> On 23.11.16 at 16:38, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
> @@ -67,6 +67,28 @@ enum x86_swint_emulation {
>      x86_swint_emulate_all,  /* Help needed with all software events */
>  };
>  
> +/*
> + * x86 event types. This enumeration is valid for:
> + *  Intel VMX: {VM_ENTRY,VM_EXIT,IDT_VECTORING}_INTR_INFO[10:8]
> + *  AMD SVM: eventinj[10:8] and exitintinfo[10:8] (types 0-4 only)
> + */
> +enum x86_event_type {
> +    X86_EVENTTYPE_EXT_INTR,         /* External interrupt */
> +    X86_EVENTTYPE_NMI = 2,          /* NMI */
> +    X86_EVENTTYPE_HW_EXCEPTION,     /* Hardware exception */
> +    X86_EVENTTYPE_SW_INTERRUPT,     /* Software interrupt (CD nn) */
> +    X86_EVENTTYPE_PRI_SW_EXCEPTION, /* ICEBP (F1) */
> +    X86_EVENTTYPE_SW_EXCEPTION,     /* INT3 (CC), INTO (CE) */
> +};
> +
> +struct x86_event {
> +    int16_t       vector;
> +    uint8_t       type;         /* X86_EVENTTYPE_* */

Do we perhaps want to make the compiler warn about possibly
incomplete switch statements, but making this an 8-bit field of
type enum x86_event_type? (That would perhaps imply making
vector and insn_len bitfields too; see also below.)

> +    uint8_t       insn_len;     /* Instruction length */
> +    uint32_t      error_code;   /* HVM_DELIVER_NO_ERROR_CODE if n/a */
> +    unsigned long cr2;          /* Only for TRAP_page_fault h/w exception */
> +};

Also I have to admit I'm not really happy about the mixing of fixed
width and fundamental types. Can I talk you into using only the
latter?

> --- a/xen/include/asm-x86/hvm/vmx/vvmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vvmx.h
> @@ -112,8 +112,8 @@ void nvmx_vcpu_destroy(struct vcpu *v);
>  int nvmx_vcpu_reset(struct vcpu *v);
>  uint64_t nvmx_vcpu_eptp_base(struct vcpu *v);
>  enum hvm_intblk nvmx_intr_blocked(struct vcpu *v);
> -bool_t nvmx_intercepts_exception(struct vcpu *v, unsigned int trap,
> -                                 int error_code);
> +bool_t nvmx_intercepts_exception(
> +    struct vcpu *v, unsigned int vector, int error_code);

This reformatting doesn't appear to be in line with other nearby
code. But iirc you've got an ack from the VMX side already...

Anyway, with or without the comments addressed,
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

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