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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan


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