* [PATCH v3 1/9] pc-bios: s390x: cio.c cleanup and compile fix
2020-05-27 7:49 [PATCH v3 0/9] pc-bios: s390x: Cleanup part 1 Janosch Frank
@ 2020-05-27 7:49 ` Janosch Frank
2020-05-27 8:24 ` Christian Borntraeger
2020-05-27 7:49 ` [PATCH v3 2/9] pc-bios: s390x: Consolidate timing functions into time.h Janosch Frank
` (8 subsequent siblings)
9 siblings, 1 reply; 19+ messages in thread
From: Janosch Frank @ 2020-05-27 7:49 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, thuth, david
Let's initialize the structs at the beginning to ease reading and also
zeroing all other fields. This also makes the compiler stop
complaining about sense_id_ccw.flags being ored into when it's not
initialized.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
pc-bios/s390-ccw/cio.c | 40 ++++++++++++++++++++++------------------
1 file changed, 22 insertions(+), 18 deletions(-)
diff --git a/pc-bios/s390-ccw/cio.c b/pc-bios/s390-ccw/cio.c
index 339ec5fbe7..dbae1e240e 100644
--- a/pc-bios/s390-ccw/cio.c
+++ b/pc-bios/s390-ccw/cio.c
@@ -49,13 +49,13 @@ void enable_subchannel(SubChannelId schid)
uint16_t cu_type(SubChannelId schid)
{
- Ccw1 sense_id_ccw;
SenseId sense_data;
-
- sense_id_ccw.cmd_code = CCW_CMD_SENSE_ID;
- sense_id_ccw.cda = ptr2u32(&sense_data);
- sense_id_ccw.count = sizeof(sense_data);
- sense_id_ccw.flags |= CCW_FLAG_SLI;
+ Ccw1 sense_id_ccw = {
+ .cmd_code = CCW_CMD_SENSE_ID,
+ .count = sizeof(sense_data),
+ .flags = CCW_FLAG_SLI,
+ .cda = ptr2u32(&sense_data),
+ };
if (do_cio(schid, CU_TYPE_UNKNOWN, ptr2u32(&sense_id_ccw), CCW_FMT1)) {
panic("Failed to run SenseID CCw\n");
@@ -67,13 +67,13 @@ uint16_t cu_type(SubChannelId schid)
int basic_sense(SubChannelId schid, uint16_t cutype, void *sense_data,
uint16_t data_size)
{
- Ccw1 senseCcw;
+ Ccw1 senseCcw = {
+ .cmd_code = CCW_CMD_BASIC_SENSE,
+ .count = data_size,
+ .cda = ptr2u32(sense_data),
+ };
Irb irb;
- senseCcw.cmd_code = CCW_CMD_BASIC_SENSE;
- senseCcw.cda = ptr2u32(sense_data);
- senseCcw.count = data_size;
-
return __do_cio(schid, ptr2u32(&senseCcw), CCW_FMT1, &irb);
}
@@ -314,7 +314,17 @@ static void print_irb_err(Irb *irb)
*/
static int __do_cio(SubChannelId schid, uint32_t ccw_addr, int fmt, Irb *irb)
{
- CmdOrb orb = {};
+ /*
+ * QEMU's CIO implementation requires prefetch and 64-bit idaws. We
+ * allow all paths.
+ */
+ CmdOrb orb = {
+ .fmt = fmt,
+ .pfch = 1,
+ .c64 = 1,
+ .lpm = 0xFF,
+ .cpa = ccw_addr,
+ };
int rc;
IPL_assert(fmt == 0 || fmt == 1, "Invalid ccw format");
@@ -324,12 +334,6 @@ static int __do_cio(SubChannelId schid, uint32_t ccw_addr, int fmt, Irb *irb)
IPL_assert(ccw_addr <= 0xFFFFFF - 8, "Invalid ccw address");
}
- orb.fmt = fmt;
- orb.pfch = 1; /* QEMU's cio implementation requires prefetch */
- orb.c64 = 1; /* QEMU's cio implementation requires 64-bit idaws */
- orb.lpm = 0xFF; /* All paths allowed */
- orb.cpa = ccw_addr;
-
rc = ssch(schid, &orb);
if (rc == 1 || rc == 2) {
/* Subchannel status pending or busy. Eat status and ask for retry. */
--
2.25.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 1/9] pc-bios: s390x: cio.c cleanup and compile fix
2020-05-27 7:49 ` [PATCH v3 1/9] pc-bios: s390x: cio.c cleanup and compile fix Janosch Frank
@ 2020-05-27 8:24 ` Christian Borntraeger
2020-05-27 8:34 ` Janosch Frank
0 siblings, 1 reply; 19+ messages in thread
From: Christian Borntraeger @ 2020-05-27 8:24 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: qemu-s390x, cohuck, thuth, david
On 27.05.20 09:49, Janosch Frank wrote:
> Let's initialize the structs at the beginning to ease reading and also
> zeroing all other fields. This also makes the compiler stop
> complaining about sense_id_ccw.flags being ored into when it's not
> initialized.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> ---
> pc-bios/s390-ccw/cio.c | 40 ++++++++++++++++++++++------------------
> 1 file changed, 22 insertions(+), 18 deletions(-)
>
> diff --git a/pc-bios/s390-ccw/cio.c b/pc-bios/s390-ccw/cio.c
> index 339ec5fbe7..dbae1e240e 100644
> --- a/pc-bios/s390-ccw/cio.c
> +++ b/pc-bios/s390-ccw/cio.c
> @@ -49,13 +49,13 @@ void enable_subchannel(SubChannelId schid)
>
> uint16_t cu_type(SubChannelId schid)
> {
> - Ccw1 sense_id_ccw;
> SenseId sense_data;
> -
> - sense_id_ccw.cmd_code = CCW_CMD_SENSE_ID;
> - sense_id_ccw.cda = ptr2u32(&sense_data);
> - sense_id_ccw.count = sizeof(sense_data);
> - sense_id_ccw.flags |= CCW_FLAG_SLI;
> + Ccw1 sense_id_ccw = {
> + .cmd_code = CCW_CMD_SENSE_ID,
> + .count = sizeof(sense_data),
> + .flags = CCW_FLAG_SLI,
> + .cda = ptr2u32(&sense_data),
> + };
nitpicking, since you change the order anyway you _could_ swap count and flag
to match the order of struct Ccw. Only necessary when redoing.
>
> if (do_cio(schid, CU_TYPE_UNKNOWN, ptr2u32(&sense_id_ccw), CCW_FMT1)) {
> panic("Failed to run SenseID CCw\n");
> @@ -67,13 +67,13 @@ uint16_t cu_type(SubChannelId schid)
> int basic_sense(SubChannelId schid, uint16_t cutype, void *sense_data,
> uint16_t data_size)
> {
> - Ccw1 senseCcw;
> + Ccw1 senseCcw = {
> + .cmd_code = CCW_CMD_BASIC_SENSE,
> + .count = data_size,
> + .cda = ptr2u32(sense_data),
> + };
> Irb irb;
>
> - senseCcw.cmd_code = CCW_CMD_BASIC_SENSE;
> - senseCcw.cda = ptr2u32(sense_data);
> - senseCcw.count = data_size;
> -
here it is fine, due to the lack of flags.
Was this actually a bug before that senseCcw.flags was
not zeroed out?
[...]
Other than that
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3 1/9] pc-bios: s390x: cio.c cleanup and compile fix
2020-05-27 8:24 ` Christian Borntraeger
@ 2020-05-27 8:34 ` Janosch Frank
0 siblings, 0 replies; 19+ messages in thread
From: Janosch Frank @ 2020-05-27 8:34 UTC (permalink / raw)
To: Christian Borntraeger, qemu-devel; +Cc: qemu-s390x, cohuck, thuth, david
[-- Attachment #1.1: Type: text/plain, Size: 2724 bytes --]
On 5/27/20 10:24 AM, Christian Borntraeger wrote:
>
>
> On 27.05.20 09:49, Janosch Frank wrote:
>> Let's initialize the structs at the beginning to ease reading and also
>> zeroing all other fields. This also makes the compiler stop
>> complaining about sense_id_ccw.flags being ored into when it's not
>> initialized.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>> ---
>> pc-bios/s390-ccw/cio.c | 40 ++++++++++++++++++++++------------------
>> 1 file changed, 22 insertions(+), 18 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/cio.c b/pc-bios/s390-ccw/cio.c
>> index 339ec5fbe7..dbae1e240e 100644
>> --- a/pc-bios/s390-ccw/cio.c
>> +++ b/pc-bios/s390-ccw/cio.c
>> @@ -49,13 +49,13 @@ void enable_subchannel(SubChannelId schid)
>>
>> uint16_t cu_type(SubChannelId schid)
>> {
>> - Ccw1 sense_id_ccw;
>> SenseId sense_data;
>> -
>> - sense_id_ccw.cmd_code = CCW_CMD_SENSE_ID;
>> - sense_id_ccw.cda = ptr2u32(&sense_data);
>> - sense_id_ccw.count = sizeof(sense_data);
>> - sense_id_ccw.flags |= CCW_FLAG_SLI;
>> + Ccw1 sense_id_ccw = {
>> + .cmd_code = CCW_CMD_SENSE_ID,
>> + .count = sizeof(sense_data),
>> + .flags = CCW_FLAG_SLI,
>> + .cda = ptr2u32(&sense_data),
>> + };
>
> nitpicking, since you change the order anyway you _could_ swap count and flag
> to match the order of struct Ccw. Only necessary when redoing.
Sure, if it makes you happy :-)
>
>
>>
>> if (do_cio(schid, CU_TYPE_UNKNOWN, ptr2u32(&sense_id_ccw), CCW_FMT1)) {
>> panic("Failed to run SenseID CCw\n");
>> @@ -67,13 +67,13 @@ uint16_t cu_type(SubChannelId schid)
>> int basic_sense(SubChannelId schid, uint16_t cutype, void *sense_data,
>> uint16_t data_size)
>> {
>> - Ccw1 senseCcw;
>> + Ccw1 senseCcw = {
>> + .cmd_code = CCW_CMD_BASIC_SENSE,
>> + .count = data_size,
>> + .cda = ptr2u32(sense_data),
>> + };
>> Irb irb;
>>
>> - senseCcw.cmd_code = CCW_CMD_BASIC_SENSE;
>> - senseCcw.cda = ptr2u32(sense_data);
>> - senseCcw.count = data_size;
>> -
>
> here it is fine, due to the lack of flags.
> Was this actually a bug before that senseCcw.flags was
> not zeroed out?
I'd guess so, seems like the compiler went into other code paths with my
recent changes and then started complaining.
But it makes sense to clean them up anyway.
>
>
> [...]
>
> Other than that
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Thanks!
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 2/9] pc-bios: s390x: Consolidate timing functions into time.h
2020-05-27 7:49 [PATCH v3 0/9] pc-bios: s390x: Cleanup part 1 Janosch Frank
2020-05-27 7:49 ` [PATCH v3 1/9] pc-bios: s390x: cio.c cleanup and compile fix Janosch Frank
@ 2020-05-27 7:49 ` Janosch Frank
2020-05-27 8:36 ` Christian Borntraeger
2020-05-27 7:49 ` [PATCH v3 3/9] pc-bios: s390x: Move sleep and yield to helper.h Janosch Frank
` (7 subsequent siblings)
9 siblings, 1 reply; 19+ messages in thread
From: Janosch Frank @ 2020-05-27 7:49 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, thuth, david
Let's consolidate timing related functions into one header.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
| 1 +
pc-bios/s390-ccw/netmain.c | 15 +++------------
pc-bios/s390-ccw/s390-ccw.h | 2 --
pc-bios/s390-ccw/time.h | 23 +++++++++++++++++++++++
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, 32 insertions(+), 29 deletions(-)
create mode 100644 pc-bios/s390-ccw/time.h
--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..9514b76596 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);
diff --git a/pc-bios/s390-ccw/time.h b/pc-bios/s390-ccw/time.h
new file mode 100644
index 0000000000..ed6d982371
--- /dev/null
+++ b/pc-bios/s390-ccw/time.h
@@ -0,0 +1,23 @@
+#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;
+}
+
+#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;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 2/9] pc-bios: s390x: Consolidate timing functions into time.h
2020-05-27 7:49 ` [PATCH v3 2/9] pc-bios: s390x: Consolidate timing functions into time.h Janosch Frank
@ 2020-05-27 8:36 ` Christian Borntraeger
2020-05-27 8:41 ` Janosch Frank
0 siblings, 1 reply; 19+ messages in thread
From: Christian Borntraeger @ 2020-05-27 8:36 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: qemu-s390x, cohuck, thuth, david
On 27.05.20 09:49, 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 | 2 --
> pc-bios/s390-ccw/time.h | 23 +++++++++++++++++++++++
what happened to Thomas comment about a header with the same name from SLOF.
I think it is safer to renamme this one to s390-time.h?
> 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, 32 insertions(+), 29 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"
>
> #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..9514b76596 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);
> diff --git a/pc-bios/s390-ccw/time.h b/pc-bios/s390-ccw/time.h
> new file mode 100644
> index 0000000000..ed6d982371
> --- /dev/null
> +++ b/pc-bios/s390-ccw/time.h
> @@ -0,0 +1,23 @@
> +#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;
> +}
> +
> +#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;
> }
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3 2/9] pc-bios: s390x: Consolidate timing functions into time.h
2020-05-27 8:36 ` Christian Borntraeger
@ 2020-05-27 8:41 ` Janosch Frank
0 siblings, 0 replies; 19+ messages in thread
From: Janosch Frank @ 2020-05-27 8:41 UTC (permalink / raw)
To: Christian Borntraeger, qemu-devel; +Cc: qemu-s390x, cohuck, thuth, david
[-- Attachment #1.1: Type: text/plain, Size: 6022 bytes --]
On 5/27/20 10:36 AM, Christian Borntraeger wrote:
>
>
> On 27.05.20 09:49, 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 | 2 --
>> pc-bios/s390-ccw/time.h | 23 +++++++++++++++++++++++
>
> what happened to Thomas comment about a header with the same name from SLOF.
> I think it is safer to renamme this one to s390-time.h?
I extensively talked with Thomas about that on IRC and it doesn't seem
to be a problem right now. Since I got no answer to my mail asking if
s390-time.h would be better and he said I don't need to change it when
it works, I just left it like it was and waited for more comments.
>
>> 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, 32 insertions(+), 29 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"
>>
>> #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..9514b76596 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);
>> diff --git a/pc-bios/s390-ccw/time.h b/pc-bios/s390-ccw/time.h
>> new file mode 100644
>> index 0000000000..ed6d982371
>> --- /dev/null
>> +++ b/pc-bios/s390-ccw/time.h
>> @@ -0,0 +1,23 @@
>> +#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;
>> +}
>> +
>> +#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;
>> }
>>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 3/9] pc-bios: s390x: Move sleep and yield to helper.h
2020-05-27 7:49 [PATCH v3 0/9] pc-bios: s390x: Cleanup part 1 Janosch Frank
2020-05-27 7:49 ` [PATCH v3 1/9] pc-bios: s390x: cio.c cleanup and compile fix Janosch Frank
2020-05-27 7:49 ` [PATCH v3 2/9] pc-bios: s390x: Consolidate timing functions into time.h Janosch Frank
@ 2020-05-27 7:49 ` Janosch Frank
2020-05-27 8:34 ` Christian Borntraeger
2020-05-27 7:49 ` [PATCH v3 4/9] pc-bios: s390x: Get rid of magic offsets into the lowcore Janosch Frank
` (6 subsequent siblings)
9 siblings, 1 reply; 19+ messages in thread
From: Janosch Frank @ 2020-05-27 7:49 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, thuth, david
They are definitely helper functions.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
pc-bios/s390-ccw/helper.h | 17 +++++++++++++++++
pc-bios/s390-ccw/s390-ccw.h | 16 ----------------
pc-bios/s390-ccw/virtio-net.c | 1 +
pc-bios/s390-ccw/virtio-scsi.c | 1 +
4 files changed, 19 insertions(+), 16 deletions(-)
diff --git a/pc-bios/s390-ccw/helper.h b/pc-bios/s390-ccw/helper.h
index 78d5bc7442..d1b8b14d30 100644
--- a/pc-bios/s390-ccw/helper.h
+++ b/pc-bios/s390-ccw/helper.h
@@ -14,6 +14,7 @@
#define S390_CCW_HELPER_H
#include "s390-ccw.h"
+#include "time.h"
/* Avoids compiler warnings when casting a pointer to a u32 */
static inline uint32_t ptr2u32(void *ptr)
@@ -28,4 +29,20 @@ static inline void *u32toptr(uint32_t n)
return (void *)(uint64_t)n;
}
+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/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index 9514b76596..c5820e43ae 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -142,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/virtio-net.c b/pc-bios/s390-ccw/virtio-net.c
index 4de03728bb..f018d58994 100644
--- a/pc-bios/s390-ccw/virtio-net.c
+++ b/pc-bios/s390-ccw/virtio-net.c
@@ -20,6 +20,7 @@
#include "s390-ccw.h"
#include "virtio.h"
#include "time.h"
+#include "helper.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 0620651da8..907f8a08bd 100644
--- a/pc-bios/s390-ccw/virtio-scsi.c
+++ b/pc-bios/s390-ccw/virtio-scsi.c
@@ -15,6 +15,7 @@
#include "scsi.h"
#include "virtio-scsi.h"
#include "time.h"
+#include "helper.h"
static ScsiDevice default_scsi_device;
static VirtioScsiCmdReq req;
--
2.25.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 3/9] pc-bios: s390x: Move sleep and yield to helper.h
2020-05-27 7:49 ` [PATCH v3 3/9] pc-bios: s390x: Move sleep and yield to helper.h Janosch Frank
@ 2020-05-27 8:34 ` Christian Borntraeger
2020-05-27 8:35 ` Janosch Frank
0 siblings, 1 reply; 19+ messages in thread
From: Christian Borntraeger @ 2020-05-27 8:34 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: qemu-s390x, cohuck, thuth, david
On 27.05.20 09:49, Janosch Frank wrote:
> They are definitely helper functions.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> pc-bios/s390-ccw/helper.h | 17 +++++++++++++++++
> pc-bios/s390-ccw/s390-ccw.h | 16 ----------------
> pc-bios/s390-ccw/virtio-net.c | 1 +
> pc-bios/s390-ccw/virtio-scsi.c | 1 +
> 4 files changed, 19 insertions(+), 16 deletions(-)
>
> diff --git a/pc-bios/s390-ccw/helper.h b/pc-bios/s390-ccw/helper.h
> index 78d5bc7442..d1b8b14d30 100644
> --- a/pc-bios/s390-ccw/helper.h
> +++ b/pc-bios/s390-ccw/helper.h
> @@ -14,6 +14,7 @@
> #define S390_CCW_HELPER_H
>
> #include "s390-ccw.h"
> +#include "time.h"
>
> /* Avoids compiler warnings when casting a pointer to a u32 */
> static inline uint32_t ptr2u32(void *ptr)
> @@ -28,4 +29,20 @@ static inline void *u32toptr(uint32_t n)
> return (void *)(uint64_t)n;
> }
>
> +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();
> + }
This actually asks for a future cleanup patch to replace the busy wait with a sleeping wait.
Anyway,
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> +}
> +
> #endif
> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
> index 9514b76596..c5820e43ae 100644
> --- a/pc-bios/s390-ccw/s390-ccw.h
> +++ b/pc-bios/s390-ccw/s390-ccw.h
> @@ -142,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/virtio-net.c b/pc-bios/s390-ccw/virtio-net.c
> index 4de03728bb..f018d58994 100644
> --- a/pc-bios/s390-ccw/virtio-net.c
> +++ b/pc-bios/s390-ccw/virtio-net.c
> @@ -20,6 +20,7 @@
> #include "s390-ccw.h"
> #include "virtio.h"
> #include "time.h"
> +#include "helper.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 0620651da8..907f8a08bd 100644
> --- a/pc-bios/s390-ccw/virtio-scsi.c
> +++ b/pc-bios/s390-ccw/virtio-scsi.c
> @@ -15,6 +15,7 @@
> #include "scsi.h"
> #include "virtio-scsi.h"
> #include "time.h"
> +#include "helper.h"
>
> static ScsiDevice default_scsi_device;
> static VirtioScsiCmdReq req;
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3 3/9] pc-bios: s390x: Move sleep and yield to helper.h
2020-05-27 8:34 ` Christian Borntraeger
@ 2020-05-27 8:35 ` Janosch Frank
0 siblings, 0 replies; 19+ messages in thread
From: Janosch Frank @ 2020-05-27 8:35 UTC (permalink / raw)
To: Christian Borntraeger, qemu-devel; +Cc: qemu-s390x, cohuck, thuth, david
[-- Attachment #1.1: Type: text/plain, Size: 3389 bytes --]
On 5/27/20 10:34 AM, Christian Borntraeger wrote:
>
>
> On 27.05.20 09:49, Janosch Frank wrote:
>> They are definitely helper functions.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>> pc-bios/s390-ccw/helper.h | 17 +++++++++++++++++
>> pc-bios/s390-ccw/s390-ccw.h | 16 ----------------
>> pc-bios/s390-ccw/virtio-net.c | 1 +
>> pc-bios/s390-ccw/virtio-scsi.c | 1 +
>> 4 files changed, 19 insertions(+), 16 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/helper.h b/pc-bios/s390-ccw/helper.h
>> index 78d5bc7442..d1b8b14d30 100644
>> --- a/pc-bios/s390-ccw/helper.h
>> +++ b/pc-bios/s390-ccw/helper.h
>> @@ -14,6 +14,7 @@
>> #define S390_CCW_HELPER_H
>>
>> #include "s390-ccw.h"
>> +#include "time.h"
>>
>> /* Avoids compiler warnings when casting a pointer to a u32 */
>> static inline uint32_t ptr2u32(void *ptr)
>> @@ -28,4 +29,20 @@ static inline void *u32toptr(uint32_t n)
>> return (void *)(uint64_t)n;
>> }
>>
>> +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();
>> + }
>
> This actually asks for a future cleanup patch to replace the busy wait with a sleeping wait.
Yes, already on my list, same as Pierre's busy wait in the kvm unit tests.
One impossible thing at a time.
>
> Anyway,
>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Thanks!
>
>> +}
>> +
>> #endif
>> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
>> index 9514b76596..c5820e43ae 100644
>> --- a/pc-bios/s390-ccw/s390-ccw.h
>> +++ b/pc-bios/s390-ccw/s390-ccw.h
>> @@ -142,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/virtio-net.c b/pc-bios/s390-ccw/virtio-net.c
>> index 4de03728bb..f018d58994 100644
>> --- a/pc-bios/s390-ccw/virtio-net.c
>> +++ b/pc-bios/s390-ccw/virtio-net.c
>> @@ -20,6 +20,7 @@
>> #include "s390-ccw.h"
>> #include "virtio.h"
>> #include "time.h"
>> +#include "helper.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 0620651da8..907f8a08bd 100644
>> --- a/pc-bios/s390-ccw/virtio-scsi.c
>> +++ b/pc-bios/s390-ccw/virtio-scsi.c
>> @@ -15,6 +15,7 @@
>> #include "scsi.h"
>> #include "virtio-scsi.h"
>> #include "time.h"
>> +#include "helper.h"
>>
>> static ScsiDevice default_scsi_device;
>> static VirtioScsiCmdReq req;
>>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 4/9] pc-bios: s390x: Get rid of magic offsets into the lowcore
2020-05-27 7:49 [PATCH v3 0/9] pc-bios: s390x: Cleanup part 1 Janosch Frank
` (2 preceding siblings ...)
2020-05-27 7:49 ` [PATCH v3 3/9] pc-bios: s390x: Move sleep and yield to helper.h Janosch Frank
@ 2020-05-27 7:49 ` Janosch Frank
2020-05-27 8:50 ` Christian Borntraeger
2020-05-27 7:49 ` [PATCH v3 5/9] pc-bios: s390x: Rename and use PSW_MASK_ZMODE constant Janosch Frank
` (5 subsequent siblings)
9 siblings, 1 reply; 19+ messages in thread
From: Janosch Frank @ 2020-05-27 7:49 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, thuth, david
If we have a lowcore struct that has members for offsets that we want
to touch, why not use it?
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
pc-bios/s390-ccw/cio.h | 17 +++++++++++------
pc-bios/s390-ccw/main.c | 8 +++-----
2 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/pc-bios/s390-ccw/cio.h b/pc-bios/s390-ccw/cio.h
index aaa432dedd..1e5d4e92e1 100644
--- a/pc-bios/s390-ccw/cio.h
+++ b/pc-bios/s390-ccw/cio.h
@@ -122,12 +122,17 @@ typedef struct schib {
} __attribute__ ((packed, aligned(4))) Schib;
typedef struct subchannel_id {
- __u32 cssid:8;
- __u32:4;
- __u32 m:1;
- __u32 ssid:2;
- __u32 one:1;
- __u32 sch_no:16;
+ union {
+ struct {
+ __u16 cssid:8;
+ __u16 reserved:4;
+ __u16 m:1;
+ __u16 ssid:2;
+ __u16 one:1;
+ };
+ __u16 sch_id;
+ };
+ __u16 sch_no;
} __attribute__ ((packed, aligned(4))) SubChannelId;
struct chsc_header {
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 4e65b411e1..8b912454c9 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -36,11 +36,9 @@ LowCore *lowcore; /* Yes, this *is* a pointer to address 0 */
*/
void write_subsystem_identification(void)
{
- SubChannelId *schid = (SubChannelId *) 184;
- uint32_t *zeroes = (uint32_t *) 188;
-
- *schid = blk_schid;
- *zeroes = 0;
+ lowcore->subchannel_id = blk_schid.sch_id;
+ lowcore->subchannel_nr = blk_schid.sch_no;
+ lowcore->io_int_parm = 0;
}
void write_iplb_location(void)
--
2.25.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 4/9] pc-bios: s390x: Get rid of magic offsets into the lowcore
2020-05-27 7:49 ` [PATCH v3 4/9] pc-bios: s390x: Get rid of magic offsets into the lowcore Janosch Frank
@ 2020-05-27 8:50 ` Christian Borntraeger
2020-05-27 11:23 ` Janosch Frank
0 siblings, 1 reply; 19+ messages in thread
From: Christian Borntraeger @ 2020-05-27 8:50 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: qemu-s390x, cohuck, thuth, david
On 27.05.20 09:49, Janosch Frank wrote:
> If we have a lowcore struct that has members for offsets that we want
> to touch, why not use it?
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> ---
> pc-bios/s390-ccw/cio.h | 17 +++++++++++------
> pc-bios/s390-ccw/main.c | 8 +++-----
> 2 files changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/pc-bios/s390-ccw/cio.h b/pc-bios/s390-ccw/cio.h
> index aaa432dedd..1e5d4e92e1 100644
> --- a/pc-bios/s390-ccw/cio.h
> +++ b/pc-bios/s390-ccw/cio.h
> @@ -122,12 +122,17 @@ typedef struct schib {
> } __attribute__ ((packed, aligned(4))) Schib;
>
> typedef struct subchannel_id {
> - __u32 cssid:8;
> - __u32:4;
> - __u32 m:1;
> - __u32 ssid:2;
> - __u32 one:1;
> - __u32 sch_no:16;
> + union {
> + struct {
> + __u16 cssid:8;
> + __u16 reserved:4;
> + __u16 m:1;
> + __u16 ssid:2;
> + __u16 one:1;
> + };
> + __u16 sch_id;
> + };
> + __u16 sch_no;
> } __attribute__ ((packed, aligned(4))) SubChannelId;
> > struct chsc_header {
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index 4e65b411e1..8b912454c9 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -36,11 +36,9 @@ LowCore *lowcore; /* Yes, this *is* a pointer to address 0 */
> */
> void write_subsystem_identification(void)
> {
> - SubChannelId *schid = (SubChannelId *) 184;
> - uint32_t *zeroes = (uint32_t *) 188;
> -
> - *schid = blk_schid;
> - *zeroes = 0;
> + lowcore->subchannel_id = blk_schid.sch_id;
> + lowcore->subchannel_nr = blk_schid.sch_no;
> + lowcore->io_int_parm = 0;
> }
>
> void write_iplb_location(void)
As an alternative, why not change the definition of Lowcore and replace both 16bit
fields lowcore->subchannel_id and lowcore->subchannel_nr with SubChannelID.
You can then simply assign lowcore->schid= blk_schid;
?
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3 4/9] pc-bios: s390x: Get rid of magic offsets into the lowcore
2020-05-27 8:50 ` Christian Borntraeger
@ 2020-05-27 11:23 ` Janosch Frank
0 siblings, 0 replies; 19+ messages in thread
From: Janosch Frank @ 2020-05-27 11:23 UTC (permalink / raw)
To: Christian Borntraeger, qemu-devel; +Cc: qemu-s390x, cohuck, thuth, david
[-- Attachment #1.1: Type: text/plain, Size: 2306 bytes --]
On 5/27/20 10:50 AM, Christian Borntraeger wrote:
>
>
> On 27.05.20 09:49, Janosch Frank wrote:
>> If we have a lowcore struct that has members for offsets that we want
>> to touch, why not use it?
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>> ---
>> pc-bios/s390-ccw/cio.h | 17 +++++++++++------
>> pc-bios/s390-ccw/main.c | 8 +++-----
>> 2 files changed, 14 insertions(+), 11 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/cio.h b/pc-bios/s390-ccw/cio.h
>> index aaa432dedd..1e5d4e92e1 100644
>> --- a/pc-bios/s390-ccw/cio.h
>> +++ b/pc-bios/s390-ccw/cio.h
>> @@ -122,12 +122,17 @@ typedef struct schib {
>> } __attribute__ ((packed, aligned(4))) Schib;
>>
>> typedef struct subchannel_id {
>> - __u32 cssid:8;
>> - __u32:4;
>> - __u32 m:1;
>> - __u32 ssid:2;
>> - __u32 one:1;
>> - __u32 sch_no:16;
>> + union {
>> + struct {
>> + __u16 cssid:8;
>> + __u16 reserved:4;
>> + __u16 m:1;
>> + __u16 ssid:2;
>> + __u16 one:1;
>> + };
>> + __u16 sch_id;
>> + };
>> + __u16 sch_no;
>> } __attribute__ ((packed, aligned(4))) SubChannelId;
>> > struct chsc_header {
>> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
>> index 4e65b411e1..8b912454c9 100644
>> --- a/pc-bios/s390-ccw/main.c
>> +++ b/pc-bios/s390-ccw/main.c
>> @@ -36,11 +36,9 @@ LowCore *lowcore; /* Yes, this *is* a pointer to address 0 */
>> */
>> void write_subsystem_identification(void)
>> {
>> - SubChannelId *schid = (SubChannelId *) 184;
>> - uint32_t *zeroes = (uint32_t *) 188;
>> -
>> - *schid = blk_schid;
>> - *zeroes = 0;
>> + lowcore->subchannel_id = blk_schid.sch_id;
>> + lowcore->subchannel_nr = blk_schid.sch_no;
>> + lowcore->io_int_parm = 0;
>> }
>>
>> void write_iplb_location(void)
>
> As an alternative, why not change the definition of Lowcore and replace both 16bit
> fields lowcore->subchannel_id and lowcore->subchannel_nr with SubChannelID.
> You can then simply assign lowcore->schid= blk_schid;
>
> ?
>
I think Pierre voted against that in the internal review if I remember
correctly.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 5/9] pc-bios: s390x: Rename and use PSW_MASK_ZMODE constant
2020-05-27 7:49 [PATCH v3 0/9] pc-bios: s390x: Cleanup part 1 Janosch Frank
` (3 preceding siblings ...)
2020-05-27 7:49 ` [PATCH v3 4/9] pc-bios: s390x: Get rid of magic offsets into the lowcore Janosch Frank
@ 2020-05-27 7:49 ` Janosch Frank
2020-05-27 7:49 ` [PATCH v3 6/9] pc-bios: s390x: Use PSW masks where possible and introduce PSW_MASK_SHORT_ADDR Janosch Frank
` (4 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: Janosch Frank @ 2020-05-27 7:49 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, thuth, david
ZMODE has a lot of ambiguity with the ESAME architecture mode, but is
actually 64 bit addressing.
As PSW_MASK_64 is now effectively 33 bit long and the PSWLegacy struct
has 2 32 bit members, let's also use a unsigned long pointer in
dasd-ipl.c instead when oring the constant into a 8 byte PSW.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
pc-bios/s390-ccw/dasd-ipl.c | 5 ++---
pc-bios/s390-ccw/s390-arch.h | 2 +-
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/pc-bios/s390-ccw/dasd-ipl.c b/pc-bios/s390-ccw/dasd-ipl.c
index 0fc879bb8e..0dbad051a2 100644
--- a/pc-bios/s390-ccw/dasd-ipl.c
+++ b/pc-bios/s390-ccw/dasd-ipl.c
@@ -206,7 +206,7 @@ static void run_ipl2(SubChannelId schid, uint16_t cutype, uint32_t addr)
*/
void dasd_ipl(SubChannelId schid, uint16_t cutype)
{
- PSWLegacy *pswl = (PSWLegacy *) 0x00;
+ unsigned long *pswl = 0x0;
uint32_t ipl2_addr;
/* Construct Read IPL CCW and run it to read IPL1 from boot disk */
@@ -229,7 +229,6 @@ void dasd_ipl(SubChannelId schid, uint16_t cutype)
run_ipl2(schid, cutype, ipl2_addr);
/* Transfer control to the guest operating system */
- pswl->mask |= PSW_MASK_EAMODE; /* Force z-mode */
- pswl->addr |= PSW_MASK_BAMODE; /* ... */
+ *pswl |= PSW_MASK_64; /* Force 64 bit addressing */
jump_to_low_kernel();
}
diff --git a/pc-bios/s390-ccw/s390-arch.h b/pc-bios/s390-ccw/s390-arch.h
index 5f36361c02..73852029d4 100644
--- a/pc-bios/s390-ccw/s390-arch.h
+++ b/pc-bios/s390-ccw/s390-arch.h
@@ -29,7 +29,7 @@ _Static_assert(sizeof(struct PSWLegacy) == 8, "PSWLegacy size incorrect");
#define PSW_MASK_WAIT 0x0002000000000000ULL
#define PSW_MASK_EAMODE 0x0000000100000000ULL
#define PSW_MASK_BAMODE 0x0000000080000000ULL
-#define PSW_MASK_ZMODE (PSW_MASK_EAMODE | PSW_MASK_BAMODE)
+#define PSW_MASK_64 (PSW_MASK_EAMODE | PSW_MASK_BAMODE)
/* Low core mapping */
typedef struct LowCore {
--
2.25.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v3 6/9] pc-bios: s390x: Use PSW masks where possible and introduce PSW_MASK_SHORT_ADDR
2020-05-27 7:49 [PATCH v3 0/9] pc-bios: s390x: Cleanup part 1 Janosch Frank
` (4 preceding siblings ...)
2020-05-27 7:49 ` [PATCH v3 5/9] pc-bios: s390x: Rename and use PSW_MASK_ZMODE constant Janosch Frank
@ 2020-05-27 7:49 ` Janosch Frank
2020-05-27 7:49 ` [PATCH v3 7/9] pc-bios: s390x: Move panic() into header and add infinite loop Janosch Frank
` (3 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: Janosch Frank @ 2020-05-27 7:49 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, thuth, david
Let's move some of the PSW mask defines into s390-arch.h and use them
in jump2ipl.c. Also let's introduce a new constant for the address
mask of 8 byte (short) PSWs.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
pc-bios/s390-ccw/jump2ipl.c | 10 ++++------
pc-bios/s390-ccw/s390-arch.h | 2 ++
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
index 4eba2510b0..767012bf0c 100644
--- a/pc-bios/s390-ccw/jump2ipl.c
+++ b/pc-bios/s390-ccw/jump2ipl.c
@@ -8,12 +8,10 @@
#include "libc.h"
#include "s390-ccw.h"
+#include "s390-arch.h"
#define KERN_IMAGE_START 0x010000UL
-#define PSW_MASK_64 0x0000000100000000ULL
-#define PSW_MASK_32 0x0000000080000000ULL
-#define PSW_MASK_SHORTPSW 0x0008000000000000ULL
-#define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_32 | PSW_MASK_64)
+#define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64)
typedef struct ResetInfo {
uint64_t ipl_psw;
@@ -54,7 +52,7 @@ void jump_to_IPL_code(uint64_t address)
current->ipl_psw = (uint64_t) &jump_to_IPL_2;
current->ipl_psw |= RESET_PSW_MASK;
- current->ipl_continue = address & 0x7fffffff;
+ current->ipl_continue = address & PSW_MASK_SHORT_ADDR;
debug_print_int("set IPL addr to", current->ipl_continue);
@@ -86,7 +84,7 @@ void jump_to_low_kernel(void)
/* Trying to get PSW at zero address */
if (*((uint64_t *)0) & RESET_PSW_MASK) {
- jump_to_IPL_code((*((uint64_t *)0)) & 0x7fffffff);
+ jump_to_IPL_code((*((uint64_t *)0)) & PSW_MASK_SHORT_ADDR);
}
/* No other option left, so use the Linux kernel start address */
diff --git a/pc-bios/s390-ccw/s390-arch.h b/pc-bios/s390-ccw/s390-arch.h
index 73852029d4..6da44d4436 100644
--- a/pc-bios/s390-ccw/s390-arch.h
+++ b/pc-bios/s390-ccw/s390-arch.h
@@ -26,9 +26,11 @@ _Static_assert(sizeof(struct PSWLegacy) == 8, "PSWLegacy size incorrect");
/* s390 psw bit masks */
#define PSW_MASK_IOINT 0x0200000000000000ULL
+#define PSW_MASK_SHORTPSW 0x0008000000000000ULL
#define PSW_MASK_WAIT 0x0002000000000000ULL
#define PSW_MASK_EAMODE 0x0000000100000000ULL
#define PSW_MASK_BAMODE 0x0000000080000000ULL
+#define PSW_MASK_SHORT_ADDR 0x000000007fffffffULL
#define PSW_MASK_64 (PSW_MASK_EAMODE | PSW_MASK_BAMODE)
/* Low core mapping */
--
2.25.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v3 7/9] pc-bios: s390x: Move panic() into header and add infinite loop
2020-05-27 7:49 [PATCH v3 0/9] pc-bios: s390x: Cleanup part 1 Janosch Frank
` (5 preceding siblings ...)
2020-05-27 7:49 ` [PATCH v3 6/9] pc-bios: s390x: Use PSW masks where possible and introduce PSW_MASK_SHORT_ADDR Janosch Frank
@ 2020-05-27 7:49 ` Janosch Frank
2020-05-27 7:49 ` [PATCH v3 8/9] pc-bios: s390x: Use ebcdic2ascii table Janosch Frank
` (2 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: Janosch Frank @ 2020-05-27 7:49 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, thuth, david
panic() was defined for the ccw and net bios, i.e. twice, so it's
cleaner to rather put it into the header.
Also let's add an infinite loop into the assembly of disabled_wait() so
the caller doesn't need to take care of it.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
pc-bios/s390-ccw/main.c | 7 -------
pc-bios/s390-ccw/netmain.c | 8 --------
pc-bios/s390-ccw/s390-ccw.h | 9 +++++++--
pc-bios/s390-ccw/start.S | 5 +++--
4 files changed, 10 insertions(+), 19 deletions(-)
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 8b912454c9..146a50760b 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -46,13 +46,6 @@ void write_iplb_location(void)
lowcore->ptr_iplb = ptr2u32(&iplb);
}
-void panic(const char *string)
-{
- sclp_print(string);
- disabled_wait();
- while (1) { }
-}
-
unsigned int get_loadparm_index(void)
{
return atoui(loadparm_str);
diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
index 73def8de4f..ef7ce1e01d 100644
--- a/pc-bios/s390-ccw/netmain.c
+++ b/pc-bios/s390-ccw/netmain.c
@@ -439,14 +439,6 @@ static int net_try_direct_tftp_load(filename_ip_t *fn_ip)
return rc;
}
-void panic(const char *string)
-{
- sclp_print(string);
- for (;;) {
- disabled_wait();
- }
-}
-
void write_subsystem_identification(void)
{
SubChannelId *schid = (SubChannelId *) 184;
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index c5820e43ae..36b884cced 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -50,12 +50,11 @@ typedef unsigned long long __u64;
#include "iplb.h"
/* start.s */
-void disabled_wait(void);
+void disabled_wait(void) __attribute__ ((__noreturn__));
void consume_sclp_int(void);
void consume_io_int(void);
/* main.c */
-void panic(const char *string);
void write_subsystem_identification(void);
void write_iplb_location(void);
extern char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
@@ -91,6 +90,12 @@ bool menu_is_enabled_enum(void);
#define MAX_BOOT_ENTRIES 31
+static inline void panic(const char *string)
+{
+ sclp_print(string);
+ disabled_wait();
+}
+
static inline void fill_hex(char *out, unsigned char val)
{
const char hex[] = "0123456789abcdef";
diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
index aa8fceb19d..35be141d8d 100644
--- a/pc-bios/s390-ccw/start.S
+++ b/pc-bios/s390-ccw/start.S
@@ -47,8 +47,9 @@ memsetxc:
*/
.globl disabled_wait
disabled_wait:
- larl %r1,disabled_wait_psw
- lpswe 0(%r1)
+ larl %r1,disabled_wait_psw
+ lpswe 0(%r1)
+1: j 1b
/*
--
2.25.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v3 8/9] pc-bios: s390x: Use ebcdic2ascii table
2020-05-27 7:49 [PATCH v3 0/9] pc-bios: s390x: Cleanup part 1 Janosch Frank
` (6 preceding siblings ...)
2020-05-27 7:49 ` [PATCH v3 7/9] pc-bios: s390x: Move panic() into header and add infinite loop Janosch Frank
@ 2020-05-27 7:49 ` Janosch Frank
2020-05-27 7:49 ` [PATCH v3 9/9] pc-bios: s390x: Make u32 ptr check explicit Janosch Frank
2020-05-27 10:30 ` [PATCH v3 0/9] pc-bios: s390x: Cleanup part 1 no-reply
9 siblings, 0 replies; 19+ messages in thread
From: Janosch Frank @ 2020-05-27 7:49 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, thuth, david
Why should we do conversion of a ebcdic value if we have a handy table
where we could look up the ascii value instead?
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
pc-bios/s390-ccw/bootmap.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index d13b7cbd15..97205674e5 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -328,9 +328,7 @@ static void print_eckd_ldl_msg(ECKD_IPL_mode_t mode)
msg[0] = '2';
break;
default:
- msg[0] = vlbl->LDL_version;
- msg[0] &= 0x0f; /* convert EBCDIC */
- msg[0] |= 0x30; /* to ASCII (digit) */
+ msg[0] = ebc2asc[vlbl->LDL_version];
msg[1] = '?';
break;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v3 9/9] pc-bios: s390x: Make u32 ptr check explicit
2020-05-27 7:49 [PATCH v3 0/9] pc-bios: s390x: Cleanup part 1 Janosch Frank
` (7 preceding siblings ...)
2020-05-27 7:49 ` [PATCH v3 8/9] pc-bios: s390x: Use ebcdic2ascii table Janosch Frank
@ 2020-05-27 7:49 ` Janosch Frank
2020-05-27 10:30 ` [PATCH v3 0/9] pc-bios: s390x: Cleanup part 1 no-reply
9 siblings, 0 replies; 19+ messages in thread
From: Janosch Frank @ 2020-05-27 7:49 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, qemu-s390x, cohuck, thuth, david
Let's make it a bit more clear that we check the full 64 bits to fit
into the 32 we return.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Suggested-by: David Hildenbrand <david@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
pc-bios/s390-ccw/helper.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/pc-bios/s390-ccw/helper.h b/pc-bios/s390-ccw/helper.h
index d1b8b14d30..6b8b5a54a3 100644
--- a/pc-bios/s390-ccw/helper.h
+++ b/pc-bios/s390-ccw/helper.h
@@ -19,7 +19,7 @@
/* Avoids compiler warnings when casting a pointer to a u32 */
static inline uint32_t ptr2u32(void *ptr)
{
- IPL_assert((uint64_t)ptr <= 0xffffffff, "ptr2u32: ptr too large");
+ IPL_assert((uint64_t)ptr <= 0xffffffffull, "ptr2u32: ptr too large");
return (uint32_t)(uint64_t)ptr;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 0/9] pc-bios: s390x: Cleanup part 1
2020-05-27 7:49 [PATCH v3 0/9] pc-bios: s390x: Cleanup part 1 Janosch Frank
` (8 preceding siblings ...)
2020-05-27 7:49 ` [PATCH v3 9/9] pc-bios: s390x: Make u32 ptr check explicit Janosch Frank
@ 2020-05-27 10:30 ` no-reply
9 siblings, 0 replies; 19+ messages in thread
From: no-reply @ 2020-05-27 10:30 UTC (permalink / raw)
To: frankja; +Cc: thuth, david, cohuck, qemu-devel, borntraeger, qemu-s390x
Patchew URL: https://patchew.org/QEMU/20200527074920.43669-1-frankja@linux.ibm.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 20200527074920.43669-1-frankja@linux.ibm.com
Subject: [PATCH v3 0/9] pc-bios: s390x: Cleanup part 1
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Switched to a new branch 'test'
799f5f1 pc-bios: s390x: Make u32 ptr check explicit
64c8922 pc-bios: s390x: Use ebcdic2ascii table
3e12724 pc-bios: s390x: Move panic() into header and add infinite loop
49db366 pc-bios: s390x: Use PSW masks where possible and introduce PSW_MASK_SHORT_ADDR
1370a46 pc-bios: s390x: Rename and use PSW_MASK_ZMODE constant
9f1140a pc-bios: s390x: Get rid of magic offsets into the lowcore
dba1abc pc-bios: s390x: Move sleep and yield to helper.h
f694d50 pc-bios: s390x: Consolidate timing functions into time.h
ec7ac62 pc-bios: s390x: cio.c cleanup and compile fix
=== OUTPUT BEGIN ===
1/9 Checking commit ec7ac623cb81 (pc-bios: s390x: cio.c cleanup and compile fix)
2/9 Checking commit f694d5005ea2 (pc-bios: s390x: Consolidate timing functions into time.h)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#77:
new file mode 100644
total: 0 errors, 1 warnings, 127 lines checked
Patch 2/9 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/9 Checking commit dba1abc5f5c8 (pc-bios: s390x: Move sleep and yield to helper.h)
4/9 Checking commit 9f1140afc2da (pc-bios: s390x: Get rid of magic offsets into the lowcore)
ERROR: spaces required around that ':' (ctx:VxV)
#30: FILE: pc-bios/s390-ccw/cio.h:127:
+ __u16 cssid:8;
^
ERROR: spaces required around that ':' (ctx:VxV)
#31: FILE: pc-bios/s390-ccw/cio.h:128:
+ __u16 reserved:4;
^
ERROR: spaces required around that ':' (ctx:VxV)
#32: FILE: pc-bios/s390-ccw/cio.h:129:
+ __u16 m:1;
^
ERROR: spaces required around that ':' (ctx:VxV)
#33: FILE: pc-bios/s390-ccw/cio.h:130:
+ __u16 ssid:2;
^
ERROR: spaces required around that ':' (ctx:VxV)
#34: FILE: pc-bios/s390-ccw/cio.h:131:
+ __u16 one:1;
^
total: 5 errors, 0 warnings, 37 lines checked
Patch 4/9 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
5/9 Checking commit 1370a46cdd57 (pc-bios: s390x: Rename and use PSW_MASK_ZMODE constant)
6/9 Checking commit 49db366a3248 (pc-bios: s390x: Use PSW masks where possible and introduce PSW_MASK_SHORT_ADDR)
7/9 Checking commit 3e12724946c3 (pc-bios: s390x: Move panic() into header and add infinite loop)
8/9 Checking commit 64c89223f1f8 (pc-bios: s390x: Use ebcdic2ascii table)
9/9 Checking commit 799f5f138abb (pc-bios: s390x: Make u32 ptr check explicit)
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20200527074920.43669-1-frankja@linux.ibm.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 19+ messages in thread