On Fri, 17 Mar 2017 05:03:44 -0600
"Jan Beulich" <jbeul...@suse.com> wrote:
> >>> On 10.03.17 at 16:50, <ito...@bitdefender.com> 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@lists.xen.org
https://lists.xen.org/xen-devel
