Re: [Xen-devel] [PATCH] x86/monitor: add support for descriptor access events

[Vlad-Ioan TOPAN <itopan@xxxxxxxxxxxxxxx>]

On Fri, 17 Mar 2017 05:03:44 -0600
"Jan Beulich" <JBeulich@xxxxxxxx> wrote:

> >>> On 10.03.17 at 16:50, <itopan@xxxxxxxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -3645,6 +3645,41 @@ gp_fault:
> >      return X86EMUL_EXCEPTION;
> >  }
> >  
> > +int hvm_descriptor_access_intercept(uint64_t exit_info, uint64_t 
> > exit_qualification, 
> > +                                    uint8_t descriptor, bool_t is_write)
> 
> bool (also elsewhere)

Ok.

> > +{
> > +    struct vcpu *v = current;
> > +    struct domain *d = v->domain;
> 
> curr and currd respectively.

Ok.

> > +    struct hvm_emulate_ctxt ctxt = {};
> 
> Please move this into the more narrow scope it's needed in.

Ok.

> > +    int rc;
> > +
> > +    if ( d->arch.monitor.descriptor_access_enabled )
> > +    {
> > +        ASSERT(v->arch.vm_event);
> > +        hvm_monitor_descriptor_access(exit_info, exit_qualification, 
> > descriptor, is_write);
> > +    }
> > +    else
> > +    {
> > +        hvm_emulate_init_once(&ctxt, NULL, guest_cpu_user_regs());
> > +        rc = hvm_emulate_one(&ctxt);
> > +        switch ( rc )
> > +        {
> > +        case X86EMUL_UNHANDLEABLE:
> > +            hvm_inject_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC);
> 
> Is #UD the right exception here? In fact, is delivering an exception
> sensible in this case at all?

Possibly not; it's how that case is handled elsewhere. For the given
set of instructions at least, X86EMUL_UNHANDLEABLE should only be
returnable due to some internal bug/error (e.g. invalid/unknown
HVMCOPY_* constant returned by hvm_fetch_from_guest_linear() or an
invalid selector passed to ->write_segment() etc.); what would be the
best way to handle that case?

> > @@ -3369,6 +3384,33 @@ static void vmx_handle_xrstors(void)
> >      domain_crash(current->domain);
> >  }
> >  
> > +static void vmx_handle_descriptor_access(uint32_t exit_reason)
> > +{
> > +    uint8_t instr_id;
> > +    uint64_t instr_info;
> > +    uint64_t exit_qualification;
> > +    uint8_t descriptor = VM_EVENT_DESC_INVALID;
> > +
> > +    __vmread(EXIT_QUALIFICATION, &exit_qualification);
> > +    __vmread(VMX_INSTRUCTION_INFO, &instr_info);
> > +
> > +    instr_id = (instr_info >> 28) & 3; /* Instruction identity: bits 29:28 
> > */
> > +
> > +    if ( exit_reason == EXIT_REASON_ACCESS_GDTR_OR_IDTR )
> > +        if ( instr_id & 1 )
> > +            descriptor = VM_EVENT_DESC_IDTR;
> > +        else
> > +            descriptor = VM_EVENT_DESC_GDTR;
> > +    else
> > +        if ( instr_id & 1 )
> > +            descriptor = VM_EVENT_DESC_TR;
> > +        else
> > +            descriptor = VM_EVENT_DESC_LDTR;
> 
> Please use conditional expressions instead of if/else in such cases.

Ok.

> > +    hvm_descriptor_access_intercept(instr_info, exit_qualification, 
> > descriptor,
> > +                                    instr_id >= 2);
> 
> instr_id & 2 (to be consistent with the other code. But anyway, even
> better would be to use manifest constants instead of all these literal
> numbers.

(instr_id & 2) makes sense, but the 1 & 2 literals are unnamed criteria
inferred from the encoding to simplify the code, they don't have an
explicit meaning (at least in the Intel docs).

> > @@ -444,6 +445,7 @@ int hvm_event_needs_reinjection(uint8_t type, uint8_t 
> > vector);
> >  uint8_t hvm_combine_hw_exceptions(uint8_t vec1, uint8_t vec2);
> >  
> >  void hvm_set_rdtsc_exiting(struct domain *d, bool_t enable);
> > +void hvm_set_descriptor_access_exiting(struct domain *d, bool_t enable);
> 
> Dead declaration.

It somehow got away from a previous (internal) version of the patch; it
will be removed.

> > --- a/xen/include/asm-x86/monitor.h
> > +++ b/xen/include/asm-x86/monitor.h
> > @@ -77,6 +77,7 @@ static inline uint32_t 
> > arch_monitor_get_capabilities(struct domain *d)
> >                     (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST) |
> >                     (1U << XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION) |
> >                     (1U << XEN_DOMCTL_MONITOR_EVENT_CPUID) |
> > +                   (1U << XEN_DOMCTL_MONITOR_EVENT_DESC_ACCESS) |
> >                     (1U << XEN_DOMCTL_MONITOR_EVENT_INTERRUPT);
> 
> If I was the maintainer of this code, I'd ask for the elements to
> remain ordered numerically, i.e. matching ...
> 
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -1087,6 +1087,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
> >  #define XEN_DOMCTL_MONITOR_EVENT_CPUID                 6
> >  #define XEN_DOMCTL_MONITOR_EVENT_PRIVILEGED_CALL       7
> >  #define XEN_DOMCTL_MONITOR_EVENT_INTERRUPT             8
> > +#define XEN_DOMCTL_MONITOR_EVENT_DESC_ACCESS           9
> 
> ... the sequence here.

Ok.

> > @@ -259,6 +261,24 @@ struct vm_event_mov_to_msr {
> >      uint64_t value;
> >  };
> >  
> > +#define VM_EVENT_DESC_INVALID        0
> 
> What is this good for?

The default (uninitialized) value is given a semantic of "invalid" to
make potential problems due to incorrectly / incompletely initialized
or corrupted data more obvious (I assume the .pad* fields are checked to
be 0 for the same reason).

> Jan

Thank you for the review; pending further feedback on the
X86EMUL_UNHANDLEABLE case I'll post an updated patch.

--
Vlad-Ioan TOPAN
Linux Kernel Development Lead
Bitdefender

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