Re: [Xen-devel] [PATCH v4] hvm/svm: Implement Debug events

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.03.18 at 11:46, <aisaila@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -172,6 +172,24 @@ static void svm_enable_msr_interception(struct domain 
> *d, uint32_t msr)
>          svm_intercept_msr(v, msr, MSR_INTERCEPT_WRITE);
>  }
>  
> +static void svm_set_icebp_interception(struct domain *d, bool enable)
> +{
> +    struct vcpu *v;

While I agree that the hook's parameter would better not be a
pointer to const, the local variable here surely should be.

> @@ -2656,9 +2663,28 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>          HVMTRACE_0D(SMI);
>          break;
>  
> +    case VMEXIT_ICEBP:
>      case VMEXIT_EXCEPTION_DB:
>          if ( !v->domain->debugger_attached )
> -            hvm_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
> +        {
> +            int rc;
> +            unsigned int trap_type = exit_reason == VMEXIT_ICEBP ?
> +                X86_EVENTTYPE_PRI_SW_EXCEPTION : X86_EVENTTYPE_HW_EXCEPTION;
> +
> +            inst_len = 0;
> +
> +            if ( trap_type >= X86_EVENTTYPE_SW_INTERRUPT )
> +                inst_len = __get_instruction_length(v, INSTR_ICEBP);

>= (other == ) implies more than a single type is covered. How
does that fit with passing the unique INSTR_ICEBP to the function?
I don't see the point anyway to set the type to one of two
possible values and then compare against a third. Things would
likely quite a bit more obvious if you had an if/else pair and did
both type and insn len assignments separately for each case.

> @@ -581,6 +596,16 @@ static inline bool_t hvm_enable_msr_interception(struct 
> domain *d, uint32_t msr)
>      return 0;
>  }
>  
> +static inline bool hvm_set_icebp_interception(struct domain *d, bool enable)
> +{
> +    if( hvm_funcs.set_icebp_interception )

Contrary to what your revision log says, there's still a style issue
here plus ...

> +    {
> +        hvm_funcs.set_icebp_interception(d, enable);
> +        return 1;

... true here and ...

> +    }
> +    return 0;

... false here.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel