[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 13/34] xen/riscv: introduce cmpxchg.h
On 23.01.2024 13:18, Oleksii wrote: > On Tue, 2024-01-23 at 11:28 +0100, Jan Beulich wrote: >> On 23.01.2024 11:15, Oleksii wrote: >>> On Mon, 2024-01-22 at 17:27 +0100, Jan Beulich wrote: >>>> On 22.12.2023 16:12, Oleksii Kurochko wrote: >>>>> +static inline unsigned long __xchg(volatile void *ptr, >>>>> unsigned >>>>> long x, int size) >>>>> +{ >>>>> + switch (size) { >>>>> + case 1: >>>>> + return __cmpxchg_case_1(ptr, (uint32_t)-1, x); >>>>> + case 2: >>>>> + return __cmpxchg_case_2(ptr, (uint32_t)-1, x); >>>> >>>> How are these going to work? You'll compare against ~0, and if >>>> the >>>> value >>>> in memory isn't ~0, memory won't be updated; you will only >>>> (correctly) >>>> return the value found in memory. >>>> >>>> Or wait - looking at __cmpxchg_case_{1,2}() far further down, you >>>> ignore >>>> "old" there. Which apparently means they'll work for the use >>>> here, >>>> but >>>> not for the use in __cmpxchg(). >>> Yes, the trick is that old is ignored and is read in >>> __emulate_cmpxchg_case1_2() before __cmpxchg_case_4 is called: >>> do >>> { >>> read_val = >>> read_func(aligned_ptr); >>> swapped_new = read_val & >>> ~mask; >>> swapped_new |= >>> masked_new; >>> ret = cmpxchg_func(aligned_ptr, read_val, >>> swapped_new); >>> } while ( ret != read_val >>> ); >>> read_val it is 'old'. >>> >>> But now I am not 100% sure that it is correct for __cmpxchg... >> >> It just can't be correct - you can't ignore "old" there. I think you >> want simple cmpxchg primitives, which xchg then uses in a loop (while >> cmpxchg uses them plainly). > But xchg doesn't require 'old' value, so it should be ignored in some > way by cmpxchg. Well, no. If you have only cmpxchg, I think your only choice is - as said - to read the old value and then loop over cmpxchg until that succeeds. Not really different from other operations which need emulating using cmpxchg. >>>>> +static always_inline unsigned short __cmpxchg_case_2(volatile >>>>> uint32_t *ptr, >>>>> + uint32_t >>>>> old, >>>>> + uint32_t >>>>> new) >>>>> +{ >>>>> + (void) old; >>>>> + >>>>> + if (((unsigned long)ptr & 3) == 3) >>>>> + { >>>>> +#ifdef CONFIG_64BIT >>>>> + return __emulate_cmpxchg_case1_2((uint64_t *)ptr, new, >>>>> + readq, >>>>> __cmpxchg_case_8, >>>>> 0xffffU); >>>> >>>> What if ((unsigned long)ptr & 7) == 7 (which is a sub-case of >>>> what >>>> the >>>> if() above checks for? Isn't it more reasonable to require >>>> aligned >>>> 16-bit quantities here? Or if mis-aligned addresses are okay, you >>>> could >>>> as well emulate using __cmpxchg_case_4(). >>> Yes, it will be more reasonable. I'll use IS_ALIGNED instead. >> >> Not sure I get your use of "instead" here correctly. There's more >> to change here than just the if() condition. > I meant something like: > > if ( IS_ALIGNED(ptr, 16) ) > __emulate_cmpxchg_case1_2(...); > else > assert_failed("ptr isn't aligned\n"); Except that you'd better not use assert_failed() directly anywhere, and the above is easier as ASSERT(IS_ALIGNED(ptr, 16)); __emulate_cmpxchg_case1_2(...); anyway (leaving aside that I guess you mean 2, not 16). Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |