[net-next,3/4] s390/diag: add diag26c support

Submitted by Julian Wiedmann on June 19, 2017, 11:22 a.m.

Details

Message ID 20170619112225.30471-4-jwi@linux.vnet.ibm.com
State Changes Requested
Delegated to: David Miller
Headers show

Commit Message

Julian Wiedmann June 19, 2017, 11:22 a.m.
Implement support for the hypervisor diagnose 0x26c
('Access Certain System Information').
It passes a request buffer and a subfunction code, and receives
a response buffer and a return code.

Also add the scaffolding for the 'MAC Services' subfunction.
It may be used by network devices to obtain a hypervisor-managed
MAC address.

Signed-off-by: Julian Wiedmann <jwi@linux.vnet.ibm.com>
Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 arch/s390/include/asm/diag.h | 26 ++++++++++++++++++++++++++
 arch/s390/kernel/diag.c      | 29 +++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+)

Comments

David Miller June 19, 2017, 2:47 p.m.
From: Julian Wiedmann <jwi@linux.vnet.ibm.com>
Date: Mon, 19 Jun 2017 13:22:24 +0200

> +#define DIAG26C_GET_MAC	0x0000
> +struct diag26c_mac_req {
> +	u32	resp_buf_len;
> +	u32	resp_version;
> +	u16	op_code;
> +	u16	devno;
> +	u8	res[4];
> +} __packed;

The packed attribute is not necessary here, the structure will be
perfectly packed together because of the types used and the order of
the members.

__packed is to be used only in the last possible resort for
correctness and every effort whatsoever should be used to avoid using
it.

> +
> +struct diag26c_mac_resp {
> +	u32	version;
> +	u8	mac[ETH_ALEN];
> +	u16	res;
> +} __packed __aligned(8);

Using packed with an 8 byte alignment is even more unnecessary.

Again, it is not needed, so please don't use it.

> + */
> +static inline int __diag26c(void *req, void *resp, enum diag26c_sc subcode)

Do not mark functions inline in *.c files, let the compiler decide.
Martin Schwidefsky June 19, 2017, 3:34 p.m.
Hi Dave,

On Mon, 19 Jun 2017 10:47:26 -0400 (EDT)
David Miller <davem@davemloft.net> wrote:

> From: Julian Wiedmann <jwi@linux.vnet.ibm.com>
> Date: Mon, 19 Jun 2017 13:22:24 +0200
> 
> > +#define DIAG26C_GET_MAC	0x0000
> > +struct diag26c_mac_req {
> > +	u32	resp_buf_len;
> > +	u32	resp_version;
> > +	u16	op_code;
> > +	u16	devno;
> > +	u8	res[4];
> > +} __packed;  
> 
> The packed attribute is not necessary here, the structure will be
> perfectly packed together because of the types used and the order of
> the members.

We (as in the s390 guys) tend to add __packed to hardware and hypervisor
structures even if the attribute is not strictly necessary. Most of the
diagnose related structures look that way. Dunno if it is worth to change
them.

I agree that __packed should be avoided for software defined structures.

> __packed is to be used only in the last possible resort for
> correctness and every effort whatsoever should be used to avoid using
> it.
> 
> > +
> > +struct diag26c_mac_resp {
> > +	u32	version;
> > +	u8	mac[ETH_ALEN];
> > +	u16	res;
> > +} __packed __aligned(8);  
> 
> Using packed with an 8 byte alignment is even more unnecessary.
> 
> Again, it is not needed, so please don't use it.

The diag26c struct needs to be aligned on a doubleword boundary, the
__aligned(8) is necessary. The __packed attribute is again superfluous but
follows along the lines of the other diag structures.

I do not mind the extra __packed attributes, but if you care about them
we could remove them from the structures in diag.h.

> > + */
> > +static inline int __diag26c(void *req, void *resp, enum diag26c_sc subcode)  
> 
> Do not mark functions inline in *.c files, let the compiler decide.
> 

Here I disagree. Basically all of our functions with assembly code are
static inline, it is a common pattern even in C files. Sometimes the compiler
*is* stupid and won't inline a function. And on s390 function calls do not
come for free.
David Miller June 19, 2017, 5:37 p.m.
From: Martin Schwidefsky <schwidefsky@de.ibm.com>
Date: Mon, 19 Jun 2017 17:34:25 +0200

> We (as in the s390 guys) tend to add __packed to hardware and hypervisor
> structures even if the attribute is not strictly necessary. Most of the
> diagnose related structures look that way. Dunno if it is worth to change
> them.

It causes gcc to generate bad code on certain platforms (yes, probably not
yours) and is in general something to avoid.

Please do not use __packed unless absolutely necessary.

> The diag26c struct needs to be aligned on a doubleword boundary, the
> __aligned(8) is necessary.

That's fine.

> The __packed attribute is again superfluous but follows along the
> lines of the other diag structures.

Please remove it.
Martin Schwidefsky June 21, 2017, 3:30 p.m.
Hi Dave,

On Mon, 19 Jun 2017 13:37:43 -0400 (EDT)
David Miller <davem@davemloft.net> wrote:

> From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> Date: Mon, 19 Jun 2017 17:34:25 +0200
> 
> > We (as in the s390 guys) tend to add __packed to hardware and hypervisor
> > structures even if the attribute is not strictly necessary. Most of the
> > diagnose related structures look that way. Dunno if it is worth to change
> > them.  
> 
> It causes gcc to generate bad code on certain platforms (yes, probably not
> yours) and is in general something to avoid.
> 
> Please do not use __packed unless absolutely necessary.
> 
> > The diag26c struct needs to be aligned on a doubleword boundary, the
> > __aligned(8) is necessary.  
> 
> That's fine.
> 
> > The __packed attribute is again superfluous but follows along the
> > lines of the other diag structures.  
> 
> Please remove it.

I looked at the various structures with the packed attribute in arch/s390
and found that we could remove a lot of them. As __packed also changes the
alignment of the structure removing the attribute from structures defined
in arch/s390/include/uapi/asm may cause trouble in user space. I stayed
away from the uapi headers.

For the rest of the code about 120 packed attributes could be removed, see:

https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git/log/?h=packed-cleanup

bloat-o-meter found only a few functions where the removal of packed
makes a difference (gcc 7.1.0):

add/remove: 1/1 grow/shrink: 8/7 up/down: 240/-308 (-68)
function                                     old     new   delta
pin_scb.isra                                   -     200    +200
stp_sync_clock                               982    1000     +18
stp_work_fn                                  416     420      +4
sca_ext_call_pending                         290     294      +4
kvm_arch_vcpu_create                        2008    2012      +4
get_vcpu_asce                               1206    1210      +4
hw_perf_event_update                        1738    1740      +2
clp_add_pci_device                          1400    1402      +2
__clp_rescan                                 162     164      +2
stp_timing_state_show                        138     134      -4
stp_timing_mode_show                         134     130      -4
stp_ctn_type_show                            134     130      -4
stp_time_offset_show                         150     144      -6
ccw_device_accumulate_irb                   1838    1832      -6
clp_misc_ioctl                              1142    1102     -40
timing_alert_interrupt                       158     114     -44
pin_scb                                      200       -    -200
Total: Before=155168810, After=155168742, chg -0.00%

The code gets minimally better. I am not convinced yet that this is worth
the hassle. These structures are architecture specific and the s390 CPUs
are perfectly fine with unaligned accesses.

Patch hide | download patch | download mbox

diff --git a/arch/s390/include/asm/diag.h b/arch/s390/include/asm/diag.h
index 8acf482162ed..96abd80528fc 100644
--- a/arch/s390/include/asm/diag.h
+++ b/arch/s390/include/asm/diag.h
@@ -8,6 +8,7 @@ 
 #ifndef _ASM_S390_DIAG_H
 #define _ASM_S390_DIAG_H
 
+#include <linux/if_ether.h>
 #include <linux/percpu.h>
 
 enum diag_stat_enum {
@@ -24,6 +25,7 @@  enum diag_stat_enum {
 	DIAG_STAT_X224,
 	DIAG_STAT_X250,
 	DIAG_STAT_X258,
+	DIAG_STAT_X26C,
 	DIAG_STAT_X288,
 	DIAG_STAT_X2C4,
 	DIAG_STAT_X2FC,
@@ -225,6 +227,30 @@  struct diag204_x_phys_block {
 	struct diag204_x_phys_cpu cpus[];
 } __packed;
 
+enum diag26c_sc {
+	DIAG26C_MAC_SERVICES = 0x00000030
+};
+
+enum diag26c_version {
+	DIAG26C_VERSION2 = 0x00000002	/* z/VM 5.4.0 */
+};
+
+#define DIAG26C_GET_MAC	0x0000
+struct diag26c_mac_req {
+	u32	resp_buf_len;
+	u32	resp_version;
+	u16	op_code;
+	u16	devno;
+	u8	res[4];
+} __packed;
+
+struct diag26c_mac_resp {
+	u32	version;
+	u8	mac[ETH_ALEN];
+	u16	res;
+} __packed __aligned(8);
+
 int diag204(unsigned long subcode, unsigned long size, void *addr);
 int diag224(void *ptr);
+int diag26c(void *req, void *resp, enum diag26c_sc subcode);
 #endif /* _ASM_S390_DIAG_H */
diff --git a/arch/s390/kernel/diag.c b/arch/s390/kernel/diag.c
index ac6abcd3fe6a..349914571772 100644
--- a/arch/s390/kernel/diag.c
+++ b/arch/s390/kernel/diag.c
@@ -38,6 +38,7 @@  static const struct diag_desc diag_map[NR_DIAG_STAT] = {
 	[DIAG_STAT_X224] = { .code = 0x224, .name = "EBCDIC-Name Table" },
 	[DIAG_STAT_X250] = { .code = 0x250, .name = "Block I/O" },
 	[DIAG_STAT_X258] = { .code = 0x258, .name = "Page-Reference Services" },
+	[DIAG_STAT_X26C] = { .code = 0x26c, .name = "Certain System Information" },
 	[DIAG_STAT_X288] = { .code = 0x288, .name = "Time Bomb" },
 	[DIAG_STAT_X2C4] = { .code = 0x2c4, .name = "FTP Services" },
 	[DIAG_STAT_X2FC] = { .code = 0x2fc, .name = "Guest Performance Data" },
@@ -236,3 +237,31 @@  int diag224(void *ptr)
 	return rc;
 }
 EXPORT_SYMBOL(diag224);
+
+/*
+ * Diagnose 26C: Access Certain System Information
+ */
+static inline int __diag26c(void *req, void *resp, enum diag26c_sc subcode)
+{
+	register unsigned long _req asm("2") = (addr_t) req;
+	register unsigned long _resp asm("3") = (addr_t) resp;
+	register unsigned long _subcode asm("4") = subcode;
+	register unsigned long _rc asm("5") = -EOPNOTSUPP;
+
+	asm volatile(
+		"	sam31\n"
+		"	diag	%[rx],%[ry],0x26c\n"
+		"0:	sam64\n"
+		EX_TABLE(0b,0b)
+		: "+d" (_rc)
+		: [rx] "d" (_req), "d" (_resp), [ry] "d" (_subcode)
+		: "cc", "memory");
+	return _rc;
+}
+
+int diag26c(void *req, void *resp, enum diag26c_sc subcode)
+{
+	diag_stat_inc(DIAG_STAT_X26C);
+	return __diag26c(req, resp, subcode);
+}
+EXPORT_SYMBOL(diag26c);