Re: [PATCH v5 11/12] pc-bios: s390x: Fix bootmap.c passing PSWs as addresses

[Thomas Huth]

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:

   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.

>   }
>   
>   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.

>   } __attribute((packed)) ComponentEntry;
>   
>   typedef struct ComponentHeader {

 Thomas