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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.03.17 at 20:21, <itopan@xxxxxxxxxxxxxxx> wrote:
> On Fri, 17 Mar 2017 05:03:44 -0600
> "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>> >>> On 10.03.17 at 16:50, <itopan@xxxxxxxxxxxxxxx> wrote:
>> > +    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?

I think crashing the domain in such cases is better - injecting
whatever non-architectural exception is only going to defer
problems, making later analysis more difficult.

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

I don't follow - if there are no names, you're free to give them
names. I'm sure you and the VMX maintainers can agree on
something suitable.

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

No need to give it a name though, afaics - just start valid values
at 1.

> (I assume the .pad* fields are checked to
> be 0 for the same reason).

No, they're being checked so that later assigning them a meaning
will be possible without breaking backwards compatibility.

Jan


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