| Message ID | 20200514123729.156283-3-frankja@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pc-bios: s390x: Cleanup part 1 | expand |
[...] > > -static inline void yield(void) > -{ > - asm volatile ("diag 0,0,0x44" > - : : > - : "memory", "cc"); > -} > - Nit: Looks weird that yield() is moved to time.h Wonder if there is a better fit for this function. > #define MAX_SECTOR_SIZE 4096 > > -static inline void sleep(unsigned int seconds) > -{ > - ulong target = get_second() + seconds; > - > - while (get_second() < target) { > - yield(); > - } > -} > - > static inline void IPL_assert(bool term, const char *message) > { > if (!term) { > diff --git a/pc-bios/s390-ccw/time.h b/pc-bios/s390-ccw/time.h > new file mode 100644 > index 0000000000..899de83ae7 > --- /dev/null > +++ b/pc-bios/s390-ccw/time.h > @@ -0,0 +1,39 @@ > +#ifndef TIME_H > +#define TIME_H > + > +static inline u64 get_clock(void) > +{ > + u64 r; > + > + asm volatile("stck %0" : "=Q" (r) : : "cc"); > + return r; > +} > + > +static inline u64 get_time_ms(void) > +{ > + /* Bit 51 is incremented each microsecond */ > + return (get_clock() >> 12) / 1000; > +} > + > +static inline u64 get_time_seconds(void) > +{ > + return (get_time_ms()) / 1000; return get_time_ms() / 1000;
On 5/18/20 2:01 PM, David Hildenbrand wrote: > [...] > >> >> -static inline void yield(void) >> -{ >> - asm volatile ("diag 0,0,0x44" >> - : : >> - : "memory", "cc"); >> -} >> - > > Nit: Looks weird that yield() is moved to time.h > > Wonder if there is a better fit for this functi helper.h or maybe into the libc? > >> #define MAX_SECTOR_SIZE 4096 >> >> -static inline void sleep(unsigned int seconds) >> -{ >> - ulong target = get_second() + seconds; >> - >> - while (get_second() < target) { >> - yield(); >> - } >> -} >> - >> static inline void IPL_assert(bool term, const char *message) >> { >> if (!term) { >> diff --git a/pc-bios/s390-ccw/time.h b/pc-bios/s390-ccw/time.h >> new file mode 100644 >> index 0000000000..899de83ae7 >> --- /dev/null >> +++ b/pc-bios/s390-ccw/time.h >> @@ -0,0 +1,39 @@ >> +#ifndef TIME_H >> +#define TIME_H >> + >> +static inline u64 get_clock(void) >> +{ >> + u64 r; >> + >> + asm volatile("stck %0" : "=Q" (r) : : "cc"); >> + return r; >> +} >> + >> +static inline u64 get_time_ms(void) >> +{ >> + /* Bit 51 is incremented each microsecond */ >> + return (get_clock() >> 12) / 1000; >> +} >> + >> +static inline u64 get_time_seconds(void) >> +{ >> + return (get_time_ms()) / 1000; > > return get_time_ms() / 1000; Sure
On 18.05.20 14:09, Janosch Frank wrote: > On 5/18/20 2:01 PM, David Hildenbrand wrote: >> [...] >> >>> >>> -static inline void yield(void) >>> -{ >>> - asm volatile ("diag 0,0,0x44" >>> - : : >>> - : "memory", "cc"); >>> -} >>> - >> >> Nit: Looks weird that yield() is moved to time.h >> >> Wonder if there is a better fit for this functi > helper.h or maybe into the libc? Maybe helper.h if it doesn't result in too much hassle.
On 14/05/2020 14.37, Janosch Frank wrote: > Let's consolidate timing related functions into one header. > > Signed-off-by: Janosch Frank <frankja@linux.ibm.com> > --- > pc-bios/s390-ccw/menu.c | 1 + > pc-bios/s390-ccw/netmain.c | 15 +++---------- > pc-bios/s390-ccw/s390-ccw.h | 18 ---------------- > pc-bios/s390-ccw/time.h | 39 ++++++++++++++++++++++++++++++++++ > pc-bios/s390-ccw/virtio-net.c | 1 + > pc-bios/s390-ccw/virtio-scsi.c | 1 + > pc-bios/s390-ccw/virtio.c | 18 +++------------- > 7 files changed, 48 insertions(+), 45 deletions(-) > create mode 100644 pc-bios/s390-ccw/time.h > > diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c > index ce3815b201..7925c33248 100644 > --- a/pc-bios/s390-ccw/menu.c > +++ b/pc-bios/s390-ccw/menu.c > @@ -12,6 +12,7 @@ > #include "libc.h" > #include "s390-ccw.h" > #include "sclp.h" > +#include "time.h" > > #define KEYCODE_NO_INP '\0' > #define KEYCODE_ESCAPE '\033' > diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c > index 309ffa30d9..73def8de4f 100644 > --- a/pc-bios/s390-ccw/netmain.c > +++ b/pc-bios/s390-ccw/netmain.c > @@ -35,6 +35,7 @@ > #include "s390-ccw.h" > #include "cio.h" > #include "virtio.h" > +#include "time.h" netmain.c already contains a #include <time.h> which pulls in the header with the same name from libnet in SLOF ... so I wonder why you don't run into trouble here, I'd expect that your local time.h now rather gets included twice. Did you rebuild s390-netboot.img without problems? Anyway, time.h is maybe not the best name for a new header here... Thomas
On 5/20/20 9:53 PM, Thomas Huth wrote: > On 14/05/2020 14.37, Janosch Frank wrote: >> Let's consolidate timing related functions into one header. >> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com> >> --- >> pc-bios/s390-ccw/menu.c | 1 + >> pc-bios/s390-ccw/netmain.c | 15 +++---------- >> pc-bios/s390-ccw/s390-ccw.h | 18 ---------------- >> pc-bios/s390-ccw/time.h | 39 ++++++++++++++++++++++++++++++++++ >> pc-bios/s390-ccw/virtio-net.c | 1 + >> pc-bios/s390-ccw/virtio-scsi.c | 1 + >> pc-bios/s390-ccw/virtio.c | 18 +++------------- >> 7 files changed, 48 insertions(+), 45 deletions(-) >> create mode 100644 pc-bios/s390-ccw/time.h >> >> diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c >> index ce3815b201..7925c33248 100644 >> --- a/pc-bios/s390-ccw/menu.c >> +++ b/pc-bios/s390-ccw/menu.c >> @@ -12,6 +12,7 @@ >> #include "libc.h" >> #include "s390-ccw.h" >> #include "sclp.h" >> +#include "time.h" >> >> #define KEYCODE_NO_INP '\0' >> #define KEYCODE_ESCAPE '\033' >> diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c >> index 309ffa30d9..73def8de4f 100644 >> --- a/pc-bios/s390-ccw/netmain.c >> +++ b/pc-bios/s390-ccw/netmain.c >> @@ -35,6 +35,7 @@ >> #include "s390-ccw.h" >> #include "cio.h" >> #include "virtio.h" >> +#include "time.h" > > netmain.c already contains a #include <time.h> which pulls in the header > with the same name from libnet in SLOF ... so I wonder why you don't run > into trouble here, I'd expect that your local time.h now rather gets > included twice. Did you rebuild s390-netboot.img without problems? > Anyway, time.h is maybe not the best name for a new header here... > > Thomas > That doesn't seem to be a problem when compiling. However we could just move all of the function into helper.h as it's really not a lot of code especially after splitting it up into helper.h and time.h. Or we could use s390-time.h
diff --git a/pc-bios/s390-ccw/menu.c b/pc-bios/s390-ccw/menu.c index ce3815b201..7925c33248 100644 --- a/pc-bios/s390-ccw/menu.c +++ b/pc-bios/s390-ccw/menu.c @@ -12,6 +12,7 @@ #include "libc.h" #include "s390-ccw.h" #include "sclp.h" +#include "time.h" #define KEYCODE_NO_INP '\0' #define KEYCODE_ESCAPE '\033' diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c index 309ffa30d9..73def8de4f 100644 --- a/pc-bios/s390-ccw/netmain.c +++ b/pc-bios/s390-ccw/netmain.c @@ -35,6 +35,7 @@ #include "s390-ccw.h" #include "cio.h" #include "virtio.h" +#include "time.h" #define DEFAULT_BOOT_RETRIES 10 #define DEFAULT_TFTP_RETRIES 20 @@ -57,24 +58,14 @@ static SubChannelId net_schid = { .one = 1 }; static uint8_t mac[6]; static uint64_t dest_timer; -static uint64_t get_timer_ms(void) -{ - uint64_t clk; - - asm volatile(" stck %0 " : : "Q"(clk) : "memory"); - - /* Bit 51 is incremented each microsecond */ - return (clk >> (63 - 51)) / 1000; -} - void set_timer(int val) { - dest_timer = get_timer_ms() + val; + dest_timer = get_time_ms() + val; } int get_timer(void) { - return dest_timer - get_timer_ms(); + return dest_timer - get_time_ms(); } int get_sec_ticks(void) diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h index 21f27e7990..c5820e43ae 100644 --- a/pc-bios/s390-ccw/s390-ccw.h +++ b/pc-bios/s390-ccw/s390-ccw.h @@ -74,8 +74,6 @@ unsigned long virtio_load_direct(ulong rec_list1, ulong rec_list2, bool virtio_is_supported(SubChannelId schid); void virtio_blk_setup_device(SubChannelId schid); int virtio_read(ulong sector, void *load_addr); -u64 get_clock(void); -ulong get_second(void); /* bootmap.c */ void zipl_load(void); @@ -144,24 +142,8 @@ static inline void debug_print_addr(const char *desc, void *p) #define KVM_S390_VIRTIO_SET_STATUS 2 #define KVM_S390_VIRTIO_CCW_NOTIFY 3 -static inline void yield(void) -{ - asm volatile ("diag 0,0,0x44" - : : - : "memory", "cc"); -} - #define MAX_SECTOR_SIZE 4096 -static inline void sleep(unsigned int seconds) -{ - ulong target = get_second() + seconds; - - while (get_second() < target) { - yield(); - } -} - static inline void IPL_assert(bool term, const char *message) { if (!term) { diff --git a/pc-bios/s390-ccw/time.h b/pc-bios/s390-ccw/time.h new file mode 100644 index 0000000000..899de83ae7 --- /dev/null +++ b/pc-bios/s390-ccw/time.h @@ -0,0 +1,39 @@ +#ifndef TIME_H +#define TIME_H + +static inline u64 get_clock(void) +{ + u64 r; + + asm volatile("stck %0" : "=Q" (r) : : "cc"); + return r; +} + +static inline u64 get_time_ms(void) +{ + /* Bit 51 is incremented each microsecond */ + return (get_clock() >> 12) / 1000; +} + +static inline u64 get_time_seconds(void) +{ + return (get_time_ms()) / 1000; +} + +static inline void yield(void) +{ + asm volatile ("diag 0,0,0x44" + : : + : "memory", "cc"); +} + +static inline void sleep(unsigned int seconds) +{ + ulong target = get_time_seconds() + seconds; + + while (get_time_seconds() < target) { + yield(); + } +} + +#endif diff --git a/pc-bios/s390-ccw/virtio-net.c b/pc-bios/s390-ccw/virtio-net.c index ff7f4dad25..4de03728bb 100644 --- a/pc-bios/s390-ccw/virtio-net.c +++ b/pc-bios/s390-ccw/virtio-net.c @@ -19,6 +19,7 @@ #include <ethernet.h> #include "s390-ccw.h" #include "virtio.h" +#include "time.h" #ifndef DEBUG_VIRTIO_NET #define DEBUG_VIRTIO_NET 0 diff --git a/pc-bios/s390-ccw/virtio-scsi.c b/pc-bios/s390-ccw/virtio-scsi.c index 4fe4b9d261..0620651da8 100644 --- a/pc-bios/s390-ccw/virtio-scsi.c +++ b/pc-bios/s390-ccw/virtio-scsi.c @@ -14,6 +14,7 @@ #include "virtio.h" #include "scsi.h" #include "virtio-scsi.h" +#include "time.h" static ScsiDevice default_scsi_device; static VirtioScsiCmdReq req; diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c index fb40ca9828..43717b83d7 100644 --- a/pc-bios/s390-ccw/virtio.c +++ b/pc-bios/s390-ccw/virtio.c @@ -15,6 +15,7 @@ #include "virtio-scsi.h" #include "bswap.h" #include "helper.h" +#include "time.h" #define VRING_WAIT_REPLY_TIMEOUT 30 @@ -157,19 +158,6 @@ void vring_send_buf(VRing *vr, void *p, int len, int flags) } } -u64 get_clock(void) -{ - u64 r; - - asm volatile("stck %0" : "=Q" (r) : : "cc"); - return r; -} - -ulong get_second(void) -{ - return (get_clock() >> 12) / 1000000; -} - int vr_poll(VRing *vr) { if (vr->used->idx == vr->used_idx) { @@ -194,7 +182,7 @@ int vr_poll(VRing *vr) */ int vring_wait_reply(void) { - ulong target_second = get_second() + vdev.wait_reply_timeout; + ulong target_second = get_time_seconds() + vdev.wait_reply_timeout; /* Wait for any queue to be updated by the host */ do { @@ -207,7 +195,7 @@ int vring_wait_reply(void) if (r) { return 0; } - } while (!vdev.wait_reply_timeout || (get_second() < target_second)); + } while (!vdev.wait_reply_timeout || (get_time_seconds() < target_second)); return 1; }
Let's consolidate timing related functions into one header. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- pc-bios/s390-ccw/menu.c | 1 + pc-bios/s390-ccw/netmain.c | 15 +++---------- pc-bios/s390-ccw/s390-ccw.h | 18 ---------------- pc-bios/s390-ccw/time.h | 39 ++++++++++++++++++++++++++++++++++ pc-bios/s390-ccw/virtio-net.c | 1 + pc-bios/s390-ccw/virtio-scsi.c | 1 + pc-bios/s390-ccw/virtio.c | 18 +++------------- 7 files changed, 48 insertions(+), 45 deletions(-) create mode 100644 pc-bios/s390-ccw/time.h