[PATCH v2 0/2] pc-bios/s390 fixes for reboot-to-vfio-ccw

[PATCH v2 0/2] pc-bios/s390 fixes for reboot-to-vfio-ccw

[Eric Farman]

Thomas, Conny,

Here is the updated series for the re-IPL fixes mentioned yesterday [1]

I've updated the reviews/acks to Patch 1 (Thank you Conny, Janosch!),
and implemented Thomas' suggestion to clear out the memory where S390EP
exists early in our processing. This both preserves the delicate balance
we have today, and fixes the scenario I've been wrestling with.

For this second patch, I opted to #define the address x10008 and use it
in both the new memset and existing memcmp, rather than having the
address used directly in two places. I put this definition in s390-arch.h
as it's shared by both files, even though it's not strictly an
"s390 architecture" thing. Considering its importance, it seems like a
reasonable fit.

Tested with my permutations of virtio/vfio/chreipl/install things,
but surely missing some others that I don't have readily configured.

[1] https://lore.kernel.org/qemu-devel/20201119165729.63351-1-farman@linux.ibm.com/

Eric Farman (2):
  pc-bios: s390x: Ensure Read IPL memory is clean
  pc-bios: s390x: Clear out leftover S390EP string

 pc-bios/s390-ccw/dasd-ipl.c  | 3 +++
 pc-bios/s390-ccw/jump2ipl.c  | 2 +-
 pc-bios/s390-ccw/main.c      | 6 ++++++
 pc-bios/s390-ccw/s390-arch.h | 3 +++
 4 files changed, 13 insertions(+), 1 deletion(-)

-- 
2.17.1

[PATCH v2 1/2] pc-bios: s390x: Ensure Read IPL memory is clean

[Eric Farman]

If, for example, we boot off a virtio device and chreipl to a vfio-ccw
device, the space at lowcore will be non-zero. We build a Read IPL CCW
at address zero, but it will have leftover PSW data that will conflict
with the Format-0 CCW being generated:

0x0: 00080000 80010000
       ------ Ccw0.cda
              -- Ccw0.chainData
                -- Reserved bits

The data address will be overwritten with the correct value (0x0), but
the apparent data chain bit will cause subsequent memory to be used as
the target of the data store, which may not be where we expect (0x0).

Clear out this space when we boot from DASD, so that we know it exists
exactly as we expect.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
Reviewed-by: Janosch Frank <frankja@de.ibm.com>
Acked-by: Cornelia Huck <cohuck@redhat.com>
---
 pc-bios/s390-ccw/dasd-ipl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/pc-bios/s390-ccw/dasd-ipl.c b/pc-bios/s390-ccw/dasd-ipl.c
index 0fc879bb8e..71cbae2f16 100644
--- a/pc-bios/s390-ccw/dasd-ipl.c
+++ b/pc-bios/s390-ccw/dasd-ipl.c
@@ -100,6 +100,9 @@ static void make_readipl(void)
 {
     Ccw0 *ccwIplRead = (Ccw0 *)0x00;
 
+    /* Clear out any existing data */
+    memset(ccwIplRead, 0, sizeof(Ccw0));
+
     /* Create Read IPL ccw at address 0 */
     ccwIplRead->cmd_code = CCW_CMD_READ_IPL;
     ccwIplRead->cda = 0x00; /* Read into address 0x00 in main memory */
-- 
2.17.1

[PATCH v2 2/2] pc-bios: s390x: Clear out leftover S390EP string

[Eric Farman]

A Linux binary will have the string "S390EP" at address 0x10008,
which is important in getting the guest up off the ground. In the
case of a reboot (specifically chreipl going to a new device),
we should defer to the PSW at address zero for the new config,
which will re-write "S390EP" from the new image.

Let's clear it out at this point so that a reipl to, say, a DASD
passthrough device drives the IPL path from scratch without disrupting
disrupting the order of operations for other boots.

Rather than hardcoding the address of this magic (again), let's
define it somewhere so that the two users are visibly related.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 pc-bios/s390-ccw/jump2ipl.c  | 2 +-
 pc-bios/s390-ccw/main.c      | 6 ++++++
 pc-bios/s390-ccw/s390-arch.h | 3 +++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
index fbae45b03c..b9c70d64a5 100644
--- a/pc-bios/s390-ccw/jump2ipl.c
+++ b/pc-bios/s390-ccw/jump2ipl.c
@@ -78,7 +78,7 @@ void jump_to_low_kernel(void)
      * kernel start address (when jumping to the PSW-at-zero address instead,
      * the kernel startup code fails when we booted from a network device).
      */
-    if (!memcmp((char *)0x10008, "S390EP", 6)) {
+    if (!memcmp((char *)S390EP, "S390EP", 6)) {
         jump_to_IPL_code(KERN_IMAGE_START);
     }
 
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index fc4bfaa455..5d2b7ba94d 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -178,6 +178,12 @@ static void boot_setup(void)
     memcpy(lpmsg + 10, loadparm_str, 8);
     sclp_print(lpmsg);
 
+    /*
+     * Clear out any potential S390EP magic (see jump_to_low_kernel()),
+     * so we don't taint our decision-making process during a reboot.
+     */
+    memset((char *)S390EP, 0, 6);
+
     have_iplb = store_iplb(&iplb);
 }
 
diff --git a/pc-bios/s390-ccw/s390-arch.h b/pc-bios/s390-ccw/s390-arch.h
index 6da44d4436..a741488aaa 100644
--- a/pc-bios/s390-ccw/s390-arch.h
+++ b/pc-bios/s390-ccw/s390-arch.h
@@ -95,6 +95,9 @@ typedef struct LowCore {
 
 extern LowCore *lowcore;
 
+/* Location of "S390EP" in a Linux binary (see arch/s390/boot/head.S) */
+#define S390EP 0x10008
+
 static inline void set_prefix(uint32_t address)
 {
     asm volatile("spx %0" : : "m" (address) : "memory");
-- 
2.17.1

Re: [PATCH v2 2/2] pc-bios: s390x: Clear out leftover S390EP string

[Christian Borntraeger]

On 20.11.20 17:01, Eric Farman wrote:
> A Linux binary will have the string "S390EP" at address 0x10008,
> which is important in getting the guest up off the ground. In the
> case of a reboot (specifically chreipl going to a new device),
> we should defer to the PSW at address zero for the new config,
> which will re-write "S390EP" from the new image.
> 
> Let's clear it out at this point so that a reipl to, say, a DASD
> passthrough device drives the IPL path from scratch without disrupting
> disrupting the order of operations for other boots.
> 
> Rather than hardcoding the address of this magic (again), let's
> define it somewhere so that the two users are visibly related.


Hmmm, this might have side effects, e.g. if you do something like a kdump
or kexec to a non-Linux binary that happens to have code at 0x10008, no?

As far as I can tell, the problem should only happen for a ccw type IPL
so why not

[...]
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -178,6 +178,12 @@ static void boot_setup(void)
>      memcpy(lpmsg + 10, loadparm_str, 8);
>      sclp_print(lpmsg);
>  
> +    /*
> +     * Clear out any potential S390EP magic (see jump_to_low_kernel()),
> +     * so we don't taint our decision-making process during a reboot.
> +     */
> +    memset((char *)S390EP, 0, 6);


move this into find_subch
in here:
------------- snip ---------------
            case CU_TYPE_DASD_3990:
            case CU_TYPE_DASD_2107:
                return true;
------------- snip ---------------

Re: [PATCH v2 2/2] pc-bios: s390x: Clear out leftover S390EP string

[Thomas Huth]

On 23/11/2020 08.39, Christian Borntraeger wrote:
> On 20.11.20 17:01, Eric Farman wrote:
>> A Linux binary will have the string "S390EP" at address 0x10008,
>> which is important in getting the guest up off the ground. In the
>> case of a reboot (specifically chreipl going to a new device),
>> we should defer to the PSW at address zero for the new config,
>> which will re-write "S390EP" from the new image.
>>
>> Let's clear it out at this point so that a reipl to, say, a DASD
>> passthrough device drives the IPL path from scratch without disrupting
>> disrupting the order of operations for other boots.
>>
>> Rather than hardcoding the address of this magic (again), let's
>> define it somewhere so that the two users are visibly related.
> 
> 
> Hmmm, this might have side effects, e.g. if you do something like a kdump
> or kexec to a non-Linux binary that happens to have code at 0x10008, no?

Do these scenarios really go through the s390-ccw bios again, or do they
rather bypass the bios and jump directly into the new kernel?

> As far as I can tell, the problem should only happen for a ccw type IPL
> so why not

Not sure whether it really can only happen in these cases... for example,
would it also be possible to reboot from a Linux kernel into a
kvm-unit-test? ... these also do not have the S390EP magic, IIRC.

> [...]
>> --- a/pc-bios/s390-ccw/main.c
>> +++ b/pc-bios/s390-ccw/main.c
>> @@ -178,6 +178,12 @@ static void boot_setup(void)
>>      memcpy(lpmsg + 10, loadparm_str, 8);
>>      sclp_print(lpmsg);
>>  
>> +    /*
>> +     * Clear out any potential S390EP magic (see jump_to_low_kernel()),
>> +     * so we don't taint our decision-making process during a reboot.
>> +     */
>> +    memset((char *)S390EP, 0, 6);
> 
> 
> move this into find_subch
> in here:
> ------------- snip ---------------
>             case CU_TYPE_DASD_3990:
>             case CU_TYPE_DASD_2107:
>                 return true;
> ------------- snip ---------------
> 

That would be is_dev_possibly_bootable() now? ... not sure whether this is
the best location... maybe put it better at the beginning of dasd_ipl() instead?

 Thomas

Re: [PATCH v2 2/2] pc-bios: s390x: Clear out leftover S390EP string

[Christian Borntraeger]

On 23.11.20 09:05, Thomas Huth wrote:
> On 23/11/2020 08.39, Christian Borntraeger wrote:
>> On 20.11.20 17:01, Eric Farman wrote:
>>> A Linux binary will have the string "S390EP" at address 0x10008,
>>> which is important in getting the guest up off the ground. In the
>>> case of a reboot (specifically chreipl going to a new device),
>>> we should defer to the PSW at address zero for the new config,
>>> which will re-write "S390EP" from the new image.
>>>
>>> Let's clear it out at this point so that a reipl to, say, a DASD
>>> passthrough device drives the IPL path from scratch without disrupting
>>> disrupting the order of operations for other boots.
>>>
>>> Rather than hardcoding the address of this magic (again), let's
>>> define it somewhere so that the two users are visibly related.
>>
>>
>> Hmmm, this might have side effects, e.g. if you do something like a kdump
>> or kexec to a non-Linux binary that happens to have code at 0x10008, no?
> 
> Do these scenarios really go through the s390-ccw bios again, or do they
> rather bypass the bios and jump directly into the new kernel?

Right they jump directly into the new kernel. So this patch could actually
be "good enough" for 5.2 as is?

Re: [PATCH v2 2/2] pc-bios: s390x: Clear out leftover S390EP string

[Thomas Huth]

On 23/11/2020 09.07, Christian Borntraeger wrote:
> 
> 
> On 23.11.20 09:05, Thomas Huth wrote:
>> On 23/11/2020 08.39, Christian Borntraeger wrote:
>>> On 20.11.20 17:01, Eric Farman wrote:
>>>> A Linux binary will have the string "S390EP" at address 0x10008,
>>>> which is important in getting the guest up off the ground. In the
>>>> case of a reboot (specifically chreipl going to a new device),
>>>> we should defer to the PSW at address zero for the new config,
>>>> which will re-write "S390EP" from the new image.
>>>>
>>>> Let's clear it out at this point so that a reipl to, say, a DASD
>>>> passthrough device drives the IPL path from scratch without disrupting
>>>> disrupting the order of operations for other boots.
>>>>
>>>> Rather than hardcoding the address of this magic (again), let's
>>>> define it somewhere so that the two users are visibly related.
>>>
>>>
>>> Hmmm, this might have side effects, e.g. if you do something like a kdump
>>> or kexec to a non-Linux binary that happens to have code at 0x10008, no?
>>
>> Do these scenarios really go through the s390-ccw bios again, or do they
>> rather bypass the bios and jump directly into the new kernel?
> 
> Right they jump directly into the new kernel. So this patch could actually
> be "good enough" for 5.2 as is?

I think it should be fine, yes. I'll give it a try with my usual s390-ccw
bios tests, and if there are no regressions (and no other objections on the
mailing list here), I'll prepare a pull request.

 Thomas