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

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

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?

>> +int hvm_emulate_one_no_write(
>> +    struct hvm_emulate_ctxt *hvmemul_ctxt)
>> +{
> 
> This must be pretty redundant with hvm_emulate_one(), and hence
> most if not all of the redundancy should be factored out.

Will do.

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

>> +int hvm_get_insn_length(
>> +    struct hvm_emulate_ctxt *hvmemul_ctxt)
>> +{
> 
> There again looks to be quite a bit of redundancy here. Please let's
> avoid having n copies of (almost) the same code.

Will factor it out.

>> --- /dev/null
>> +++ b/xen/arch/x86/inat-tables.c
> 
> I'm not going to look at this in much detail, just a couple of general
> notes:
> - at least some of the information is redundant with the full x86
>   emulator; as before redundancy should be avoided
> - many if not all of the arrays here appear to only be used locally,
>   and hence ought to be static
> - some of the tables are extremely sparse (take
>   inat_escape_table_3_3[] as an example); would be nice to collapse
>   those
> - making future changes/additions to these tables is going to be
>   pretty hard with them having neither suitable names nor comments
> - coding style (also elsewhere) seems to be Linux'es, yet you don't
>   mention in the description that they come from Linux (and if they
>   don't you'd be asked to convert them to Xen style)

Yes, the files do come from Linux, hence the coding style and the rest
of the issues. We've left them as they are on purpose, to try to reflect
that. I was under the impression that at least some of them say so at
the beginning of the file.

I'll make sure to mention this explicitly in subsequent retries.

Of course, that means that I can't really explain what the original
author intended (related to the rest of your critique).


Thanks,
Razvan Cojocaru