Re: [PATCH v9 03/17] x86/split_lock: Align x86_capability to unsigned long to avoid split locked access

[Thomas Gleixner]

On Tue, 25 Jun 2019, Fenghua Yu wrote:
> On Mon, Jun 24, 2019 at 03:12:49PM +0000, David Laight wrote:
> > > @@ -93,7 +93,9 @@ struct cpuinfo_x86 {
> > >  	__u32			extended_cpuid_level;
> > >  	/* Maximum supported CPUID level, -1=no CPUID: */
> > >  	int			cpuid_level;
> > > -	__u32			x86_capability[NCAPINTS + NBUGINTS];
> > > +	/* Aligned to size of unsigned long to avoid split lock in atomic ops */
> > 
> > Wrong comment.
> > Something like:
> > 	/* Align to sizeof (unsigned long) because the array is passed to the
> > 	 * atomic bit-op functions which require an aligned unsigned long []. */
> 
> The problem we try to fix here is not because "the array is passed to the
> atomic bit-op functions which require an aligned unsigned long []".
> 
> The problem is because of the possible split lock issue. If it's not because
> of split lock issue, there is no need to have this patch.
> 
> So I would think my comment is right to point out explicitly why we need
> this alignment.

The underlying problem why you need that alignemnt is that the invocation
of the bitops does a type cast. And that's independent of split lock. Split
lock makes the problem visible. So the alignment papers over that. And
while this 'works' in x86 it's fundamentaly broken on big endian. So no,
your comment is not right to the point because it gives the wrong
information.

> > 
> > > +	__u32			x86_capability[NCAPINTS + NBUGINTS]
> > > +				__aligned(sizeof(unsigned long));
> > 
> > It might be better to use a union (maybe unnamed) here.
> 
> That would be another patch. This patch just simply fixes the split lock
> issue.

Why? That's a straight forward and obvious fix and way better than these
alignment games. It's still wrong for BE....

So anyway, this wants a comment which explains the underlying issue and not
a comment which blurbs about split locks.

Thanks,

	tglx