Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[David Howells]

Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> +#define get_scalar_volatile_pointer(x) ({ \
> +	typeof(x) *__p = &(x); \
> +	volatile typeof(x) *__vp = __p; \
> +	(void)(long)*__p; __vp; })
> +#define ACCESS_ONCE(x) (*get_scalar_volatile_pointer(x))

Might this cause two loads from memory under some conditions?  Once for the
fourth line and once for the fifth?

(Apologies if this has already been discussed)

David

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Linus Torvalds]

On Mon, Nov 24, 2014 at 5:30 AM, David Howells <dhowells@redhat.com> wrote:
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>
>> +#define get_scalar_volatile_pointer(x) ({ \
>> +     typeof(x) *__p = &(x); \
>> +     volatile typeof(x) *__vp = __p; \
>> +     (void)(long)*__p; __vp; })
>> +#define ACCESS_ONCE(x) (*get_scalar_volatile_pointer(x))
>
> Might this cause two loads from memory under some conditions?  Once for the
> fourth line and once for the fifth?

Hmm. Since the fourth line isn't volatile, I'd expect gcc to optimize
it away. But you're right, it might be safer to make sure the "test
type" part is something like

   (void)sizeof((long)*__p)

instead - the "sizeof()" means that it will never actually get
evaluated, but the type-checking of casting *__p to (long) will still
catch the case of *__p being some non-scalar type that cannot be cast.

                   Linus

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Alexei Starovoitov]

On Mon, Nov 24, 2014 at 5:30 AM, David Howells <dhowells@redhat.com> wrote:
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>
>> +#define get_scalar_volatile_pointer(x) ({ \
>> +     typeof(x) *__p = &(x); \
>> +     volatile typeof(x) *__vp = __p; \
>> +     (void)(long)*__p; __vp; })
>> +#define ACCESS_ONCE(x) (*get_scalar_volatile_pointer(x))

If the goal is to catch non-scalar users, the following is shorter:
#define ACCESS_ONCE(x) (((typeof(x))0) + *(volatile typeof(x) *)&(x))

it will block union and struct accesses.
If you want to allow union and disallow struct, then replace + with ,

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Linus Torvalds]

On Mon, Nov 24, 2014 at 10:02 AM, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> If the goal is to catch non-scalar users, the following is shorter:
> #define ACCESS_ONCE(x) (((typeof(x))0) + *(volatile typeof(x) *)&(x))

Me likey. It probably works well in practice, although I think

 - the "(typeof(x))0)" seems unnecessary and wrong. Why not just "0"?
The typeof is not just longer, but it is incorrect for pointer types
(you can add 0 to a pointer, but you cannot add two pointers together)

 - it does mean that the resulting type ends up being upgraded to
"int", for the usual C type reasons.

Note that the "upgraded to 'int'" is true with or without the
"(typeof(x))0". If you add two 'char' values, the addition is still
done in 'int'.

Maybe you *meant* that typeof to fix the second problem, like so:

  (typeof(x)) (0 + *(volatile typeof(x) *)&(x))

Hmm? That casts the result of the addition, not the zero.

             Linus

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Christian Borntraeger]

Am 24.11.2014 um 19:35 schrieb Linus Torvalds:
> On Mon, Nov 24, 2014 at 10:02 AM, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>>
>> If the goal is to catch non-scalar users, the following is shorter:
>> #define ACCESS_ONCE(x) (((typeof(x))0) + *(volatile typeof(x) *)&(x))
> 
> Me likey. It probably works well in practice, although I think
> 
>  - the "(typeof(x))0)" seems unnecessary and wrong. Why not just "0"?
> The typeof is not just longer, but it is incorrect for pointer types
> (you can add 0 to a pointer, but you cannot add two pointers together)
> 
>  - it does mean that the resulting type ends up being upgraded to
> "int", for the usual C type reasons.
> 
> Note that the "upgraded to 'int'" is true with or without the
> "(typeof(x))0". If you add two 'char' values, the addition is still
> done in 'int'.
> 
> Maybe you *meant* that typeof to fix the second problem, like so:
> 
>   (typeof(x)) (0 + *(volatile typeof(x) *)&(x))
> 
> Hmm? That casts the result of the addition, not the zero.

Looks really nice, but does not work with ACCESS_ONCE is on the left-hand side:


include/linux/rculist.h: In function 'hlist_add_before_rcu':
./arch/x86/include/asm/barrier.h:127:18: error: lvalue required as left operand of assignment
  ACCESS_ONCE(*p) = (v);      \

Alexei's variant is also broken:

include/linux/cgroup.h: In function 'task_css':
include/linux/compiler.h:381:40: error: invalid operands to binary + (have 'struct css_set *' and 'struct css_set * volatile')
 #define ACCESS_ONCE(x) (((typeof(x))0) + *(volatile typeof(x) *)&(x))

Anyone with a new propopal? ;-)                                        ^

Christian

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Linus Torvalds]

On Mon, Nov 24, 2014 at 11:07 AM, Christian Borntraeger
<borntraeger@de.ibm.com> wrote:
>
> Looks really nice, but does not work with ACCESS_ONCE is on the left-hand side:

Oh, I forgot about that. And that was indeed why I had done that whole
helper macro originally, with ACCESS_ONCE() itself just being the
dereference of the pointer.

Duh.

                  Linus

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Paul E. McKenney]

On Mon, Nov 24, 2014 at 11:14:42AM -0800, Linus Torvalds wrote:
> On Mon, Nov 24, 2014 at 11:07 AM, Christian Borntraeger
> <borntraeger@de.ibm.com> wrote:
> >
> > Looks really nice, but does not work with ACCESS_ONCE is on the left-hand side:
> 
> Oh, I forgot about that. And that was indeed why I had done that whole
> helper macro originally, with ACCESS_ONCE() itself just being the
> dereference of the pointer.

OK, how about the following?

It complains if the variable is too large, for example, long long on
32-bit systems or large structures.  It is OK loading from and storing
to small structures as well, which I am having a hard time thinking of
as a disadvantage.

							Thanx, Paul

------------------------------------------------------------------------

#define get_scalar_volatile_pointer(x) ({ \
	volatile typeof(x) *__vp = &(x); \
	BUILD_BUG_ON(sizeof(*__vp) != sizeof(char) && \
		     sizeof(*__vp) != sizeof(short) && \
		     sizeof(*__vp) != sizeof(int) && \
		     sizeof(*__vp) != sizeof(long)); \
	__vp; })
#define ACCESS_ONCE(x) (*get_scalar_volatile_pointer(x))

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Linus Torvalds]

On Mon, Nov 24, 2014 at 11:42 AM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
>
> OK, how about the following?

Ugh. Disgusting.

Why the heck isn't it just "sizeof(*__vp) <= sizeof(long)"?

If the architecture has a 3-byte scalar type, then it probably has a
3-byte load.

> It complains if the variable is too large, for example, long long on
> 32-bit systems or large structures.  It is OK loading from and storing
> to small structures as well, which I am having a hard time thinking of
> as a disadvantage.

.. but that's *exactly* the gcc bug in question. It's a word-sized
struct that gcc loads twice.

                          Linus

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Paul E. McKenney]

On Mon, Nov 24, 2014 at 12:19:11PM -0800, Linus Torvalds wrote:
> On Mon, Nov 24, 2014 at 11:42 AM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> >
> > OK, how about the following?
> 
> Ugh. Disgusting.
> 
> Why the heck isn't it just "sizeof(*__vp) <= sizeof(long)"?
> 
> If the architecture has a 3-byte scalar type, then it probably has a
> 3-byte load.

Because I was allowing for the possibility of a 3-byte struct, which
as you point out below...

> > It complains if the variable is too large, for example, long long on
> > 32-bit systems or large structures.  It is OK loading from and storing
> > to small structures as well, which I am having a hard time thinking of
> > as a disadvantage.
> 
> .. but that's *exactly* the gcc bug in question. It's a word-sized
> struct that gcc loads twice.

...was a stupid thought anyway.

OK, how about the attempt below?  The initialization of __p complains
for structures and unions, but gets optimized out.

							Thanx, Paul

-------------------------------------------------------------------------

#define get_scalar_volatile_pointer(x) ({ \
	typeof(x) __maybe_unused __p = 0; \
	volatile typeof(x) *__vp = &(x); \
	BUILD_BUG_ON(sizeof(*__vp) > sizeof(long)); \
	__vp; })
#define ACCESS_ONCE(x) (*get_scalar_volatile_pointer(x))

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Christian Borntraeger]

Am 24.11.2014 um 20:42 schrieb Paul E. McKenney:
> On Mon, Nov 24, 2014 at 11:14:42AM -0800, Linus Torvalds wrote:
>> On Mon, Nov 24, 2014 at 11:07 AM, Christian Borntraeger
>> <borntraeger@de.ibm.com> wrote:
>>>
>>> Looks really nice, but does not work with ACCESS_ONCE is on the left-hand side:
>>
>> Oh, I forgot about that. And that was indeed why I had done that whole
>> helper macro originally, with ACCESS_ONCE() itself just being the
>> dereference of the pointer.
> 
> OK, how about the following?
> 
> It complains if the variable is too large, for example, long long on
> 32-bit systems or large structures.  It is OK loading from and storing
> to small structures as well, which I am having a hard time thinking of
> as a disadvantage.

Well, the motivation for this series was that gcc  4.6 and 4.7 might ignore volatile for
such a case, see the original thread and this data structure

union ipte_control {
        unsigned long val;
        struct {
                unsigned long k  : 1;
                unsigned long kh : 31;
                unsigned long kg : 32;
        };
};

> ------------------------------------------------------------------------
> 
> #define get_scalar_volatile_pointer(x) ({ \
> 	volatile typeof(x) *__vp = &(x); \
> 	BUILD_BUG_ON(sizeof(*__vp) != sizeof(char) && \
> 		     sizeof(*__vp) != sizeof(short) && \
> 		     sizeof(*__vp) != sizeof(int) && \
> 		     sizeof(*__vp) != sizeof(long)); \
> 	__vp; })
> #define ACCESS_ONCE(x) (*get_scalar_volatile_pointer(x))
> 

This gives also several compiler errors when accessing u64 on a 32bit system. This is expected, but more widespread than expected - ouch.

Christian

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[David Howells]

Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> Looks really nice, but does not work with ACCESS_ONCE is on the left-hand side:
> 
> 
> include/linux/rculist.h: In function 'hlist_add_before_rcu':
> ./arch/x86/include/asm/barrier.h:127:18: error: lvalue required as left operand of assignment
>   ACCESS_ONCE(*p) = (v);      \
> 
> Alexei's variant is also broken:
> 
> include/linux/cgroup.h: In function 'task_css':
> include/linux/compiler.h:381:40: error: invalid operands to binary + (have 'struct css_set *' and 'struct css_set * volatile')
>  #define ACCESS_ONCE(x) (((typeof(x))0) + *(volatile typeof(x) *)&(x))
> 
> Anyone with a new propopal? ;-)                                        ^

Reserve ACCESS_ONCE() for reading and add an ASSIGN_ONCE() or something like
that for writing?

David

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Linus Torvalds]

On Mon, Nov 24, 2014 at 12:04 PM, David Howells <dhowells@redhat.com> wrote:
>
> Reserve ACCESS_ONCE() for reading and add an ASSIGN_ONCE() or something like
> that for writing?

I wouldn't mind that. We've had situations where reading and writing
isn't really similar - like alpha where reading a byte is atomic, but
writing one isn't.

Then we could also make it have the "get_user()/put_user()" kind of
semantics - .and then use the same "sizeopf()" tricks that we use for
get_user/put_user.

That would actually work around the gcc bug a completely different way:

  #define ACCESS_ONCE(p) \
      ({ typeof(*p) __val; __read_once_size(p, &__val, sizeof(__val)); __val; })

and then we can do things like this:

  static __always_inline void __read_once_size(volatile void *p, void
*res, int size)
  {
       switch (size) {
       case 1: *(u8 *)res = *(volatile u8 *)p; break;
       case 2: *(u16 *)res = *(volatile u16 *)p; break;
       case 4: *(u32 *)res = *(volatile u32 *)p; break;
#ifdef CONFIG_64BIT
       case 8: *(u64 *)res = *(volatile u64 *)p; break;
#endif
       }
  }

and same for ASSIGN_ONCE(val, p).

That also hopefully avoids the whole "oops, gcc has a bug", because
the actual volatile access is always done using a scalar type, even if
the type of "__val" may in fact be a structure.

Christian, how painful would that be? Sorry to try to make you do a
totally different approach..

                  Linus

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Christian Borntraeger]

Am 24.11.2014 um 21:34 schrieb Linus Torvalds:
> On Mon, Nov 24, 2014 at 12:04 PM, David Howells <dhowells@redhat.com> wrote:
>>
>> Reserve ACCESS_ONCE() for reading and add an ASSIGN_ONCE() or something like
>> that for writing?
> 
> I wouldn't mind that. We've had situations where reading and writing
> isn't really similar - like alpha where reading a byte is atomic, but
> writing one isn't.
> 
> Then we could also make it have the "get_user()/put_user()" kind of
> semantics - .and then use the same "sizeopf()" tricks that we use for
> get_user/put_user.
> 
> That would actually work around the gcc bug a completely different way:
> 
>   #define ACCESS_ONCE(p) \
>       ({ typeof(*p) __val; __read_once_size(p, &__val, sizeof(__val)); __val; })
> 
> and then we can do things like this:
> 
>   static __always_inline void __read_once_size(volatile void *p, void
> *res, int size)
>   {
>        switch (size) {
>        case 1: *(u8 *)res = *(volatile u8 *)p; break;
>        case 2: *(u16 *)res = *(volatile u16 *)p; break;
>        case 4: *(u32 *)res = *(volatile u32 *)p; break;
> #ifdef CONFIG_64BIT
>        case 8: *(u64 *)res = *(volatile u64 *)p; break;
> #endif
>        }
>   }
> 
> and same for ASSIGN_ONCE(val, p).
> 
> That also hopefully avoids the whole "oops, gcc has a bug", because
> the actual volatile access is always done using a scalar type, even if
> the type of "__val" may in fact be a structure.
> 
> Christian, how painful would that be? Sorry to try to make you do a
> totally different approach..

That looks like a lot of changes all over ACCESS_ONCE -> ASSIGN_ONCE:
git grep "ACCESS_ONCE.*=.*" 
gives me 200 placea not in Documentation.

Then there is still the 64bit accesses on 32bit via ACCESS_ONCE problem, which we could detect with a default cause in your code. We would need to audit and fix all places :-/


So the last proposal from Alexei, seems easier (for me at least :-) )

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Linus Torvalds]

On Mon, Nov 24, 2014 at 12:53 PM, Christian Borntraeger
<borntraeger@de.ibm.com> wrote:
>
> That looks like a lot of changes all over ACCESS_ONCE -> ASSIGN_ONCE:
> git grep "ACCESS_ONCE.*=.*"
> gives me 200 placea not in Documentation.

Yeah, that's a bit annoying.

How about a combination of the two:

 - accept the fact that right now ACCESS_ONCE() is fairly widespread
(even for writing)

 - but also admit that we'd be better off with a nicer interface

and make the solution be:

 - make ACCESS_ONCE() only work on scalars, and deprecate it

 - add new "read_once()" and "write_once()" interfaces that *do* work
on (appropriately sized) structures and unions, and start migrating
things over. In particular, start with the ones that can no longer use
ACCESS_ONCE() because they aren't scalar..

That second point would make the conversion patches actually easier to
read. Instead of this:

 static inline int arch_spin_is_locked(arch_spinlock_t *lock)
 {
-       struct __raw_tickets tmp = ACCESS_ONCE(lock->tickets);
+       arch_spinlock_t tmp = {};

-       return tmp.tail != tmp.head;
+       tmp.head_tail =ACCESS_ONCE(lock->head_tail);
+       return tmp.tickets.tail != tmp.tickets.head;
 }

which isn't *complex*, but is also not an obvious conversion, we'd have just

 static inline int arch_spin_is_locked(arch_spinlock_t *lock)
 {
-       struct __raw_tickets tmp = ACCESS_ONCE(lock->tickets);
-       struct __raw_tickets tmp = read_once(lock->tickets);

        return tmp.tail != tmp.head;
 }

which is a much simpler and more obvious change.

And then we could slowly try to migrate existing ACCESS_ONCE() users
over (particularly writers).

Hmm? Too much?

                     Linus

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Christian Borntraeger]

Am 24.11.2014 um 22:02 schrieb Linus Torvalds:
> On Mon, Nov 24, 2014 at 12:53 PM, Christian Borntraeger
> <borntraeger@de.ibm.com> wrote:
>>
>> That looks like a lot of changes all over ACCESS_ONCE -> ASSIGN_ONCE:
>> git grep "ACCESS_ONCE.*=.*"
>> gives me 200 placea not in Documentation.
> 
> Yeah, that's a bit annoying.
> 
> How about a combination of the two:
> 
>  - accept the fact that right now ACCESS_ONCE() is fairly widespread
> (even for writing)
> 
>  - but also admit that we'd be better off with a nicer interface
> 
> and make the solution be:
> 
>  - make ACCESS_ONCE() only work on scalars, and deprecate it
> 
>  - add new "read_once()" and "write_once()" interfaces that *do* work
> on (appropriately sized) structures and unions, and start migrating
> things over. In particular, start with the ones that can no longer use
> ACCESS_ONCE() because they aren't scalar..
> 
> That second point would make the conversion patches actually easier to
> read. Instead of this:
> 
>  static inline int arch_spin_is_locked(arch_spinlock_t *lock)
>  {
> -       struct __raw_tickets tmp = ACCESS_ONCE(lock->tickets);
> +       arch_spinlock_t tmp = {};
> 
> -       return tmp.tail != tmp.head;
> +       tmp.head_tail =ACCESS_ONCE(lock->head_tail);
> +       return tmp.tickets.tail != tmp.tickets.head;
>  }
> 
> which isn't *complex*, but is also not an obvious conversion, we'd have just
> 
>  static inline int arch_spin_is_locked(arch_spinlock_t *lock)
>  {
> -       struct __raw_tickets tmp = ACCESS_ONCE(lock->tickets);
> -       struct __raw_tickets tmp = read_once(lock->tickets);
> 
>         return tmp.tail != tmp.head;
>  }
> 
> which is a much simpler and more obvious change.
> 
> And then we could slowly try to migrate existing ACCESS_ONCE() users
> over (particularly writers).
> 
> Hmm? Too much?

I will give it a try. I will start with Alexei's version for ACCESS_ONCE and your snippets to build read_once and write_once. The only open question is, what to do with the "too large" accesses. Pauls initial patch showed several 
places, e.g. kernel/sched/fair.c accessing an u64 even on 32bit:
[...]
   age_stamp = ACCESS_ONCE(rq->age_stamp);
        avg = ACCESS_ONCE(rq->rt_avg);
[...]

I think I will simply not touch those...



Christian

Re: [PATCH/RFC 6/7] arm64: Replace ACCESS_ONCE for spinlock code with barriers

[Christian Borntraeger]

Am 24.11.2014 um 14:03 schrieb Christian Borntraeger:
> ACCESS_ONCE does not work reliably on non-scalar types. For
> example gcc 4.6 and 4.7 might remove the volatile tag for such
> accesses during the SRA (scalar replacement of aggregates) step
> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145)
> 
> Change the spinlock code to access the lock with a barrier.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/arm64/include/asm/spinlock.h | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/spinlock.h b/arch/arm64/include/asm/spinlock.h
> index c45b7b1..f72dc64 100644
> --- a/arch/arm64/include/asm/spinlock.h
> +++ b/arch/arm64/include/asm/spinlock.h
> @@ -99,12 +99,15 @@ static inline int arch_spin_value_unlocked(arch_spinlock_t lock)
> 
>  static inline int arch_spin_is_locked(arch_spinlock_t *lock)
>  {
> -	return !arch_spin_value_unlocked(ACCESS_ONCE(*lock));
> +	arch_spinlock_t lockval = *lock;
> +	barrier();
> +	return !arch_spin_value_unlocked(lockval);
>  }
> 
>  static inline int arch_spin_is_contended(arch_spinlock_t *lock)
>  {
> -	arch_spinlock_t lockval = ACCESS_ONCE(*lock);
> +	arch_spinlock_t lockval = *lock;
> +	barrier();
>  	return (lockval.next - lockval.owner) > 1;
>  }
>  #define arch_spin_is_contended	arch_spin_is_contended
> 
FWIW,

we could also make this with ACCESS_ONCE, but this requires to change the definition of arch_spinlock_t for arm64 to be a union. I am a bit reluctant to do these changes without being able to test. Let me know if this is preferred and if somebody else can test.

Christian

Re: [PATCH/RFC 7/7] kernel: Force ACCESS_ONCE to work only on scalar types

[Alexei Starovoitov]

On Mon, Nov 24, 2014 at 4:00 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Mon, Nov 24, 2014 at 2:58 PM, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>>
>> I've changed gcc pr58145-1.c reproducer to use
>> __read_once_size() approach above
>
> I don't think you did.
>
>> modified reproducer:
>> struct S { unsigned int data; };
>> void bar(int val)
>> {
>>   struct S _s = { .data = val };
>>   *(volatile struct S *) 0x880000UL = ACCESS_ONCE(&_s);
>> }
>
> My approach never had "volatile struct S *". The only volatile
> pointers were the actual byte/word/etc pointers, and those generated

you're right. In my invalid snippet above the ACCESS_ONCE
to struct on stack gets optimized away and only 'volatile struct *'
in left hand side is triggering the bug.

Have tried the following which blends your proposal
with original code from Christian:
/* bad
#define ACCESS_ONCE(x) *((volatile typeof(x) *)&(x))
*/

/* good */
#define ACCESS_ONCE(p) \
      ({ typeof(*p) __val; __read_once_size(p, &__val, sizeof(__val)); __val; })

static __always_inline void __read_once_size(volatile void *p, void
*res, int size)
{
     switch (size) {
     case 1: *(u8 *)res = *(volatile u8 *)p; break;
     case 2: *(u16 *)res = *(volatile u16 *)p; break;
     case 4: *(u32 *)res = *(volatile u32 *)p; break;
     case 8: *(u64 *)res = *(volatile u64 *)p; break;
     }
}

union ipte_control {
        unsigned long val;
        struct {
                unsigned long k  : 1;
                unsigned long kh : 31;
                unsigned long kg : 32;
        };
};

struct kvm_vcpu {
        union ipte_control ic;
};

void ipte_unlock_siif(struct kvm_vcpu *vcpu)
{
        union ipte_control old, new, *ic;

        ic = &vcpu->ic;
        do {
                new = old = ACCESS_ONCE(ic);
                new.kh--;
                if (!new.kh)
                        new.k = 0;
        } while (cmpxchg(&ic->val, old.val, new.val) != old.val);
}

generated code looks correct with and without strict-aliasing
and volatile marking is preserved properly.
(to check for volatile marks add -fdump-tree-optimized
 and look for {v} in *.optimized)

> Pretty? No. But then, the standard C aliasing rules are so broken that
> "pretty" doesn't really come into play..

Agree. I don't see any warnings or code generation issues with and
without strict-aliasing with your original __read_once_size(), so no need
to play union tricks. Initially I was worried that extra always_inline
function will make generated code worse in critical paths where
ACCESS_ONCE is used, but after looking close enough, it seems
all should be fine.

Note, with unmodified ACCESS_ONCE all architectures (even x64)
are missing volatile markings with gcc 4.6.3, 4.7.2 for Christian's
use case.