[PATCH 0/6] x86/platform/uv/BAU: UV4 message completion and initialization updates

[PATCH 0/6] x86/platform/uv/BAU: UV4 message completion and initialization updates

[Andrew Banman]

The following patch series adds the necessary functionality to make the BAU
on UV4 operational. The purpose of these patches is to implement the correct
message completion logic on UV4 and to fix two initialization errors.

As of this patch set, the BAU operates without errors and performance tests
show TLB shootdowns take up to 42% less time with the BAU enabled.

The patches are summarized as follows:

(1) Make the wait_completion routine part of the bau_operations interface,
    and add a uv4_wait_completion routine to employ new completion logic.

    The message completion logic for previous generations relies on software-
    defined timeouts that are not implemented on UV4. Without these patches,
    the BAU driver on UV4 erroneously identifies a UV2-WAR timeout during
    normal operation.

    [PATCH 1/6] x86/platform/uv/BAU: Declare bau_operations struct after
    [PATCH 2/6] x86/platform/uv/BAU: Add status_mmr_loc to locate message
    [PATCH 3/6] x86/platform/uv/BAU: Add wait_completion to bau_operations
    [PATCH 4/6] x86/platform/uv/BAU: Implement uv4_wait_completion with

(2) Fix an initialization error in which writing to the software acknowledge
    clear register causes a HUB error to assert, resulting in an NMI.

    [PATCH 5/6] x86/platform/uv/BAU: Remove initial write to swack

(3) Populate a message payload field to verify messages at the destination.
    Without this verification, the destination agent triggers a HUB error,
    resulting in an NMI.

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


Please see the commit messages for details on the motivation and content of
each patch.

Thank you,

Andrew Banman
<abanman@hpe.com>
Linux Kernel Engineer
Hewlett Packard Enterprise

[PATCH 1/6] x86/platform/uv/BAU: Declare bau_operations struct after other BAU structs

[Andrew Banman]

We must declare bau_operations after the other BAU structs so that we may
reference them in the bau_operations function declarations.

Signed-off-by: Andrew Banman <abanman@hpe.com>
Acked-by: Mike Travis <mike.travis@hpe.com>
---
 arch/x86/include/asm/uv/uv_bau.h | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/uv/uv_bau.h b/arch/x86/include/asm/uv/uv_bau.h
index 57ab86d..33234bc 100644
--- a/arch/x86/include/asm/uv/uv_bau.h
+++ b/arch/x86/include/asm/uv/uv_bau.h
@@ -385,17 +385,6 @@ struct uv2_3_bau_msg_header {
 	/* bits 127:120 */
 };
 
-/* Abstracted BAU functions */
-struct bau_operations {
-	unsigned long (*read_l_sw_ack)(void);
-	unsigned long (*read_g_sw_ack)(int pnode);
-	unsigned long (*bau_gpa_to_offset)(unsigned long vaddr);
-	void (*write_l_sw_ack)(unsigned long mmr);
-	void (*write_g_sw_ack)(int pnode, unsigned long mmr);
-	void (*write_payload_first)(int pnode, unsigned long mmr);
-	void (*write_payload_last)(int pnode, unsigned long mmr);
-};
-
 /*
  * The activation descriptor:
  * The format of the message to send, plus all accompanying control
@@ -644,6 +633,17 @@ struct bau_control {
 	struct hub_and_pnode	*thp;
 };
 
+/* Abstracted BAU functions */
+struct bau_operations {
+	unsigned long	(*read_l_sw_ack)(void);
+	unsigned long	(*read_g_sw_ack)(int pnode);
+	unsigned long	(*bau_gpa_to_offset)(unsigned long vaddr);
+	void		(*write_l_sw_ack)(unsigned long mmr);
+	void		(*write_g_sw_ack)(int pnode, unsigned long mmr);
+	void		(*write_payload_first)(int pnode, unsigned long mmr);
+	void		(*write_payload_last)(int pnode, unsigned long mmr);
+};
+
 static inline void write_mmr_data_broadcast(int pnode, unsigned long mmr_image)
 {
 	write_gmmr(pnode, UVH_BAU_DATA_BROADCAST, mmr_image);
-- 
1.8.2.1

[PATCH 2/6] x86/platform/uv/BAU: Add status_mmr_loc to locate message status bits

[Andrew Banman]

The location of the ERROR and BUSY status bits depends on the descriptor
index, i.e. the CPU, of the message. We determine this location ahead of
the wait_completion loop to avoid repeating the calculation.

Split out the status location calculation into a new routine,
status_mmr_loc, to be used within each uv*_wait_completion routine.

Signed-off-by: Andrew Banman <abanman@hpe.com>
Acked-by: Mike Travis <mike.travis@hpe.com>
---
 arch/x86/platform/uv/tlb_uv.c | 41 +++++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 16 deletions(-)

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
@@ -533,6 +533,22 @@ static inline void end_uvhub_quiesce(str
 	atom_asr(-1, (struct atomic_short *)&hmaster->uvhub_quiesce);
 }
 
+/*
+ * The ERROR and BUSY status registers are located pairwise over the STATUS_0
+ * and STATUS_1 mmrs; each an array[32] of 2 bits. Given CPU desc, determine
+ * the correct mmr and index for the message status.
+ */
+void status_mmr_loc(unsigned long *mmr, int *index, int desc)
+{
+	if (desc < UV_CPUS_PER_AS) {
+		*mmr = UVH_LB_BAU_SB_ACTIVATION_STATUS_0;
+		*index = desc * UV_ACT_STATUS_SIZE;
+	} else {
+		*mmr = UVH_LB_BAU_SB_ACTIVATION_STATUS_1;
+		*index = (desc - UV_CPUS_PER_AS) * UV_ACT_STATUS_SIZE;
+	}
+}
+
 static unsigned long uv1_read_status(unsigned long mmr_offset, int right_shift)
 {
 	unsigned long descriptor_status;
@@ -548,13 +564,16 @@ static unsigned long uv1_read_status(uns
  * return COMPLETE, RETRY(PLUGGED or TIMEOUT) or GIVEUP
  */
 static int uv1_wait_completion(struct bau_desc *bau_desc,
-				unsigned long mmr_offset, int right_shift,
 				struct bau_control *bcp, long try)
 {
 	unsigned long descriptor_status;
+	unsigned long mmr_offset;
+	int right_shift;
+	int desc = bcp->uvhub_cpu;
 	cycles_t ttm;
 	struct ptc_stats *stat = bcp->statp;
 
+	status_mmr_loc(&mmr_offset, &right_shift, desc);
 	descriptor_status = uv1_read_status(mmr_offset, right_shift);
 	/* spin on the status MMR, waiting for it to go idle */
 	while ((descriptor_status != DS_IDLE)) {
@@ -640,15 +659,17 @@ int handle_uv2_busy(struct bau_control *
 }
 
 static int uv2_3_wait_completion(struct bau_desc *bau_desc,
-				unsigned long mmr_offset, int right_shift,
 				struct bau_control *bcp, long try)
 {
 	unsigned long descriptor_stat;
+	unsigned long mmr_offset;
 	cycles_t ttm;
 	int desc = bcp->uvhub_cpu;
+	int right_shift;
 	long busy_reps = 0;
 	struct ptc_stats *stat = bcp->statp;
 
+	status_mmr_loc(&mmr_offset, &right_shift, desc);
 	descriptor_stat = uv2_3_read_status(mmr_offset, right_shift, desc);
 
 	/* spin on the status MMR, waiting for it to go idle */
@@ -712,22 +733,10 @@ static int uv2_3_wait_completion(struct
  */
 static int wait_completion(struct bau_desc *bau_desc, struct bau_control *bcp, long try)
 {
-	int right_shift;
-	unsigned long mmr_offset;
-	int desc = bcp->uvhub_cpu;
-
-	if (desc < UV_CPUS_PER_AS) {
-		mmr_offset = UVH_LB_BAU_SB_ACTIVATION_STATUS_0;
-		right_shift = desc * UV_ACT_STATUS_SIZE;
-	} else {
-		mmr_offset = UVH_LB_BAU_SB_ACTIVATION_STATUS_1;
-		right_shift = ((desc - UV_CPUS_PER_AS) * UV_ACT_STATUS_SIZE);
-	}
-
 	if (bcp->uvhub_version == 1)
-		return uv1_wait_completion(bau_desc, mmr_offset, right_shift, bcp, try);
+		return uv1_wait_completion(bau_desc, bcp, try);
 	else
-		return uv2_3_wait_completion(bau_desc, mmr_offset, right_shift, bcp, try);
+		return uv2_3_wait_completion(bau_desc, bcp, try);
 }
 
 /*

[PATCH 3/6] x86/platform/uv/BAU: Add wait_completion to bau_operations

[Andrew Banman]

Remove the present wait_completion routine and add a function pointer by
the same name to the bau_operations struct. Rather than switching on the
UV hub version during message processing, set the architecture-specific
uv*_wait_completion during initialization.

Signed-off-by: Andrew Banman <abanman@hpe.com>
Acked-by: Mike Travis <mike.travis@hpe.com>
---
 arch/x86/include/asm/uv/uv_bau.h |  2 ++
 arch/x86/platform/uv/tlb_uv.c    | 41 ++++++++++++++++++++++------------------
 2 files changed, 25 insertions(+), 18 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
@@ -642,6 +642,8 @@ struct bau_operations {
 	void		(*write_g_sw_ack)(int pnode, unsigned long mmr);
 	void		(*write_payload_first)(int pnode, unsigned long mmr);
 	void		(*write_payload_last)(int pnode, unsigned long mmr);
+	int		(*wait_completion)(struct bau_desc*,
+				struct bau_control*, long try);
 };
 
 static inline void write_mmr_data_broadcast(int pnode, unsigned long mmr_image)
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
@@ -23,9 +23,25 @@
 #include <asm/irq_vectors.h>
 #include <asm/timer.h>
 
+static int uv1_wait_completion(struct bau_desc *bau_desc,
+				struct bau_control *bcp, long try);
+static int uv2_3_wait_completion(struct bau_desc *bau_desc,
+				struct bau_control *bcp, long try);
+
 static struct bau_operations ops;
 
-static struct bau_operations uv123_bau_ops = {
+static struct bau_operations uv1_bau_ops = {
+	.bau_gpa_to_offset       = uv_gpa_to_offset,
+	.read_l_sw_ack           = read_mmr_sw_ack,
+	.read_g_sw_ack           = read_gmmr_sw_ack,
+	.write_l_sw_ack          = write_mmr_sw_ack,
+	.write_g_sw_ack          = write_gmmr_sw_ack,
+	.write_payload_first     = write_mmr_payload_first,
+	.write_payload_last      = write_mmr_payload_last,
+	.wait_completion         = uv1_wait_completion,
+};
+
+static struct bau_operations uv23_bau_ops = {
 	.bau_gpa_to_offset       = uv_gpa_to_offset,
 	.read_l_sw_ack           = read_mmr_sw_ack,
 	.read_g_sw_ack           = read_gmmr_sw_ack,
@@ -33,6 +49,7 @@ static struct bau_operations uv123_bau_o
 	.write_g_sw_ack          = write_gmmr_sw_ack,
 	.write_payload_first     = write_mmr_payload_first,
 	.write_payload_last      = write_mmr_payload_last,
+	.wait_completion         = uv2_3_wait_completion,
 };
 
 static struct bau_operations uv4_bau_ops = {
@@ -43,6 +60,7 @@ static struct bau_operations uv4_bau_ops
 	.write_g_sw_ack          = write_gmmr_proc_sw_ack,
 	.write_payload_first     = write_mmr_proc_payload_first,
 	.write_payload_last      = write_mmr_proc_payload_last,
+	.wait_completion         = uv2_3_wait_completion,
 };
 
 
@@ -727,19 +745,6 @@ static int uv2_3_wait_completion(struct
 }
 
 /*
- * There are 2 status registers; each and array[32] of 2 bits. Set up for
- * which register to read and position in that register based on cpu in
- * current hub.
- */
-static int wait_completion(struct bau_desc *bau_desc, struct bau_control *bcp, long try)
-{
-	if (bcp->uvhub_version == 1)
-		return uv1_wait_completion(bau_desc, bcp, try);
-	else
-		return uv2_3_wait_completion(bau_desc, bcp, try);
-}
-
-/*
  * Our retries are blocked by all destination sw ack resources being
  * in use, and a timeout is pending. In that case hardware immediately
  * returns the ERROR that looks like a destination timeout.
@@ -967,7 +972,7 @@ int uv_flush_send_and_wait(struct cpumas
 		write_mmr_activation(index);
 
 		try++;
-		completion_stat = wait_completion(bau_desc, bcp, try);
+		completion_stat = ops.wait_completion(bau_desc, bcp, try);
 
 		handle_cmplt(completion_stat, bau_desc, bcp, hmaster, stat);
 
@@ -2176,11 +2181,11 @@ static int __init uv_bau_init(void)
 	if (is_uv4_hub())
 		ops = uv4_bau_ops;
 	else if (is_uv3_hub())
-		ops = uv123_bau_ops;
+		ops = uv23_bau_ops;
 	else if (is_uv2_hub())
-		ops = uv123_bau_ops;
+		ops = uv23_bau_ops;
 	else if (is_uv1_hub())
-		ops = uv123_bau_ops;
+		ops = uv1_bau_ops;
 
 	for_each_possible_cpu(cur_cpu) {
 		mask = &per_cpu(uv_flush_tlb_mask, cur_cpu);

[PATCH 4/6] x86/platform/uv/BAU: Implement uv4_wait_completion with read_status

[Andrew Banman]

UV4 does not employ a software-timeout as in previous generations so a new
wait_completion routine without this logic is required. Certain completion
statuses require the AUX status bit in addition to ERROR and BUSY.

Add the read_status routine to construct the full completion status. Use
read_status in the uv4_wait_completion routine to handle all possible
completion statuses.

Signed-off-by: Andrew Banman <abanman@hpe.com>
Acked-by: Mike Travis <mike.travis@hpe.com>
---
 arch/x86/platform/uv/tlb_uv.c | 65 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 64 insertions(+), 1 deletion(-)

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
@@ -27,6 +27,8 @@ static int uv1_wait_completion(struct ba
 				struct bau_control *bcp, long try);
 static int uv2_3_wait_completion(struct bau_desc *bau_desc,
 				struct bau_control *bcp, long try);
+static int uv4_wait_completion(struct bau_desc *bau_desc,
+				struct bau_control *bcp, long try);
 
 static struct bau_operations ops;
 
@@ -60,7 +62,7 @@ static struct bau_operations uv4_bau_ops
 	.write_g_sw_ack          = write_gmmr_proc_sw_ack,
 	.write_payload_first     = write_mmr_proc_payload_first,
 	.write_payload_last      = write_mmr_proc_payload_last,
-	.wait_completion         = uv2_3_wait_completion,
+	.wait_completion         = uv4_wait_completion,
 };
 
 
@@ -742,6 +744,67 @@ static int uv2_3_wait_completion(struct
 	}
 	bcp->conseccompletes++;
 	return FLUSH_COMPLETE;
+}
+
+/*
+ * Returns the status of current BAU message for cpu desc as a bit field
+ * [Error][Busy][Aux]
+ */
+static unsigned long read_status(unsigned long status_mmr, int index, int desc)
+{
+	unsigned long descriptor_status;
+
+	descriptor_status =
+		((read_lmmr(status_mmr) >> index) & UV_ACT_STATUS_MASK) << 1;
+
+	descriptor_status |=
+		(read_lmmr(UVH_LB_BAU_SB_ACTIVATION_STATUS_2) >> desc) & 0x1;
+
+	return descriptor_status;
+}
+
+static int uv4_wait_completion(struct bau_desc *bau_desc,
+				struct bau_control *bcp, long try)
+{
+	unsigned long descriptor_stat;
+	unsigned long err_busy_mmr;
+	int err_busy_index;
+	int desc = bcp->uvhub_cpu;
+	struct ptc_stats *stat = bcp->statp;
+
+	status_mmr_loc(&err_busy_mmr, &err_busy_index, desc);
+	descriptor_stat = read_status(err_busy_mmr, err_busy_index, desc);
+
+	/* spin on the status MMR, waiting for it to go idle */
+	while (descriptor_stat != UV2H_DESC_IDLE) {
+		switch (descriptor_stat) {
+		case UV2H_DESC_SOURCE_TIMEOUT:
+			stat->s_stimeout++;
+			return FLUSH_GIVEUP;
+
+		case UV2H_DESC_DEST_TIMEOUT:
+			stat->s_dtimeout++;
+			bcp->conseccompletes = 0;
+			return FLUSH_RETRY_TIMEOUT;
+
+		case UV2H_DESC_DEST_STRONG_NACK:
+			stat->s_plugged++;
+			bcp->conseccompletes = 0;
+			return FLUSH_RETRY_PLUGGED;
+
+		case UV2H_DESC_DEST_PUT_ERR:
+			bcp->conseccompletes = 0;
+			return FLUSH_GIVEUP;
+
+		default:
+			/* descriptor_stat is still BUSY */
+			cpu_relax();
+		}
+		descriptor_stat =
+			read_status(err_busy_mmr, err_busy_index, desc);
+	}
+	bcp->conseccompletes++;
+	return FLUSH_COMPLETE;
 }
 
 /*

[PATCH 5/6] x86/platform/uv/BAU: Remove initial write to swack register

[Andrew Banman]

Writing to the software acknowledge clear register when there are no
pending messages causes a HUB error to assert. The original intent of this
write was to clear the pending bits before start of operation, but this is
an incorrect method and has been determined to be unnecessary.

Signed-off-by: Andrew Banman <abanman@hpe.com>
Acked-by: Mike Travis <mike.travis@hpe.com>
---
 arch/x86/platform/uv/tlb_uv.c | 1 -
 1 file changed, 1 deletion(-)

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
@@ -1924,7 +1924,6 @@ static void pq_init(int node, int pnode)
 
 	ops.write_payload_first(pnode, first);
 	ops.write_payload_last(pnode, last);
-	ops.write_g_sw_ack(pnode, 0xffffUL);
 
 	/* in effect, all msg_type's are set to MSG_NOOP */
 	memset(pqp, 0, sizeof(struct bau_pq_entry) * DEST_Q_SIZE);

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

[Andrew Banman]

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.

Re: [PATCH 1/6] x86/platform/uv/BAU: Declare bau_operations struct after other BAU structs

[Thomas Gleixner]

On Tue, 14 Feb 2017, Andrew Banman wrote:

> We must declare bau_operations after the other BAU structs so that we may
> reference them in the bau_operations function declarations.

This changelog is misleading. I really had to look twice to figure out that
this is just a preparatory patch for adding stuff which references a bau
struct later. There is no 'MUST declare after'. You always can forward
declare structs when you only use a pointer in the struct op
declaration. Sure you can avoid it, but then please tell so, e.g.:

  Move the bau_operations declaration after bau struct declaration so the
  bau structs can be referenced when adding new functions to
  bau_operations. That way we avoid forward declarations of the bau
  structs.

Hmm?

>  
> +/* Abstracted BAU functions */
> +struct bau_operations {
> +	unsigned long	(*read_l_sw_ack)(void);
> +	unsigned long	(*read_g_sw_ack)(int pnode);
> +	unsigned long	(*bau_gpa_to_offset)(unsigned long vaddr);
> +	void		(*write_l_sw_ack)(unsigned long mmr);
> +	void		(*write_g_sw_ack)(int pnode, unsigned long mmr);
> +	void		(*write_payload_first)(int pnode, unsigned long mmr);
> +	void		(*write_payload_last)(int pnode, unsigned long mmr);

I appreciate that you made them tabular aligned!

Thanks,

	tglx

Re: [PATCH 2/6] x86/platform/uv/BAU: Add status_mmr_loc to locate message status bits

[Thomas Gleixner]

On Tue, 14 Feb 2017, Andrew Banman wrote:

> The location of the ERROR and BUSY status bits depends on the descriptor
> index, i.e. the CPU, of the message. We determine this location ahead of
> the wait_completion loop to avoid repeating the calculation.
> 
> Split out the status location calculation into a new routine,
> status_mmr_loc, to be used within each uv*_wait_completion routine.

And the reason for this is? You just tell WHAT you are doing, not the WHY.

Looking at the patch which implements the uv4 wait function it uses the
thing as well. So for the casual reader there is no point.

The only reason i figured why you want to do that is to reduce the number
of arguments to the wait function, correct?

If yes, then spell it out. If no, please enlighten me.

Thanks,

	tglx

Re: [PATCH 3/6] x86/platform/uv/BAU: Add wait_completion to bau_operations

[Thomas Gleixner]

On Tue, 14 Feb 2017, Andrew Banman wrote:

> Remove the present wait_completion routine and add a function pointer by
> the same name to the bau_operations struct. Rather than switching on the
> UV hub version during message processing, set the architecture-specific
> uv*_wait_completion during initialization.

Well written changelog!


> +static int uv1_wait_completion(struct bau_desc *bau_desc,
> +				struct bau_control *bcp, long try);
> +static int uv2_3_wait_completion(struct bau_desc *bau_desc,
> +				struct bau_control *bcp, long try);

You could spare the forward declarations by moving the uv*_bau_ops down
right before uv_bau_init(). And while at it, the uv*_bau_ops can be made
__initconst.

>  static struct bau_operations ops;

That one can be made __ro_after_init as the only possible modification
happens in the init function.

Thanks,

	tglx

Re: [PATCH 4/6] x86/platform/uv/BAU: Implement uv4_wait_completion with read_status

[Thomas Gleixner]

On Tue, 14 Feb 2017, Andrew Banman wrote:
> UV4 does not employ a software-timeout as in previous generations so a new
> wait_completion routine without this logic is required. Certain completion
> statuses require the AUX status bit in addition to ERROR and BUSY.
> 
> Add the read_status routine to construct the full completion status. Use
> read_status in the uv4_wait_completion routine to handle all possible
> completion statuses.

Ok.

> +/*
> + * Returns the status of current BAU message for cpu desc as a bit field
> + * [Error][Busy][Aux]
> + */
> +static unsigned long read_status(unsigned long status_mmr, int index, int desc)
> +{
> +	unsigned long descriptor_status;
> +
> +	descriptor_status =
> +		((read_lmmr(status_mmr) >> index) & UV_ACT_STATUS_MASK) << 1;
> +
> +	descriptor_status |=
> +		(read_lmmr(UVH_LB_BAU_SB_ACTIVATION_STATUS_2) >> desc) & 0x1;

You can spare all those ugly line breaks by chosing a short variable
name. You explain already in the comment that the returned value is the
status for a cpu descriptor. So where is the point of making the local
helper variable repeat than info?

> +	return descriptor_status;
> +}
> +
> +static int uv4_wait_completion(struct bau_desc *bau_desc,
> +				struct bau_control *bcp, long try)
> +{
> +	unsigned long descriptor_stat;
> +	unsigned long err_busy_mmr;
> +	int err_busy_index;
> +	int desc = bcp->uvhub_cpu;
> +	struct ptc_stats *stat = bcp->statp;

We usually order the local variables in reverse fir tree mode:

> +	struct ptc_stats *stat = bcp->statp;
> +	unsigned long descriptor_stat;
> +	unsigned long err_busy_mmr;
> +	int desc = bcp->uvhub_cpu;
> +	int err_busy_index;

It's simpler to parse than the random line length mode above.

> +	status_mmr_loc(&err_busy_mmr, &err_busy_index, desc);
> +	descriptor_stat = read_status(err_busy_mmr, err_busy_index, desc);
> +
> +	/* spin on the status MMR, waiting for it to go idle */
> +	while (descriptor_stat != UV2H_DESC_IDLE) {
> +		switch (descriptor_stat) {
> +		case UV2H_DESC_SOURCE_TIMEOUT:
> +			stat->s_stimeout++;
> +			return FLUSH_GIVEUP;
> +
> +		case UV2H_DESC_DEST_TIMEOUT:
> +			stat->s_dtimeout++;
> +			bcp->conseccompletes = 0;
> +			return FLUSH_RETRY_TIMEOUT;
> +
> +		case UV2H_DESC_DEST_STRONG_NACK:
> +			stat->s_plugged++;
> +			bcp->conseccompletes = 0;
> +			return FLUSH_RETRY_PLUGGED;
> +
> +		case UV2H_DESC_DEST_PUT_ERR:
> +			bcp->conseccompletes = 0;
> +			return FLUSH_GIVEUP;
> +
> +		default:
> +			/* descriptor_stat is still BUSY */
> +			cpu_relax();
> +		}
> +		descriptor_stat =
> +			read_status(err_busy_mmr, err_busy_index, desc);

Again, making the variable name shorter spares you this ugly line
break. 'stat' is clear enough.

Other than those nitpicks, that's all fine.

Thanks,

	tglx

Re: [PATCH 5/6] x86/platform/uv/BAU: Remove initial write to swack register

[Thomas Gleixner]

On Tue, 14 Feb 2017, Andrew Banman wrote:

> Writing to the software acknowledge clear register when there are no
> pending messages causes a HUB error to assert. The original intent of this
> write was to clear the pending bits before start of operation, but this is
> an incorrect method and has been determined to be unnecessary.

This is a bug fix, independent of the rest of the series, right?

So it should go to the beginning of that series or even better sent out as
a seperate patch so it can be picked up right away. Hiding bug fixes in the
middle of a functional/feature series makes them slip through.

Thanks,

	tglx

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

[Thomas Gleixner]

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

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

[Andrew Banman]

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.
>
> 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
>

Re: [PATCH 2/6] x86/platform/uv/BAU: Add status_mmr_loc to locate message status bits

[Andrew Banman]

On 02/16/2017 12:07 PM, Thomas Gleixner wrote:
> On Tue, 14 Feb 2017, Andrew Banman wrote:
>
>> The location of the ERROR and BUSY status bits depends on the descriptor
>> index, i.e. the CPU, of the message. We determine this location ahead of
>> the wait_completion loop to avoid repeating the calculation.
>>
>> Split out the status location calculation into a new routine,
>> status_mmr_loc, to be used within each uv*_wait_completion routine.
>
> And the reason for this is? You just tell WHAT you are doing, not the WHY.
>
> Looking at the patch which implements the uv4 wait function it uses the
> thing as well. So for the casual reader there is no point.
>
> The only reason i figured why you want to do that is to reduce the number
> of arguments to the wait function, correct?
>
> If yes, then spell it out. If no, please enlighten me.

Yes, the purpose was to re-scope the mmr location-finding logic into the 
uv*_wait_completion routines to make the call to wait_completion less 
complicated.

Though, on second thought, the *mmr and *index values in status_mmr_loc 
should really be set during initialization because they do not change. 
The calculation is always done with desc=uvhub_cpu.

I would add status_mmr and status_index to struct bau_control and move 
the calculation to scan_sock; there's no need for a subroutine in this case.

Thanks,

Andrew Banman

>
> Thanks,
>
> 	tglx
>