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

From: Andrew Banman
Date: Thu Feb 16 2017 - 16:13:09 EST

I will resubmit this patch and [Patch 5/6] as separate patches. They are both independent of the wait_completion feature.

I agree with your suggestions here. The updated version will have KernelDoc comments and proper u** integer types. I'd also like to add a preparatory patch to implement the hub version enum constants. This patch will replace all the magic numbers used to check uvhub_version. Like so:

[Patch 1/2] x86/platform/uv/BAU: Add uv_bau_version enumerated constants
[PATCH 2/2] x86/platform/uv/BAU: Add payload descriptor qualifier

Thank you for the suggestions!

Andrew Banman

On 02/16/2017 12:42 PM, Thomas Gleixner wrote:
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.


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,

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.