[Qemu-devel] [PATCH 0/3] s390: Fix virtio-scsi IPL quirks

[Qemu-devel] [PATCH 0/3] s390: Fix virtio-scsi IPL quirks

[Viktor Mihajlovski]

IPL from virtio-scsi currently uses a non-standard parameter
type definition to pass boot parameters from QEMU to the
BIOS.

There are two potential issues with this approach:
o If the guest operating systems requests a re-ipl of type CCW
  where the boot device is a virtio-scsi HBA, this goes unnoticed
  by QEMU. The BIOS will detect that it's IPLing from a SCSI
  device, but it will boot the first LUN found, which might not
  be the one used for the initial boot.
o The guest operating system can be confused by an unknown
  IPL parameter block type. If the OS hasn't previously used 
  diag308 to store the IPL info but is changed to do so, a
  user-observable change in behavior will happen.

The following patches address the issues above. 

Viktor Mihajlovski (3):
  s390: Refactor IPL parameter block generation
  s390: Ensure IPL from SCSI works as expected
  s390: Do not pass inofficial IPL type to the guest

 hw/s390x/ipl.c             | 112 ++++++++++++++++++++++++++++++++-------------
 pc-bios/s390-ccw/bootmap.c |   7 +++
 pc-bios/s390-ccw/iplb.h    |  15 +++++-
 3 files changed, 100 insertions(+), 34 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/3] s390: Refactor IPL parameter block generation

[Viktor Mihajlovski]

Splitting out the the CCW device extraction allows reuse.

Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
---
 hw/s390x/ipl.c | 81 ++++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 51 insertions(+), 30 deletions(-)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index fdeaec3..58e33c5 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -279,44 +279,52 @@ static void s390_ipl_set_boot_menu(S390IPLState *ipl)
     *timeout = cpu_to_be32(splash_time);
 }
 
+static CcwDevice *s390_get_ccw_device(DeviceState *dev_st)
+{
+    CcwDevice *ccw_dev = NULL;
+
+    if (dev_st) {
+        VirtioCcwDevice *virtio_ccw_dev = (VirtioCcwDevice *)
+            object_dynamic_cast(OBJECT(qdev_get_parent_bus(dev_st)->parent),
+                                TYPE_VIRTIO_CCW_DEVICE);
+        if (virtio_ccw_dev) {
+            ccw_dev = CCW_DEVICE(virtio_ccw_dev);
+        } else {
+            SCSIDevice *sd = (SCSIDevice *)
+                object_dynamic_cast(OBJECT(dev_st),
+                                    TYPE_SCSI_DEVICE);
+            if (sd) {
+                SCSIBus *bus = scsi_bus_from_device(sd);
+                VirtIOSCSI *vdev = container_of(bus, VirtIOSCSI, bus);
+                VirtIOSCSICcw *scsi_ccw = container_of(vdev, VirtIOSCSICcw,
+                                                       vdev);
+
+                ccw_dev = (CcwDevice *)object_dynamic_cast(OBJECT(scsi_ccw),
+                                                           TYPE_CCW_DEVICE);
+            }
+        }
+    }
+    return ccw_dev;
+}
+
 static bool s390_gen_initial_iplb(S390IPLState *ipl)
 {
     DeviceState *dev_st;
+    CcwDevice *ccw_dev = NULL;
 
     dev_st = get_boot_device(0);
     if (dev_st) {
-        VirtioCcwDevice *virtio_ccw_dev = (VirtioCcwDevice *)
-            object_dynamic_cast(OBJECT(qdev_get_parent_bus(dev_st)->parent),
-                TYPE_VIRTIO_CCW_DEVICE);
+        ccw_dev = s390_get_ccw_device(dev_st);
+    }
+
+    /*
+     * Currently allow IPL only from CCW devices.
+     */
+    if (ccw_dev) {
         SCSIDevice *sd = (SCSIDevice *) object_dynamic_cast(OBJECT(dev_st),
                                                             TYPE_SCSI_DEVICE);
-        VirtIONet *vn = (VirtIONet *) object_dynamic_cast(OBJECT(dev_st),
-                                                          TYPE_VIRTIO_NET);
-
-        if (vn) {
-            ipl->netboot = true;
-        }
-        if (virtio_ccw_dev) {
-            CcwDevice *ccw_dev = CCW_DEVICE(virtio_ccw_dev);
-
-            ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_CCW_LEN);
-            ipl->iplb.blk0_len =
-                cpu_to_be32(S390_IPLB_MIN_CCW_LEN - S390_IPLB_HEADER_LEN);
-            ipl->iplb.pbt = S390_IPL_TYPE_CCW;
-            ipl->iplb.ccw.devno = cpu_to_be16(ccw_dev->sch->devno);
-            ipl->iplb.ccw.ssid = ccw_dev->sch->ssid & 3;
-        } else if (sd) {
-            SCSIBus *bus = scsi_bus_from_device(sd);
-            VirtIOSCSI *vdev = container_of(bus, VirtIOSCSI, bus);
-            VirtIOSCSICcw *scsi_ccw = container_of(vdev, VirtIOSCSICcw, vdev);
-            CcwDevice *ccw_dev;
-
-            ccw_dev = (CcwDevice *)object_dynamic_cast(OBJECT(scsi_ccw),
-                                                       TYPE_CCW_DEVICE);
-            if (!ccw_dev) {       /* It might be a PCI device instead */
-                return false;
-            }
 
+        if (sd) {
             ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN);
             ipl->iplb.blk0_len =
                 cpu_to_be32(S390_IPLB_MIN_QEMU_SCSI_LEN - S390_IPLB_HEADER_LEN);
@@ -327,12 +335,25 @@ static bool s390_gen_initial_iplb(S390IPLState *ipl)
             ipl->iplb.scsi.devno = cpu_to_be16(ccw_dev->sch->devno);
             ipl->iplb.scsi.ssid = ccw_dev->sch->ssid & 3;
         } else {
-            return false; /* unknown device */
+            VirtIONet *vn = (VirtIONet *) object_dynamic_cast(OBJECT(dev_st),
+                                                              TYPE_VIRTIO_NET);
+
+            ipl->iplb.len = cpu_to_be32(S390_IPLB_MIN_CCW_LEN);
+            ipl->iplb.blk0_len =
+                cpu_to_be32(S390_IPLB_MIN_CCW_LEN - S390_IPLB_HEADER_LEN);
+            ipl->iplb.pbt = S390_IPL_TYPE_CCW;
+            ipl->iplb.ccw.devno = cpu_to_be16(ccw_dev->sch->devno);
+            ipl->iplb.ccw.ssid = ccw_dev->sch->ssid & 3;
+
+            if (vn) {
+                ipl->netboot = true;
+            }
         }
 
         if (!s390_ipl_set_loadparm(ipl->iplb.loadparm)) {
             ipl->iplb.flags |= DIAG308_FLAGS_LP_VALID;
         }
+
         return true;
     }
 
-- 
1.9.1

[Qemu-devel] [PATCH 2/3] s390: Ensure IPL from SCSI works as expected

[Viktor Mihajlovski]

Operating systems may request an IPL from a virtio-scsi device
by specifying an IPL parameter type of CCW. In this case QEMU
won't set up the IPLB correctly. The BIOS will still detect
it's a SCSI device to boot from, but it will now have to search
for the first LUN and attempt to boot from there.
However this may not be the original boot LUN if there's more than
one SCSI disk attached to the HBA.

With this change QEMU will detect that the request is for a
SCSI device and will rebuild the initial IPL parameter info
if it's the SCSI device used for the first boot. In consequence
the BIOS can use the boot LUN from the IPL information block.

In case a different SCSI device has been set, the BIOS will find
and use the first available LUN.

Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
---
 hw/s390x/ipl.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 58e33c5..fb554ab 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -427,7 +427,8 @@ unref_mr:
     return img_size;
 }
 
-static bool is_virtio_net_device(IplParameterBlock *iplb)
+static bool is_virtio_ccw_device_of_type(IplParameterBlock *iplb,
+                                         int virtio_id)
 {
     uint8_t cssid;
     uint8_t ssid;
@@ -447,13 +448,23 @@ static bool is_virtio_net_device(IplParameterBlock *iplb)
             sch = css_find_subch(1, cssid, ssid, schid);
 
             if (sch && sch->devno == devno) {
-                return sch->id.cu_model == VIRTIO_ID_NET;
+                return sch->id.cu_model == virtio_id;
             }
         }
     }
     return false;
 }
 
+static bool is_virtio_net_device(IplParameterBlock *iplb)
+{
+    return is_virtio_ccw_device_of_type(iplb, VIRTIO_ID_NET);
+}
+
+static bool is_virtio_scsi_device(IplParameterBlock *iplb)
+{
+    return is_virtio_ccw_device_of_type(iplb, VIRTIO_ID_SCSI);
+}
+
 void s390_ipl_update_diag308(IplParameterBlock *iplb)
 {
     S390IPLState *ipl = get_ipl_device();
@@ -478,6 +489,22 @@ void s390_reipl_request(void)
     S390IPLState *ipl = get_ipl_device();
 
     ipl->reipl_requested = true;
+    if (ipl->iplb_valid &&
+        !ipl->netboot &&
+        ipl->iplb.pbt == S390_IPL_TYPE_CCW &&
+        is_virtio_scsi_device(&ipl->iplb)) {
+        CcwDevice *ccw_dev = s390_get_ccw_device(get_boot_device(0));
+
+        if (ccw_dev &&
+            cpu_to_be16(ccw_dev->sch->devno) == ipl->iplb.ccw.devno &&
+            (ccw_dev->sch->ssid & 3) == ipl->iplb.ccw.ssid) {
+            /*
+             * this is the original boot device's SCSI
+             * so restore IPL parameter info from it
+             */
+            ipl->iplb_valid = s390_gen_initial_iplb(ipl);
+        }
+    }
     qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
 }
 
-- 
1.9.1

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

[Viktor Mihajlovski]

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

[David Hildenbrand]

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

[Viktor VM Mihajlovski]

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 1/3] s390: Refactor IPL parameter block generation

[Farhan Ali]

On 04/05/2018 11:07 AM, Viktor Mihajlovski wrote:
> Splitting out the the CCW device extraction allows reuse.
> 
> Signed-off-by: Viktor Mihajlovski<mihajlov@linux.vnet.ibm.com>
> ---
>   hw/s390x/ipl.c | 81 ++++++++++++++++++++++++++++++++++++----------------------
>   1 file changed, 51 insertions(+), 30 deletions(-)

Reviewed-by: Farhan Ali <alifm@linux.vnet.ibm.com>

Re: [Qemu-devel] [PATCH 2/3] s390: Ensure IPL from SCSI works as expected

[Farhan Ali]

On 04/05/2018 11:07 AM, Viktor Mihajlovski wrote:
> Operating systems may request an IPL from a virtio-scsi device
> by specifying an IPL parameter type of CCW. In this case QEMU
> won't set up the IPLB correctly. The BIOS will still detect
> it's a SCSI device to boot from, but it will now have to search
> for the first LUN and attempt to boot from there.
> However this may not be the original boot LUN if there's more than
> one SCSI disk attached to the HBA.
> 
> With this change QEMU will detect that the request is for a
> SCSI device and will rebuild the initial IPL parameter info
> if it's the SCSI device used for the first boot. In consequence
> the BIOS can use the boot LUN from the IPL information block.
> 
> In case a different SCSI device has been set, the BIOS will find
> and use the first available LUN.
> 
> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> ---
>   hw/s390x/ipl.c | 31 +++++++++++++++++++++++++++++--
>   1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 58e33c5..fb554ab 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -427,7 +427,8 @@ unref_mr:
>       return img_size;
>   }
> 
> -static bool is_virtio_net_device(IplParameterBlock *iplb)
> +static bool is_virtio_ccw_device_of_type(IplParameterBlock *iplb,
> +                                         int virtio_id)
>   {
>       uint8_t cssid;
>       uint8_t ssid;
> @@ -447,13 +448,23 @@ static bool is_virtio_net_device(IplParameterBlock *iplb)
>               sch = css_find_subch(1, cssid, ssid, schid);
> 
>               if (sch && sch->devno == devno) {
> -                return sch->id.cu_model == VIRTIO_ID_NET;
> +                return sch->id.cu_model == virtio_id;
>               }
>           }
>       }
>       return false;
>   }
> 
> +static bool is_virtio_net_device(IplParameterBlock *iplb)
> +{
> +    return is_virtio_ccw_device_of_type(iplb, VIRTIO_ID_NET);
> +}
> +
> +static bool is_virtio_scsi_device(IplParameterBlock *iplb)
> +{
> +    return is_virtio_ccw_device_of_type(iplb, VIRTIO_ID_SCSI);
> +}
> +
>   void s390_ipl_update_diag308(IplParameterBlock *iplb)
>   {
>       S390IPLState *ipl = get_ipl_device();
> @@ -478,6 +489,22 @@ void s390_reipl_request(void)
>       S390IPLState *ipl = get_ipl_device();
> 
>       ipl->reipl_requested = true;
> +    if (ipl->iplb_valid &&
> +        !ipl->netboot &&
> +        ipl->iplb.pbt == S390_IPL_TYPE_CCW &&
> +        is_virtio_scsi_device(&ipl->iplb)) {
> +        CcwDevice *ccw_dev = s390_get_ccw_device(get_boot_device(0));
> +
> +        if (ccw_dev &&
> +            cpu_to_be16(ccw_dev->sch->devno) == ipl->iplb.ccw.devno &&
> +            (ccw_dev->sch->ssid & 3) == ipl->iplb.ccw.ssid) {
> +            /*
> +             * this is the original boot device's SCSI
> +             * so restore IPL parameter info from it
> +             */
> +            ipl->iplb_valid = s390_gen_initial_iplb(ipl);
> +        }
> +    }
>       qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>   }
> 
This does feel a little hackish, but I can't think of a better way to 
ensure the correct SCSI device is used for booting.

Reviewed-by: Farhan Ali <alifm@linux.vnet.ibm.com>

Re: [Qemu-devel] [PATCH 1/3] s390: Refactor IPL parameter block generation

[Thomas Huth]

On 05.04.2018 17:07, Viktor Mihajlovski wrote:
> Splitting out the the CCW device extraction allows reuse.
> 
> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> ---
>  hw/s390x/ipl.c | 81 ++++++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 51 insertions(+), 30 deletions(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index fdeaec3..58e33c5 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -279,44 +279,52 @@ static void s390_ipl_set_boot_menu(S390IPLState *ipl)
>      *timeout = cpu_to_be32(splash_time);
>  }
>  
> +static CcwDevice *s390_get_ccw_device(DeviceState *dev_st)
> +{
> +    CcwDevice *ccw_dev = NULL;
> +
> +    if (dev_st) {
> +        VirtioCcwDevice *virtio_ccw_dev = (VirtioCcwDevice *)
> +            object_dynamic_cast(OBJECT(qdev_get_parent_bus(dev_st)->parent),
> +                                TYPE_VIRTIO_CCW_DEVICE);
> +        if (virtio_ccw_dev) {
> +            ccw_dev = CCW_DEVICE(virtio_ccw_dev);
> +        } else {
> +            SCSIDevice *sd = (SCSIDevice *)
> +                object_dynamic_cast(OBJECT(dev_st),
> +                                    TYPE_SCSI_DEVICE);
> +            if (sd) {
> +                SCSIBus *bus = scsi_bus_from_device(sd);
> +                VirtIOSCSI *vdev = container_of(bus, VirtIOSCSI, bus);
> +                VirtIOSCSICcw *scsi_ccw = container_of(vdev, VirtIOSCSICcw,
> +                                                       vdev);
> +
> +                ccw_dev = (CcwDevice *)object_dynamic_cast(OBJECT(scsi_ccw),
> +                                                           TYPE_CCW_DEVICE);
> +            }
> +        }
> +    }
> +    return ccw_dev;
> +}
> +
>  static bool s390_gen_initial_iplb(S390IPLState *ipl)
>  {
>      DeviceState *dev_st;
> +    CcwDevice *ccw_dev = NULL;
>  
>      dev_st = get_boot_device(0);
>      if (dev_st) {
> -        VirtioCcwDevice *virtio_ccw_dev = (VirtioCcwDevice *)
> -            object_dynamic_cast(OBJECT(qdev_get_parent_bus(dev_st)->parent),
> -                TYPE_VIRTIO_CCW_DEVICE);
> +        ccw_dev = s390_get_ccw_device(dev_st);
> +    }
> +
> +    /*
> +     * Currently allow IPL only from CCW devices.
> +     */
> +    if (ccw_dev) {
>          SCSIDevice *sd = (SCSIDevice *) object_dynamic_cast(OBJECT(dev_st),
>                                                              TYPE_SCSI_DEVICE);

The SCSIDevice dynamic cast now has to be done twice ... but I guess
that's ok (we're only doing this for the boot device, so it this should
not be time-critical, right?).

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

Re: [Qemu-devel] [PATCH 2/3] s390: Ensure IPL from SCSI works as expected

[Thomas Huth]

On 05.04.2018 17:07, Viktor Mihajlovski wrote:
> Operating systems may request an IPL from a virtio-scsi device
> by specifying an IPL parameter type of CCW. In this case QEMU
> won't set up the IPLB correctly. The BIOS will still detect
> it's a SCSI device to boot from, but it will now have to search
> for the first LUN and attempt to boot from there.
> However this may not be the original boot LUN if there's more than
> one SCSI disk attached to the HBA.
> 
> With this change QEMU will detect that the request is for a
> SCSI device and will rebuild the initial IPL parameter info
> if it's the SCSI device used for the first boot. In consequence
> the BIOS can use the boot LUN from the IPL information block.
> 
> In case a different SCSI device has been set, the BIOS will find
> and use the first available LUN.
> 
> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> ---
>  hw/s390x/ipl.c | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 58e33c5..fb554ab 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -427,7 +427,8 @@ unref_mr:
>      return img_size;
>  }
>  
> -static bool is_virtio_net_device(IplParameterBlock *iplb)
> +static bool is_virtio_ccw_device_of_type(IplParameterBlock *iplb,
> +                                         int virtio_id)
>  {
>      uint8_t cssid;
>      uint8_t ssid;
> @@ -447,13 +448,23 @@ static bool is_virtio_net_device(IplParameterBlock *iplb)
>              sch = css_find_subch(1, cssid, ssid, schid);
>  
>              if (sch && sch->devno == devno) {
> -                return sch->id.cu_model == VIRTIO_ID_NET;
> +                return sch->id.cu_model == virtio_id;
>              }
>          }
>      }
>      return false;
>  }
>  
> +static bool is_virtio_net_device(IplParameterBlock *iplb)
> +{
> +    return is_virtio_ccw_device_of_type(iplb, VIRTIO_ID_NET);
> +}
> +
> +static bool is_virtio_scsi_device(IplParameterBlock *iplb)
> +{
> +    return is_virtio_ccw_device_of_type(iplb, VIRTIO_ID_SCSI);
> +}

Not sure whether we need a separate wrapper function for this ... using
is_virtio_ccw_device_of_type(iplb, VIRTIO_ID_SCSI) in place would be
fine, too, I think.

>  void s390_ipl_update_diag308(IplParameterBlock *iplb)
>  {
>      S390IPLState *ipl = get_ipl_device();
> @@ -478,6 +489,22 @@ void s390_reipl_request(void)
>      S390IPLState *ipl = get_ipl_device();
>  
>      ipl->reipl_requested = true;
> +    if (ipl->iplb_valid &&
> +        !ipl->netboot &&
> +        ipl->iplb.pbt == S390_IPL_TYPE_CCW &&
> +        is_virtio_scsi_device(&ipl->iplb)) {
> +        CcwDevice *ccw_dev = s390_get_ccw_device(get_boot_device(0));
> +
> +        if (ccw_dev &&
> +            cpu_to_be16(ccw_dev->sch->devno) == ipl->iplb.ccw.devno &&
> +            (ccw_dev->sch->ssid & 3) == ipl->iplb.ccw.ssid) {
> +            /*
> +             * this is the original boot device's SCSI
> +             * so restore IPL parameter info from it
> +             */
> +            ipl->iplb_valid = s390_gen_initial_iplb(ipl);
> +        }
> +    }
>      qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>  }

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

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

[Thomas Huth]

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 1/3] s390: Refactor IPL parameter block generation

[Cornelia Huck]

On Fri, 6 Apr 2018 09:52:35 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 05.04.2018 17:07, Viktor Mihajlovski wrote:
> > Splitting out the the CCW device extraction allows reuse.
> > 
> > Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> > ---
> >  hw/s390x/ipl.c | 81 ++++++++++++++++++++++++++++++++++++----------------------
> >  1 file changed, 51 insertions(+), 30 deletions(-)
> > 
> > diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> > index fdeaec3..58e33c5 100644
> > --- a/hw/s390x/ipl.c
> > +++ b/hw/s390x/ipl.c
> > @@ -279,44 +279,52 @@ static void s390_ipl_set_boot_menu(S390IPLState *ipl)
> >      *timeout = cpu_to_be32(splash_time);
> >  }
> >  
> > +static CcwDevice *s390_get_ccw_device(DeviceState *dev_st)
> > +{
> > +    CcwDevice *ccw_dev = NULL;
> > +
> > +    if (dev_st) {
> > +        VirtioCcwDevice *virtio_ccw_dev = (VirtioCcwDevice *)
> > +            object_dynamic_cast(OBJECT(qdev_get_parent_bus(dev_st)->parent),
> > +                                TYPE_VIRTIO_CCW_DEVICE);
> > +        if (virtio_ccw_dev) {
> > +            ccw_dev = CCW_DEVICE(virtio_ccw_dev);
> > +        } else {
> > +            SCSIDevice *sd = (SCSIDevice *)
> > +                object_dynamic_cast(OBJECT(dev_st),
> > +                                    TYPE_SCSI_DEVICE);
> > +            if (sd) {
> > +                SCSIBus *bus = scsi_bus_from_device(sd);
> > +                VirtIOSCSI *vdev = container_of(bus, VirtIOSCSI, bus);
> > +                VirtIOSCSICcw *scsi_ccw = container_of(vdev, VirtIOSCSICcw,
> > +                                                       vdev);
> > +
> > +                ccw_dev = (CcwDevice *)object_dynamic_cast(OBJECT(scsi_ccw),
> > +                                                           TYPE_CCW_DEVICE);
> > +            }
> > +        }
> > +    }
> > +    return ccw_dev;
> > +}
> > +
> >  static bool s390_gen_initial_iplb(S390IPLState *ipl)
> >  {
> >      DeviceState *dev_st;
> > +    CcwDevice *ccw_dev = NULL;
> >  
> >      dev_st = get_boot_device(0);
> >      if (dev_st) {
> > -        VirtioCcwDevice *virtio_ccw_dev = (VirtioCcwDevice *)
> > -            object_dynamic_cast(OBJECT(qdev_get_parent_bus(dev_st)->parent),
> > -                TYPE_VIRTIO_CCW_DEVICE);
> > +        ccw_dev = s390_get_ccw_device(dev_st);
> > +    }
> > +
> > +    /*
> > +     * Currently allow IPL only from CCW devices.
> > +     */
> > +    if (ccw_dev) {
> >          SCSIDevice *sd = (SCSIDevice *) object_dynamic_cast(OBJECT(dev_st),
> >                                                              TYPE_SCSI_DEVICE);  
> 
> The SCSIDevice dynamic cast now has to be done twice ... but I guess
> that's ok (we're only doing this for the boot device, so it this should
> not be time-critical, right?).

Agreed, that does not matter.

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

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

[Cornelia Huck]

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 0/3] s390: Fix virtio-scsi IPL quirks

[Cornelia Huck]

On Thu,  5 Apr 2018 17:07:21 +0200
Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote:

> IPL from virtio-scsi currently uses a non-standard parameter
> type definition to pass boot parameters from QEMU to the
> BIOS.
> 
> There are two potential issues with this approach:
> o If the guest operating systems requests a re-ipl of type CCW
>   where the boot device is a virtio-scsi HBA, this goes unnoticed
>   by QEMU. The BIOS will detect that it's IPLing from a SCSI
>   device, but it will boot the first LUN found, which might not
>   be the one used for the initial boot.
> o The guest operating system can be confused by an unknown
>   IPL parameter block type. If the OS hasn't previously used 
>   diag308 to store the IPL info but is changed to do so, a
>   user-observable change in behavior will happen.
> 
> The following patches address the issues above. 
> 
> Viktor Mihajlovski (3):
>   s390: Refactor IPL parameter block generation
>   s390: Ensure IPL from SCSI works as expected
>   s390: Do not pass inofficial IPL type to the guest
> 
>  hw/s390x/ipl.c             | 112 ++++++++++++++++++++++++++++++++-------------
>  pc-bios/s390-ccw/bootmap.c |   7 +++
>  pc-bios/s390-ccw/iplb.h    |  15 +++++-
>  3 files changed, 100 insertions(+), 34 deletions(-)
> 

This looks reasonable enough to queue for 2.12 (with a bios rebuild),
especially as I also plan to queue the cpu_synchronize_state() patch.
Just waiting for a R-b on the bios part.

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

[Viktor VM Mihajlovski]

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

[Christian Borntraeger]

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

Re: [Qemu-devel] [PATCH 0/3] s390: Fix virtio-scsi IPL quirks

[Christian Borntraeger]

On 04/05/2018 05:07 PM, Viktor Mihajlovski wrote:
> IPL from virtio-scsi currently uses a non-standard parameter
> type definition to pass boot parameters from QEMU to the
> BIOS.
> 
> There are two potential issues with this approach:
> o If the guest operating systems requests a re-ipl of type CCW
>   where the boot device is a virtio-scsi HBA, this goes unnoticed
>   by QEMU. The BIOS will detect that it's IPLing from a SCSI
>   device, but it will boot the first LUN found, which might not
>   be the one used for the initial boot.
> o The guest operating system can be confused by an unknown
>   IPL parameter block type. If the OS hasn't previously used 
>   diag308 to store the IPL info but is changed to do so, a
>   user-observable change in behavior will happen.
> 
> The following patches address the issues above. 
> 
> Viktor Mihajlovski (3):
>   s390: Refactor IPL parameter block generation
>   s390: Ensure IPL from SCSI works as expected
>   s390: Do not pass inofficial IPL type to the guest
> 
>  hw/s390x/ipl.c             | 112 ++++++++++++++++++++++++++++++++-------------
>  pc-bios/s390-ccw/bootmap.c |   7 +++
>  pc-bios/s390-ccw/iplb.h    |  15 +++++-
>  3 files changed, 100 insertions(+), 34 deletions(-)
> 

Test looks good so far.

Re: [Qemu-devel] [PATCH 0/3] s390: Fix virtio-scsi IPL quirks

[Cornelia Huck]

On Fri, 6 Apr 2018 14:30:45 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 04/05/2018 05:07 PM, Viktor Mihajlovski wrote:
> > IPL from virtio-scsi currently uses a non-standard parameter
> > type definition to pass boot parameters from QEMU to the
> > BIOS.
> > 
> > There are two potential issues with this approach:
> > o If the guest operating systems requests a re-ipl of type CCW
> >   where the boot device is a virtio-scsi HBA, this goes unnoticed
> >   by QEMU. The BIOS will detect that it's IPLing from a SCSI
> >   device, but it will boot the first LUN found, which might not
> >   be the one used for the initial boot.
> > o The guest operating system can be confused by an unknown
> >   IPL parameter block type. If the OS hasn't previously used 
> >   diag308 to store the IPL info but is changed to do so, a
> >   user-observable change in behavior will happen.
> > 
> > The following patches address the issues above. 
> > 
> > Viktor Mihajlovski (3):
> >   s390: Refactor IPL parameter block generation
> >   s390: Ensure IPL from SCSI works as expected
> >   s390: Do not pass inofficial IPL type to the guest
> > 
> >  hw/s390x/ipl.c             | 112 ++++++++++++++++++++++++++++++++-------------
> >  pc-bios/s390-ccw/bootmap.c |   7 +++
> >  pc-bios/s390-ccw/iplb.h    |  15 +++++-
> >  3 files changed, 100 insertions(+), 34 deletions(-)
> >   
> 
> Test looks good so far.

OK, I'll queue this to s390-fixes, then. I won't send a pull request
before Monday, so there's still a chance to fix this should problems
show up.

Thanks for testing!