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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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

~Andrew

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


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