Re: [PATCH] x86emul: use unambiguous register names

["Jan Beulich" <JBeulich@suse.com>]

>>> On 03.01.17 at 14:30, <andrew.cooper3@citrix.com> wrote:
> On 03/01/17 13:01, Jan Beulich wrote:
>> --- a/xen/arch/x86/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate.c
>> @@ -21,6 +21,8 @@
>>  #undef cpuid
>>  #undef wbinvd
>>  
>> +#define r(name) r ## name
>> +
> 
> Hmm.  I am no overwhelmed with this syntax, but I can't propose an
> alternative, so ok.

I kind of suspected such a response.

>> @@ -2716,36 +2716,36 @@ x86_emulate(
>>          struct segment_register cs, sreg;
>>  
>>      case 0x00 ... 0x05: add: /* add */
>> -        emulate_2op_SrcV("add", src, dst, _regs.eflags);
>> +        emulate_2op_SrcV("add", src, dst, _regs.r(flags));
>>          break;
>>  
> 
> All of these types of operations only adjust the arithmetic flags, so
> could legitimately use eflags alone.  Is it worth reducing?

Yes, but not now and here: Using _eflags breaks the inline asm which
these macros resolve to.

>> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
>> @@ -583,41 +583,9 @@ x86_emulate(
>>      const struct x86_emulate_ops *ops);
>>  
>>  #ifndef NDEBUG
>> -/*
>> - * In debug builds, wrap x86_emulate() with some assertions about its expected
>> - * behaviour.
>> - */
> 
> I'd leave this comment here as well.

Hmm, in that case I'd drop it at the definition site. I don't think we
need to have the comment in both places. What do you think?

> Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks (but I'll wait for your feedback to the above),
Jan


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