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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.11.16 at 15:42, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 24/11/16 13:56, Jan Beulich wrote:
>>>>> On 23.11.16 at 16:38, <andrew.cooper3@xxxxxxxxxx> 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?
> 
> I am open to idea of swapping things around, but wonder whether this
> would be better done in a separate patch to avoid interfering with this
> mechanical movement.

Okay then.

>>> --- 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...
> 
> The first version also had an int => unsigned int change for error_code.
> 
> Now, the only difference is trap => vector, but I would like to keep it
> for consistency with the other changes.

Consistency? My comment was solely about the change in indentation
and breaking between lines, and there I think consistency with
surrounding code is more important than consistency with other
changes.

Jan


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