mbox series

[v5,00/12] pc-bios: s390x: Cleanup part 1

Message ID 20200624075226.92728-1-frankja@linux.ibm.com (mailing list archive)
Headers show
Series pc-bios: s390x: Cleanup part 1 | expand

Message

Janosch Frank June 24, 2020, 7:52 a.m. UTC
The bios is in dire need for a cleanup as there are still a lot of
magic constants being used throughout as well as duplicated code.

In the first part of this series we consolidate constants and
functions, as well as doing some minor cleanups and fixes.

The patches are available here:
https://github.com/frankjaa/qemu/pull/new/cleanup_bios

v5:
	* Fixed whitespace damage
	* Removed reset PSW mask changes in dasd-ipl.c
	* Added jump2ipl.c cleanup patches

v4:
	* Renamed time.h to s390-time.h
	* Fixed function names in sleep()
	* Changed order of sense_id_ccw initialization
	* Added missing include before sleep()

v3:
	* Dropped 0x00 to 0x0/0 patch
	* Moved some timing functions into helper.h instead of time.h
	* Fixed IPL psw manipulation in dasd-ipl.c
 	* Minor cosmetic fixes found by review

v2:
	* Included cio fixup to get rid of compile errors...
	* Minor cosmetic fixes found by review


Janosch Frank (12):
  pc-bios: s390x: cio.c cleanup and compile fix
  pc-bios: s390x: Consolidate timing functions into time.h
  pc-bios: s390x: Move sleep and yield to helper.h
  pc-bios: s390x: Get rid of magic offsets into the lowcore
  pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes
  pc-bios: s390x: Rename PSW_MASK_ZMODE to PSW_MASK_64
  pc-bios: s390x: Use PSW masks where possible and introduce
    PSW_MASK_SHORT_ADDR
  pc-bios: s390x: Move panic() into header and add infinite loop
  pc-bios: s390x: Use ebcdic2ascii table
  pc-bios: s390x: Make u32 ptr check explicit
  pc-bios: s390x: Fix bootmap.c passing PSWs as addresses
  pc-bios: s390x: Cleanup jump to ipl code

 pc-bios/s390-ccw/bootmap.c     |  9 ++++----
 pc-bios/s390-ccw/bootmap.h     |  2 +-
 pc-bios/s390-ccw/cio.c         | 40 +++++++++++++++++++---------------
 pc-bios/s390-ccw/cio.h         | 17 ++++++++++-----
 pc-bios/s390-ccw/dasd-ipl.c    |  3 ---
 pc-bios/s390-ccw/helper.h      | 19 +++++++++++++++-
 pc-bios/s390-ccw/jump2ipl.c    | 35 ++++++++++++-----------------
 pc-bios/s390-ccw/main.c        | 15 +++----------
 pc-bios/s390-ccw/menu.c        |  1 +
 pc-bios/s390-ccw/netmain.c     | 23 +++----------------
 pc-bios/s390-ccw/s390-arch.h   |  4 +++-
 pc-bios/s390-ccw/s390-ccw.h    | 27 ++++++-----------------
 pc-bios/s390-ccw/s390-time.h   | 23 +++++++++++++++++++
 pc-bios/s390-ccw/start.S       |  5 +++--
 pc-bios/s390-ccw/virtio-net.c  |  2 ++
 pc-bios/s390-ccw/virtio-scsi.c |  2 ++
 pc-bios/s390-ccw/virtio.c      | 18 +++------------
 17 files changed, 120 insertions(+), 125 deletions(-)
 create mode 100644 pc-bios/s390-ccw/s390-time.h

Comments

no-reply@patchew.org June 24, 2020, 8:06 a.m. UTC | #1
Patchew URL: https://patchew.org/QEMU/20200624075226.92728-1-frankja@linux.ibm.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH v5 00/12] pc-bios: s390x: Cleanup part 1
Type: series
Message-id: 20200624075226.92728-1-frankja@linux.ibm.com

=== 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 ===

From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20200624075226.92728-1-frankja@linux.ibm.com -> patchew/20200624075226.92728-1-frankja@linux.ibm.com
Switched to a new branch 'test'
f7789d0 pc-bios: s390x: Cleanup jump to ipl code
8680ca2 pc-bios: s390x: Fix bootmap.c passing PSWs as addresses
830483b pc-bios: s390x: Make u32 ptr check explicit
f313bb2 pc-bios: s390x: Use ebcdic2ascii table
00728a2 pc-bios: s390x: Move panic() into header and add infinite loop
d025a9c pc-bios: s390x: Use PSW masks where possible and introduce PSW_MASK_SHORT_ADDR
e33e9ff pc-bios: s390x: Rename PSW_MASK_ZMODE to PSW_MASK_64
c98ec6e pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes
f22b573 pc-bios: s390x: Get rid of magic offsets into the lowcore
aafb016 pc-bios: s390x: Move sleep and yield to helper.h
855c1c9 pc-bios: s390x: Consolidate timing functions into time.h
b906f41 pc-bios: s390x: cio.c cleanup and compile fix

=== OUTPUT BEGIN ===
1/12 Checking commit b906f41df28f (pc-bios: s390x: cio.c cleanup and compile fix)
2/12 Checking commit 855c1c9be4fe (pc-bios: s390x: Consolidate timing functions into time.h)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#94: 
new file mode 100644

total: 0 errors, 1 warnings, 142 lines checked

Patch 2/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
3/12 Checking commit aafb016fdfe0 (pc-bios: s390x: Move sleep and yield to helper.h)
4/12 Checking commit f22b573bc73e (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/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

5/12 Checking commit c98ec6eb101b (pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes)
6/12 Checking commit e33e9fff145a (pc-bios: s390x: Rename PSW_MASK_ZMODE to PSW_MASK_64)
7/12 Checking commit d025a9ce4c67 (pc-bios: s390x: Use PSW masks where possible and introduce PSW_MASK_SHORT_ADDR)
8/12 Checking commit 00728a2fdf9e (pc-bios: s390x: Move panic() into header and add infinite loop)
9/12 Checking commit f313bb25948c (pc-bios: s390x: Use ebcdic2ascii table)
10/12 Checking commit 830483bf940d (pc-bios: s390x: Make u32 ptr check explicit)
11/12 Checking commit 8680ca210f71 (pc-bios: s390x: Fix bootmap.c passing PSWs as addresses)
12/12 Checking commit f7789d0e1906 (pc-bios: s390x: Cleanup jump to ipl code)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200624075226.92728-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
Cornelia Huck June 24, 2020, 10:44 a.m. UTC | #2
On Wed, 24 Jun 2020 03:52:14 -0400
Janosch Frank <frankja@linux.ibm.com> wrote:

> The bios is in dire need for a cleanup as there are still a lot of
> magic constants being used throughout as well as duplicated code.
> 
> In the first part of this series we consolidate constants and
> functions, as well as doing some minor cleanups and fixes.
> 
> The patches are available here:
> https://github.com/frankjaa/qemu/pull/new/cleanup_bios
> 
> v5:
> 	* Fixed whitespace damage
> 	* Removed reset PSW mask changes in dasd-ipl.c
> 	* Added jump2ipl.c cleanup patches
> 
> v4:
> 	* Renamed time.h to s390-time.h
> 	* Fixed function names in sleep()
> 	* Changed order of sense_id_ccw initialization
> 	* Added missing include before sleep()
> 
> v3:
> 	* Dropped 0x00 to 0x0/0 patch
> 	* Moved some timing functions into helper.h instead of time.h
> 	* Fixed IPL psw manipulation in dasd-ipl.c
>  	* Minor cosmetic fixes found by review
> 
> v2:
> 	* Included cio fixup to get rid of compile errors...
> 	* Minor cosmetic fixes found by review
> 
> 
> Janosch Frank (12):
>   pc-bios: s390x: cio.c cleanup and compile fix
>   pc-bios: s390x: Consolidate timing functions into time.h
>   pc-bios: s390x: Move sleep and yield to helper.h
>   pc-bios: s390x: Get rid of magic offsets into the lowcore
>   pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes
>   pc-bios: s390x: Rename PSW_MASK_ZMODE to PSW_MASK_64
>   pc-bios: s390x: Use PSW masks where possible and introduce
>     PSW_MASK_SHORT_ADDR
>   pc-bios: s390x: Move panic() into header and add infinite loop
>   pc-bios: s390x: Use ebcdic2ascii table
>   pc-bios: s390x: Make u32 ptr check explicit
>   pc-bios: s390x: Fix bootmap.c passing PSWs as addresses
>   pc-bios: s390x: Cleanup jump to ipl code
> 
>  pc-bios/s390-ccw/bootmap.c     |  9 ++++----
>  pc-bios/s390-ccw/bootmap.h     |  2 +-
>  pc-bios/s390-ccw/cio.c         | 40 +++++++++++++++++++---------------
>  pc-bios/s390-ccw/cio.h         | 17 ++++++++++-----
>  pc-bios/s390-ccw/dasd-ipl.c    |  3 ---
>  pc-bios/s390-ccw/helper.h      | 19 +++++++++++++++-
>  pc-bios/s390-ccw/jump2ipl.c    | 35 ++++++++++++-----------------
>  pc-bios/s390-ccw/main.c        | 15 +++----------
>  pc-bios/s390-ccw/menu.c        |  1 +
>  pc-bios/s390-ccw/netmain.c     | 23 +++----------------
>  pc-bios/s390-ccw/s390-arch.h   |  4 +++-
>  pc-bios/s390-ccw/s390-ccw.h    | 27 ++++++-----------------
>  pc-bios/s390-ccw/s390-time.h   | 23 +++++++++++++++++++
>  pc-bios/s390-ccw/start.S       |  5 +++--
>  pc-bios/s390-ccw/virtio-net.c  |  2 ++
>  pc-bios/s390-ccw/virtio-scsi.c |  2 ++
>  pc-bios/s390-ccw/virtio.c      | 18 +++------------
>  17 files changed, 120 insertions(+), 125 deletions(-)
>  create mode 100644 pc-bios/s390-ccw/s390-time.h
> 

Hm... what's the general status of this? Most of the patches have at
least one R-b/A-b already, I see.

Do the s390-ccw boot maintainers want to pick this (once the rest has
been looked at) and then send me a pull req, or should I pick it when
it is good to go? Softfreeze is less than two weeks away :)
Thomas Huth June 24, 2020, 10:46 a.m. UTC | #3
On 24/06/2020 12.44, Cornelia Huck wrote:
> On Wed, 24 Jun 2020 03:52:14 -0400
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> The bios is in dire need for a cleanup as there are still a lot of
>> magic constants being used throughout as well as duplicated code.
>>
>> In the first part of this series we consolidate constants and
>> functions, as well as doing some minor cleanups and fixes.
>>
>> The patches are available here:
>> https://github.com/frankjaa/qemu/pull/new/cleanup_bios
>>
>> v5:
>> 	* Fixed whitespace damage
>> 	* Removed reset PSW mask changes in dasd-ipl.c
>> 	* Added jump2ipl.c cleanup patches
>>
>> v4:
>> 	* Renamed time.h to s390-time.h
>> 	* Fixed function names in sleep()
>> 	* Changed order of sense_id_ccw initialization
>> 	* Added missing include before sleep()
>>
>> v3:
>> 	* Dropped 0x00 to 0x0/0 patch
>> 	* Moved some timing functions into helper.h instead of time.h
>> 	* Fixed IPL psw manipulation in dasd-ipl.c
>>   	* Minor cosmetic fixes found by review
>>
>> v2:
>> 	* Included cio fixup to get rid of compile errors...
>> 	* Minor cosmetic fixes found by review
>>
>>
>> Janosch Frank (12):
>>    pc-bios: s390x: cio.c cleanup and compile fix
>>    pc-bios: s390x: Consolidate timing functions into time.h
>>    pc-bios: s390x: Move sleep and yield to helper.h
>>    pc-bios: s390x: Get rid of magic offsets into the lowcore
>>    pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes
>>    pc-bios: s390x: Rename PSW_MASK_ZMODE to PSW_MASK_64
>>    pc-bios: s390x: Use PSW masks where possible and introduce
>>      PSW_MASK_SHORT_ADDR
>>    pc-bios: s390x: Move panic() into header and add infinite loop
>>    pc-bios: s390x: Use ebcdic2ascii table
>>    pc-bios: s390x: Make u32 ptr check explicit
>>    pc-bios: s390x: Fix bootmap.c passing PSWs as addresses
>>    pc-bios: s390x: Cleanup jump to ipl code
>>
>>   pc-bios/s390-ccw/bootmap.c     |  9 ++++----
>>   pc-bios/s390-ccw/bootmap.h     |  2 +-
>>   pc-bios/s390-ccw/cio.c         | 40 +++++++++++++++++++---------------
>>   pc-bios/s390-ccw/cio.h         | 17 ++++++++++-----
>>   pc-bios/s390-ccw/dasd-ipl.c    |  3 ---
>>   pc-bios/s390-ccw/helper.h      | 19 +++++++++++++++-
>>   pc-bios/s390-ccw/jump2ipl.c    | 35 ++++++++++++-----------------
>>   pc-bios/s390-ccw/main.c        | 15 +++----------
>>   pc-bios/s390-ccw/menu.c        |  1 +
>>   pc-bios/s390-ccw/netmain.c     | 23 +++----------------
>>   pc-bios/s390-ccw/s390-arch.h   |  4 +++-
>>   pc-bios/s390-ccw/s390-ccw.h    | 27 ++++++-----------------
>>   pc-bios/s390-ccw/s390-time.h   | 23 +++++++++++++++++++
>>   pc-bios/s390-ccw/start.S       |  5 +++--
>>   pc-bios/s390-ccw/virtio-net.c  |  2 ++
>>   pc-bios/s390-ccw/virtio-scsi.c |  2 ++
>>   pc-bios/s390-ccw/virtio.c      | 18 +++------------
>>   17 files changed, 120 insertions(+), 125 deletions(-)
>>   create mode 100644 pc-bios/s390-ccw/s390-time.h
>>
> 
> Hm... what's the general status of this? Most of the patches have at
> least one R-b/A-b already, I see.
> 
> Do the s390-ccw boot maintainers want to pick this (once the rest has
> been looked at) and then send me a pull req, or should I pick it when
> it is good to go? Softfreeze is less than two weeks away :)

I'd like to review the missing parts and run my tests with the patches 
applied ... I'm just a little bit swamped right now, so please give me 
some more time...

  Thomas
Janosch Frank June 24, 2020, 10:57 a.m. UTC | #4
On 6/24/20 12:46 PM, Thomas Huth wrote:
> On 24/06/2020 12.44, Cornelia Huck wrote:
>> On Wed, 24 Jun 2020 03:52:14 -0400
>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>
>>> The bios is in dire need for a cleanup as there are still a lot of
>>> magic constants being used throughout as well as duplicated code.
>>>
>>> In the first part of this series we consolidate constants and
>>> functions, as well as doing some minor cleanups and fixes.
>>>
>>> The patches are available here:
>>> https://github.com/frankjaa/qemu/pull/new/cleanup_bios
>>>
>>> v5:
>>> 	* Fixed whitespace damage
>>> 	* Removed reset PSW mask changes in dasd-ipl.c
>>> 	* Added jump2ipl.c cleanup patches
>>>
>>> v4:
>>> 	* Renamed time.h to s390-time.h
>>> 	* Fixed function names in sleep()
>>> 	* Changed order of sense_id_ccw initialization
>>> 	* Added missing include before sleep()
>>>
>>> v3:
>>> 	* Dropped 0x00 to 0x0/0 patch
>>> 	* Moved some timing functions into helper.h instead of time.h
>>> 	* Fixed IPL psw manipulation in dasd-ipl.c
>>>   	* Minor cosmetic fixes found by review
>>>
>>> v2:
>>> 	* Included cio fixup to get rid of compile errors...
>>> 	* Minor cosmetic fixes found by review
>>>
>>>
>>> Janosch Frank (12):
>>>    pc-bios: s390x: cio.c cleanup and compile fix
>>>    pc-bios: s390x: Consolidate timing functions into time.h
>>>    pc-bios: s390x: Move sleep and yield to helper.h
>>>    pc-bios: s390x: Get rid of magic offsets into the lowcore
>>>    pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes
>>>    pc-bios: s390x: Rename PSW_MASK_ZMODE to PSW_MASK_64
>>>    pc-bios: s390x: Use PSW masks where possible and introduce
>>>      PSW_MASK_SHORT_ADDR
>>>    pc-bios: s390x: Move panic() into header and add infinite loop
>>>    pc-bios: s390x: Use ebcdic2ascii table
>>>    pc-bios: s390x: Make u32 ptr check explicit
>>>    pc-bios: s390x: Fix bootmap.c passing PSWs as addresses
>>>    pc-bios: s390x: Cleanup jump to ipl code
>>>
>>>   pc-bios/s390-ccw/bootmap.c     |  9 ++++----
>>>   pc-bios/s390-ccw/bootmap.h     |  2 +-
>>>   pc-bios/s390-ccw/cio.c         | 40 +++++++++++++++++++---------------
>>>   pc-bios/s390-ccw/cio.h         | 17 ++++++++++-----
>>>   pc-bios/s390-ccw/dasd-ipl.c    |  3 ---
>>>   pc-bios/s390-ccw/helper.h      | 19 +++++++++++++++-
>>>   pc-bios/s390-ccw/jump2ipl.c    | 35 ++++++++++++-----------------
>>>   pc-bios/s390-ccw/main.c        | 15 +++----------
>>>   pc-bios/s390-ccw/menu.c        |  1 +
>>>   pc-bios/s390-ccw/netmain.c     | 23 +++----------------
>>>   pc-bios/s390-ccw/s390-arch.h   |  4 +++-
>>>   pc-bios/s390-ccw/s390-ccw.h    | 27 ++++++-----------------
>>>   pc-bios/s390-ccw/s390-time.h   | 23 +++++++++++++++++++
>>>   pc-bios/s390-ccw/start.S       |  5 +++--
>>>   pc-bios/s390-ccw/virtio-net.c  |  2 ++
>>>   pc-bios/s390-ccw/virtio-scsi.c |  2 ++
>>>   pc-bios/s390-ccw/virtio.c      | 18 +++------------
>>>   17 files changed, 120 insertions(+), 125 deletions(-)
>>>   create mode 100644 pc-bios/s390-ccw/s390-time.h
>>>
>>
>> Hm... what's the general status of this? Most of the patches have at
>> least one R-b/A-b already, I see.
>>
>> Do the s390-ccw boot maintainers want to pick this (once the rest has
>> been looked at) and then send me a pull req, or should I pick it when
>> it is good to go? Softfreeze is less than two weeks away :)
> 
> I'd like to review the missing parts and run my tests with the patches 
> applied ... I'm just a little bit swamped right now, so please give me 
> some more time...
> 
>   Thomas

I'd suggest we hold off on the RFC patch since it makes testing all of
the boot methods necessary and I was only able to test direct, dasd
passthrough and virtio-ccw up to now. Time to build a testing environment...

I can move that patch into the next series for further discussion. It
was mostly added to prove the 64 bit PSW mask removal in patch #5.
Cornelia Huck June 24, 2020, 11:08 a.m. UTC | #5
On Wed, 24 Jun 2020 12:46:49 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 24/06/2020 12.44, Cornelia Huck wrote:
> > On Wed, 24 Jun 2020 03:52:14 -0400
> > Janosch Frank <frankja@linux.ibm.com> wrote:
> >   
> >> The bios is in dire need for a cleanup as there are still a lot of
> >> magic constants being used throughout as well as duplicated code.
> >>
> >> In the first part of this series we consolidate constants and
> >> functions, as well as doing some minor cleanups and fixes.
> >>
> >> The patches are available here:
> >> https://github.com/frankjaa/qemu/pull/new/cleanup_bios
> >>
> >> v5:
> >> 	* Fixed whitespace damage
> >> 	* Removed reset PSW mask changes in dasd-ipl.c
> >> 	* Added jump2ipl.c cleanup patches
> >>
> >> v4:
> >> 	* Renamed time.h to s390-time.h
> >> 	* Fixed function names in sleep()
> >> 	* Changed order of sense_id_ccw initialization
> >> 	* Added missing include before sleep()
> >>
> >> v3:
> >> 	* Dropped 0x00 to 0x0/0 patch
> >> 	* Moved some timing functions into helper.h instead of time.h
> >> 	* Fixed IPL psw manipulation in dasd-ipl.c
> >>   	* Minor cosmetic fixes found by review
> >>
> >> v2:
> >> 	* Included cio fixup to get rid of compile errors...
> >> 	* Minor cosmetic fixes found by review
> >>
> >>
> >> Janosch Frank (12):
> >>    pc-bios: s390x: cio.c cleanup and compile fix
> >>    pc-bios: s390x: Consolidate timing functions into time.h
> >>    pc-bios: s390x: Move sleep and yield to helper.h
> >>    pc-bios: s390x: Get rid of magic offsets into the lowcore
> >>    pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes
> >>    pc-bios: s390x: Rename PSW_MASK_ZMODE to PSW_MASK_64
> >>    pc-bios: s390x: Use PSW masks where possible and introduce
> >>      PSW_MASK_SHORT_ADDR
> >>    pc-bios: s390x: Move panic() into header and add infinite loop
> >>    pc-bios: s390x: Use ebcdic2ascii table
> >>    pc-bios: s390x: Make u32 ptr check explicit
> >>    pc-bios: s390x: Fix bootmap.c passing PSWs as addresses
> >>    pc-bios: s390x: Cleanup jump to ipl code
> >>
> >>   pc-bios/s390-ccw/bootmap.c     |  9 ++++----
> >>   pc-bios/s390-ccw/bootmap.h     |  2 +-
> >>   pc-bios/s390-ccw/cio.c         | 40 +++++++++++++++++++---------------
> >>   pc-bios/s390-ccw/cio.h         | 17 ++++++++++-----
> >>   pc-bios/s390-ccw/dasd-ipl.c    |  3 ---
> >>   pc-bios/s390-ccw/helper.h      | 19 +++++++++++++++-
> >>   pc-bios/s390-ccw/jump2ipl.c    | 35 ++++++++++++-----------------
> >>   pc-bios/s390-ccw/main.c        | 15 +++----------
> >>   pc-bios/s390-ccw/menu.c        |  1 +
> >>   pc-bios/s390-ccw/netmain.c     | 23 +++----------------
> >>   pc-bios/s390-ccw/s390-arch.h   |  4 +++-
> >>   pc-bios/s390-ccw/s390-ccw.h    | 27 ++++++-----------------
> >>   pc-bios/s390-ccw/s390-time.h   | 23 +++++++++++++++++++
> >>   pc-bios/s390-ccw/start.S       |  5 +++--
> >>   pc-bios/s390-ccw/virtio-net.c  |  2 ++
> >>   pc-bios/s390-ccw/virtio-scsi.c |  2 ++
> >>   pc-bios/s390-ccw/virtio.c      | 18 +++------------
> >>   17 files changed, 120 insertions(+), 125 deletions(-)
> >>   create mode 100644 pc-bios/s390-ccw/s390-time.h
> >>  
> > 
> > Hm... what's the general status of this? Most of the patches have at
> > least one R-b/A-b already, I see.
> > 
> > Do the s390-ccw boot maintainers want to pick this (once the rest has
> > been looked at) and then send me a pull req, or should I pick it when
> > it is good to go? Softfreeze is less than two weeks away :)  
> 
> I'd like to review the missing parts and run my tests with the patches 
> applied ... I'm just a little bit swamped right now, so please give me 
> some more time...

Sure, please don't feel hurried :)
Thomas Huth June 30, 2020, 8:48 a.m. UTC | #6
On 24/06/2020 12.57, Janosch Frank wrote:
> On 6/24/20 12:46 PM, Thomas Huth wrote:
>> On 24/06/2020 12.44, Cornelia Huck wrote:
>>> On Wed, 24 Jun 2020 03:52:14 -0400
>>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>>
>>>> The bios is in dire need for a cleanup as there are still a lot of
>>>> magic constants being used throughout as well as duplicated code.
>>>>
>>>> In the first part of this series we consolidate constants and
>>>> functions, as well as doing some minor cleanups and fixes.
>>>>
>>>> The patches are available here:
>>>> https://github.com/frankjaa/qemu/pull/new/cleanup_bios
>>>>
>>>> v5:
>>>> 	* Fixed whitespace damage
>>>> 	* Removed reset PSW mask changes in dasd-ipl.c
>>>> 	* Added jump2ipl.c cleanup patches
>>>>
>>>> v4:
>>>> 	* Renamed time.h to s390-time.h
>>>> 	* Fixed function names in sleep()
>>>> 	* Changed order of sense_id_ccw initialization
>>>> 	* Added missing include before sleep()
>>>>
>>>> v3:
>>>> 	* Dropped 0x00 to 0x0/0 patch
>>>> 	* Moved some timing functions into helper.h instead of time.h
>>>> 	* Fixed IPL psw manipulation in dasd-ipl.c
>>>>    	* Minor cosmetic fixes found by review
>>>>
>>>> v2:
>>>> 	* Included cio fixup to get rid of compile errors...
>>>> 	* Minor cosmetic fixes found by review
>>>>
>>>>
>>>> Janosch Frank (12):
>>>>     pc-bios: s390x: cio.c cleanup and compile fix
>>>>     pc-bios: s390x: Consolidate timing functions into time.h
>>>>     pc-bios: s390x: Move sleep and yield to helper.h
>>>>     pc-bios: s390x: Get rid of magic offsets into the lowcore
>>>>     pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes
>>>>     pc-bios: s390x: Rename PSW_MASK_ZMODE to PSW_MASK_64
>>>>     pc-bios: s390x: Use PSW masks where possible and introduce
>>>>       PSW_MASK_SHORT_ADDR
>>>>     pc-bios: s390x: Move panic() into header and add infinite loop
>>>>     pc-bios: s390x: Use ebcdic2ascii table
>>>>     pc-bios: s390x: Make u32 ptr check explicit
>>>>     pc-bios: s390x: Fix bootmap.c passing PSWs as addresses
>>>>     pc-bios: s390x: Cleanup jump to ipl code
>>>>
>>>>    pc-bios/s390-ccw/bootmap.c     |  9 ++++----
>>>>    pc-bios/s390-ccw/bootmap.h     |  2 +-
>>>>    pc-bios/s390-ccw/cio.c         | 40 +++++++++++++++++++---------------
>>>>    pc-bios/s390-ccw/cio.h         | 17 ++++++++++-----
>>>>    pc-bios/s390-ccw/dasd-ipl.c    |  3 ---
>>>>    pc-bios/s390-ccw/helper.h      | 19 +++++++++++++++-
>>>>    pc-bios/s390-ccw/jump2ipl.c    | 35 ++++++++++++-----------------
>>>>    pc-bios/s390-ccw/main.c        | 15 +++----------
>>>>    pc-bios/s390-ccw/menu.c        |  1 +
>>>>    pc-bios/s390-ccw/netmain.c     | 23 +++----------------
>>>>    pc-bios/s390-ccw/s390-arch.h   |  4 +++-
>>>>    pc-bios/s390-ccw/s390-ccw.h    | 27 ++++++-----------------
>>>>    pc-bios/s390-ccw/s390-time.h   | 23 +++++++++++++++++++
>>>>    pc-bios/s390-ccw/start.S       |  5 +++--
>>>>    pc-bios/s390-ccw/virtio-net.c  |  2 ++
>>>>    pc-bios/s390-ccw/virtio-scsi.c |  2 ++
>>>>    pc-bios/s390-ccw/virtio.c      | 18 +++------------
>>>>    17 files changed, 120 insertions(+), 125 deletions(-)
>>>>    create mode 100644 pc-bios/s390-ccw/s390-time.h
>>>>
>>>
>>> Hm... what's the general status of this? Most of the patches have at
>>> least one R-b/A-b already, I see.
>>>
>>> Do the s390-ccw boot maintainers want to pick this (once the rest has
>>> been looked at) and then send me a pull req, or should I pick it when
>>> it is good to go? Softfreeze is less than two weeks away :)
>>
>> I'd like to review the missing parts and run my tests with the patches
>> applied ... I'm just a little bit swamped right now, so please give me
>> some more time...
>>
>>    Thomas
> 
> I'd suggest we hold off on the RFC patch since it makes testing all of
> the boot methods necessary and I was only able to test direct, dasd
> passthrough and virtio-ccw up to now. Time to build a testing environment...
> 
> I can move that patch into the next series for further discussion. It
> was mostly added to prove the 64 bit PSW mask removal in patch #5.

I think patches 1-4 and 6-10 are ready to go, I also tested them now 
with my usual manual regression tests and there was no issue.

For patch 5, I first like to get some feedback from Jason, to understand 
the original intention of the removed code there.

Patch 11 needs a rework.

And for patch 12, I'd appreciate some feedback from Christian.

I'm planning to send a s390-ccw pull request for the softfreeze on 
Thursday, so if you want to see 5, 11 or 12 included there, it would be 
good to get this done within the next two days (otherwise, I think it's 
also not too critical to delay them to QEMU 5.2 instead).

  Thomas