| Message ID | 20201030122823.347140-1-borntraeger@de.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390-bios: Skip writing iplb location to low core for ccw ipl | expand |
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) >
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...
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.
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... >
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.
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)