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

[George Dunlap <dunlapg@xxxxxxxxx>]

On Fri, Feb 16, 2018 at 5:02 PM, Andrew Cooper
<andrew.cooper3@xxxxxxxxxx> wrote:
> 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.

+1

FWIW I'd also like to avoid using 'signed''.

 -George

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