Re: [Qemu-devel] [PULL 02/29] s390x/ipl: support diagnose 308 subcodes 5 and 6

[Paolo Bonzini <pbonzini@redhat.com>]

On 18/02/2015 21:21, Christian Borntraeger wrote:
> From: Fan Zhang <zhangfan@linux.vnet.ibm.com>
> 
> To support dynamically updating the IPL device from inside the KVM
> guest on the s390 platform, DIAG 308 instruction is intercepted
> in QEMU to handle the request.
> 
> Subcode 5 allows to specify a new boot device, which is saved for
> later in the s390_ipl device. This also allows to switch from an
> external kernel to a boot device.
> 
> Subcode 6 retrieves boot device configuration that has been previously
> set.
> 
> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
> Signed-off-by: Fan Zhang <zhangfan@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  hw/s390x/ipl.c             | 87 ++++++++++++++++++++++++++++++++++++++--------
>  hw/s390x/ipl.h             | 24 +++++++++++++
>  hw/s390x/s390-virtio.c     |  2 ++
>  target-s390x/misc_helper.c | 33 ++++++++++++++++--
>  4 files changed, 129 insertions(+), 17 deletions(-)
>  create mode 100644 hw/s390x/ipl.h
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index 4014a6a..231713d 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -18,6 +18,7 @@
>  #include "hw/sysbus.h"
>  #include "hw/s390x/virtio-ccw.h"
>  #include "hw/s390x/css.h"
> +#include "ipl.h"
>  
>  #define KERN_IMAGE_START                0x010000UL
>  #define KERN_PARM_AREA                  0x010480UL
> @@ -52,12 +53,17 @@ typedef struct S390IPLState {
>      uint64_t start_addr;
>      uint64_t bios_start_addr;
>      bool enforce_bios;
> +    IplParameterBlock iplb;
> +    bool iplb_valid;
>  
>      /*< public >*/
>      char *kernel;
>      char *initrd;
>      char *cmdline;
>      char *firmware;
> +    uint8_t cssid;
> +    uint8_t ssid;
> +    uint16_t devno;
>  } S390IPLState;
>  
>  
> @@ -164,6 +170,69 @@ static Property s390_ipl_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +/*
> + * In addition to updating the iplstate, this function returns:
> + * - 0 if system was ipled with external kernel
> + * - -1 if no valid boot device was found
> + * - ccw id of the boot device otherwise
> + */
> +static uint64_t s390_update_iplstate(CPUS390XState *env, S390IPLState *ipl)

This should probably return uint32_t, because otherwise...

> +    return ipl->cssid << 24 | ipl->ssid << 16 | ipl->devno;

... a cssid above 127 results in the bits of the high word all set to 1.

The reason is that, even though ipl->cssid is a uint8_t, before the
shift it is extended to int.  Then the return statement does a sign
extension from int to uint64_t.

Paolo

> +}
> +
> +int s390_ipl_update_diag308(IplParameterBlock *iplb)
> +{
> +    S390IPLState *ipl;
> +
> +    ipl = S390_IPL(object_resolve_path(TYPE_S390_IPL, NULL));
> +    if (ipl) {
> +        ipl->iplb = *iplb;
> +        ipl->iplb_valid = true;
> +        return 0;
> +    }
> +    return -1;
> +}
> +
> +IplParameterBlock *s390_ipl_get_iplb(void)
> +{
> +    S390IPLState *ipl;
> +
> +    ipl = S390_IPL(object_resolve_path(TYPE_S390_IPL, NULL));
> +    if (!ipl || !ipl->iplb_valid) {
> +        return NULL;
> +    }
> +    return &ipl->iplb;
> +}
> +
>  static void s390_ipl_reset(DeviceState *dev)
>  {
>      S390IPLState *ipl = S390_IPL(dev);
> @@ -173,21 +242,9 @@ static void s390_ipl_reset(DeviceState *dev)
>      env->psw.addr = ipl->start_addr;
>      env->psw.mask = IPL_PSW_MASK;
>  
> -    if (!ipl->kernel) {
> -        /* Tell firmware, if there is a preferred boot device */
> -        env->regs[7] = -1;
> -        DeviceState *dev_st = get_boot_device(0);
> -        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;
> -            }
> -        }
> +    if (!ipl->kernel || ipl->iplb_valid) {
> +        env->psw.addr = ipl->bios_start_addr;
> +        env->regs[7] = s390_update_iplstate(env, ipl);
>      }
>  
>      s390_cpu_set_state(CPU_STATE_OPERATING, cpu);
> diff --git a/hw/s390x/ipl.h b/hw/s390x/ipl.h
> new file mode 100644
> index 0000000..f1d082f
> --- /dev/null
> +++ b/hw/s390x/ipl.h
> @@ -0,0 +1,24 @@
> +/*
> + * s390 IPL device
> + *
> + * Copyright 2015 IBM Corp.
> + * Author(s): Zhang Fan <bjfanzh@cn.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +
> +#ifndef HW_S390_IPL_H
> +#define HW_S390_IPL_H
> +
> +typedef struct IplParameterBlock {
> +      uint8_t  reserved1[110];
> +      uint16_t devno;
> +      uint8_t  reserved2[88];
> +} IplParameterBlock;
> +
> +int s390_ipl_update_diag308(IplParameterBlock *iplb);
> +IplParameterBlock *s390_ipl_get_iplb(void);
> +
> +#endif
> diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
> index 13f9e49..412e49b 100644
> --- a/hw/s390x/s390-virtio.c
> +++ b/hw/s390x/s390-virtio.c
> @@ -143,6 +143,8 @@ void s390_init_ipl_dev(const char *kernel_filename,
>      qdev_prop_set_string(dev, "cmdline", kernel_cmdline);
>      qdev_prop_set_string(dev, "firmware", firmware);
>      qdev_prop_set_bit(dev, "enforce_bios", enforce_bios);
> +    object_property_add_child(qdev_get_machine(), "s390-ipl",
> +                              OBJECT(dev), NULL);
>      qdev_init_nofail(dev);
>  }
>  
> diff --git a/target-s390x/misc_helper.c b/target-s390x/misc_helper.c
> index ef9758a..1c3df8e 100644
> --- a/target-s390x/misc_helper.c
> +++ b/target-s390x/misc_helper.c
> @@ -25,6 +25,7 @@
>  #include <string.h>
>  #include "sysemu/kvm.h"
>  #include "qemu/timer.h"
> +#include "exec/address-spaces.h"
>  #ifdef CONFIG_KVM
>  #include <linux/kvm.h>
>  #endif
> @@ -34,6 +35,7 @@
>  #include "sysemu/cpus.h"
>  #include "sysemu/sysemu.h"
>  #include "hw/s390x/ebcdic.h"
> +#include "hw/s390x/ipl.h"
>  #endif
>  
>  /* #define DEBUG_HELPER */
> @@ -151,12 +153,15 @@ static int load_normal_reset(S390CPU *cpu)
>      return 0;
>  }
>  
> +#define DIAG_308_RC_OK              0x0001
>  #define DIAG_308_RC_NO_CONF         0x0102
>  #define DIAG_308_RC_INVALID         0x0402
> +
>  void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3)
>  {
>      uint64_t addr =  env->regs[r1];
>      uint64_t subcode = env->regs[r3];
> +    IplParameterBlock *iplb;
>  
>      if (env->psw.mask & PSW_MASK_PSTATE) {
>          program_interrupt(env, PGM_PRIVILEGED, ILEN_LATER_INC);
> @@ -180,14 +185,38 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3)
>              program_interrupt(env, PGM_SPECIFICATION, ILEN_LATER_INC);
>              return;
>          }
> -        env->regs[r1+1] = DIAG_308_RC_INVALID;
> +        if (!address_space_access_valid(&address_space_memory, addr,
> +                                        sizeof(IplParameterBlock), false)) {
> +            program_interrupt(env, PGM_ADDRESSING, ILEN_LATER_INC);
> +            return;
> +        }
> +        iplb = g_malloc0(sizeof(struct IplParameterBlock));
> +        cpu_physical_memory_read(addr, iplb, sizeof(struct IplParameterBlock));
> +        if (!s390_ipl_update_diag308(iplb)) {
> +            env->regs[r1 + 1] = DIAG_308_RC_OK;
> +        } else {
> +            env->regs[r1 + 1] = DIAG_308_RC_INVALID;
> +        }
> +        g_free(iplb);
>          return;
>      case 6:
>          if ((r1 & 1) || (addr & 0x0fffULL)) {
>              program_interrupt(env, PGM_SPECIFICATION, ILEN_LATER_INC);
>              return;
>          }
> -        env->regs[r1+1] = DIAG_308_RC_NO_CONF;
> +        if (!address_space_access_valid(&address_space_memory, addr,
> +                                        sizeof(IplParameterBlock), true)) {
> +            program_interrupt(env, PGM_ADDRESSING, ILEN_LATER_INC);
> +            return;
> +        }
> +        iplb = s390_ipl_get_iplb();
> +        if (iplb) {
> +            cpu_physical_memory_write(addr, iplb,
> +                                      sizeof(struct IplParameterBlock));
> +            env->regs[r1 + 1] = DIAG_308_RC_OK;
> +        } else {
> +            env->regs[r1 + 1] = DIAG_308_RC_NO_CONF;
> +        }
>          return;
>      default:
>          hw_error("Unhandled diag308 subcode %" PRIx64, subcode);
>