linux-kernel - Re: [PATCH 6/6] x86/platform/uv/BAU: Add payload descriptor qualifier

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.20.1702161930120.3543@nanos>
Date:   Thu, 16 Feb 2017 19:42:07 +0100 (CET)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Andrew Banman <abanman@....com>
cc:     mingo@...hat.com, akpm@...ux-foundation.org, hpa@...or.com,
        mike.travis@....com, rja@....com, sivanich@....com, x86@...nel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 6/6] x86/platform/uv/BAU: Add payload descriptor
 qualifier

On Tue, 14 Feb 2017, Andrew Banman wrote:

> On UV4, the destination agent verifies each message by checking the
> descriptor qualifier field of the message payload. Messages without this
> field set to 0x534749 will cause a hub error to assert.

Ok.

What's missing here is:

  Seperate the message structs for uv123 and uv4. 

> Make this the default action for future architectures, anticipating they
> will have the same requirement.

That's a guarantee to cause issues when uv5 comes around. The way better
solution for this is to do:

enum uv_bau_version {
     UV_BAU_V1 = 1,
     UV_BAU_V2,
     UV_BAU_V3,
     UV_BAU_V4,
};

Make bau->uvhub_version type uv_bau_version and use the enum constants in
the switch case. That way the compiler will catch you when you add
UV_BAU_V5 and forgot to update that switch case. That's probably handy to
have that in a few other places which switch on the bau version.

> -struct bau_msg_payload {
> +struct uv1_2_3_bau_msg_payload {
>  	unsigned long	address;		/* signifies a page or all
>  						   TLB's of the cpu */
>  	/* 64 bits */
> @@ -236,6 +238,20 @@ struct bau_msg_payload {
>  	unsigned int	reserved1:32;		/* not usable */
>  };
>  
> +struct uv4_bau_msg_payload {
> +	unsigned long	address;		/* signifies a page or all
> +						 * TLB's of the cpu
> +						 */

Please get rid of these tail comments. Either document the struct members
with a comment above the member or even better use the KernelDoc comment
format above the struct to document it.

> +	/* 64 bits */

And these are horrible. I had to look twice where this belongs to. I know
you copied existing crap, but that does not make it any better. And really,
if you want to express the size of a member here because you have to talk
to hardware then use the proper types we have for this: u64, u32, u16 ....

> +	unsigned short	sending_cpu;		/* filled in by sender */
> +	/* 16 bits */
> +	unsigned short	acknowledge_count;	/* filled in by destination */
> +	/* 16 bits */
> +	unsigned int	reserved1:8;		/* not usable */
> +	unsigned int	qualifier:24;		/* descriptor qualifier filled
> +						 * in by sender

> @@ -1200,6 +1200,7 @@ const struct cpumask *uv_flush_tlb_other
>  	struct bau_control *bcp;
>  	unsigned long descriptor_status;
>  	unsigned long status;
> +	unsigned long address;

Same types can go into a single line. No value in wasting lines.

Thanks,

	tglx