Re: [Qemu-devel] [PATCH 3/3] s390: Do not pass inofficial IPL type to the guest
On 04/05/2018 05:07 PM, Viktor Mihajlovski wrote: > IPL over a virtio-scsi device requires special handling not > available in the real architecture. For this purpose the IPL > type 0xFF has been chosen as means of communication between > QEMU and the pc-bios. However, a guest OS could be confused > by seeing an unknown IPL type. > > This change sets the IPL parameter type to 0x02 (CCW) to prevent > this. Pre-existing Linux has looked up the IPL parameters only in > the case of FCP IPL. This means that the behavior should stay > the same even if Linux checks for the IPL type unconditionally. > > Signed-off-by: Viktor Mihajlovski Reviewed-by: Christian Borntraeger I will do some testing on the whole series and answer to the cover letter. Just in case. > --- > pc-bios/s390-ccw/bootmap.c | 7 +++ > pc-bios/s390-ccw/iplb.h| 15 +-- > 2 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c > index fc2a9fe..9287b7a 100644 > --- a/pc-bios/s390-ccw/bootmap.c > +++ b/pc-bios/s390-ccw/bootmap.c > @@ -70,6 +70,13 @@ static void jump_to_IPL_code(uint64_t address) > { > /* store the subsystem information _after_ the bootmap was loaded */ > write_subsystem_identification(); > + > +/* prevent unknown IPL types in the guest */ > +if (iplb.pbt == S390_IPL_TYPE_QEMU_SCSI) { > +iplb.pbt = S390_IPL_TYPE_CCW; > +set_iplb(&iplb); > +} > + > /* > * The IPL PSW is at address 0. We also must not overwrite the > * content of non-BIOS memory after we loaded the guest, so we > diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h > index 7dfce4f..5357a36 100644 > --- a/pc-bios/s390-ccw/iplb.h > +++ b/pc-bios/s390-ccw/iplb.h > @@ -97,16 +97,27 @@ extern QemuIplParameters qipl; > #define S390_IPL_TYPE_CCW 0x02 > #define S390_IPL_TYPE_QEMU_SCSI 0xff > > -static inline bool store_iplb(IplParameterBlock *iplb) > +static inline bool manage_iplb(IplParameterBlock *iplb, bool store) > { > register unsigned long addr asm("0") = (unsigned long) iplb; > register unsigned long rc asm("1") = 0; > > asm volatile ("diag %0,%2,0x308\n" >: "+d" (addr), "+d" (rc) > - : "d" (6) > + : "d" (store ? 6 : 5) >: "memory", "cc"); > return rc == 0x01; > } > > + > +static inline bool store_iplb(IplParameterBlock *iplb) > +{ > +return manage_iplb(iplb, true); > +} > + > +static inline bool set_iplb(IplParameterBlock *iplb) > +{ > +return manage_iplb(iplb, false); > +} > + > #endif /* IPLB_H */ >
Re: [Qemu-devel] [PATCH 3/3] s390: Do not pass inofficial IPL type to the guest
On 06.04.2018 11:28, Thomas Huth wrote: > On 05.04.2018 17:07, Viktor Mihajlovski wrote: >> IPL over a virtio-scsi device requires special handling not >> available in the real architecture. For this purpose the IPL >> type 0xFF has been chosen as means of communication between >> QEMU and the pc-bios. However, a guest OS could be confused >> by seeing an unknown IPL type. >> >> This change sets the IPL parameter type to 0x02 (CCW) to prevent >> this. Pre-existing Linux has looked up the IPL parameters only in >> the case of FCP IPL. This means that the behavior should stay >> the same even if Linux checks for the IPL type unconditionally. >> >> Signed-off-by: Viktor Mihajlovski >> --- >> pc-bios/s390-ccw/bootmap.c | 7 +++ >> pc-bios/s390-ccw/iplb.h| 15 +-- >> 2 files changed, 20 insertions(+), 2 deletions(-) >> >> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c >> index fc2a9fe..9287b7a 100644 >> --- a/pc-bios/s390-ccw/bootmap.c >> +++ b/pc-bios/s390-ccw/bootmap.c >> @@ -70,6 +70,13 @@ static void jump_to_IPL_code(uint64_t address) >> { >> /* store the subsystem information _after_ the bootmap was loaded */ >> write_subsystem_identification(); >> + >> +/* prevent unknown IPL types in the guest */ >> +if (iplb.pbt == S390_IPL_TYPE_QEMU_SCSI) { >> +iplb.pbt = S390_IPL_TYPE_CCW; >> +set_iplb(&iplb); >> +} >> + >> /* >> * The IPL PSW is at address 0. We also must not overwrite the >> * content of non-BIOS memory after we loaded the guest, so we >> diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h >> index 7dfce4f..5357a36 100644 >> --- a/pc-bios/s390-ccw/iplb.h >> +++ b/pc-bios/s390-ccw/iplb.h >> @@ -97,16 +97,27 @@ extern QemuIplParameters qipl; >> #define S390_IPL_TYPE_CCW 0x02 >> #define S390_IPL_TYPE_QEMU_SCSI 0xff >> >> -static inline bool store_iplb(IplParameterBlock *iplb) >> +static inline bool manage_iplb(IplParameterBlock *iplb, bool store) >> { >> register unsigned long addr asm("0") = (unsigned long) iplb; >> register unsigned long rc asm("1") = 0; >> >> asm volatile ("diag %0,%2,0x308\n" >>: "+d" (addr), "+d" (rc) >> - : "d" (6) >> + : "d" (store ? 6 : 5) >>: "memory", "cc"); > > I can't find a proper public specification for diag 308, so no clue how > to review this properly. Christian, could you please have a look? > > Thomas > If it helps, here's the respective Linux implementation: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/s390/kernel/ipl.c#n182 -- Regards, Viktor Mihajlovski
Re: [Qemu-devel] [PATCH 3/3] s390: Do not pass inofficial IPL type to the guest
On Fri, 6 Apr 2018 11:28:31 +0200 Thomas Huth wrote: > On 05.04.2018 17:07, Viktor Mihajlovski wrote: > > IPL over a virtio-scsi device requires special handling not > > available in the real architecture. For this purpose the IPL > > type 0xFF has been chosen as means of communication between > > QEMU and the pc-bios. However, a guest OS could be confused > > by seeing an unknown IPL type. > > > > This change sets the IPL parameter type to 0x02 (CCW) to prevent > > this. Pre-existing Linux has looked up the IPL parameters only in > > the case of FCP IPL. This means that the behavior should stay > > the same even if Linux checks for the IPL type unconditionally. > > > > Signed-off-by: Viktor Mihajlovski > > --- > > pc-bios/s390-ccw/bootmap.c | 7 +++ > > pc-bios/s390-ccw/iplb.h| 15 +-- > > 2 files changed, 20 insertions(+), 2 deletions(-) > > > > diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c > > index fc2a9fe..9287b7a 100644 > > --- a/pc-bios/s390-ccw/bootmap.c > > +++ b/pc-bios/s390-ccw/bootmap.c > > @@ -70,6 +70,13 @@ static void jump_to_IPL_code(uint64_t address) > > { > > /* store the subsystem information _after_ the bootmap was loaded */ > > write_subsystem_identification(); > > + > > +/* prevent unknown IPL types in the guest */ > > +if (iplb.pbt == S390_IPL_TYPE_QEMU_SCSI) { > > +iplb.pbt = S390_IPL_TYPE_CCW; > > +set_iplb(&iplb); > > +} > > + > > /* > > * The IPL PSW is at address 0. We also must not overwrite the > > * content of non-BIOS memory after we loaded the guest, so we > > diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h > > index 7dfce4f..5357a36 100644 > > --- a/pc-bios/s390-ccw/iplb.h > > +++ b/pc-bios/s390-ccw/iplb.h > > @@ -97,16 +97,27 @@ extern QemuIplParameters qipl; > > #define S390_IPL_TYPE_CCW 0x02 > > #define S390_IPL_TYPE_QEMU_SCSI 0xff > > > > -static inline bool store_iplb(IplParameterBlock *iplb) > > +static inline bool manage_iplb(IplParameterBlock *iplb, bool store) > > { > > register unsigned long addr asm("0") = (unsigned long) iplb; > > register unsigned long rc asm("1") = 0; > > > > asm volatile ("diag %0,%2,0x308\n" > >: "+d" (addr), "+d" (rc) > > - : "d" (6) > > + : "d" (store ? 6 : 5) > >: "memory", "cc"); > > I can't find a proper public specification for diag 308, so no clue how > to review this properly. Christian, could you please have a look? It does match the diag 308 store/load implementation in target/s390x/diag.c... but a pointer to an official spec (if public) would still be appreciated :)
Re: [Qemu-devel] [PATCH 3/3] s390: Do not pass inofficial IPL type to the guest
On 05.04.2018 17:07, Viktor Mihajlovski wrote: > IPL over a virtio-scsi device requires special handling not > available in the real architecture. For this purpose the IPL > type 0xFF has been chosen as means of communication between > QEMU and the pc-bios. However, a guest OS could be confused > by seeing an unknown IPL type. > > This change sets the IPL parameter type to 0x02 (CCW) to prevent > this. Pre-existing Linux has looked up the IPL parameters only in > the case of FCP IPL. This means that the behavior should stay > the same even if Linux checks for the IPL type unconditionally. > > Signed-off-by: Viktor Mihajlovski > --- > pc-bios/s390-ccw/bootmap.c | 7 +++ > pc-bios/s390-ccw/iplb.h| 15 +-- > 2 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c > index fc2a9fe..9287b7a 100644 > --- a/pc-bios/s390-ccw/bootmap.c > +++ b/pc-bios/s390-ccw/bootmap.c > @@ -70,6 +70,13 @@ static void jump_to_IPL_code(uint64_t address) > { > /* store the subsystem information _after_ the bootmap was loaded */ > write_subsystem_identification(); > + > +/* prevent unknown IPL types in the guest */ > +if (iplb.pbt == S390_IPL_TYPE_QEMU_SCSI) { > +iplb.pbt = S390_IPL_TYPE_CCW; > +set_iplb(&iplb); > +} > + > /* > * The IPL PSW is at address 0. We also must not overwrite the > * content of non-BIOS memory after we loaded the guest, so we > diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h > index 7dfce4f..5357a36 100644 > --- a/pc-bios/s390-ccw/iplb.h > +++ b/pc-bios/s390-ccw/iplb.h > @@ -97,16 +97,27 @@ extern QemuIplParameters qipl; > #define S390_IPL_TYPE_CCW 0x02 > #define S390_IPL_TYPE_QEMU_SCSI 0xff > > -static inline bool store_iplb(IplParameterBlock *iplb) > +static inline bool manage_iplb(IplParameterBlock *iplb, bool store) > { > register unsigned long addr asm("0") = (unsigned long) iplb; > register unsigned long rc asm("1") = 0; > > asm volatile ("diag %0,%2,0x308\n" >: "+d" (addr), "+d" (rc) > - : "d" (6) > + : "d" (store ? 6 : 5) >: "memory", "cc"); I can't find a proper public specification for diag 308, so no clue how to review this properly. Christian, could you please have a look? Thomas
Re: [Qemu-devel] [PATCH 3/3] s390: Do not pass inofficial IPL type to the guest
On 05.04.2018 17:11, David Hildenbrand wrote: > On 05.04.2018 17:07, Viktor Mihajlovski wrote: >> IPL over a virtio-scsi device requires special handling not >> available in the real architecture. For this purpose the IPL >> type 0xFF has been chosen as means of communication between >> QEMU and the pc-bios. However, a guest OS could be confused >> by seeing an unknown IPL type. >> >> This change sets the IPL parameter type to 0x02 (CCW) to prevent >> this. Pre-existing Linux has looked up the IPL parameters only in >> the case of FCP IPL. This means that the behavior should stay >> the same even if Linux checks for the IPL type unconditionally. >> >> Signed-off-by: Viktor Mihajlovski >> --- >> pc-bios/s390-ccw/bootmap.c | 7 +++ >> pc-bios/s390-ccw/iplb.h| 15 +-- >> 2 files changed, 20 insertions(+), 2 deletions(-) >> >> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c >> index fc2a9fe..9287b7a 100644 >> --- a/pc-bios/s390-ccw/bootmap.c >> +++ b/pc-bios/s390-ccw/bootmap.c >> @@ -70,6 +70,13 @@ static void jump_to_IPL_code(uint64_t address) >> { >> /* store the subsystem information _after_ the bootmap was loaded */ >> write_subsystem_identification(); >> + >> +/* prevent unknown IPL types in the guest */ >> +if (iplb.pbt == S390_IPL_TYPE_QEMU_SCSI) { >> +iplb.pbt = S390_IPL_TYPE_CCW; >> +set_iplb(&iplb); >> +} > > Confused, doesn't this imply that a system reset immediately after this > instruction will result in something different getting booted? > Not if the other (QEMU) patches of this series are applied. Without them, the behavior is the same as with an re-ipl (Today's Linux will always request a CCW re-ipl). [...] -- Regards, Viktor Mihajlovski
Re: [Qemu-devel] [PATCH 3/3] s390: Do not pass inofficial IPL type to the guest
On 05.04.2018 17:07, Viktor Mihajlovski wrote: > IPL over a virtio-scsi device requires special handling not > available in the real architecture. For this purpose the IPL > type 0xFF has been chosen as means of communication between > QEMU and the pc-bios. However, a guest OS could be confused > by seeing an unknown IPL type. > > This change sets the IPL parameter type to 0x02 (CCW) to prevent > this. Pre-existing Linux has looked up the IPL parameters only in > the case of FCP IPL. This means that the behavior should stay > the same even if Linux checks for the IPL type unconditionally. > > Signed-off-by: Viktor Mihajlovski > --- > pc-bios/s390-ccw/bootmap.c | 7 +++ > pc-bios/s390-ccw/iplb.h| 15 +-- > 2 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c > index fc2a9fe..9287b7a 100644 > --- a/pc-bios/s390-ccw/bootmap.c > +++ b/pc-bios/s390-ccw/bootmap.c > @@ -70,6 +70,13 @@ static void jump_to_IPL_code(uint64_t address) > { > /* store the subsystem information _after_ the bootmap was loaded */ > write_subsystem_identification(); > + > +/* prevent unknown IPL types in the guest */ > +if (iplb.pbt == S390_IPL_TYPE_QEMU_SCSI) { > +iplb.pbt = S390_IPL_TYPE_CCW; > +set_iplb(&iplb); > +} Confused, doesn't this imply that a system reset immediately after this instruction will result in something different getting booted? > + > /* > * The IPL PSW is at address 0. We also must not overwrite the > * content of non-BIOS memory after we loaded the guest, so we > diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h > index 7dfce4f..5357a36 100644 > --- a/pc-bios/s390-ccw/iplb.h > +++ b/pc-bios/s390-ccw/iplb.h > @@ -97,16 +97,27 @@ extern QemuIplParameters qipl; > #define S390_IPL_TYPE_CCW 0x02 > #define S390_IPL_TYPE_QEMU_SCSI 0xff > > -static inline bool store_iplb(IplParameterBlock *iplb) > +static inline bool manage_iplb(IplParameterBlock *iplb, bool store) > { > register unsigned long addr asm("0") = (unsigned long) iplb; > register unsigned long rc asm("1") = 0; > > asm volatile ("diag %0,%2,0x308\n" >: "+d" (addr), "+d" (rc) > - : "d" (6) > + : "d" (store ? 6 : 5) >: "memory", "cc"); > return rc == 0x01; > } > > + > +static inline bool store_iplb(IplParameterBlock *iplb) > +{ > +return manage_iplb(iplb, true); > +} > + > +static inline bool set_iplb(IplParameterBlock *iplb) > +{ > +return manage_iplb(iplb, false); > +} > + > #endif /* IPLB_H */ > -- Thanks, David / dhildenb