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

[Christian Borntraeger]

Am 19.03.2015 um 10:31 schrieb Paolo Bonzini:
> 
> 
> On 18/02/2015 21:21, Christian Borntraeger wrote:
>> From: Fan Zhang <address@hidden>
>>
>> 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 <address@hidden>
>> Reviewed-by: David Hildenbrand <address@hidden>
>> Acked-by: Christian Borntraeger <address@hidden>
>> Signed-off-by: Jens Freimann <address@hidden>
>> Signed-off-by: Fan Zhang <address@hidden>
>> Signed-off-by: Christian Borntraeger <address@hidden>
>> ---
>>  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

You are right. The good news is, that it does not matter, as the s390-ccw
bios will shift and mask to get the devno and the ssid. It ignores the cssid
because it does not enable the multiple channel subsystem feature (in the
same way as no other operating system on z) and it ignores the upper
32 bits if the value is != -1.

We will fix that anyway, of course.

Christian
> 
>> +}
>> +
>> +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 <address@hidden>
>> + *
>> + * 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);
>>
>