[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v5 11/12] pc-bios: s390x: Fix bootmap.c passing PSWs as addre
From: |
Janosch Frank |
Subject: |
Re: [PATCH v5 11/12] pc-bios: s390x: Fix bootmap.c passing PSWs as addresses |
Date: |
Fri, 26 Jun 2020 10:02:05 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0 |
On 6/25/20 2:46 PM, Thomas Huth wrote:
> On 24/06/2020 09.52, Janosch Frank wrote:
>> The 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 | 2 +-
>> 2 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
>> index 97205674e5..8547a140df 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->psw & PSW_MASK_SHORT_ADDR;
>
> Are you really sure about this one here? The address does not seem to be
> used for any of the jump_to_IPL() functions. And in the zipl sources, I
> can also see spots like this:
This one slipped through and is indeed wrong.
>
> entry->compdat.load_address = data.load_address;
>
> ... without any further short mask bits. So I somehow doubt that this
> change is really ok?
>
>> 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->psw & PSW_MASK_SHORT_ADDR);
>
> That one should be fine, I think.
Yes, as it is a execute type entry, this needs to be a PSW and therefore
needs to be masked.
>
>> }
>>
>> static void ipl_scsi(void)
>> diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
>> index 12a0166aae..e07f87e690 100644
>> --- a/pc-bios/s390-ccw/bootmap.h
>> +++ b/pc-bios/s390-ccw/bootmap.h
>> @@ -68,7 +68,7 @@ typedef struct ComponentEntry {
>> ScsiBlockPtr data;
>> uint8_t pad[7];
>> uint8_t component_type;
>> - uint64_t load_address;
>> + uint64_t psw;
>
> I'd recommend to keep the load_address name. It's the same name as used
> in the zipl sources, and as far as I can see, the field does not always
> contain a PSW.
The problem is that this is a union in zipl containing an address, psw
or signature header.
I guess we should also make it a union and use the proper members so it
is clear what we retrieve from the entry. If it is a PSW we need to mask
it if it is a component address masking might be a bad idea.
But I absolutely do not want to have this named PSW and then being used
like a normal address. It took me way too long to figure out why my
guest wasn't booting anymore.
Time for a new series of patches :)
>
>> } __attribute((packed)) ComponentEntry;
>>
>> typedef struct ComponentHeader {
>>
>
> Thomas
>
signature.asc
Description: OpenPGP digital signature
- [PATCH v5 02/12] pc-bios: s390x: Consolidate timing functions into time.h, (continued)
- [PATCH v5 02/12] pc-bios: s390x: Consolidate timing functions into time.h, Janosch Frank, 2020/06/24
- [PATCH v5 01/12] pc-bios: s390x: cio.c cleanup and compile fix, Janosch Frank, 2020/06/24
- [PATCH v5 07/12] pc-bios: s390x: Use PSW masks where possible and introduce PSW_MASK_SHORT_ADDR, Janosch Frank, 2020/06/24
- [PATCH v5 06/12] pc-bios: s390x: Rename PSW_MASK_ZMODE to PSW_MASK_64, Janosch Frank, 2020/06/24
- [PATCH v5 11/12] pc-bios: s390x: Fix bootmap.c passing PSWs as addresses, Janosch Frank, 2020/06/24
- [PATCH v5 05/12] pc-bios: s390x: Remove unneeded dasd-ipl.c reset psw mask changes, Janosch Frank, 2020/06/24
- [PATCH v5 08/12] pc-bios: s390x: Move panic() into header and add infinite loop, Janosch Frank, 2020/06/24
- [PATCH v5 04/12] pc-bios: s390x: Get rid of magic offsets into the lowcore, Janosch Frank, 2020/06/24
- [PATCH v5 10/12] pc-bios: s390x: Make u32 ptr check explicit, Janosch Frank, 2020/06/24
- [RFC v5 12/12] pc-bios: s390x: Cleanup jump to ipl code, Janosch Frank, 2020/06/24