1. Patchew
  2. QEMU
  3. [Qemu-devel] [PATCH 0/3] s390: Fix virtio-scsi IPL quirks
  4. View patch

[Qemu-devel] [PATCH 3/3] s390: Do not pass inofficial IPL type to the guest

Viktor Mihajlovski posted 3 patches 7 years, 10 months ago
[Qemu-devel] [PATCH 3/3] s390: Do not pass inofficial IPL type to the guest
Posted by Viktor Mihajlovski 7 years, 10 months ago 
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 <mihajlov@linux.vnet.ibm.com>
---
 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 */
-- 
1.9.1
Re: [Qemu-devel] [PATCH 3/3] s390: Do not pass inofficial IPL type to the guest
Posted by David Hildenbrand 7 years, 10 months ago 
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 <mihajlov@linux.vnet.ibm.com>
> ---
>  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
Re: [Qemu-devel] [PATCH 3/3] s390: Do not pass inofficial IPL type to the guest
Posted by Viktor VM Mihajlovski 7 years, 10 months ago 
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 <mihajlov@linux.vnet.ibm.com>
>> ---
>>  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
Posted by Thomas Huth 7 years, 10 months ago 
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 <mihajlov@linux.vnet.ibm.com>
> ---
>  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
Posted by Cornelia Huck 7 years, 10 months ago 
On Fri, 6 Apr 2018 11:28:31 +0200
Thomas Huth <thuth@redhat.com> 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 <mihajlov@linux.vnet.ibm.com>
> > ---
> >  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
Posted by Viktor VM Mihajlovski 7 years, 10 months ago 
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 <mihajlov@linux.vnet.ibm.com>
>> ---
>>  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
Posted by Christian Borntraeger 7 years, 10 months ago 

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 <mihajlov@linux.vnet.ibm.com>

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

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 */
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.