[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 2/5] xen/ppc: Implement bitops.h


  • To: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Fri, 8 Sep 2023 08:17:48 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=q1iV9rvlr/jPTJH0NXdUnoA3qf0pAnHfvDnppZSjljY=; b=k6Nzl3P9/8pU9hiWGYEBC/vFaK3AhH3fH+nZsbXgCe+aqIBNP70+84FwE30l2ec+eMYnYUaGT/oT1HM41gBh45/mQODU0ItsijLBbM5h1rxCM0Pz75ujSpNAOO50RARCMGndqUk+VYasL1SbF6A64Yu8B7IoL0RZrxE2HW59kxOl2DI6n+b6eGzINf/0u9pbG4E/kp9awkCk0gCtPst/MSftNLMRL9CjdM6FeggTeR/hsBYIBl/bd/sFJbbrBU7tSh7+mVJfykTUShRhwBaVg9O8WZfRC/NjiqCVtyfTeWo653VLtAgv9NSXRqMKy6Dj7Voh7G6VkQ1yuZasw3EawA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=B+lL0dJDXPNOD1tjcPL0QtqOOstm4RodHXnVg8hlaMARoVLOCTQ0N3RNz+0Rvih5FfPZuCkYPOp4IX4sH5mvMwVCSIlunxmaWJ8RjtZcOvAFD9hr9QkUGzTEWD8ra4naXQpJwatqc2EEh7khchSFRb+itZftYfWkL4CK1fdqtZuj0qVSe6+3t3IlirzYCNSEck6Zo5Z+bvriZVMwFKJUG15hgN2+cRdqmxhqJJnk7YjVgOHENHUZ7JvEuJtJOE2ellb+HzA4XYPRsi3NX8anRNNTzU+wgfFMOzvlGxkjwEf6ijNuSrDo76/81knH/Xs8rOxx5br5vPZ/MHEkGzAGWg==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Timothy Pearson <tpearson@xxxxxxxxxxxxxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Fri, 08 Sep 2023 06:17:58 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 08.09.2023 01:12, Shawn Anastasio wrote:
> On 9/5/23 10:19 AM, Jan Beulich wrote:
>> On 01.09.2023 20:25, Shawn Anastasio wrote:
>>> +#define DEFINE_TESTOP(fn, op, eh)                                          
>>>     \
>>> +static inline unsigned long fn(unsigned long mask, volatile unsigned int 
>>> *_p)  \
>>> +{                                                                          
>>>     \
>>> +    unsigned long old, t;                                                  
>>>     \
>>> +    unsigned int *p = (unsigned int *)_p;                                  
>>>     \
>>> +    asm volatile ( PPC_ATOMIC_ENTRY_BARRIER                                
>>>     \
>>> +                   "1: lwarx %0,0,%3,%4\n"                                 
>>>     \
>>> +                   #op "%I2 %1,%0,%2\n"                                    
>>>     \
>>> +                   "stwcx. %1,0,%3\n"                                      
>>>     \
>>> +                   "bne- 1b\n"                                             
>>>     \
>>> +                   PPC_ATOMIC_EXIT_BARRIER                                 
>>>     \
>>> +                   : "=&r" (old), "=&r" (t)                                
>>>     \
>>> +                   : "rK" (mask), "r" (p), "n" (eh)                        
>>>     \
>>> +                   : "cc", "memory" );                                     
>>>     \
>>> +    return (old & mask);                                                   
>>>     \
>>> +}
>>> +
>>> +DEFINE_TESTOP(test_and_set_bits, or, 0)
>>
>> Why can't test_and_clear_bits() not use this macro-ization? And if it
>> can't and hence there's only this single use, wouldn't it make sense
>> to avoid going through a macro here, too?
>>
> 
> I've just tried this, but unfortunately the "rK" constraint on the mask
> operand only works when instructions have both a reg/reg/reg as well as
> a reg/reg/imm16 form. This is the case for `or` but not `andc`. I guess
> it would be better to keep the two separate implementations rather than
> try to accomodate both cases in the macro somehow (e.g, by making the
> constraint's type a macro parameter as well).
> 
> I can also change the macro template into a standard function for just
> test_and_set_bits, given that it's the only user as you pointed out.
> 
> As for your previous observation about the superfluous `p` variable, it
> would seem the same applies to the macro here. Other than casting away
> the volatile qualifier on `p_` it doesn't seem to be doing much, so I'm
> inclined to remove it.

Right. Comments like this are generally intended to apply to the entire
patch or even entire series.

Jan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.