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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 19/02/18 07:50, Jan Beulich wrote:
>>>> On 16.02.18 at 18:02, <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.
> And bitfields as well, even if the spec says so just in a foot note.
>
>>> 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.
> That's for the simple reason that _there_ it doesn't matter:
> Truncation from (signed/unsigned) long to int32_t works all the
> same. Plus even if I change the cast to include "signed", the
> type of the expression that's converted to int32_t is _still_
> unsigned long (due to stub_va's type).
>
>> 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.
> Thanks. I'll add it to the cast, but again reluctantly, as there it is
> truly pointless (as per above).

IMO, this justification just goes further to show that both uses of
signed are pointless.

~Andrew

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