Re: [Xen-devel] [PATCH] x86/PV: avoid indirect call/thunk in I/O emulation

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/02/18 16:21, Jan Beulich wrote:
>>>> On 16.02.18 at 16:50, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 16/02/18 08:00, Jan Beulich wrote:
>>>>>> On 15.02.18 at 17:53, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> On 15/02/18 16:03, Jan Beulich wrote:
>>>>> --- a/xen/arch/x86/pv/emul-priv-op.c
>>>>> +++ b/xen/arch/x86/pv/emul-priv-op.c
>>>>> @@ -73,55 +73,42 @@ void (*pv_post_outb_hook)(unsigned int p
>>>>>  
>>>>>  typedef void io_emul_stub_t(struct cpu_user_regs *);
>>>>>  
>>>>> -void __x86_indirect_thunk_rcx(void);
>>>>> -
>>>>>  static io_emul_stub_t *io_emul_stub_setup(struct priv_op_ctxt *ctxt, u8 
>>>>> opcode,
>>>>>                                            unsigned int port, unsigned 
>>>>> int bytes)
>>>>>  {
>>>>>      struct stubs *this_stubs = &this_cpu(stubs);
>>>>>      unsigned long stub_va = this_stubs->addr + STUB_BUF_SIZE / 2;
>>>>> +    signed long disp;
>>>> I'm not in favour of sprinkling 'signed' all over the code base.
>>>>
>>>> long is already unambiguous, and a far more common construct to encounter. 
>>> It's this "common" which made me use "signed" here (and in similar
>>> places elsewhere) - we still have far too many cases left where plain
>>> short/int/long are used when the unsigned variant would actually be
>>> better. Hence, _at least_ until those issues are all gone, I very much
>>> prefer making explicit when signed quantities are meant.
>> But why does it actually matter?  Noone can blindly switch between
>> prefixed and unprefixed versions without understanding what is going on,
>> and disp here is a signed usage.
>>
>> Also, are you proposing that the end goal for a "clean" coding style is
>> to have all native C types explicitly prefixed with signed or unsigned? 
>> If not (and I really hope not), then putting signed in here is causing
>> extra work for us to undo later.
> Personally I would indeed prefer for "signed" to always be added
> when a signed quantity is meant. Of course you have to look at
> the context when considering to switch a plain use to unsigned,
> but quickly recognizing non-candidates of such a transformation
> is possible only when they are amended by "signed".

The first thing I ever thing when seeing "signed" is "the author doesn't
understand C", which is a direct consequence of being an redundant
keyword which is barely used.  The only case I'm aware of it actually
having a semantic use is with char, whose signed-ness is implementation
defined, rather than specified.

Personally, I find that redundant signed hinders readability, rather
than helping it, and I'd certainly prefer not to see signed inserted all
over the codebase.  Either way, the topic should be discussed as part of
CODING_STYLE, rather than being argued over in a single patch.

> That said, if dropping the "signed" is the only way to get your ack
> (and besides the expression folding you had no other comments
> so far), I certainly will (reluctantly).

The other thing I find particularly odd is that you use signed for the
declaration, but not for the cast.

I don't want to block this patch so Acked-by: Andrew Cooper
<andrew.cooper3@xxxxxxxxx>, but would prefer if we didn't litter signed
everywhere.  If you are going to insist on using signed, then please use
it consistently.

Also, I think it is important to address the subject with CODING_STYLE
and the community sooner rather than later, to avoid the argument
resurfacing on future patches.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel