[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 2/5] xen/ppc: Implement bitops.h
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |