Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1580989 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 6/6] x86/platform/uv/BAU: Add payload descriptor qualifier Andrew Banman <abanman@hpe.com> - 2017-02-15 03:00 +0100
    Re: [PATCH 6/6] x86/platform/uv/BAU: Add payload descriptor  qualifier Thomas Gleixner <tglx@linutronix.de> - 2017-02-16 19:50 +0100

#1580989 — [PATCH 6/6] x86/platform/uv/BAU: Add payload descriptor qualifier

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. Make this the
default action for future architectures, anticipating they will have
the same requirement.


Signed-off-by: Andrew Banman <abanman@hpe.com>
Acked-by: Mike Travis <mike.travis@hpe.com>
---
 arch/x86/include/asm/uv/uv_bau.h | 23 +++++++++++++++++++++--
 arch/x86/platform/uv/tlb_uv.c    | 22 +++++++++++++++++++---
 2 files changed, 40 insertions(+), 5 deletions(-)

Index: community/arch/x86/include/asm/uv/uv_bau.h
===================================================================
--- community.orig/arch/x86/include/asm/uv/uv_bau.h
+++ community/arch/x86/include/asm/uv/uv_bau.h
@@ -185,6 +185,8 @@
 #define MSG_REGULAR			1
 #define MSG_RETRY			2
 
+#define BAU_DESC_QUALIFIER		0x534749
+
 /*
  * Distribution: 32 bytes (256 bits) (bytes 0-0x1f of descriptor)
  * If the 'multilevel' flag in the header portion of the descriptor
@@ -225,7 +227,7 @@ struct bau_local_cpumask {
 /*
  * The payload is software-defined for INTD transactions
  */
-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
+						 */
+	/* 64 bits */
+	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
+						 */
+};
 
 /*
  * UV1 Message header:  16 bytes (128 bits) (bytes 0x30-0x3f of descriptor)
@@ -400,7 +416,10 @@ struct bau_desc {
 		struct uv2_3_bau_msg_header	uv2_3_hdr;
 	} header;
 
-	struct bau_msg_payload			payload;
+	union bau_payload_header {
+		struct uv1_2_3_bau_msg_payload	uv1_2_3;
+		struct uv4_bau_msg_payload	uv4;
+	} payload;
 };
 /* UV1:
  *   -payload--    ---------header------
Index: community/arch/x86/platform/uv/tlb_uv.c
===================================================================
--- community.orig/arch/x86/platform/uv/tlb_uv.c
+++ community/arch/x86/platform/uv/tlb_uv.c
@@ -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;
 
 	bcp = &per_cpu(bau_control, cpu);
 
@@ -1248,10 +1249,25 @@ const struct cpumask *uv_flush_tlb_other
 	record_send_statistics(stat, locals, hubs, remotes, bau_desc);
 
 	if (!end || (end - start) <= PAGE_SIZE)
-		bau_desc->payload.address = start;
+		address = start;
 	else
-		bau_desc->payload.address = TLB_FLUSH_ALL;
-	bau_desc->payload.sending_cpu = cpu;
+		address = TLB_FLUSH_ALL;
+
+	switch (bcp->uvhub_version) {
+	case 1:
+	case 2:
+	case 3:
+		bau_desc->payload.uv1_2_3.address = address;
+		bau_desc->payload.uv1_2_3.sending_cpu = cpu;
+		break;
+	case 4:
+	default:
+		bau_desc->payload.uv4.address = address;
+		bau_desc->payload.uv4.sending_cpu = cpu;
+		bau_desc->payload.uv4.qualifier = BAU_DESC_QUALIFIER;
+		break;
+	}
+
 	/*
 	 * uv_flush_send_and_wait returns 0 if all cpu's were messaged,
 	 * or 1 if it gave up and the original cpumask should be returned.

[toc] | [next] | [standalone]

#1582850 — 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web