[Qemu-devel] [PATCH 0/2] ipl related fixes

[Qemu-devel] [PATCH 0/2] ipl related fixes

[Christian Borntraeger]

Alex, here are two fixes for the ccw ipl. The first fix deals
with a bug that creeped in when merging with the refactoring
work, the other fix allows booting from a subchannel set > 0.

I will let you do the binary of the bios code yourself.

Christian Borntraeger (1):
  s390/ipl: Fix boot order

Dominik Dingel (1):
  s390/IPL: Allow boot from other ssid than 0

 hw/s390x/ipl.c              | 16 +++++++++-------
 pc-bios/s390-ccw/cio.h      | 20 ++++++++++++++++++++
 pc-bios/s390-ccw/main.c     |  7 +++++++
 pc-bios/s390-ccw/s390-ccw.h |  1 +
 pc-bios/s390-ccw/virtio.c   | 18 ++++++++++++++++++
 5 files changed, 55 insertions(+), 7 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH 1/2] s390/ipl: Fix boot order

[Christian Borntraeger]

The latest ipl code adoptions collided with some of the virtio
refactoring rework. This resulted in always booting the first
disk. Lets fix booting from a given ID.
The new code also checks for command lines without bootindex to
avoid random behaviour when accessing dev_st (==0).

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
CC: qemu-stable@nongnu.org
---
 hw/s390x/ipl.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 0aeb003..8b25b1c 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -156,13 +156,15 @@ static void s390_ipl_reset(DeviceState *dev)
     if (!ipl->kernel) {
         /* booting firmware, tell what device to boot from */
         DeviceState *dev_st = get_boot_device(0);
-        VirtioCcwDevice *ccw_dev = (VirtioCcwDevice *) object_dynamic_cast(
-                OBJECT(&(dev_st->parent_obj)), "virtio-blk-ccw");
-
-        if (ccw_dev) {
-            env->regs[7] = ccw_dev->sch->cssid << 24 |
-                           ccw_dev->sch->ssid << 16 |
-                           ccw_dev->sch->devno;
+        if (dev_st) {
+            VirtioCcwDevice *ccw_dev = (VirtioCcwDevice *) object_dynamic_cast(
+                    OBJECT((dev_st->parent_obj.parent)), "virtio-blk-ccw");
+
+            if (ccw_dev) {
+                env->regs[7] = ccw_dev->sch->cssid << 24 |
+                               ccw_dev->sch->ssid << 16 |
+                               ccw_dev->sch->devno;
+            }
         } else {
             env->regs[7] = -1;
         }
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 1/2] s390/ipl: Fix boot order

[Andreas Färber]

Hi,

Am 17.06.2013 14:29, schrieb Christian Borntraeger:
> The latest ipl code adoptions collided with some of the virtio

"adaptions"?

> refactoring rework. This resulted in always booting the first
> disk. Lets fix booting from a given ID.

"Let's"?

> The new code also checks for command lines without bootindex to
> avoid random behaviour when accessing dev_st (==0).
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> CC: qemu-stable@nongnu.org
> ---
>  hw/s390x/ipl.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 0aeb003..8b25b1c 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -156,13 +156,15 @@ static void s390_ipl_reset(DeviceState *dev)
>      if (!ipl->kernel) {
>          /* booting firmware, tell what device to boot from */
>          DeviceState *dev_st = get_boot_device(0);
> -        VirtioCcwDevice *ccw_dev = (VirtioCcwDevice *) object_dynamic_cast(
> -                OBJECT(&(dev_st->parent_obj)), "virtio-blk-ccw");

This should've never accessed parent_obj but simply use OBJECT(dev_st).

I would expect object_dynamic_cast() to return NULL on NULL input then,
but it seems the issue is rather that dev_st will be the VirtioDevice
and not the VirtIOS390Device.

> -
> -        if (ccw_dev) {
> -            env->regs[7] = ccw_dev->sch->cssid << 24 |
> -                           ccw_dev->sch->ssid << 16 |
> -                           ccw_dev->sch->devno;
> +        if (dev_st) {
> +            VirtioCcwDevice *ccw_dev = (VirtioCcwDevice *) object_dynamic_cast(
> +                    OBJECT((dev_st->parent_obj.parent)), "virtio-blk-ccw");

This is worse and equivalent to OBJECT(OBJECT(dev_st)->parent), with the
outer cast superfluous and parent being an Object-private field
according to include/qom/object.h.

IIRC we had once suggested to introduce an object_get_parent() accessor,
but Anthony was against it for some reason...? CC'ing.

Instead, I believe it would be permissible to access the device's bus,
which in turn has a pointer to its parent device:

OBJECT(qdev_get_parent_bus(dev_st)->parent)

> +
> +            if (ccw_dev) {
> +                env->regs[7] = ccw_dev->sch->cssid << 24 |
> +                               ccw_dev->sch->ssid << 16 |
> +                               ccw_dev->sch->devno;
> +            }

Previously, env->regs[7] would've been assigned -1 for !ccw_dev.
Functional change intentional?

Cheers,
Andreas

>          } else {
>              env->regs[7] = -1;
>          }

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PATCH 1/2] s390/ipl: Fix boot order

[Christian Borntraeger]

The latest ipl code adaptions collided with some of the virtio
refactoring rework. This resulted in always booting the first
disk. Let's fix booting from a given ID.
The new code also checks for command lines without bootindex to
avoid random behaviour when accessing dev_st (==0).

We need to get a VirtioCcwDevice from the device_state (but only 
get a VirtIODevice). An object_get_parent() accessor would be
useful here, but going via the parent_bus should also work.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
CC: qemu-stable@nongnu.org
---
Changelog: 
Version 2: Review feedback from Andreas Faerber regarding parent
accesses, typos and parent_bus.

 hw/s390x/ipl.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 0aeb003..11397ea 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -154,17 +154,18 @@ static void s390_ipl_reset(DeviceState *dev)
     env->psw.mask = IPL_PSW_MASK;
 
     if (!ipl->kernel) {
-        /* booting firmware, tell what device to boot from */
+        /* Tell firmware, if there is a preferred boot device */
+        env->regs[7] = -1;
         DeviceState *dev_st = get_boot_device(0);
-        VirtioCcwDevice *ccw_dev = (VirtioCcwDevice *) object_dynamic_cast(
-                OBJECT(&(dev_st->parent_obj)), "virtio-blk-ccw");
-
-        if (ccw_dev) {
-            env->regs[7] = ccw_dev->sch->cssid << 24 |
-                           ccw_dev->sch->ssid << 16 |
-                           ccw_dev->sch->devno;
-        } else {
-            env->regs[7] = -1;
+        if (dev_st) {
+            VirtioCcwDevice *ccw_dev = VIRTIO_CCW_DEVICE(
+                OBJECT(qdev_get_parent_bus(dev_st)->parent));
+
+            if (ccw_dev) {
+                env->regs[7] = ccw_dev->sch->cssid << 24 |
+                               ccw_dev->sch->ssid << 16 |
+                               ccw_dev->sch->devno;
+            }
         }
     }
 
-- 
1.8.1.4

[Qemu-devel] [PATCH v3] s390/ipl: Fix boot order

[Christian Borntraeger]

The latest ipl code adaptions collided with some of the virtio
refactoring rework. This resulted in always booting the first
disk. Let's fix booting from a given ID.
The new code also checks for command lines without bootindex to
avoid random behaviour when accessing dev_st (==0).

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
CC: qemu-stable@nongnu.org
---
Changelog:

Version 2: Review feedback from Andreas Faerber
Version 3: use object_dynamic_cast to avoid crashes with !ccw

 hw/s390x/ipl.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 0aeb003..d69adb2 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -154,17 +154,19 @@ static void s390_ipl_reset(DeviceState *dev)
     env->psw.mask = IPL_PSW_MASK;
 
     if (!ipl->kernel) {
-        /* booting firmware, tell what device to boot from */
+        /* Tell firmware, if there is a preferred boot device */
+        env->regs[7] = -1;
         DeviceState *dev_st = get_boot_device(0);
-        VirtioCcwDevice *ccw_dev = (VirtioCcwDevice *) object_dynamic_cast(
-                OBJECT(&(dev_st->parent_obj)), "virtio-blk-ccw");
-
-        if (ccw_dev) {
-            env->regs[7] = ccw_dev->sch->cssid << 24 |
-                           ccw_dev->sch->ssid << 16 |
-                           ccw_dev->sch->devno;
-        } else {
-            env->regs[7] = -1;
+        if (dev_st) {
+            VirtioCcwDevice *ccw_dev = (VirtioCcwDevice *) object_dynamic_cast(
+                OBJECT(qdev_get_parent_bus(dev_st)->parent),
+                TYPE_VIRTIO_CCW_DEVICE);
+
+            if (ccw_dev) {
+                env->regs[7] = ccw_dev->sch->cssid << 24 |
+                               ccw_dev->sch->ssid << 16 |
+                               ccw_dev->sch->devno;
+            }
         }
     }
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v3] s390/ipl: Fix boot order

[Andreas Färber]

Am 18.06.2013 14:31, schrieb Christian Borntraeger:
> The latest ipl code adaptions collided with some of the virtio
> refactoring rework. This resulted in always booting the first
> disk. Let's fix booting from a given ID.
> The new code also checks for command lines without bootindex to
> avoid random behaviour when accessing dev_st (==0).
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> CC: qemu-stable@nongnu.org
> ---
> Changelog:
> 
> Version 2: Review feedback from Andreas Faerber
> Version 3: use object_dynamic_cast to avoid crashes with !ccw

Reviewed-by: Andreas Färber <afaerber@suse.de>

Thanks a lot,
Andreas

> 
>  hw/s390x/ipl.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 0aeb003..d69adb2 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -154,17 +154,19 @@ static void s390_ipl_reset(DeviceState *dev)
>      env->psw.mask = IPL_PSW_MASK;
>  
>      if (!ipl->kernel) {
> -        /* booting firmware, tell what device to boot from */
> +        /* Tell firmware, if there is a preferred boot device */
> +        env->regs[7] = -1;
>          DeviceState *dev_st = get_boot_device(0);
> -        VirtioCcwDevice *ccw_dev = (VirtioCcwDevice *) object_dynamic_cast(
> -                OBJECT(&(dev_st->parent_obj)), "virtio-blk-ccw");
> -
> -        if (ccw_dev) {
> -            env->regs[7] = ccw_dev->sch->cssid << 24 |
> -                           ccw_dev->sch->ssid << 16 |
> -                           ccw_dev->sch->devno;
> -        } else {
> -            env->regs[7] = -1;
> +        if (dev_st) {
> +            VirtioCcwDevice *ccw_dev = (VirtioCcwDevice *) object_dynamic_cast(
> +                OBJECT(qdev_get_parent_bus(dev_st)->parent),
> +                TYPE_VIRTIO_CCW_DEVICE);
> +
> +            if (ccw_dev) {
> +                env->regs[7] = ccw_dev->sch->cssid << 24 |
> +                               ccw_dev->sch->ssid << 16 |
> +                               ccw_dev->sch->devno;
> +            }
>          }
>      }
>  
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v3] s390/ipl: Fix boot order

[Alexander Graf]

On 18.06.2013, at 14:31, Christian Borntraeger wrote:

> The latest ipl code adaptions collided with some of the virtio
> refactoring rework. This resulted in always booting the first
> disk. Let's fix booting from a given ID.
> The new code also checks for command lines without bootindex to
> avoid random behaviour when accessing dev_st (==0).
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> CC: qemu-stable@nongnu.org

Thanks, applied to s390-next.


Alex

[Qemu-devel] [PATCH 2/2] s390/IPL: Allow boot from other ssid than 0

[Christian Borntraeger]

From: Dominik Dingel <dingel@linux.vnet.ibm.com>

We now take the subchannel set id also into account to find the boot device.
If we want to use a subchannel set other than the default set 0, we first
need to enable the mss facility.

Signed-off-by: Dominik Dingel <dingel@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 pc-bios/s390-ccw/cio.h      | 20 ++++++++++++++++++++
 pc-bios/s390-ccw/main.c     |  7 +++++++
 pc-bios/s390-ccw/s390-ccw.h |  1 +
 pc-bios/s390-ccw/virtio.c   | 18 ++++++++++++++++++
 4 files changed, 46 insertions(+)

diff --git a/pc-bios/s390-ccw/cio.h b/pc-bios/s390-ccw/cio.h
index cb5815a..f5b4549 100644
--- a/pc-bios/s390-ccw/cio.h
+++ b/pc-bios/s390-ccw/cio.h
@@ -93,6 +93,26 @@ struct subchannel_id {
         __u32 sch_no : 16;
 } __attribute__ ((packed, aligned(4)));
 
+struct chsc_header {
+    __u16 length;
+    __u16 code;
+} __attribute__((packed));
+
+struct chsc_area_sda {
+    struct chsc_header request;
+    __u8 reserved1:4;
+    __u8 format:4;
+    __u8 reserved2;
+    __u16 operation_code;
+    __u32 reserved3;
+    __u32 reserved4;
+    __u32 operation_data_area[252];
+    struct chsc_header response;
+    __u32 reserved5:4;
+    __u32 format2:4;
+    __u32 reserved6:24;
+} __attribute__((packed));
+
 /*
  * TPI info structure
  */
diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
index 1665c57..c5d5332 100644
--- a/pc-bios/s390-ccw/main.c
+++ b/pc-bios/s390-ccw/main.c
@@ -35,6 +35,13 @@ static void virtio_setup(uint64_t dev_info)
         check_devno = true;
         dev_no = dev_info & 0xffff;
         debug_print_int("device no. ", dev_no);
+        blk_schid.ssid = (dev_info >> 16) & 0x3;
+        if (blk_schid.ssid != 0) {
+            debug_print_int("ssid ", blk_schid.ssid);
+            if (enable_mss_facility() != 0) {
+                virtio_panic("Failed to enable mss facility\n");
+            }
+        }
     }
 
     for (i = 0; i < 0x10000; i++) {
diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
index 8241b0a..5e871ac 100644
--- a/pc-bios/s390-ccw/s390-ccw.h
+++ b/pc-bios/s390-ccw/s390-ccw.h
@@ -61,6 +61,7 @@ unsigned long virtio_load_direct(ulong rec_list1, ulong rec_list2,
 bool virtio_is_blk(struct subchannel_id schid);
 void virtio_setup_block(struct subchannel_id schid);
 int virtio_read(ulong sector, void *load_addr);
+int enable_mss_facility(void);
 
 /* bootmap.c */
 int zipl_load(void);
diff --git a/pc-bios/s390-ccw/virtio.c b/pc-bios/s390-ccw/virtio.c
index f438af1..49f2d29 100644
--- a/pc-bios/s390-ccw/virtio.c
+++ b/pc-bios/s390-ccw/virtio.c
@@ -13,6 +13,8 @@
 
 struct vring block;
 
+static char chsc_page[PAGE_SIZE] __attribute__((__aligned__(PAGE_SIZE)));
+
 static long kvm_hypercall(unsigned long nr, unsigned long param1,
                           unsigned long param2)
 {
@@ -301,3 +303,19 @@ bool virtio_is_blk(struct subchannel_id schid)
     return true;
 }
 
+int enable_mss_facility(void)
+{
+    int ret;
+    struct chsc_area_sda *sda_area = (struct chsc_area_sda *) chsc_page;
+
+    memset(sda_area, 0, PAGE_SIZE);
+    sda_area->request.length = 0x0400;
+    sda_area->request.code = 0x0031;
+    sda_area->operation_code = 0x2;
+
+    ret = chsc(sda_area);
+    if ((ret == 0) && (sda_area->response.code == 0x0001)) {
+        return 0;
+    }
+    return -EIO;
+}
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 2/2] s390/IPL: Allow boot from other ssid than 0

[Alexander Graf]

On 17.06.2013, at 14:29, Christian Borntraeger wrote:

> From: Dominik Dingel <dingel@linux.vnet.ibm.com>
> 
> We now take the subchannel set id also into account to find the boot device.
> If we want to use a subchannel set other than the default set 0, we first
> need to enable the mss facility.
> 
> Signed-off-by: Dominik Dingel <dingel@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>

Thanks, applied to s390-next.


Alex