* [PATCH 1/7] pc-bios: s390x: Fix bootmap.c zipl component entry data handling
2020-07-15 9:40 [PATCH 0/7] pc-bios: s390x: Cleanup part 2 Janosch Frank
@ 2020-07-15 9:40 ` Janosch Frank
2020-07-17 15:05 ` Thomas Huth
2020-07-22 6:50 ` Christian Borntraeger
2020-07-15 9:40 ` [PATCH 2/7] pc-bios: s390x: Cleanup jump to ipl code Janosch Frank
` (5 subsequent siblings)
6 siblings, 2 replies; 33+ messages in thread
From: Janosch Frank @ 2020-07-15 9:40 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, thuth, cohuck, david
The two main types of zipl component entries are execute and
load/data. The last member of the component entry struct therefore
denotes either a PSW or an address. Let's make this a bit more clear
by introducing a union and cleaning up the code that uses that struct
member.
The execute type component entries written by zipl contain short PSWs,
not addresses. Let's mask them and only pass the address part to
jump_to_IPL_code(uint64_t address) because it expects an address as
visible by the name of the argument.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
pc-bios/s390-ccw/bootmap.c | 5 +++--
pc-bios/s390-ccw/bootmap.h | 7 ++++++-
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
index 97205674e5..8747c4ea26 100644
--- a/pc-bios/s390-ccw/bootmap.c
+++ b/pc-bios/s390-ccw/bootmap.c
@@ -10,6 +10,7 @@
#include "libc.h"
#include "s390-ccw.h"
+#include "s390-arch.h"
#include "bootmap.h"
#include "virtio.h"
#include "bswap.h"
@@ -436,7 +437,7 @@ static void zipl_load_segment(ComponentEntry *entry)
char *blk_no = &err_msg[30]; /* where to print blockno in (those ZZs) */
blockno = entry->data.blockno;
- address = entry->load_address;
+ address = entry->compdat.load_addr;
debug_print_int("loading segment at block", blockno);
debug_print_int("addr", address);
@@ -514,7 +515,7 @@ static void zipl_run(ScsiBlockPtr *pte)
IPL_assert(entry->component_type == ZIPL_COMP_ENTRY_EXEC, "No EXEC entry");
/* should not return */
- jump_to_IPL_code(entry->load_address);
+ jump_to_IPL_code(entry->compdat.load_psw & PSW_MASK_SHORT_ADDR);
}
static void ipl_scsi(void)
diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
index 12a0166aae..3946aa3f8d 100644
--- a/pc-bios/s390-ccw/bootmap.h
+++ b/pc-bios/s390-ccw/bootmap.h
@@ -64,11 +64,16 @@ typedef struct BootMapTable {
BootMapPointer entry[];
} __attribute__ ((packed)) BootMapTable;
+typedef union ComponentEntryData {
+ uint64_t load_psw;
+ uint64_t load_addr;
+} ComponentEntryData;
+
typedef struct ComponentEntry {
ScsiBlockPtr data;
uint8_t pad[7];
uint8_t component_type;
- uint64_t load_address;
+ ComponentEntryData compdat;
} __attribute((packed)) ComponentEntry;
typedef struct ComponentHeader {
--
2.25.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 1/7] pc-bios: s390x: Fix bootmap.c zipl component entry data handling
2020-07-15 9:40 ` [PATCH 1/7] pc-bios: s390x: Fix bootmap.c zipl component entry data handling Janosch Frank
@ 2020-07-17 15:05 ` Thomas Huth
2020-07-22 6:50 ` Christian Borntraeger
1 sibling, 0 replies; 33+ messages in thread
From: Thomas Huth @ 2020-07-17 15:05 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: borntraeger, cohuck, david
On 15/07/2020 11.40, Janosch Frank wrote:
> The two main types of zipl component entries are execute and
> load/data. The last member of the component entry struct therefore
> denotes either a PSW or an address. Let's make this a bit more clear
> by introducing a union and cleaning up the code that uses that struct
> member.
>
> The execute type component entries written by zipl contain short PSWs,
> not addresses. Let's mask them and only pass the address part to
> jump_to_IPL_code(uint64_t address) because it expects an address as
> visible by the name of the argument.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> pc-bios/s390-ccw/bootmap.c | 5 +++--
> pc-bios/s390-ccw/bootmap.h | 7 ++++++-
> 2 files changed, 9 insertions(+), 3 deletions(-)
Reviewed-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/7] pc-bios: s390x: Fix bootmap.c zipl component entry data handling
2020-07-15 9:40 ` [PATCH 1/7] pc-bios: s390x: Fix bootmap.c zipl component entry data handling Janosch Frank
2020-07-17 15:05 ` Thomas Huth
@ 2020-07-22 6:50 ` Christian Borntraeger
2020-07-22 7:30 ` Janosch Frank
1 sibling, 1 reply; 33+ messages in thread
From: Christian Borntraeger @ 2020-07-22 6:50 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: thuth, cohuck, david
On 15.07.20 11:40, Janosch Frank wrote:
> The two main types of zipl component entries are execute and
> load/data. The last member of the component entry struct therefore
> denotes either a PSW or an address. Let's make this a bit more clear
> by introducing a union and cleaning up the code that uses that struct
> member.
>
> The execute type component entries written by zipl contain short PSWs,
> not addresses. Let's mask them and only pass the address part to
> jump_to_IPL_code(uint64_t address) because it expects an address as
> visible by the name of the argument.
If zipl actually specifies a PSW, shouldnt we actually USE that PSW including
the zipl specified mask?
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> pc-bios/s390-ccw/bootmap.c | 5 +++--
> pc-bios/s390-ccw/bootmap.h | 7 ++++++-
> 2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> index 97205674e5..8747c4ea26 100644
> --- a/pc-bios/s390-ccw/bootmap.c
> +++ b/pc-bios/s390-ccw/bootmap.c
> @@ -10,6 +10,7 @@
>
> #include "libc.h"
> #include "s390-ccw.h"
> +#include "s390-arch.h"
> #include "bootmap.h"
> #include "virtio.h"
> #include "bswap.h"
> @@ -436,7 +437,7 @@ static void zipl_load_segment(ComponentEntry *entry)
> char *blk_no = &err_msg[30]; /* where to print blockno in (those ZZs) */
>
> blockno = entry->data.blockno;
> - address = entry->load_address;
> + address = entry->compdat.load_addr;
>
> debug_print_int("loading segment at block", blockno);
> debug_print_int("addr", address);
> @@ -514,7 +515,7 @@ static void zipl_run(ScsiBlockPtr *pte)
> IPL_assert(entry->component_type == ZIPL_COMP_ENTRY_EXEC, "No EXEC entry");
>
> /* should not return */
> - jump_to_IPL_code(entry->load_address);
> + jump_to_IPL_code(entry->compdat.load_psw & PSW_MASK_SHORT_ADDR);
> }
>
> static void ipl_scsi(void)
> diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
> index 12a0166aae..3946aa3f8d 100644
> --- a/pc-bios/s390-ccw/bootmap.h
> +++ b/pc-bios/s390-ccw/bootmap.h
> @@ -64,11 +64,16 @@ typedef struct BootMapTable {
> BootMapPointer entry[];
> } __attribute__ ((packed)) BootMapTable;
>
> +typedef union ComponentEntryData {
> + uint64_t load_psw;
> + uint64_t load_addr;
> +} ComponentEntryData;
> +
> typedef struct ComponentEntry {
> ScsiBlockPtr data;
> uint8_t pad[7];
> uint8_t component_type;
> - uint64_t load_address;
> + ComponentEntryData compdat;
> } __attribute((packed)) ComponentEntry;
>
> typedef struct ComponentHeader {
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 1/7] pc-bios: s390x: Fix bootmap.c zipl component entry data handling
2020-07-22 6:50 ` Christian Borntraeger
@ 2020-07-22 7:30 ` Janosch Frank
2020-07-22 7:33 ` Christian Borntraeger
0 siblings, 1 reply; 33+ messages in thread
From: Janosch Frank @ 2020-07-22 7:30 UTC (permalink / raw)
To: Christian Borntraeger, qemu-devel; +Cc: thuth, cohuck, david
[-- Attachment #1.1: Type: text/plain, Size: 2994 bytes --]
On 7/22/20 8:50 AM, Christian Borntraeger wrote:
>
>
> On 15.07.20 11:40, Janosch Frank wrote:
>> The two main types of zipl component entries are execute and
>> load/data. The last member of the component entry struct therefore
>> denotes either a PSW or an address. Let's make this a bit more clear
>> by introducing a union and cleaning up the code that uses that struct
>> member.
>>
>> The execute type component entries written by zipl contain short PSWs,
>> not addresses. Let's mask them and only pass the address part to
>> jump_to_IPL_code(uint64_t address) because it expects an address as
>> visible by the name of the argument.
>
> If zipl actually specifies a PSW, shouldnt we actually USE that PSW including
> the zipl specified mask?
I expected the current approach to have some kind of meaning behind it,
if there isn't I'll be the first one to make it more sensible.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>> pc-bios/s390-ccw/bootmap.c | 5 +++--
>> pc-bios/s390-ccw/bootmap.h | 7 ++++++-
>> 2 files changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
>> index 97205674e5..8747c4ea26 100644
>> --- a/pc-bios/s390-ccw/bootmap.c
>> +++ b/pc-bios/s390-ccw/bootmap.c
>> @@ -10,6 +10,7 @@
>>
>> #include "libc.h"
>> #include "s390-ccw.h"
>> +#include "s390-arch.h"
>> #include "bootmap.h"
>> #include "virtio.h"
>> #include "bswap.h"
>> @@ -436,7 +437,7 @@ static void zipl_load_segment(ComponentEntry *entry)
>> char *blk_no = &err_msg[30]; /* where to print blockno in (those ZZs) */
>>
>> blockno = entry->data.blockno;
>> - address = entry->load_address;
>> + address = entry->compdat.load_addr;
>>
>> debug_print_int("loading segment at block", blockno);
>> debug_print_int("addr", address);
>> @@ -514,7 +515,7 @@ static void zipl_run(ScsiBlockPtr *pte)
>> IPL_assert(entry->component_type == ZIPL_COMP_ENTRY_EXEC, "No EXEC entry");
>>
>> /* should not return */
>> - jump_to_IPL_code(entry->load_address);
>> + jump_to_IPL_code(entry->compdat.load_psw & PSW_MASK_SHORT_ADDR);
>> }
>>
>> static void ipl_scsi(void)
>> diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
>> index 12a0166aae..3946aa3f8d 100644
>> --- a/pc-bios/s390-ccw/bootmap.h
>> +++ b/pc-bios/s390-ccw/bootmap.h
>> @@ -64,11 +64,16 @@ typedef struct BootMapTable {
>> BootMapPointer entry[];
>> } __attribute__ ((packed)) BootMapTable;
>>
>> +typedef union ComponentEntryData {
>> + uint64_t load_psw;
>> + uint64_t load_addr;
>> +} ComponentEntryData;
>> +
>> typedef struct ComponentEntry {
>> ScsiBlockPtr data;
>> uint8_t pad[7];
>> uint8_t component_type;
>> - uint64_t load_address;
>> + ComponentEntryData compdat;
>> } __attribute((packed)) ComponentEntry;
>>
>> typedef struct ComponentHeader {
>>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 1/7] pc-bios: s390x: Fix bootmap.c zipl component entry data handling
2020-07-22 7:30 ` Janosch Frank
@ 2020-07-22 7:33 ` Christian Borntraeger
2020-07-22 8:06 ` Janosch Frank
0 siblings, 1 reply; 33+ messages in thread
From: Christian Borntraeger @ 2020-07-22 7:33 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: thuth, cohuck, david
On 22.07.20 09:30, Janosch Frank wrote:
> On 7/22/20 8:50 AM, Christian Borntraeger wrote:
>>
>>
>> On 15.07.20 11:40, Janosch Frank wrote:
>>> The two main types of zipl component entries are execute and
>>> load/data. The last member of the component entry struct therefore
>>> denotes either a PSW or an address. Let's make this a bit more clear
>>> by introducing a union and cleaning up the code that uses that struct
>>> member.
>>>
>>> The execute type component entries written by zipl contain short PSWs,
>>> not addresses. Let's mask them and only pass the address part to
>>> jump_to_IPL_code(uint64_t address) because it expects an address as
>>> visible by the name of the argument.
>>
>> If zipl actually specifies a PSW, shouldnt we actually USE that PSW including
>> the zipl specified mask?
>
> I expected the current approach to have some kind of meaning behind it,
> if there isn't I'll be the first one to make it more sensible.
I think this was just something to make it work with Linux, especially when Linux
still started in ESA mode.(So we faked a mask that is good enough to boot Linux and
then used the address). But I think the proper solution is to really use the
full PSW and go with that according to the CZAM rules.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/7] pc-bios: s390x: Fix bootmap.c zipl component entry data handling
2020-07-22 7:33 ` Christian Borntraeger
@ 2020-07-22 8:06 ` Janosch Frank
0 siblings, 0 replies; 33+ messages in thread
From: Janosch Frank @ 2020-07-22 8:06 UTC (permalink / raw)
To: Christian Borntraeger, qemu-devel; +Cc: thuth, cohuck, david
[-- Attachment #1.1: Type: text/plain, Size: 1409 bytes --]
On 7/22/20 9:33 AM, Christian Borntraeger wrote:
>
>
> On 22.07.20 09:30, Janosch Frank wrote:
>> On 7/22/20 8:50 AM, Christian Borntraeger wrote:
>>>
>>>
>>> On 15.07.20 11:40, Janosch Frank wrote:
>>>> The two main types of zipl component entries are execute and
>>>> load/data. The last member of the component entry struct therefore
>>>> denotes either a PSW or an address. Let's make this a bit more clear
>>>> by introducing a union and cleaning up the code that uses that struct
>>>> member.
>>>>
>>>> The execute type component entries written by zipl contain short PSWs,
>>>> not addresses. Let's mask them and only pass the address part to
>>>> jump_to_IPL_code(uint64_t address) because it expects an address as
>>>> visible by the name of the argument.
>>>
>>> If zipl actually specifies a PSW, shouldnt we actually USE that PSW including
>>> the zipl specified mask?
>>
>> I expected the current approach to have some kind of meaning behind it,
>> if there isn't I'll be the first one to make it more sensible.
> I think this was just something to make it work with Linux, especially when Linux
> still started in ESA mode.(So we faked a mask that is good enough to boot Linux and
> then used the address). But I think the proper solution is to really use the
> full PSW and go with that according to the CZAM rules.
>
Okay, I'll come up with a proper fix.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 2/7] pc-bios: s390x: Cleanup jump to ipl code
2020-07-15 9:40 [PATCH 0/7] pc-bios: s390x: Cleanup part 2 Janosch Frank
2020-07-15 9:40 ` [PATCH 1/7] pc-bios: s390x: Fix bootmap.c zipl component entry data handling Janosch Frank
@ 2020-07-15 9:40 ` Janosch Frank
2020-07-17 15:13 ` Thomas Huth
2020-07-15 9:40 ` [PATCH 3/7] pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes Janosch Frank
` (4 subsequent siblings)
6 siblings, 1 reply; 33+ messages in thread
From: Janosch Frank @ 2020-07-15 9:40 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, thuth, cohuck, david
jump_to_IPL_code takes a 64 bit address, masks it with the short psw
address mask and later branches to it using a full 64 bit register.
* As the masking is not necessary, let's remove it
* Without the mask we can save the ipl address to a static 64 bit
function ptr as we later branch to it
* Let's also clean up the variable names and remove the now unneeded
ResetInfo
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
pc-bios/s390-ccw/jump2ipl.c | 27 +++++++++++----------------
1 file changed, 11 insertions(+), 16 deletions(-)
diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
index 767012bf0c..aef37cea76 100644
--- a/pc-bios/s390-ccw/jump2ipl.c
+++ b/pc-bios/s390-ccw/jump2ipl.c
@@ -13,20 +13,15 @@
#define KERN_IMAGE_START 0x010000UL
#define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64)
-typedef struct ResetInfo {
- uint64_t ipl_psw;
- uint32_t ipl_continue;
-} ResetInfo;
-
-static ResetInfo save;
+static void (*ipl_continue)(void);
+static uint64_t psw_save;
static void jump_to_IPL_2(void)
{
- ResetInfo *current = 0;
+ uint64_t *psw_current = 0;
- void (*ipl)(void) = (void *) (uint64_t) current->ipl_continue;
- *current = save;
- ipl(); /* should not return */
+ *psw_current = psw_save;
+ ipl_continue(); /* should not return */
}
void jump_to_IPL_code(uint64_t address)
@@ -46,15 +41,15 @@ void jump_to_IPL_code(uint64_t address)
* content of non-BIOS memory after we loaded the guest, so we
* save the original content and restore it in jump_to_IPL_2.
*/
- ResetInfo *current = 0;
+ uint64_t *psw_current = 0;
- save = *current;
+ psw_save = *psw_current;
- current->ipl_psw = (uint64_t) &jump_to_IPL_2;
- current->ipl_psw |= RESET_PSW_MASK;
- current->ipl_continue = address & PSW_MASK_SHORT_ADDR;
+ *psw_current = (uint64_t) &jump_to_IPL_2;
+ *psw_current |= RESET_PSW_MASK;
+ ipl_continue = (void *)address;
- debug_print_int("set IPL addr to", current->ipl_continue);
+ debug_print_int("set IPL addr to", (uint64_t)ipl_continue);
/* Ensure the guest output starts fresh */
sclp_print("\n");
--
2.25.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 2/7] pc-bios: s390x: Cleanup jump to ipl code
2020-07-15 9:40 ` [PATCH 2/7] pc-bios: s390x: Cleanup jump to ipl code Janosch Frank
@ 2020-07-17 15:13 ` Thomas Huth
2020-07-21 13:07 ` Janosch Frank
2020-07-21 13:54 ` Christian Borntraeger
0 siblings, 2 replies; 33+ messages in thread
From: Thomas Huth @ 2020-07-17 15:13 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: borntraeger, cohuck, david
On 15/07/2020 11.40, Janosch Frank wrote:
> jump_to_IPL_code takes a 64 bit address, masks it with the short psw
> address mask and later branches to it using a full 64 bit register.
>
> * As the masking is not necessary, let's remove it
> * Without the mask we can save the ipl address to a static 64 bit
> function ptr as we later branch to it
> * Let's also clean up the variable names and remove the now unneeded
> ResetInfo
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> pc-bios/s390-ccw/jump2ipl.c | 27 +++++++++++----------------
> 1 file changed, 11 insertions(+), 16 deletions(-)
>
> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
> index 767012bf0c..aef37cea76 100644
> --- a/pc-bios/s390-ccw/jump2ipl.c
> +++ b/pc-bios/s390-ccw/jump2ipl.c
> @@ -13,20 +13,15 @@
> #define KERN_IMAGE_START 0x010000UL
> #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64)
>
> -typedef struct ResetInfo {
> - uint64_t ipl_psw;
> - uint32_t ipl_continue;
> -} ResetInfo;
> -
> -static ResetInfo save;
> +static void (*ipl_continue)(void);
> +static uint64_t psw_save;
Christian, do you remember whether there was a reason that we saved the
"ipl_continue" in the low-core in the past?
The changes here look ok to me, but I still wonder why it has been more
"complicated" before...?
Acked-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 2/7] pc-bios: s390x: Cleanup jump to ipl code
2020-07-17 15:13 ` Thomas Huth
@ 2020-07-21 13:07 ` Janosch Frank
2020-07-21 13:54 ` Christian Borntraeger
1 sibling, 0 replies; 33+ messages in thread
From: Janosch Frank @ 2020-07-21 13:07 UTC (permalink / raw)
To: Thomas Huth, qemu-devel; +Cc: borntraeger, cohuck, david
[-- Attachment #1.1: Type: text/plain, Size: 1644 bytes --]
On 7/17/20 5:13 PM, Thomas Huth wrote:
> On 15/07/2020 11.40, Janosch Frank wrote:
>> jump_to_IPL_code takes a 64 bit address, masks it with the short psw
>> address mask and later branches to it using a full 64 bit register.
>>
>> * As the masking is not necessary, let's remove it
>> * Without the mask we can save the ipl address to a static 64 bit
>> function ptr as we later branch to it
>> * Let's also clean up the variable names and remove the now unneeded
>> ResetInfo
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>> pc-bios/s390-ccw/jump2ipl.c | 27 +++++++++++----------------
>> 1 file changed, 11 insertions(+), 16 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
>> index 767012bf0c..aef37cea76 100644
>> --- a/pc-bios/s390-ccw/jump2ipl.c
>> +++ b/pc-bios/s390-ccw/jump2ipl.c
>> @@ -13,20 +13,15 @@
>> #define KERN_IMAGE_START 0x010000UL
>> #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64)
>>
>> -typedef struct ResetInfo {
>> - uint64_t ipl_psw;
>> - uint32_t ipl_continue;
>> -} ResetInfo;
>> -
>> -static ResetInfo save;
>> +static void (*ipl_continue)(void);
>> +static uint64_t psw_save;
>
> Christian, do you remember whether there was a reason that we saved the
> "ipl_continue" in the low-core in the past?
>
> The changes here look ok to me, but I still wonder why it has been more
> "complicated" before...?
Unfortunately looking at 962982329029acb6651f81b47cb401e593bb62df where
this was introduced also doesn't clear that up.
>
> Acked-by: Thomas Huth <thuth@redhat.com>
Thanks!
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 2/7] pc-bios: s390x: Cleanup jump to ipl code
2020-07-17 15:13 ` Thomas Huth
2020-07-21 13:07 ` Janosch Frank
@ 2020-07-21 13:54 ` Christian Borntraeger
1 sibling, 0 replies; 33+ messages in thread
From: Christian Borntraeger @ 2020-07-21 13:54 UTC (permalink / raw)
To: Thomas Huth, Janosch Frank, qemu-devel; +Cc: cohuck, david
On 17.07.20 17:13, Thomas Huth wrote:
> On 15/07/2020 11.40, Janosch Frank wrote:
>> jump_to_IPL_code takes a 64 bit address, masks it with the short psw
>> address mask and later branches to it using a full 64 bit register.
>>
>> * As the masking is not necessary, let's remove it
>> * Without the mask we can save the ipl address to a static 64 bit
>> function ptr as we later branch to it
>> * Let's also clean up the variable names and remove the now unneeded
>> ResetInfo
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>> pc-bios/s390-ccw/jump2ipl.c | 27 +++++++++++----------------
>> 1 file changed, 11 insertions(+), 16 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
>> index 767012bf0c..aef37cea76 100644
>> --- a/pc-bios/s390-ccw/jump2ipl.c
>> +++ b/pc-bios/s390-ccw/jump2ipl.c
>> @@ -13,20 +13,15 @@
>> #define KERN_IMAGE_START 0x010000UL
>> #define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_64)
>>
>> -typedef struct ResetInfo {
>> - uint64_t ipl_psw;
>> - uint32_t ipl_continue;
>> -} ResetInfo;
>> -
>> -static ResetInfo save;
>> +static void (*ipl_continue)(void);
>> +static uint64_t psw_save;
>
> Christian, do you remember whether there was a reason that we saved the
> "ipl_continue" in the low-core in the past?
>
> The changes here look ok to me, but I still wonder why it has been more
> "complicated" before...?
This construct was made to restore the memory outside of the loader to the
original content. The new code should also work I guess.
>
> Acked-by: Thomas Huth <thuth@redhat.com>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 3/7] pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes
2020-07-15 9:40 [PATCH 0/7] pc-bios: s390x: Cleanup part 2 Janosch Frank
2020-07-15 9:40 ` [PATCH 1/7] pc-bios: s390x: Fix bootmap.c zipl component entry data handling Janosch Frank
2020-07-15 9:40 ` [PATCH 2/7] pc-bios: s390x: Cleanup jump to ipl code Janosch Frank
@ 2020-07-15 9:40 ` Janosch Frank
2020-07-20 11:45 ` Thomas Huth
2020-07-15 9:40 ` [PATCH 4/7] pc-bios: s390x: Rework data initialization Janosch Frank
` (3 subsequent siblings)
6 siblings, 1 reply; 33+ messages in thread
From: Janosch Frank @ 2020-07-15 9:40 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, thuth, cohuck, david
jump_to_low_kernel() and the functions that it calls will already or
64 bit addressing into the reset psw mask when executing
jump_to_IPL_2() after the diag308 subcode 1.
The kernel proper is then branched to rather than doing a full PSW
change.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
pc-bios/s390-ccw/dasd-ipl.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/pc-bios/s390-ccw/dasd-ipl.c b/pc-bios/s390-ccw/dasd-ipl.c
index 0fc879bb8e..e8f2846740 100644
--- a/pc-bios/s390-ccw/dasd-ipl.c
+++ b/pc-bios/s390-ccw/dasd-ipl.c
@@ -206,7 +206,6 @@ static void run_ipl2(SubChannelId schid, uint16_t cutype, uint32_t addr)
*/
void dasd_ipl(SubChannelId schid, uint16_t cutype)
{
- PSWLegacy *pswl = (PSWLegacy *) 0x00;
uint32_t ipl2_addr;
/* Construct Read IPL CCW and run it to read IPL1 from boot disk */
@@ -229,7 +228,5 @@ 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; /* ... */
jump_to_low_kernel();
}
--
2.25.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 3/7] pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes
2020-07-15 9:40 ` [PATCH 3/7] pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes Janosch Frank
@ 2020-07-20 11:45 ` Thomas Huth
2020-07-20 12:16 ` Janosch Frank
0 siblings, 1 reply; 33+ messages in thread
From: Thomas Huth @ 2020-07-20 11:45 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: borntraeger, cohuck, david
On 15/07/2020 11.40, Janosch Frank wrote:
> jump_to_low_kernel() and the functions that it calls will already or
> 64 bit addressing into the reset psw mask when executing
> jump_to_IPL_2() after the diag308 subcode 1.
But there is also a check in jump_to_low_kernel that could be affected
by your change:
if (*((uint64_t *)0) & RESET_PSW_MASK) {
jump_to_IPL_code((*((uint64_t *)0)) & PSW_MASK_SHORT_ADDR);
}
... but I assume that there should not be any kernels out there in the
wild which do not have these bits set, so I think your modifications
here should be ok.
Acked-by: Thomas Huth <thuth@redhat.com>
> The kernel proper is then branched to rather than doing a full PSW
> change.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> pc-bios/s390-ccw/dasd-ipl.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/pc-bios/s390-ccw/dasd-ipl.c b/pc-bios/s390-ccw/dasd-ipl.c
> index 0fc879bb8e..e8f2846740 100644
> --- a/pc-bios/s390-ccw/dasd-ipl.c
> +++ b/pc-bios/s390-ccw/dasd-ipl.c
> @@ -206,7 +206,6 @@ static void run_ipl2(SubChannelId schid, uint16_t cutype, uint32_t addr)
> */
> void dasd_ipl(SubChannelId schid, uint16_t cutype)
> {
> - PSWLegacy *pswl = (PSWLegacy *) 0x00;
> uint32_t ipl2_addr;
>
> /* Construct Read IPL CCW and run it to read IPL1 from boot disk */
> @@ -229,7 +228,5 @@ 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; /* ... */
> jump_to_low_kernel();
> }
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 3/7] pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes
2020-07-20 11:45 ` Thomas Huth
@ 2020-07-20 12:16 ` Janosch Frank
2020-07-21 7:10 ` Thomas Huth
0 siblings, 1 reply; 33+ messages in thread
From: Janosch Frank @ 2020-07-20 12:16 UTC (permalink / raw)
To: Thomas Huth, qemu-devel; +Cc: borntraeger, cohuck, david
[-- Attachment #1.1: Type: text/plain, Size: 1926 bytes --]
On 7/20/20 1:45 PM, Thomas Huth wrote:
> On 15/07/2020 11.40, Janosch Frank wrote:
>> jump_to_low_kernel() and the functions that it calls will already or
>> 64 bit addressing into the reset psw mask when executing
>> jump_to_IPL_2() after the diag308 subcode 1.
>
> But there is also a check in jump_to_low_kernel that could be affected
> by your change:
>
> if (*((uint64_t *)0) & RESET_PSW_MASK) {
> jump_to_IPL_code((*((uint64_t *)0)) & PSW_MASK_SHORT_ADDR);
> }
>
> ... but I assume that there should not be any kernels out there in the
> wild which do not have these bits set, so I think your modifications
> here should be ok.
The mask needs to have the short bit indication set so it needs to be !=
0 anyway, no?
>
> Acked-by: Thomas Huth <thuth@redhat.com>
>
>
>> The kernel proper is then branched to rather than doing a full PSW
>> change.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>> pc-bios/s390-ccw/dasd-ipl.c | 3 ---
>> 1 file changed, 3 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/dasd-ipl.c b/pc-bios/s390-ccw/dasd-ipl.c
>> index 0fc879bb8e..e8f2846740 100644
>> --- a/pc-bios/s390-ccw/dasd-ipl.c
>> +++ b/pc-bios/s390-ccw/dasd-ipl.c
>> @@ -206,7 +206,6 @@ static void run_ipl2(SubChannelId schid, uint16_t cutype, uint32_t addr)
>> */
>> void dasd_ipl(SubChannelId schid, uint16_t cutype)
>> {
>> - PSWLegacy *pswl = (PSWLegacy *) 0x00;
>> uint32_t ipl2_addr;
>>
>> /* Construct Read IPL CCW and run it to read IPL1 from boot disk */
>> @@ -229,7 +228,5 @@ 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; /* ... */
>> jump_to_low_kernel();
>> }
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 3/7] pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes
2020-07-20 12:16 ` Janosch Frank
@ 2020-07-21 7:10 ` Thomas Huth
0 siblings, 0 replies; 33+ messages in thread
From: Thomas Huth @ 2020-07-21 7:10 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: borntraeger, cohuck, david
On 20/07/2020 14.16, Janosch Frank wrote:
> On 7/20/20 1:45 PM, Thomas Huth wrote:
>> On 15/07/2020 11.40, Janosch Frank wrote:
>>> jump_to_low_kernel() and the functions that it calls will already or
>>> 64 bit addressing into the reset psw mask when executing
>>> jump_to_IPL_2() after the diag308 subcode 1.
>>
>> But there is also a check in jump_to_low_kernel that could be affected
>> by your change:
>>
>> if (*((uint64_t *)0) & RESET_PSW_MASK) {
>> jump_to_IPL_code((*((uint64_t *)0)) & PSW_MASK_SHORT_ADDR);
>> }
>>
>> ... but I assume that there should not be any kernels out there in the
>> wild which do not have these bits set, so I think your modifications
>> here should be ok.
>
> The mask needs to have the short bit indication set so it needs to be !=
> 0 anyway, no?
That's true. If there were still broken images out there, they should
have stopped working since commit 104130cb7c106378da anyway.
Thomas
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 4/7] pc-bios: s390x: Rework data initialization
2020-07-15 9:40 [PATCH 0/7] pc-bios: s390x: Cleanup part 2 Janosch Frank
` (2 preceding siblings ...)
2020-07-15 9:40 ` [PATCH 3/7] pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes Janosch Frank
@ 2020-07-15 9:40 ` Janosch Frank
2020-07-20 11:56 ` Thomas Huth
2020-07-15 9:40 ` [PATCH 5/7] pc-bios: s390x: Replace lowcore offsets with pointers in dasd-ipl.c Janosch Frank
` (2 subsequent siblings)
6 siblings, 1 reply; 33+ messages in thread
From: Janosch Frank @ 2020-07-15 9:40 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, thuth, cohuck, david
Sometimes a memset is nicer to read than multiple struct->data = 0;
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
---
pc-bios/s390-ccw/dasd-ipl.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/pc-bios/s390-ccw/dasd-ipl.c b/pc-bios/s390-ccw/dasd-ipl.c
index e8f2846740..0543334ed4 100644
--- a/pc-bios/s390-ccw/dasd-ipl.c
+++ b/pc-bios/s390-ccw/dasd-ipl.c
@@ -167,16 +167,13 @@ static void ipl1_fixup(void)
ccwSeek->cda = ptr2u32(seekData);
ccwSeek->chain = 1;
ccwSeek->count = sizeof(*seekData);
- seekData->reserved = 0x00;
- seekData->cyl = 0x00;
- seekData->head = 0x00;
+ memset(seekData, 0, sizeof(*seekData));
ccwSearchID->cmd_code = CCW_CMD_DASD_SEARCH_ID_EQ;
ccwSearchID->cda = ptr2u32(searchData);
ccwSearchID->chain = 1;
ccwSearchID->count = sizeof(*searchData);
- searchData->cyl = 0;
- searchData->head = 0;
+ memset(searchData, 0, sizeof(*searchData));
searchData->record = 2;
/* Go back to Search CCW if correct record not yet found */
--
2.25.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 4/7] pc-bios: s390x: Rework data initialization
2020-07-15 9:40 ` [PATCH 4/7] pc-bios: s390x: Rework data initialization Janosch Frank
@ 2020-07-20 11:56 ` Thomas Huth
2020-07-20 12:10 ` Janosch Frank
0 siblings, 1 reply; 33+ messages in thread
From: Thomas Huth @ 2020-07-20 11:56 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: borntraeger, cohuck, david
On 15/07/2020 11.40, Janosch Frank wrote:
> Sometimes a memset is nicer to read than multiple struct->data = 0;
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
> pc-bios/s390-ccw/dasd-ipl.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/pc-bios/s390-ccw/dasd-ipl.c b/pc-bios/s390-ccw/dasd-ipl.c
> index e8f2846740..0543334ed4 100644
> --- a/pc-bios/s390-ccw/dasd-ipl.c
> +++ b/pc-bios/s390-ccw/dasd-ipl.c
> @@ -167,16 +167,13 @@ static void ipl1_fixup(void)
> ccwSeek->cda = ptr2u32(seekData);
> ccwSeek->chain = 1;
> ccwSeek->count = sizeof(*seekData);
> - seekData->reserved = 0x00;
> - seekData->cyl = 0x00;
> - seekData->head = 0x00;
> + memset(seekData, 0, sizeof(*seekData));
Sounds ok for me if the whole struct gets cleared (though I wonder
whether this is really worth the effort)...
> ccwSearchID->cmd_code = CCW_CMD_DASD_SEARCH_ID_EQ;
> ccwSearchID->cda = ptr2u32(searchData);
> ccwSearchID->chain = 1;
> ccwSearchID->count = sizeof(*searchData);
> - searchData->cyl = 0;
> - searchData->head = 0;
> + memset(searchData, 0, sizeof(*searchData));
> searchData->record = 2;
... but that looks rather worse to me, and the generated code will
likely also be slightly worse (since ->record is cleared first and then
set to 2 again).
Maybe rather drop this patch?
Thomas
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 4/7] pc-bios: s390x: Rework data initialization
2020-07-20 11:56 ` Thomas Huth
@ 2020-07-20 12:10 ` Janosch Frank
0 siblings, 0 replies; 33+ messages in thread
From: Janosch Frank @ 2020-07-20 12:10 UTC (permalink / raw)
To: Thomas Huth, qemu-devel; +Cc: borntraeger, cohuck, david
[-- Attachment #1.1: Type: text/plain, Size: 1646 bytes --]
On 7/20/20 1:56 PM, Thomas Huth wrote:
> On 15/07/2020 11.40, Janosch Frank wrote:
>> Sometimes a memset is nicer to read than multiple struct->data = 0;
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>> pc-bios/s390-ccw/dasd-ipl.c | 7 ++-----
>> 1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/dasd-ipl.c b/pc-bios/s390-ccw/dasd-ipl.c
>> index e8f2846740..0543334ed4 100644
>> --- a/pc-bios/s390-ccw/dasd-ipl.c
>> +++ b/pc-bios/s390-ccw/dasd-ipl.c
>> @@ -167,16 +167,13 @@ static void ipl1_fixup(void)
>> ccwSeek->cda = ptr2u32(seekData);
>> ccwSeek->chain = 1;
>> ccwSeek->count = sizeof(*seekData);
>> - seekData->reserved = 0x00;
>> - seekData->cyl = 0x00;
>> - seekData->head = 0x00;
>> + memset(seekData, 0, sizeof(*seekData));
>
> Sounds ok for me if the whole struct gets cleared (though I wonder
> whether this is really worth the effort)...
>
>> ccwSearchID->cmd_code = CCW_CMD_DASD_SEARCH_ID_EQ;
>> ccwSearchID->cda = ptr2u32(searchData);
>> ccwSearchID->chain = 1;
>> ccwSearchID->count = sizeof(*searchData);
>> - searchData->cyl = 0;
>> - searchData->head = 0;
>> + memset(searchData, 0, sizeof(*searchData));
>> searchData->record = 2;
>
> ... but that looks rather worse to me, and the generated code will
> likely also be slightly worse (since ->record is cleared first and then
> set to 2 again).
>
> Maybe rather drop this patch?
Sure, I'm definitely not hard set on this patch :)
>
> Thomas
>
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 5/7] pc-bios: s390x: Replace lowcore offsets with pointers in dasd-ipl.c
2020-07-15 9:40 [PATCH 0/7] pc-bios: s390x: Cleanup part 2 Janosch Frank
` (3 preceding siblings ...)
2020-07-15 9:40 ` [PATCH 4/7] pc-bios: s390x: Rework data initialization Janosch Frank
@ 2020-07-15 9:40 ` Janosch Frank
2020-07-21 7:00 ` Thomas Huth
2020-07-15 9:40 ` [PATCH 6/7] pc-bios: s390x: Use PSW constants in start.S Janosch Frank
2020-07-15 9:40 ` [PATCH 7/7] pc-bios: s390x: Setup io and ext new psws only once Janosch Frank
6 siblings, 1 reply; 33+ messages in thread
From: Janosch Frank @ 2020-07-15 9:40 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, thuth, cohuck, david
Let's replace some more constant offsets with references into the
lowcore for better readability.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
pc-bios/s390-ccw/dasd-ipl.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/pc-bios/s390-ccw/dasd-ipl.c b/pc-bios/s390-ccw/dasd-ipl.c
index 0543334ed4..9ab9a0fa12 100644
--- a/pc-bios/s390-ccw/dasd-ipl.c
+++ b/pc-bios/s390-ccw/dasd-ipl.c
@@ -120,8 +120,8 @@ static void run_readipl(SubChannelId schid, uint16_t cutype)
*/
static void check_ipl1(void)
{
- Ccw0 *ccwread = (Ccw0 *)0x08;
- Ccw0 *ccwtic = (Ccw0 *)0x10;
+ Ccw0 *ccwread = (Ccw0 *) &lowcore->ccw1;
+ Ccw0 *ccwtic = (Ccw0 *) &lowcore->ccw2;
if (ccwread->cmd_code != CCW_CMD_DASD_READ ||
ccwtic->cmd_code != CCW_CMD_TIC) {
@@ -143,15 +143,15 @@ static void check_ipl2(uint32_t ipl2_addr)
static uint32_t read_ipl2_addr(void)
{
- Ccw0 *ccwtic = (Ccw0 *)0x10;
+ Ccw0 *ccwtic = (Ccw0 *)&lowcore->ccw2;
return ccwtic->cda;
}
static void ipl1_fixup(void)
{
- Ccw0 *ccwSeek = (Ccw0 *) 0x08;
- Ccw0 *ccwSearchID = (Ccw0 *) 0x10;
+ Ccw0 *ccwSeek = (Ccw0 *) &lowcore->ccw1;
+ Ccw0 *ccwSearchID = (Ccw0 *) &lowcore->ccw2;
Ccw0 *ccwSearchTic = (Ccw0 *) 0x18;
Ccw0 *ccwRead = (Ccw0 *) 0x20;
CcwSeekData *seekData = (CcwSeekData *) 0x30;
--
2.25.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 5/7] pc-bios: s390x: Replace lowcore offsets with pointers in dasd-ipl.c
2020-07-15 9:40 ` [PATCH 5/7] pc-bios: s390x: Replace lowcore offsets with pointers in dasd-ipl.c Janosch Frank
@ 2020-07-21 7:00 ` Thomas Huth
0 siblings, 0 replies; 33+ messages in thread
From: Thomas Huth @ 2020-07-21 7:00 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: borntraeger, cohuck, david
On 15/07/2020 11.40, Janosch Frank wrote:
> Let's replace some more constant offsets with references into the
> lowcore for better readability.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> pc-bios/s390-ccw/dasd-ipl.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/pc-bios/s390-ccw/dasd-ipl.c b/pc-bios/s390-ccw/dasd-ipl.c
> index 0543334ed4..9ab9a0fa12 100644
> --- a/pc-bios/s390-ccw/dasd-ipl.c
> +++ b/pc-bios/s390-ccw/dasd-ipl.c
> @@ -120,8 +120,8 @@ static void run_readipl(SubChannelId schid, uint16_t cutype)
> */
> static void check_ipl1(void)
> {
> - Ccw0 *ccwread = (Ccw0 *)0x08;
> - Ccw0 *ccwtic = (Ccw0 *)0x10;
> + Ccw0 *ccwread = (Ccw0 *) &lowcore->ccw1;
> + Ccw0 *ccwtic = (Ccw0 *) &lowcore->ccw2;
>
> if (ccwread->cmd_code != CCW_CMD_DASD_READ ||
> ccwtic->cmd_code != CCW_CMD_TIC) {
> @@ -143,15 +143,15 @@ static void check_ipl2(uint32_t ipl2_addr)
>
> static uint32_t read_ipl2_addr(void)
> {
> - Ccw0 *ccwtic = (Ccw0 *)0x10;
> + Ccw0 *ccwtic = (Ccw0 *)&lowcore->ccw2;
>
> return ccwtic->cda;
> }
>
> static void ipl1_fixup(void)
> {
> - Ccw0 *ccwSeek = (Ccw0 *) 0x08;
> - Ccw0 *ccwSearchID = (Ccw0 *) 0x10;
> + Ccw0 *ccwSeek = (Ccw0 *) &lowcore->ccw1;
> + Ccw0 *ccwSearchID = (Ccw0 *) &lowcore->ccw2;
> Ccw0 *ccwSearchTic = (Ccw0 *) 0x18;
> Ccw0 *ccwRead = (Ccw0 *) 0x20;
> CcwSeekData *seekData = (CcwSeekData *) 0x30;
>
Reviewed-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 6/7] pc-bios: s390x: Use PSW constants in start.S
2020-07-15 9:40 [PATCH 0/7] pc-bios: s390x: Cleanup part 2 Janosch Frank
` (4 preceding siblings ...)
2020-07-15 9:40 ` [PATCH 5/7] pc-bios: s390x: Replace lowcore offsets with pointers in dasd-ipl.c Janosch Frank
@ 2020-07-15 9:40 ` Janosch Frank
2020-07-21 7:05 ` Thomas Huth
2020-07-15 9:40 ` [PATCH 7/7] pc-bios: s390x: Setup io and ext new psws only once Janosch Frank
6 siblings, 1 reply; 33+ messages in thread
From: Janosch Frank @ 2020-07-15 9:40 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, thuth, cohuck, david
Let's decrease the number of magic numbers.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
---
pc-bios/s390-ccw/s390-arch.h | 25 +++++++++++++++----------
pc-bios/s390-ccw/start.S | 9 +++++----
2 files changed, 20 insertions(+), 14 deletions(-)
diff --git a/pc-bios/s390-ccw/s390-arch.h b/pc-bios/s390-ccw/s390-arch.h
index 6da44d4436..d450c096d0 100644
--- a/pc-bios/s390-ccw/s390-arch.h
+++ b/pc-bios/s390-ccw/s390-arch.h
@@ -11,6 +11,20 @@
#ifndef S390_ARCH_H
#define S390_ARCH_H
+/* s390 psw bit masks */
+#define PSW_MASK_EXT 0x0100000000000000UL
+#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)
+#define PSW_MASK_DWAIT (PSW_MASK_64 | PSW_MASK_WAIT)
+#define PSW_MASK_EWAIT (PSW_MASK_DWAIT | PSW_MASK_IOINT | PSW_MASK_EXT)
+
+#ifndef __ASSEMBLER__
+
typedef struct PSW {
uint64_t mask;
uint64_t addr;
@@ -24,15 +38,6 @@ typedef struct PSWLegacy {
} __attribute__ ((aligned(8))) PSWLegacy;
_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 */
typedef struct LowCore {
/* prefix area: defined by architecture */
@@ -107,5 +112,5 @@ static inline uint32_t store_prefix(void)
asm volatile("stpx %0" : "=m" (address));
return address;
}
-
+#endif /* !__ASSEMBLER__ */
#endif
diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
index ce519300a1..01c4c21b26 100644
--- a/pc-bios/s390-ccw/start.S
+++ b/pc-bios/s390-ccw/start.S
@@ -9,6 +9,7 @@
* your option) any later version. See the COPYING file in the top-level
* directory.
*/
+#include "s390-arch.h"
.globl _start
_start:
@@ -108,10 +109,10 @@ io_new_code:
.align 8
disabled_wait_psw:
- .quad 0x0002000180000000,0x0000000000000000
+ .quad PSW_MASK_DWAIT, 0x0000000000000000
enabled_wait_psw:
- .quad 0x0302000180000000,0x0000000000000000
+ .quad PSW_MASK_EWAIT, 0x0000000000000000
external_new_mask:
- .quad 0x0000000180000000
+ .quad PSW_MASK_64
io_new_mask:
- .quad 0x0000000180000000
+ .quad PSW_MASK_64
--
2.25.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 6/7] pc-bios: s390x: Use PSW constants in start.S
2020-07-15 9:40 ` [PATCH 6/7] pc-bios: s390x: Use PSW constants in start.S Janosch Frank
@ 2020-07-21 7:05 ` Thomas Huth
2020-07-22 6:47 ` Christian Borntraeger
0 siblings, 1 reply; 33+ messages in thread
From: Thomas Huth @ 2020-07-21 7:05 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: borntraeger, cohuck, david
On 15/07/2020 11.40, Janosch Frank wrote:
> Let's decrease the number of magic numbers.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
> pc-bios/s390-ccw/s390-arch.h | 25 +++++++++++++++----------
> pc-bios/s390-ccw/start.S | 9 +++++----
> 2 files changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/pc-bios/s390-ccw/s390-arch.h b/pc-bios/s390-ccw/s390-arch.h
> index 6da44d4436..d450c096d0 100644
> --- a/pc-bios/s390-ccw/s390-arch.h
> +++ b/pc-bios/s390-ccw/s390-arch.h
> @@ -11,6 +11,20 @@
> #ifndef S390_ARCH_H
> #define S390_ARCH_H
>
> +/* s390 psw bit masks */
> +#define PSW_MASK_EXT 0x0100000000000000UL
> +#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)
> +#define PSW_MASK_DWAIT (PSW_MASK_64 | PSW_MASK_WAIT)
> +#define PSW_MASK_EWAIT (PSW_MASK_DWAIT | PSW_MASK_IOINT | PSW_MASK_EXT)
> +
> +#ifndef __ASSEMBLER__
> +
> typedef struct PSW {
> uint64_t mask;
> uint64_t addr;
> @@ -24,15 +38,6 @@ typedef struct PSWLegacy {
> } __attribute__ ((aligned(8))) PSWLegacy;
> _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 */
> typedef struct LowCore {
> /* prefix area: defined by architecture */
> @@ -107,5 +112,5 @@ static inline uint32_t store_prefix(void)
> asm volatile("stpx %0" : "=m" (address));
> return address;
> }
> -
> +#endif /* !__ASSEMBLER__ */
> #endif
> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
> index ce519300a1..01c4c21b26 100644
> --- a/pc-bios/s390-ccw/start.S
> +++ b/pc-bios/s390-ccw/start.S
> @@ -9,6 +9,7 @@
> * your option) any later version. See the COPYING file in the top-level
> * directory.
> */
> +#include "s390-arch.h"
>
> .globl _start
> _start:
> @@ -108,10 +109,10 @@ io_new_code:
>
> .align 8
> disabled_wait_psw:
> - .quad 0x0002000180000000,0x0000000000000000
> + .quad PSW_MASK_DWAIT, 0x0000000000000000
> enabled_wait_psw:
> - .quad 0x0302000180000000,0x0000000000000000
> + .quad PSW_MASK_EWAIT, 0x0000000000000000
> external_new_mask:
> - .quad 0x0000000180000000
> + .quad PSW_MASK_64
> io_new_mask:
> - .quad 0x0000000180000000
> + .quad PSW_MASK_64
>
This fails to compile with older versions of binutils (e.g. the ones in
RHEL7):
pc-bios/s390-ccw/start.S: Assembler messages:
pc-bios/s390-ccw/start.S:108: Error: found 'L', expected: ')'
pc-bios/s390-ccw/start.S:108: Error: found 'L', expected: ')'
pc-bios/s390-ccw/start.S:108: Error: junk at end of line, first
unrecognized character is `L'
pc-bios/s390-ccw/start.S:110: Error: found 'L', expected: ')'
pc-bios/s390-ccw/start.S:110: Error: found 'L', expected: ')'
pc-bios/s390-ccw/start.S:110: Error: found 'L', expected: ')'
pc-bios/s390-ccw/start.S:110: Error: junk at end of line, first
unrecognized character is `L'
pc-bios/s390-ccw/start.S:112: Error: found 'L', expected: ')'
pc-bios/s390-ccw/start.S:112: Error: junk at end of line, first
unrecognized character is `L'
pc-bios/s390-ccw/start.S:114: Error: found 'L', expected: ')'
pc-bios/s390-ccw/start.S:114: Error: junk at end of line, first
unrecognized character is `L'
You either need some macro-magic for this, or simply drop the patch.
Thomas
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 6/7] pc-bios: s390x: Use PSW constants in start.S
2020-07-21 7:05 ` Thomas Huth
@ 2020-07-22 6:47 ` Christian Borntraeger
0 siblings, 0 replies; 33+ messages in thread
From: Christian Borntraeger @ 2020-07-22 6:47 UTC (permalink / raw)
To: Thomas Huth, Janosch Frank, qemu-devel; +Cc: cohuck, david
On 21.07.20 09:05, Thomas Huth wrote:
> On 15/07/2020 11.40, Janosch Frank wrote:
[..]
>> #ifndef S390_ARCH_H
>> #define S390_ARCH_H
>>
>> +/* s390 psw bit masks */
>> +#define PSW_MASK_EXT 0x0100000000000000UL
>> +#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)
>> +#define PSW_MASK_DWAIT (PSW_MASK_64 | PSW_MASK_WAIT)
>> +#define PSW_MASK_EWAIT (PSW_MASK_DWAIT | PSW_MASK_IOINT | PSW_MASK_EXT)
>> +
>> +#ifndef __ASSEMBLER__
>> +
>> typedef struct PSW {
>> uint64_t mask;
>> uint64_t addr;
>> @@ -24,15 +38,6 @@ typedef struct PSWLegacy {
>> } __attribute__ ((aligned(8))) PSWLegacy;
>> _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 */
>> typedef struct LowCore {
>> /* prefix area: defined by architecture */
>> @@ -107,5 +112,5 @@ static inline uint32_t store_prefix(void)
>> asm volatile("stpx %0" : "=m" (address));
>> return address;
>> }
>> -
>> +#endif /* !__ASSEMBLER__ */
>> #endif
>> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
>> index ce519300a1..01c4c21b26 100644
>> --- a/pc-bios/s390-ccw/start.S
>> +++ b/pc-bios/s390-ccw/start.S
>> @@ -9,6 +9,7 @@
>> * your option) any later version. See the COPYING file in the top-level
>> * directory.
>> */
>> +#include "s390-arch.h"
>>
>> .globl _start
>> _start:
>> @@ -108,10 +109,10 @@ io_new_code:
>>
>> .align 8
>> disabled_wait_psw:
>> - .quad 0x0002000180000000,0x0000000000000000
>> + .quad PSW_MASK_DWAIT, 0x0000000000000000
>> enabled_wait_psw:
>> - .quad 0x0302000180000000,0x0000000000000000
>> + .quad PSW_MASK_EWAIT, 0x0000000000000000
>> external_new_mask:
>> - .quad 0x0000000180000000
>> + .quad PSW_MASK_64
I find the old numbers EASIER to parse (as I know how a PSW looks like) than a
macro that I first have to look up.
>> io_new_mask:
>> - .quad 0x0000000180000000
>> + .quad PSW_MASK_64
>>
>
> This fails to compile with older versions of binutils (e.g. the ones in
> RHEL7):
>
> pc-bios/s390-ccw/start.S: Assembler messages:
> pc-bios/s390-ccw/start.S:108: Error: found 'L', expected: ')'
> pc-bios/s390-ccw/start.S:108: Error: found 'L', expected: ')'
> pc-bios/s390-ccw/start.S:108: Error: junk at end of line, first
> unrecognized character is `L'
> pc-bios/s390-ccw/start.S:110: Error: found 'L', expected: ')'
> pc-bios/s390-ccw/start.S:110: Error: found 'L', expected: ')'
> pc-bios/s390-ccw/start.S:110: Error: found 'L', expected: ')'
> pc-bios/s390-ccw/start.S:110: Error: junk at end of line, first
> unrecognized character is `L'
> pc-bios/s390-ccw/start.S:112: Error: found 'L', expected: ')'
> pc-bios/s390-ccw/start.S:112: Error: junk at end of line, first
> unrecognized character is `L'
> pc-bios/s390-ccw/start.S:114: Error: found 'L', expected: ')'
> pc-bios/s390-ccw/start.S:114: Error: junk at end of line, first
> unrecognized character is `L'
>
> You either need some macro-magic for this, or simply drop the patch.
So I suggest to drop this patch.
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 7/7] pc-bios: s390x: Setup io and ext new psws only once
2020-07-15 9:40 [PATCH 0/7] pc-bios: s390x: Cleanup part 2 Janosch Frank
` (5 preceding siblings ...)
2020-07-15 9:40 ` [PATCH 6/7] pc-bios: s390x: Use PSW constants in start.S Janosch Frank
@ 2020-07-15 9:40 ` Janosch Frank
2020-07-15 13:13 ` Janosch Frank
6 siblings, 1 reply; 33+ messages in thread
From: Janosch Frank @ 2020-07-15 9:40 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, thuth, cohuck, david
Absolutely no need to set them up every time before we enable our
interrupt masks.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
pc-bios/s390-ccw/start.S | 30 +++++++++++++-----------------
1 file changed, 13 insertions(+), 17 deletions(-)
diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
index 01c4c21b26..0059a15d21 100644
--- a/pc-bios/s390-ccw/start.S
+++ b/pc-bios/s390-ccw/start.S
@@ -34,6 +34,12 @@ loop:
remainder:
larl %r2,memsetxc
ex %r3,0(%r2)
+ /* Store io new PSW */
+ larl %r1,io_new_psw
+ mvc 0x1f0(16),0(%r1)
+ /* Store ext new PSW */
+ larl %r1,external_new_psw
+ mvc 0x1b0(16),0(%r1)
done:
j main /* And call C */
@@ -64,11 +70,6 @@ consume_sclp_int:
stctg %c0,%c0,0(%r15)
oi 6(%r15),0x2
lctlg %c0,%c0,0(%r15)
- /* prepare external call handler */
- larl %r1, external_new_code
- stg %r1, 0x1b8
- larl %r1, external_new_mask
- mvc 0x1b0(8),0(%r1)
/* load enabled wait PSW */
larl %r1, enabled_wait_psw
lpswe 0(%r1)
@@ -81,14 +82,9 @@ consume_sclp_int:
.globl consume_io_int
consume_io_int:
/* enable I/O interrupts in cr6 */
- stctg %c6,%c6,0(%r15)
- oi 4(%r15), 0xff
- lctlg %c6,%c6,0(%r15)
- /* prepare i/o call handler */
- larl %r1, io_new_code
- stg %r1, 0x1f8
- larl %r1, io_new_mask
- mvc 0x1f0(8),0(%r1)
+ stctg %c6, %c6, 0(%r15)
+ oi 4(%r15), 0xff
+ lctlg %c6, %c6, 0(%r15)
/* load enabled wait PSW */
larl %r1, enabled_wait_psw
lpswe 0(%r1)
@@ -112,7 +108,7 @@ disabled_wait_psw:
.quad PSW_MASK_DWAIT, 0x0000000000000000
enabled_wait_psw:
.quad PSW_MASK_EWAIT, 0x0000000000000000
-external_new_mask:
- .quad PSW_MASK_64
-io_new_mask:
- .quad PSW_MASK_64
+external_new_psw:
+ .quad PSW_MASK_64, external_new_code
+io_new_psw:
+ .quad PSW_MASK_64, io_new_code
--
2.25.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 7/7] pc-bios: s390x: Setup io and ext new psws only once
2020-07-15 9:40 ` [PATCH 7/7] pc-bios: s390x: Setup io and ext new psws only once Janosch Frank
@ 2020-07-15 13:13 ` Janosch Frank
2020-07-15 13:16 ` Christian Borntraeger
0 siblings, 1 reply; 33+ messages in thread
From: Janosch Frank @ 2020-07-15 13:13 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, thuth, cohuck, david
[-- Attachment #1.1: Type: text/plain, Size: 2739 bytes --]
On 7/15/20 11:40 AM, Janosch Frank wrote:
> Absolutely no need to set them up every time before we enable our
> interrupt masks.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
So, this one doesn't seem to be a great idea as a kernel loaded to 0x0
will overwrite the ext/io new PSWs and we'll then try to load a zero PSW
when an IRQ hits.
We have two options:
* Add a piece of code that fixes the low core after a kernel has been loaded
* Leave the code as is and add a comment, so other people don't fall
into this trap.
I'd be fine with both and for now I'd settle for the second option.
> ---
> pc-bios/s390-ccw/start.S | 30 +++++++++++++-----------------
> 1 file changed, 13 insertions(+), 17 deletions(-)
>
> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
> index 01c4c21b26..0059a15d21 100644
> --- a/pc-bios/s390-ccw/start.S
> +++ b/pc-bios/s390-ccw/start.S
> @@ -34,6 +34,12 @@ loop:
> remainder:
> larl %r2,memsetxc
> ex %r3,0(%r2)
> + /* Store io new PSW */
> + larl %r1,io_new_psw
> + mvc 0x1f0(16),0(%r1)
> + /* Store ext new PSW */
> + larl %r1,external_new_psw
> + mvc 0x1b0(16),0(%r1)
> done:
> j main /* And call C */
>
> @@ -64,11 +70,6 @@ consume_sclp_int:
> stctg %c0,%c0,0(%r15)
> oi 6(%r15),0x2
> lctlg %c0,%c0,0(%r15)
> - /* prepare external call handler */
> - larl %r1, external_new_code
> - stg %r1, 0x1b8
> - larl %r1, external_new_mask
> - mvc 0x1b0(8),0(%r1)
> /* load enabled wait PSW */
> larl %r1, enabled_wait_psw
> lpswe 0(%r1)
> @@ -81,14 +82,9 @@ consume_sclp_int:
> .globl consume_io_int
> consume_io_int:
> /* enable I/O interrupts in cr6 */
> - stctg %c6,%c6,0(%r15)
> - oi 4(%r15), 0xff
> - lctlg %c6,%c6,0(%r15)
> - /* prepare i/o call handler */
> - larl %r1, io_new_code
> - stg %r1, 0x1f8
> - larl %r1, io_new_mask
> - mvc 0x1f0(8),0(%r1)
> + stctg %c6, %c6, 0(%r15)
> + oi 4(%r15), 0xff
> + lctlg %c6, %c6, 0(%r15)
> /* load enabled wait PSW */
> larl %r1, enabled_wait_psw
> lpswe 0(%r1)
> @@ -112,7 +108,7 @@ disabled_wait_psw:
> .quad PSW_MASK_DWAIT, 0x0000000000000000
> enabled_wait_psw:
> .quad PSW_MASK_EWAIT, 0x0000000000000000
> -external_new_mask:
> - .quad PSW_MASK_64
> -io_new_mask:
> - .quad PSW_MASK_64
> +external_new_psw:
> + .quad PSW_MASK_64, external_new_code
> +io_new_psw:
> + .quad PSW_MASK_64, io_new_code
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 7/7] pc-bios: s390x: Setup io and ext new psws only once
2020-07-15 13:13 ` Janosch Frank
@ 2020-07-15 13:16 ` Christian Borntraeger
2020-07-15 14:08 ` [PATCH] pc-bios: s390x: Add a comment to the io and external new PSW setup Janosch Frank
0 siblings, 1 reply; 33+ messages in thread
From: Christian Borntraeger @ 2020-07-15 13:16 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: thuth, cohuck, david
On 15.07.20 15:13, Janosch Frank wrote:
> On 7/15/20 11:40 AM, Janosch Frank wrote:
>> Absolutely no need to set them up every time before we enable our
>> interrupt masks.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>
> So, this one doesn't seem to be a great idea as a kernel loaded to 0x0
> will overwrite the ext/io new PSWs and we'll then try to load a zero PSW
> when an IRQ hits.
>
> We have two options:
> * Add a piece of code that fixes the low core after a kernel has been loaded
> * Leave the code as is and add a comment, so other people don't fall
> into this trap.
>
> I'd be fine with both and for now I'd settle for the second option.
Yes, leave it as is and add a comment.
>
>> ---
>> pc-bios/s390-ccw/start.S | 30 +++++++++++++-----------------
>> 1 file changed, 13 insertions(+), 17 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
>> index 01c4c21b26..0059a15d21 100644
>> --- a/pc-bios/s390-ccw/start.S
>> +++ b/pc-bios/s390-ccw/start.S
>> @@ -34,6 +34,12 @@ loop:
>> remainder:
>> larl %r2,memsetxc
>> ex %r3,0(%r2)
>> + /* Store io new PSW */
>> + larl %r1,io_new_psw
>> + mvc 0x1f0(16),0(%r1)
>> + /* Store ext new PSW */
>> + larl %r1,external_new_psw
>> + mvc 0x1b0(16),0(%r1)
>> done:
>> j main /* And call C */
>>
>> @@ -64,11 +70,6 @@ consume_sclp_int:
>> stctg %c0,%c0,0(%r15)
>> oi 6(%r15),0x2
>> lctlg %c0,%c0,0(%r15)
>> - /* prepare external call handler */
>> - larl %r1, external_new_code
>> - stg %r1, 0x1b8
>> - larl %r1, external_new_mask
>> - mvc 0x1b0(8),0(%r1)
>> /* load enabled wait PSW */
>> larl %r1, enabled_wait_psw
>> lpswe 0(%r1)
>> @@ -81,14 +82,9 @@ consume_sclp_int:
>> .globl consume_io_int
>> consume_io_int:
>> /* enable I/O interrupts in cr6 */
>> - stctg %c6,%c6,0(%r15)
>> - oi 4(%r15), 0xff
>> - lctlg %c6,%c6,0(%r15)
>> - /* prepare i/o call handler */
>> - larl %r1, io_new_code
>> - stg %r1, 0x1f8
>> - larl %r1, io_new_mask
>> - mvc 0x1f0(8),0(%r1)
>> + stctg %c6, %c6, 0(%r15)
>> + oi 4(%r15), 0xff
>> + lctlg %c6, %c6, 0(%r15)
>> /* load enabled wait PSW */
>> larl %r1, enabled_wait_psw
>> lpswe 0(%r1)
>> @@ -112,7 +108,7 @@ disabled_wait_psw:
>> .quad PSW_MASK_DWAIT, 0x0000000000000000
>> enabled_wait_psw:
>> .quad PSW_MASK_EWAIT, 0x0000000000000000
>> -external_new_mask:
>> - .quad PSW_MASK_64
>> -io_new_mask:
>> - .quad PSW_MASK_64
>> +external_new_psw:
>> + .quad PSW_MASK_64, external_new_code
>> +io_new_psw:
>> + .quad PSW_MASK_64, io_new_code
>>
>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH] pc-bios: s390x: Add a comment to the io and external new PSW setup
2020-07-15 13:16 ` Christian Borntraeger
@ 2020-07-15 14:08 ` Janosch Frank
2020-07-21 7:03 ` Thomas Huth
2020-07-22 6:43 ` Christian Borntraeger
0 siblings, 2 replies; 33+ messages in thread
From: Janosch Frank @ 2020-07-15 14:08 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, thuth, cohuck, david
Normally they don't need to be set up before waiting for an interrupt
but are set up on boot. The BIOS however might overwrite the lowcore
(and hence the PSWs) when loading a blob into memory and therefore
needs to set up those PSWs more often.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
pc-bios/s390-ccw/start.S | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
index 01c4c21b26..b0fcb918cc 100644
--- a/pc-bios/s390-ccw/start.S
+++ b/pc-bios/s390-ccw/start.S
@@ -64,7 +64,10 @@ consume_sclp_int:
stctg %c0,%c0,0(%r15)
oi 6(%r15),0x2
lctlg %c0,%c0,0(%r15)
- /* prepare external call handler */
+ /*
+ * Prepare external new PSW as it might have been overwritten
+ * by a loaded blob
+ */
larl %r1, external_new_code
stg %r1, 0x1b8
larl %r1, external_new_mask
@@ -84,7 +87,10 @@ consume_io_int:
stctg %c6,%c6,0(%r15)
oi 4(%r15), 0xff
lctlg %c6,%c6,0(%r15)
- /* prepare i/o call handler */
+ /*
+ * Prepare i/o new PSW as it might have been overwritten
+ * by a loaded blob
+ */
larl %r1, io_new_code
stg %r1, 0x1f8
larl %r1, io_new_mask
--
2.25.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH] pc-bios: s390x: Add a comment to the io and external new PSW setup
2020-07-15 14:08 ` [PATCH] pc-bios: s390x: Add a comment to the io and external new PSW setup Janosch Frank
@ 2020-07-21 7:03 ` Thomas Huth
2020-07-22 6:43 ` Christian Borntraeger
1 sibling, 0 replies; 33+ messages in thread
From: Thomas Huth @ 2020-07-21 7:03 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: borntraeger, cohuck, david
On 15/07/2020 16.08, Janosch Frank wrote:
> Normally they don't need to be set up before waiting for an interrupt
> but are set up on boot. The BIOS however might overwrite the lowcore
> (and hence the PSWs) when loading a blob into memory and therefore
> needs to set up those PSWs more often.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> pc-bios/s390-ccw/start.S | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
> index 01c4c21b26..b0fcb918cc 100644
> --- a/pc-bios/s390-ccw/start.S
> +++ b/pc-bios/s390-ccw/start.S
> @@ -64,7 +64,10 @@ consume_sclp_int:
> stctg %c0,%c0,0(%r15)
> oi 6(%r15),0x2
> lctlg %c0,%c0,0(%r15)
> - /* prepare external call handler */
> + /*
> + * Prepare external new PSW as it might have been overwritten
> + * by a loaded blob
> + */
> larl %r1, external_new_code
> stg %r1, 0x1b8
> larl %r1, external_new_mask
> @@ -84,7 +87,10 @@ consume_io_int:
> stctg %c6,%c6,0(%r15)
> oi 4(%r15), 0xff
> lctlg %c6,%c6,0(%r15)
> - /* prepare i/o call handler */
> + /*
> + * Prepare i/o new PSW as it might have been overwritten
> + * by a loaded blob
> + */
> larl %r1, io_new_code
> stg %r1, 0x1f8
> larl %r1, io_new_mask
>
Reviewed-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] pc-bios: s390x: Add a comment to the io and external new PSW setup
2020-07-15 14:08 ` [PATCH] pc-bios: s390x: Add a comment to the io and external new PSW setup Janosch Frank
2020-07-21 7:03 ` Thomas Huth
@ 2020-07-22 6:43 ` Christian Borntraeger
2020-07-22 7:24 ` Janosch Frank
1 sibling, 1 reply; 33+ messages in thread
From: Christian Borntraeger @ 2020-07-22 6:43 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: thuth, cohuck, david
On 15.07.20 16:08, Janosch Frank wrote:
> Normally they don't need to be set up before waiting for an interrupt
> but are set up on boot. The BIOS however might overwrite the lowcore
> (and hence the PSWs) when loading a blob into memory and therefore
> needs to set up those PSWs more often.
Now when I read the new comment this actually inidicates a bug.
When do we restore the original content? If the loaded program
does have interrupt handlers in the original image and relies on that
then we are broken, no?
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> pc-bios/s390-ccw/start.S | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
> index 01c4c21b26..b0fcb918cc 100644
> --- a/pc-bios/s390-ccw/start.S
> +++ b/pc-bios/s390-ccw/start.S
> @@ -64,7 +64,10 @@ consume_sclp_int:
> stctg %c0,%c0,0(%r15)
> oi 6(%r15),0x2
> lctlg %c0,%c0,0(%r15)
> - /* prepare external call handler */
> + /*
> + * Prepare external new PSW as it might have been overwritten
> + * by a loaded blob
> + */
> larl %r1, external_new_code
> stg %r1, 0x1b8
> larl %r1, external_new_mask
> @@ -84,7 +87,10 @@ consume_io_int:
> stctg %c6,%c6,0(%r15)
> oi 4(%r15), 0xff
> lctlg %c6,%c6,0(%r15)
> - /* prepare i/o call handler */
> + /*
> + * Prepare i/o new PSW as it might have been overwritten
> + * by a loaded blob
> + */
> larl %r1, io_new_code
> stg %r1, 0x1f8
> larl %r1, io_new_mask
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] pc-bios: s390x: Add a comment to the io and external new PSW setup
2020-07-22 6:43 ` Christian Borntraeger
@ 2020-07-22 7:24 ` Janosch Frank
2020-07-22 7:39 ` Christian Borntraeger
0 siblings, 1 reply; 33+ messages in thread
From: Janosch Frank @ 2020-07-22 7:24 UTC (permalink / raw)
To: Christian Borntraeger, qemu-devel; +Cc: thuth, cohuck, david
[-- Attachment #1.1: Type: text/plain, Size: 2460 bytes --]
On 7/22/20 8:43 AM, Christian Borntraeger wrote:
>
>
> On 15.07.20 16:08, Janosch Frank wrote:
>> Normally they don't need to be set up before waiting for an interrupt
>> but are set up on boot. The BIOS however might overwrite the lowcore
>> (and hence the PSWs) when loading a blob into memory and therefore
>> needs to set up those PSWs more often.
>
> Now when I read the new comment this actually inidicates a bug.
> When do we restore the original content? If the loaded program
> does have interrupt handlers in the original image and relies on that
> then we are broken, no?
I haven't seen references to a save/restore functionality for those
PSWs. And I also think it's not that easy to do because we have multiple
ways of loading data and if we want to print when loading we might end
up overwriting and then saving the written value for a later restore.
I need to have a closer look at how virtio works, but wouldn't we have a
chicken - egg problem with IO interrupts for IO that writes the prefix?
The BIOS often has "interesting" solutions to problems.
If you have a quick fix, be my guest and send it. If not I'd put it on
my todo list or let Stefan make it a proper dev item.
>
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>> pc-bios/s390-ccw/start.S | 10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
>> index 01c4c21b26..b0fcb918cc 100644
>> --- a/pc-bios/s390-ccw/start.S
>> +++ b/pc-bios/s390-ccw/start.S
>> @@ -64,7 +64,10 @@ consume_sclp_int:
>> stctg %c0,%c0,0(%r15)
>> oi 6(%r15),0x2
>> lctlg %c0,%c0,0(%r15)
>> - /* prepare external call handler */
>> + /*
>> + * Prepare external new PSW as it might have been overwritten
>> + * by a loaded blob
>> + */
>> larl %r1, external_new_code
>> stg %r1, 0x1b8
>> larl %r1, external_new_mask
>> @@ -84,7 +87,10 @@ consume_io_int:
>> stctg %c6,%c6,0(%r15)
>> oi 4(%r15), 0xff
>> lctlg %c6,%c6,0(%r15)
>> - /* prepare i/o call handler */
>> + /*
>> + * Prepare i/o new PSW as it might have been overwritten
>> + * by a loaded blob
>> + */
>> larl %r1, io_new_code
>> stg %r1, 0x1f8
>> larl %r1, io_new_mask
>>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] pc-bios: s390x: Add a comment to the io and external new PSW setup
2020-07-22 7:24 ` Janosch Frank
@ 2020-07-22 7:39 ` Christian Borntraeger
2020-07-22 8:05 ` Janosch Frank
0 siblings, 1 reply; 33+ messages in thread
From: Christian Borntraeger @ 2020-07-22 7:39 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: thuth, cohuck, david
On 22.07.20 09:24, Janosch Frank wrote:
> On 7/22/20 8:43 AM, Christian Borntraeger wrote:
>>
>>
>> On 15.07.20 16:08, Janosch Frank wrote:
>>> Normally they don't need to be set up before waiting for an interrupt
>>> but are set up on boot. The BIOS however might overwrite the lowcore
>>> (and hence the PSWs) when loading a blob into memory and therefore
>>> needs to set up those PSWs more often.
>>
>> Now when I read the new comment this actually inidicates a bug.
>> When do we restore the original content? If the loaded program
>> does have interrupt handlers in the original image and relies on that
>> then we are broken, no?
>
> I haven't seen references to a save/restore functionality for those
> PSWs. And I also think it's not that easy to do because we have multiple
> ways of loading data and if we want to print when loading we might end
> up overwriting and then saving the written value for a later restore.
>
> I need to have a closer look at how virtio works, but wouldn't we have a
> chicken - egg problem with IO interrupts for IO that writes the prefix?
>
> The BIOS often has "interesting" solutions to problems.
> If you have a quick fix, be my guest and send it. If not I'd put it on
> my todo list or let Stefan make it a proper dev item.
Maybe a global fixup table in BIOS memory that restores all the memory that
we messed with when we hand over control? Can you at least change the comment
here to add a fixme?
>
>>
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>> pc-bios/s390-ccw/start.S | 10 ++++++++--
>>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
>>> index 01c4c21b26..b0fcb918cc 100644
>>> --- a/pc-bios/s390-ccw/start.S
>>> +++ b/pc-bios/s390-ccw/start.S
>>> @@ -64,7 +64,10 @@ consume_sclp_int:
>>> stctg %c0,%c0,0(%r15)
>>> oi 6(%r15),0x2
>>> lctlg %c0,%c0,0(%r15)
>>> - /* prepare external call handler */
>>> + /*
>>> + * Prepare external new PSW as it might have been overwritten
>>> + * by a loaded blob
>>> + */
>>> larl %r1, external_new_code
>>> stg %r1, 0x1b8
>>> larl %r1, external_new_mask
>>> @@ -84,7 +87,10 @@ consume_io_int:
>>> stctg %c6,%c6,0(%r15)
>>> oi 4(%r15), 0xff
>>> lctlg %c6,%c6,0(%r15)
>>> - /* prepare i/o call handler */
>>> + /*
>>> + * Prepare i/o new PSW as it might have been overwritten
>>> + * by a loaded blob
>>> + */
>>> larl %r1, io_new_code
>>> stg %r1, 0x1f8
>>> larl %r1, io_new_mask
>>>
>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] pc-bios: s390x: Add a comment to the io and external new PSW setup
2020-07-22 7:39 ` Christian Borntraeger
@ 2020-07-22 8:05 ` Janosch Frank
2020-08-27 9:20 ` Thomas Huth
0 siblings, 1 reply; 33+ messages in thread
From: Janosch Frank @ 2020-07-22 8:05 UTC (permalink / raw)
To: Christian Borntraeger, qemu-devel; +Cc: thuth, cohuck, david
[-- Attachment #1.1: Type: text/plain, Size: 2901 bytes --]
On 7/22/20 9:39 AM, Christian Borntraeger wrote:
>
>
> On 22.07.20 09:24, Janosch Frank wrote:
>> On 7/22/20 8:43 AM, Christian Borntraeger wrote:
>>>
>>>
>>> On 15.07.20 16:08, Janosch Frank wrote:
>>>> Normally they don't need to be set up before waiting for an interrupt
>>>> but are set up on boot. The BIOS however might overwrite the lowcore
>>>> (and hence the PSWs) when loading a blob into memory and therefore
>>>> needs to set up those PSWs more often.
>>>
>>> Now when I read the new comment this actually inidicates a bug.
>>> When do we restore the original content? If the loaded program
>>> does have interrupt handlers in the original image and relies on that
>>> then we are broken, no?
>>
>> I haven't seen references to a save/restore functionality for those
>> PSWs. And I also think it's not that easy to do because we have multiple
>> ways of loading data and if we want to print when loading we might end
>> up overwriting and then saving the written value for a later restore.
>>
>> I need to have a closer look at how virtio works, but wouldn't we have a
>> chicken - egg problem with IO interrupts for IO that writes the prefix?
>>
>> The BIOS often has "interesting" solutions to problems.
>> If you have a quick fix, be my guest and send it. If not I'd put it on
>> my todo list or let Stefan make it a proper dev item.
>
> Maybe a global fixup table in BIOS memory that restores all the memory that
> we messed with when we hand over control? Can you at least change the comment
> here to add a fixme?
Sure
>
>>
>>>
>>>>
>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>> ---
>>>> pc-bios/s390-ccw/start.S | 10 ++++++++--
>>>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/pc-bios/s390-ccw/start.S b/pc-bios/s390-ccw/start.S
>>>> index 01c4c21b26..b0fcb918cc 100644
>>>> --- a/pc-bios/s390-ccw/start.S
>>>> +++ b/pc-bios/s390-ccw/start.S
>>>> @@ -64,7 +64,10 @@ consume_sclp_int:
>>>> stctg %c0,%c0,0(%r15)
>>>> oi 6(%r15),0x2
>>>> lctlg %c0,%c0,0(%r15)
>>>> - /* prepare external call handler */
>>>> + /*
>>>> + * Prepare external new PSW as it might have been overwritten
>>>> + * by a loaded blob
>>>> + */
>>>> larl %r1, external_new_code
>>>> stg %r1, 0x1b8
>>>> larl %r1, external_new_mask
>>>> @@ -84,7 +87,10 @@ consume_io_int:
>>>> stctg %c6,%c6,0(%r15)
>>>> oi 4(%r15), 0xff
>>>> lctlg %c6,%c6,0(%r15)
>>>> - /* prepare i/o call handler */
>>>> + /*
>>>> + * Prepare i/o new PSW as it might have been overwritten
>>>> + * by a loaded blob
>>>> + */
>>>> larl %r1, io_new_code
>>>> stg %r1, 0x1f8
>>>> larl %r1, io_new_mask
>>>>
>>
>>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] pc-bios: s390x: Add a comment to the io and external new PSW setup
2020-07-22 8:05 ` Janosch Frank
@ 2020-08-27 9:20 ` Thomas Huth
0 siblings, 0 replies; 33+ messages in thread
From: Thomas Huth @ 2020-08-27 9:20 UTC (permalink / raw)
To: Janosch Frank, Christian Borntraeger, qemu-devel; +Cc: cohuck, david
On 22/07/2020 10.05, Janosch Frank wrote:
> On 7/22/20 9:39 AM, Christian Borntraeger wrote:
>>
>>
>> On 22.07.20 09:24, Janosch Frank wrote:
>>> On 7/22/20 8:43 AM, Christian Borntraeger wrote:
>>>>
>>>>
>>>> On 15.07.20 16:08, Janosch Frank wrote:
>>>>> Normally they don't need to be set up before waiting for an interrupt
>>>>> but are set up on boot. The BIOS however might overwrite the lowcore
>>>>> (and hence the PSWs) when loading a blob into memory and therefore
>>>>> needs to set up those PSWs more often.
>>>>
>>>> Now when I read the new comment this actually inidicates a bug.
>>>> When do we restore the original content? If the loaded program
>>>> does have interrupt handlers in the original image and relies on that
>>>> then we are broken, no?
>>>
>>> I haven't seen references to a save/restore functionality for those
>>> PSWs. And I also think it's not that easy to do because we have multiple
>>> ways of loading data and if we want to print when loading we might end
>>> up overwriting and then saving the written value for a later restore.
>>>
>>> I need to have a closer look at how virtio works, but wouldn't we have a
>>> chicken - egg problem with IO interrupts for IO that writes the prefix?
>>>
>>> The BIOS often has "interesting" solutions to problems.
>>> If you have a quick fix, be my guest and send it. If not I'd put it on
>>> my todo list or let Stefan make it a proper dev item.
>>
>> Maybe a global fixup table in BIOS memory that restores all the memory that
>> we messed with when we hand over control? Can you at least change the comment
>> here to add a fixme?
>
> Sure
Hi Janosch!
Did you ever send a new version of this patch with the FIXME added? I
can't find it right now... Or shall I add a FIXME when picking this up?
(If so, could you please suggest a text?)
Thomas
^ permalink raw reply [flat|nested] 33+ messages in thread