Re: [PATCH v3] powerpc/pseries: read the lpar name from the firmware

On 07/12/2021, 15:32:39, Nathan Lynch wrote:

Hi Laurent,

Laurent Dufour [off-list ref] writes:

quoted

+/*
+ * PAPR defines, in section "7.3.16 System Parameters Option", the token 55 to
+ * read the LPAR name.
+ */
+#define SPLPAR_LPAR_NAME_TOKEN	55
+static void read_lpar_name(struct seq_file *m)
+{
+	int rc, len, token;
+	union {
+		char raw_buffer[RTAS_DATA_BUF_SIZE];
+		struct {
+			__be16 len;

This:

quoted

+			char name[RTAS_DATA_BUF_SIZE-2];

                                       ^^^^^^

should be 4000, not (4K - 2), according to PAPR (it's weird and I don't
know the reason).

That's true, PAPR defines the largest output buffer for
ibm,get-system-parameter to 4002, so we could limit this to 4002, not sure
whether this would make a big difference here. Anyway I will limit that
buffer size this way.

quoted

+		};
+	} *local_buffer;
+
+	token = rtas_token("ibm,get-system-parameter");
+	if (token == RTAS_UNKNOWN_SERVICE)
+		return;
+
+	local_buffer = kmalloc(sizeof(*local_buffer), GFP_KERNEL);
+	if (!local_buffer)
+		return;
+
+	do {
+		spin_lock(&rtas_data_buf_lock);
+		memset(rtas_data_buf, 0, RTAS_DATA_BUF_SIZE);
+		rc = rtas_call(token, 3, 1, NULL, SPLPAR_LPAR_NAME_TOKEN,
+			       __pa(rtas_data_buf), RTAS_DATA_BUF_SIZE);
+		if (!rc)
+			memcpy(local_buffer->raw_buffer, rtas_data_buf,
+			       RTAS_DATA_BUF_SIZE);
+		spin_unlock(&rtas_data_buf_lock);
+	} while (rtas_busy_delay(rc));
+
+	if (rc != 0) {
+		pr_err_once(
+			"%s %s Error calling get-system-parameter (0x%x)\n",
+			__FILE__, __func__, rc);

The __FILE__ and __func__ in the message seem unnecessary, and rc should
be reported in decimal so the error meaning is apparent.

Fair enough.

Is there a reasonable fallback for VMs where this parameter doesn't
exist? PowerVM partitions should always have it, but what do we want the
behavior to be on other hypervisors?

In that case, there is no value displayed in the /proc/powerpc/lparcfg and
the lparstat -i command will fall back to the device tree value. I can't
see any valid reason to report the value defined in the device tree here.

quoted

+	} else {
+		/* Force end of string */
+		len = be16_to_cpu(local_buffer->len);
+		if (len >= (RTAS_DATA_BUF_SIZE-2))
+			len = RTAS_DATA_BUF_SIZE-2;

Could use min() or clamp(), and it would be better to build the
expression using the value of sizeof(local_buffer->name).

Fair enough.

quoted

+		local_buffer->name[len] = '\0';

If 'len' can be (RTAS_DATA_BUF_SIZE - 2), then this writes past the end
of the buffer, no?

Oh yes, the previous test should be
		if (len >= (RTAS_DATA_BUF_SIZE-2))
			len = RTAS_DATA_BUF_SIZE-3;

Thanks,
Laurent.