s390-bios: Skip writing iplb location to low core for ccw ipl

[PATCH] s390-bios: Skip writing iplb location to low core for ccw ipl

Posted by Christian Borntraeger 3 years, 6 months ago

From: "Jason J. Herne" <jjherne@linux.ibm.com>

The architecture states that the iplb location is only written to low
core for list directed ipl and not for traditional ccw ipl. If we don't
skip this then operating systems that load by reading into low core
memory may fail to start.

We should also not write the iplb pointer for network boot as it might
overwrite content that we got via network.

Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
Fixes: 9bfc04f9ef68 ("pc-bios: s390x: Save iplb location in lowcore")
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 pc-bios/s390-ccw/main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 43c792cf9509..fc4bfaa45529 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -43,7 +43,9 @@ void write_subsystem_identification(void)
 
 void write_iplb_location(void)
 {
-    lowcore->ptr_iplb = ptr2u32(&iplb);
+    if (cutype == CU_TYPE_VIRTIO && virtio_get_device_type() != VIRTIO_ID_NET) {
+        lowcore->ptr_iplb = ptr2u32(&iplb);
+    }
 }
 
 unsigned int get_loadparm_index(void)
-- 
2.26.2

Re: [PATCH] s390-bios: Skip writing iplb location to low core for ccw ipl

Posted by Christian Borntraeger 3 years, 6 months ago

On 30.10.20 13:28, Christian Borntraeger wrote:
> From: "Jason J. Herne" <jjherne@linux.ibm.com>
> 
> The architecture states that the iplb location is only written to low
> core for list directed ipl and not for traditional ccw ipl. If we don't
> skip this then operating systems that load by reading into low core
> memory may fail to start.
> 
> We should also not write the iplb pointer for network boot as it might
> overwrite content that we got via network.
> 
> Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
> Fixes: 9bfc04f9ef68 ("pc-bios: s390x: Save iplb location in lowcore")
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

FWIW, this fixes the vfio-ccw IPL for some non Linux binaries.
> ---
>  pc-bios/s390-ccw/main.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index 43c792cf9509..fc4bfaa45529 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -43,7 +43,9 @@ void write_subsystem_identification(void)
>  
>  void write_iplb_location(void)
>  {
> -    lowcore->ptr_iplb = ptr2u32(&iplb);
> +    if (cutype == CU_TYPE_VIRTIO && virtio_get_device_type() != VIRTIO_ID_NET) {
> +        lowcore->ptr_iplb = ptr2u32(&iplb);
> +    }
>  }
>  
>  unsigned int get_loadparm_index(void)
>

Re: [PATCH] s390-bios: Skip writing iplb location to low core for ccw ipl

Posted by Cornelia Huck 3 years, 6 months ago

On Fri, 30 Oct 2020 13:28:23 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> From: "Jason J. Herne" <jjherne@linux.ibm.com>
> 
> The architecture states that the iplb location is only written to low
> core for list directed ipl and not for traditional ccw ipl. If we don't
> skip this then operating systems that load by reading into low core
> memory may fail to start.
> 
> We should also not write the iplb pointer for network boot as it might
> overwrite content that we got via network.
> 
> Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
> Fixes: 9bfc04f9ef68 ("pc-bios: s390x: Save iplb location in lowcore")
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  pc-bios/s390-ccw/main.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Thanks, queued (together with a rebuild) to s390-fixes.

Re: [PATCH] s390-bios: Skip writing iplb location to low core for ccw ipl

Posted by Thomas Huth 3 years, 6 months ago

On 30/10/2020 13.28, Christian Borntraeger wrote:
> From: "Jason J. Herne" <jjherne@linux.ibm.com>
> 
> The architecture states that the iplb location is only written to low
> core for list directed ipl and not for traditional ccw ipl. If we don't
> skip this then operating systems that load by reading into low core
> memory may fail to start.

Just double-checking: But doing write_subsystem_identification()
unconditionally is ok, right?

> We should also not write the iplb pointer for network boot as it might
> overwrite content that we got via network.

FWIW, write_iplb_location() is already just a dummy function in netmain.c,
so this should not have been an issue in the network bootloader, I hope.

> Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
> Fixes: 9bfc04f9ef68 ("pc-bios: s390x: Save iplb location in lowcore")
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  pc-bios/s390-ccw/main.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index 43c792cf9509..fc4bfaa45529 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -43,7 +43,9 @@ void write_subsystem_identification(void)
>  
>  void write_iplb_location(void)
>  {
> -    lowcore->ptr_iplb = ptr2u32(&iplb);
> +    if (cutype == CU_TYPE_VIRTIO && virtio_get_device_type() != VIRTIO_ID_NET) {
> +        lowcore->ptr_iplb = ptr2u32(&iplb);
> +    }
>  }

Acked-by: Thomas Huth <thuth@redhat.com>

Christian, Cornelia, could you please pick up the patch? I'm not sure
whether I can do another PR this week for the RC...

Re: [PATCH] s390-bios: Skip writing iplb location to low core for ccw ipl

Posted by Cornelia Huck 3 years, 6 months ago

On Tue, 3 Nov 2020 13:32:47 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 30/10/2020 13.28, Christian Borntraeger wrote:
> > From: "Jason J. Herne" <jjherne@linux.ibm.com>
> > 
> > The architecture states that the iplb location is only written to low
> > core for list directed ipl and not for traditional ccw ipl. If we don't
> > skip this then operating systems that load by reading into low core
> > memory may fail to start.  
> 
> Just double-checking: But doing write_subsystem_identification()
> unconditionally is ok, right?
> 
> > We should also not write the iplb pointer for network boot as it might
> > overwrite content that we got via network.  
> 
> FWIW, write_iplb_location() is already just a dummy function in netmain.c,
> so this should not have been an issue in the network bootloader, I hope.
> 
> > Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
> > Fixes: 9bfc04f9ef68 ("pc-bios: s390x: Save iplb location in lowcore")
> > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > ---
> >  pc-bios/s390-ccw/main.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> > index 43c792cf9509..fc4bfaa45529 100644
> > --- a/pc-bios/s390-ccw/main.c
> > +++ b/pc-bios/s390-ccw/main.c
> > @@ -43,7 +43,9 @@ void write_subsystem_identification(void)
> >  
> >  void write_iplb_location(void)
> >  {
> > -    lowcore->ptr_iplb = ptr2u32(&iplb);
> > +    if (cutype == CU_TYPE_VIRTIO && virtio_get_device_type() != VIRTIO_ID_NET) {
> > +        lowcore->ptr_iplb = ptr2u32(&iplb);
> > +    }
> >  }  
> 
> Acked-by: Thomas Huth <thuth@redhat.com>
> 
> Christian, Cornelia, could you please pick up the patch? I'm not sure
> whether I can do another PR this week for the RC...

Will do, I have other things as well anyway.

Re: [PATCH] s390-bios: Skip writing iplb location to low core for ccw ipl

Posted by Christian Borntraeger 3 years, 6 months ago


On 03.11.20 13:32, Thomas Huth wrote:
> On 30/10/2020 13.28, Christian Borntraeger wrote:
>> From: "Jason J. Herne" <jjherne@linux.ibm.com>
>>
>> The architecture states that the iplb location is only written to low
>> core for list directed ipl and not for traditional ccw ipl. If we don't
>> skip this then operating systems that load by reading into low core
>> memory may fail to start.
> 
> Just double-checking: But doing write_subsystem_identification()
> unconditionally is ok, right?

Yes, for any channel device IPL the subsystem ID is stored in absolute locations
184-187 so this should be good as virtio is a channel device.
> 
>> We should also not write the iplb pointer for network boot as it might
>> overwrite content that we got via network.
> 
> FWIW, write_iplb_location() is already just a dummy function in netmain.c,
> so this should not have been an issue in the network bootloader, I hope.

OK. I think we can keep the check for !VIRTIO_ID_NET anyway.

> 
>> Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
>> Fixes: 9bfc04f9ef68 ("pc-bios: s390x: Save iplb location in lowcore")
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  pc-bios/s390-ccw/main.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
>> index 43c792cf9509..fc4bfaa45529 100644
>> --- a/pc-bios/s390-ccw/main.c
>> +++ b/pc-bios/s390-ccw/main.c
>> @@ -43,7 +43,9 @@ void write_subsystem_identification(void)
>>  
>>  void write_iplb_location(void)
>>  {
>> -    lowcore->ptr_iplb = ptr2u32(&iplb);
>> +    if (cutype == CU_TYPE_VIRTIO && virtio_get_device_type() != VIRTIO_ID_NET) {
>> +        lowcore->ptr_iplb = ptr2u32(&iplb);
>> +    }
>>  }
> 
> Acked-by: Thomas Huth <thuth@redhat.com>
> 
> Christian, Cornelia, could you please pick up the patch? I'm not sure
> whether I can do another PR this week for the RC...
>