Re: [PATCH RFC 1/9] xen: Emulate with no writes; compute current instruction length

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 07/02/2014 07:08 PM, Jan Beulich wrote:
>>>> On 02.07.14 at 17:43, <rcojocaru@bitdefender.com> wrote:
>> On 07/02/2014 06:21 PM, Jan Beulich wrote:
>>>> +    struct x86_emulate_ctxt __attribute__((unused)) *ctxt)
>>>
>>> We don't mark unused function arguments like this (and if we did,
>>> we'd want you to use __maybe_unused).
>>
>> OK, thanks. What's the proper way to mark them? Should I go with
>> __maybe_unused then?
> 
> No - don't mark them.

Noted.

>>>> +void hvm_emulate_one_full(bool_t nowrite)
>>>> +{
>>>> +    struct hvm_emulate_ctxt ctx[1] = {};
>>>> +    int rc = X86EMUL_RETRY;
>>>> +
>>>> +    hvm_emulate_prepare(ctx, guest_cpu_user_regs());
>>>> +
>>>> +    while ( rc == X86EMUL_RETRY )
>>>> +    {
>>>> +        if ( nowrite )
>>>> +            rc = hvm_emulate_one_no_write(ctx);
>>>> +        else
>>>> +            rc = hvm_emulate_one(ctx);
>>>> +    }
>>>> +
>>>> +    switch ( rc )
>>>> +    {
>>>> +    case X86EMUL_UNHANDLEABLE:
>>>> +        hvm_inject_hw_exception(TRAP_invalid_op, 
>> HVM_DELIVER_NO_ERROR_CODE);
>>>
>>> Is it certain that #UD is always the right exception here?
>>
>> For our purposes, yes.
> 
> That's not really a good answer for code that isn't there to only suit
> you.

Of course, all I was trying to say is that as far as this function has
been used (only in conjunction with our application), it has always been
the right exception, and I was hoping that somebody might shed some
light on a scenario of possibly problematic uses of it.

>> Of course, that means that I can't really explain what the original
>> author intended (related to the rest of your critique).
> 
> Which clearly won't help acceptance of this code, assuming that's
> your goal.

Obviously acceptance is the goal, but should the Linux code be cleaned
up and handed in, or is it better to try to enhance the emulator as has
been suggested above?


Thanks,
Razvan Cojocaru