| Message ID | 20170619112225.30471-4-jwi@linux.vnet.ibm.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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.
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.
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.
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.
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);
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(+)