[Qemu-devel] [PATCH/s390-next 0/3] s390x: remaining pci related fixes

[Qemu-devel] [PATCH/s390-next 0/3] s390x: remaining pci related fixes

[Christian Borntraeger]

here are the remaining PCI patches also enabling AIS via KVM.
CPU model wise we do not put AIS in a default model for zEC12 and z13
since almost all distribution kernels do not have ais yet. This would
break -cpu zEC12 and -cpu z13 if QEMU is updated.

patches are against previous series.


Yi Min Zhao (3):
  s390x: initialize cpu firstly
  s390x/cpumodel: add zpci, aen and ais facilities
  s390x/flic: migrate ais states

 hw/intc/s390_flic.c             | 23 +++++++++++-
 hw/intc/s390_flic_kvm.c         | 78 +++++++++++++++++++++++++++++++++++++++--
 hw/s390x/s390-virtio-ccw.c      |  9 +++--
 include/hw/s390x/s390_flic.h    |  1 +
 target/s390x/cpu_features.c     |  3 ++
 target/s390x/cpu_features_def.h |  3 ++
 target/s390x/gen-features.c     |  5 +++
 target/s390x/kvm.c              |  7 ++++
 8 files changed, 122 insertions(+), 7 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH/s390-next 1/3] s390x: initialize cpu firstly

[Christian Borntraeger]

From: Yi Min Zhao <zyimin@linux.vnet.ibm.com>

By initializing the CPU firstly, we are able to retrieve and use the
CPU model features when initializing other subsystem or devices.

Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
Reviewed-by: Pierre Morel<pmorel@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index e086cb5..23e9658 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -122,6 +122,9 @@ static void ccw_init(MachineState *machine)
     s390_sclp_init();
     s390_memory_init(machine->ram_size);
 
+    /* init CPUs */
+    s390_init_cpus(machine);
+
     s390_flic_init();
 
     /* get a BUS */
@@ -138,9 +141,6 @@ static void ccw_init(MachineState *machine)
     /* register hypercalls */
     virtio_ccw_register_hcalls();
 
-    /* init CPUs */
-    s390_init_cpus(machine);
-
     if (kvm_enabled()) {
         kvm_s390_enable_css_support(s390_cpu_addr2state(0));
     }
-- 
2.7.4

[Qemu-devel] [PATCH/s390-next 2/3] s390x/cpumodel: add zpci, aen and ais facilities

[Christian Borntraeger]

From: Yi Min Zhao <zyimin@linux.vnet.ibm.com>

zPCI instructions and facilities are available since IBM zEnterprise
EC12. To support z/PCI in QEMU we enable zpci, aen and ais facilities
starting with zEC12 GA1. And we always set zpci and aen bits in max cpu
model. Later they might be switched off due to applied real cpu model.
For ais bit, we only provide it in the full cpu model beginning with
zEC12 and defer its enablement in the default cpu model to a later point
in time. At the same time, disable them for 2.9 and older machines.

Because of introducing AIS facility, we could check if it's enabled to
initialize flic->ais_supported with the real value.

Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/intc/s390_flic.c             | 3 ++-
 hw/intc/s390_flic_kvm.c         | 3 ---
 hw/s390x/s390-virtio-ccw.c      | 3 +++
 target/s390x/cpu_features.c     | 3 +++
 target/s390x/cpu_features_def.h | 3 +++
 target/s390x/gen-features.c     | 5 +++++
 target/s390x/kvm.c              | 7 +++++++
 7 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
index ff6e4ec..6e7c610 100644
--- a/hw/intc/s390_flic.c
+++ b/hw/intc/s390_flic.c
@@ -163,9 +163,10 @@ static void s390_flic_common_realize(DeviceState *dev, Error **errp)
     if (max_batch > ADAPTER_ROUTES_MAX_GSI) {
         error_setg(errp, "flic property adapter_routes_max_batch too big"
                    " (%d > %d)", max_batch, ADAPTER_ROUTES_MAX_GSI);
+        return;
     }
 
-    fs->ais_supported = true;
+    fs->ais_supported = s390_has_feat(S390_FEAT_ADAPTER_INT_SUPPRESSION);
 }
 
 static void s390_flic_class_init(ObjectClass *oc, void *data)
diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
index a587ace..d93503f 100644
--- a/hw/intc/s390_flic_kvm.c
+++ b/hw/intc/s390_flic_kvm.c
@@ -444,7 +444,6 @@ typedef struct KVMS390FLICStateClass {
 
 static void kvm_s390_flic_realize(DeviceState *dev, Error **errp)
 {
-    S390FLICState *fs = S390_FLIC_COMMON(dev);
     KVMS390FLICState *flic_state = KVM_S390_FLIC(dev);
     struct kvm_create_device cd = {0};
     struct kvm_device_attr test_attr = {0};
@@ -476,8 +475,6 @@ static void kvm_s390_flic_realize(DeviceState *dev, Error **errp)
     test_attr.group = KVM_DEV_FLIC_CLEAR_IO_IRQ;
     flic_state->clear_io_supported = !ioctl(flic_state->fd,
                                             KVM_HAS_DEVICE_ATTR, test_attr);
-
-    fs->ais_supported = false;
     return;
 fail:
     error_propagate(errp, errp_local);
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 23e9658..e484aed 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -503,6 +503,9 @@ DEFINE_CCW_MACHINE(2_10, "2.10", true);
 static void ccw_machine_2_9_instance_options(MachineState *machine)
 {
     ccw_machine_2_10_instance_options(machine);
+    s390_cpudef_featoff_greater(12, 1, S390_FEAT_ZPCI);
+    s390_cpudef_featoff_greater(12, 1, S390_FEAT_ADAPTER_INT_SUPPRESSION);
+    s390_cpudef_featoff_greater(12, 1, S390_FEAT_ADAPTER_EVENT_NOTIFICATION);
 }
 
 static void ccw_machine_2_9_class_options(MachineClass *mc)
diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
index 0436dc2..8ab5cd7 100644
--- a/target/s390x/cpu_features.c
+++ b/target/s390x/cpu_features.c
@@ -74,6 +74,9 @@ static const S390FeatDef s390_features[] = {
     FEAT_INIT("stfle53", S390_FEAT_TYPE_STFL, 53, "Various facilities introduced with z13"),
     FEAT_INIT("msa5-base", S390_FEAT_TYPE_STFL, 57, "Message-security-assist-extension-5 facility (excluding subfunctions)"),
     FEAT_INIT("ri", S390_FEAT_TYPE_STFL, 64, "CPU runtime-instrumentation facility"),
+    FEAT_INIT("zpci", S390_FEAT_TYPE_STFL, 69, "z/PCI facility"),
+    FEAT_INIT("aen", S390_FEAT_TYPE_STFL, 71, "General-purpose-adapter-event-notification facility"),
+    FEAT_INIT("ais", S390_FEAT_TYPE_STFL, 72, "General-purpose-adapter-interruption-suppression facility"),
     FEAT_INIT("te", S390_FEAT_TYPE_STFL, 73, "Transactional-execution facility"),
     FEAT_INIT("sthyi", S390_FEAT_TYPE_STFL, 74, "Store-hypervisor-information facility"),
     FEAT_INIT("aefsi", S390_FEAT_TYPE_STFL, 75, "Access-exception-fetch/store-indication facility"),
diff --git a/target/s390x/cpu_features_def.h b/target/s390x/cpu_features_def.h
index f5bb7ed..c939a00 100644
--- a/target/s390x/cpu_features_def.h
+++ b/target/s390x/cpu_features_def.h
@@ -65,6 +65,9 @@ typedef enum {
     S390_FEAT_STFLE_53,
     S390_FEAT_MSA_EXT_5,
     S390_FEAT_RUNTIME_INSTRUMENTATION,
+    S390_FEAT_ZPCI,
+    S390_FEAT_ADAPTER_EVENT_NOTIFICATION,
+    S390_FEAT_ADAPTER_INT_SUPPRESSION,
     S390_FEAT_TRANSACTIONAL_EXE,
     S390_FEAT_STORE_HYPERVISOR_INFO,
     S390_FEAT_ACCESS_EXCEPTION_FS_INDICATION,
diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
index 8ca2b47..622ee24 100644
--- a/target/s390x/gen-features.c
+++ b/target/s390x/gen-features.c
@@ -389,6 +389,9 @@ static uint16_t full_GEN12_GA1[] = {
     S390_FEAT_CONSTRAINT_TRANSACTIONAL_EXE,
     S390_FEAT_TRANSACTIONAL_EXE,
     S390_FEAT_RUNTIME_INSTRUMENTATION,
+    S390_FEAT_ZPCI,
+    S390_FEAT_ADAPTER_EVENT_NOTIFICATION,
+    S390_FEAT_ADAPTER_INT_SUPPRESSION,
     S390_FEAT_EDAT_2,
 };
 
@@ -446,6 +449,8 @@ static uint16_t default_GEN12_GA1[] = {
     S390_FEAT_CONSTRAINT_TRANSACTIONAL_EXE,
     S390_FEAT_TRANSACTIONAL_EXE,
     S390_FEAT_RUNTIME_INSTRUMENTATION,
+    S390_FEAT_ZPCI,
+    S390_FEAT_ADAPTER_EVENT_NOTIFICATION,
     S390_FEAT_EDAT_2,
 };
 
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 78ebe83..1901153 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -302,6 +302,9 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
         }
     }
 
+    /* Try to enable AIS facility */
+    kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);
+
     qemu_mutex_init(&qemu_sigp_mutex);
 
     return 0;
@@ -2635,6 +2638,10 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
         set_bit(S390_FEAT_CMM, model->features);
     }
 
+    /* set zpci and aen facilities */
+    set_bit(S390_FEAT_ZPCI, model->features);
+    set_bit(S390_FEAT_ADAPTER_EVENT_NOTIFICATION, model->features);
+
     if (s390_known_cpu_type(cpu_type)) {
         /* we want the exact model, even if some features are missing */
         model->def = s390_find_cpu_def(cpu_type, ibc_gen(unblocked_ibc),
-- 
2.7.4

[Qemu-devel] [PATCH/s390-next 3/3] s390x/flic: migrate ais states

[Christian Borntraeger]

From: Yi Min Zhao <zyimin@linux.vnet.ibm.com>

During migration we should transfer ais states to the target guest.
This patch introduces a subsection to kvm_s390_flic_vmstate and new
vmsd for qemu_flic. The ais states need to be migrated only when
ais is supported.

Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/intc/s390_flic.c          | 20 ++++++++++++
 hw/intc/s390_flic_kvm.c      | 75 ++++++++++++++++++++++++++++++++++++++++++++
 include/hw/s390x/s390_flic.h |  1 +
 3 files changed, 96 insertions(+)

diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
index 6e7c610..6eaf178 100644
--- a/hw/intc/s390_flic.c
+++ b/hw/intc/s390_flic.c
@@ -134,12 +134,32 @@ static void qemu_s390_flic_reset(DeviceState *dev)
     flic->nimm = 0;
 }
 
+bool ais_needed(void *opaque)
+{
+    S390FLICState *s = opaque;
+
+    return s->ais_supported;
+}
+
+static const VMStateDescription qemu_s390_flic_vmstate = {
+    .name = "qemu-s390-flic",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = ais_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(simm, QEMUS390FLICState),
+        VMSTATE_UINT8(nimm, QEMUS390FLICState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void qemu_s390_flic_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
     S390FLICStateClass *fsc = S390_FLIC_COMMON_CLASS(oc);
 
     dc->reset = qemu_s390_flic_reset;
+    dc->vmsd = &qemu_s390_flic_vmstate;
     fsc->register_io_adapter = qemu_s390_register_io_adapter;
     fsc->io_adapter_map = qemu_s390_io_adapter_map;
     fsc->add_adapter_routes = qemu_s390_add_adapter_routes;
diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
index d93503f..4cf73ee 100644
--- a/hw/intc/s390_flic_kvm.c
+++ b/hw/intc/s390_flic_kvm.c
@@ -413,7 +413,78 @@ out:
     return r;
 }
 
+typedef struct KVMS390FLICStateMigTmp {
+    KVMS390FLICState *parent;
+    uint8_t simm;
+    uint8_t nimm;
+} KVMS390FLICStateMigTmp;
+
+static void kvm_flic_ais_pre_save(void *opaque)
+{
+    KVMS390FLICStateMigTmp *tmp = opaque;
+    KVMS390FLICState *flic = tmp->parent;
+    struct kvm_s390_ais_all ais;
+    struct kvm_device_attr attr = {
+        .group = KVM_DEV_FLIC_AISM_ALL,
+        .addr = (uint64_t)&ais,
+        .attr = sizeof(ais),
+    };
+
+    if (ioctl(flic->fd, KVM_GET_DEVICE_ATTR, &attr)) {
+        error_report("Failed to retrieve kvm flic ais states");
+        return;
+    }
+
+    tmp->simm = ais.simm;
+    tmp->nimm = ais.nimm;
+}
+
+static int kvm_flic_ais_post_load(void *opaque, int version_id)
+{
+    KVMS390FLICStateMigTmp *tmp = opaque;
+    KVMS390FLICState *flic = tmp->parent;
+    struct kvm_s390_ais_all ais = {
+        .simm = tmp->simm,
+        .nimm = tmp->nimm,
+    };
+    struct kvm_device_attr attr = {
+        .group = KVM_DEV_FLIC_AISM_ALL,
+        .addr = (uint64_t)&ais,
+    };
+
+    if (!ais_needed(flic)) {
+        return -ENOSYS;
+    }
+
+    return ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr) ? -errno : 0;
+}
+
+static const VMStateDescription kvm_s390_flic_ais_tmp = {
+    .name = "s390-flic-ais-tmp",
+    .pre_save = kvm_flic_ais_pre_save,
+    .post_load = kvm_flic_ais_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(simm, KVMS390FLICStateMigTmp),
+        VMSTATE_UINT8(nimm, KVMS390FLICStateMigTmp),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static const VMStateDescription kvm_s390_flic_vmstate_ais = {
+    .name = "s390-flic/ais",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = ais_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_WITH_TMP(KVMS390FLICState, KVMS390FLICStateMigTmp,
+                         kvm_s390_flic_ais_tmp),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription kvm_s390_flic_vmstate = {
+    /* should have been like kvm-s390-flic,
+     * can't change without breaking compat */
     .name = "s390-flic",
     .version_id = FLIC_SAVEVM_VERSION,
     .minimum_version_id = FLIC_SAVEVM_VERSION,
@@ -428,6 +499,10 @@ static const VMStateDescription kvm_s390_flic_vmstate = {
             .flags = VMS_SINGLE,
         },
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (const VMStateDescription * []) {
+        &kvm_s390_flic_vmstate_ais,
+        NULL
     }
 };
 
diff --git a/include/hw/s390x/s390_flic.h b/include/hw/s390x/s390_flic.h
index 2f173d9..7aab6ef 100644
--- a/include/hw/s390x/s390_flic.h
+++ b/include/hw/s390x/s390_flic.h
@@ -89,6 +89,7 @@ typedef struct QEMUS390FLICState {
 void s390_flic_init(void);
 
 S390FLICState *s390_get_flic(void);
+bool ais_needed(void *opaque);
 
 #ifdef CONFIG_KVM
 DeviceState *s390_flic_kvm_create(void);
-- 
2.7.4

Re: [Qemu-devel] [PATCH/s390-next 0/3] s390x: remaining pci related fixes

[Christian Borntraeger]

On 07/13/2017 12:40 PM, Christian Borntraeger wrote:
> here are the remaining PCI patches also enabling AIS via KVM.
> CPU model wise we do not put AIS in a default model for zEC12 and z13
> since almost all distribution kernels do not have ais yet. This would
> break -cpu zEC12 and -cpu z13 if QEMU is updated.
> 
> patches are against previous series.

The current state (including netboot bios) can be found on

https://github.com/borntraeger/qemu/tree/s390-next

> 
> 
> Yi Min Zhao (3):
>   s390x: initialize cpu firstly
>   s390x/cpumodel: add zpci, aen and ais facilities
>   s390x/flic: migrate ais states
> 
>  hw/intc/s390_flic.c             | 23 +++++++++++-
>  hw/intc/s390_flic_kvm.c         | 78 +++++++++++++++++++++++++++++++++++++++--
>  hw/s390x/s390-virtio-ccw.c      |  9 +++--
>  include/hw/s390x/s390_flic.h    |  1 +
>  target/s390x/cpu_features.c     |  3 ++
>  target/s390x/cpu_features_def.h |  3 ++
>  target/s390x/gen-features.c     |  5 +++
>  target/s390x/kvm.c              |  7 ++++
>  8 files changed, 122 insertions(+), 7 deletions(-)
> 

Re: [Qemu-devel] [PATCH/s390-next 1/3] s390x: initialize cpu firstly

[Cornelia Huck]

On Thu, 13 Jul 2017 12:40:27 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> From: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> 
> By initializing the CPU firstly, we are able to retrieve and use the
> CPU model features when initializing other subsystem or devices.
> 
> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> Reviewed-by: Pierre Morel<pmorel@linux.vnet.ibm.com>

Missing blank before address.

> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index e086cb5..23e9658 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -122,6 +122,9 @@ static void ccw_init(MachineState *machine)
>      s390_sclp_init();
>      s390_memory_init(machine->ram_size);
>  
> +    /* init CPUs */
> +    s390_init_cpus(machine);
> +
>      s390_flic_init();
>  
>      /* get a BUS */
> @@ -138,9 +141,6 @@ static void ccw_init(MachineState *machine)
>      /* register hypercalls */
>      virtio_ccw_register_hcalls();
>  
> -    /* init CPUs */
> -    s390_init_cpus(machine);
> -
>      if (kvm_enabled()) {
>          kvm_s390_enable_css_support(s390_cpu_addr2state(0));
>      }

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [Qemu-devel] [PATCH/s390-next 2/3] s390x/cpumodel: add zpci, aen and ais facilities

[Cornelia Huck]

On Thu, 13 Jul 2017 12:40:28 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> From: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> 
> zPCI instructions and facilities are available since IBM zEnterprise
> EC12. To support z/PCI in QEMU we enable zpci, aen and ais facilities
> starting with zEC12 GA1. And we always set zpci and aen bits in max cpu
> model. Later they might be switched off due to applied real cpu model.
> For ais bit, we only provide it in the full cpu model beginning with
> zEC12 and defer its enablement in the default cpu model to a later point
> in time. At the same time, disable them for 2.9 and older machines.
> 
> Because of introducing AIS facility, we could check if it's enabled to
> initialize flic->ais_supported with the real value.
> 
> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  hw/intc/s390_flic.c             | 3 ++-
>  hw/intc/s390_flic_kvm.c         | 3 ---
>  hw/s390x/s390-virtio-ccw.c      | 3 +++
>  target/s390x/cpu_features.c     | 3 +++
>  target/s390x/cpu_features_def.h | 3 +++
>  target/s390x/gen-features.c     | 5 +++++
>  target/s390x/kvm.c              | 7 +++++++
>  7 files changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
> index ff6e4ec..6e7c610 100644
> --- a/hw/intc/s390_flic.c
> +++ b/hw/intc/s390_flic.c
> @@ -163,9 +163,10 @@ static void s390_flic_common_realize(DeviceState *dev, Error **errp)
>      if (max_batch > ADAPTER_ROUTES_MAX_GSI) {
>          error_setg(errp, "flic property adapter_routes_max_batch too big"
>                     " (%d > %d)", max_batch, ADAPTER_ROUTES_MAX_GSI);
> +        return;

Hunk should go into a previous patch?

(And does it really matter?)

>      }
>  
> -    fs->ais_supported = true;
> +    fs->ais_supported = s390_has_feat(S390_FEAT_ADAPTER_INT_SUPPRESSION);
>  }
>  
>  static void s390_flic_class_init(ObjectClass *oc, void *data)

> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 78ebe83..1901153 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -302,6 +302,9 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>          }
>      }
>  
> +    /* Try to enable AIS facility */
> +    kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);

What happens if you fail to enable it? You probably don't want to allow
the feature bit, then?

> +
>      qemu_mutex_init(&qemu_sigp_mutex);
>  
>      return 0;

Let's summarize to make sure that I'm not confused:

- Starting with zEC12 GA1, we provide zpci, aen, ais in the full model
- Starting with zEC12 GA1, we provide zpci and aen in the default model
- In the host model, we add zpci and aen; they might be switched off
  after applying the found model
- Compat for 2.9 and earlier switches off zpci, aen, ais
- We unconditionally enable the kvm part of ais

I'm still not sure what's supposed to happen with new qemu + old kernel
(no ais) + full zEC12 GA1 or later model.

Re: [Qemu-devel] [PATCH/s390-next 3/3] s390x/flic: migrate ais states

[Cornelia Huck]

On Thu, 13 Jul 2017 12:40:29 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> From: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> 
> During migration we should transfer ais states to the target guest.
> This patch introduces a subsection to kvm_s390_flic_vmstate and new
> vmsd for qemu_flic. The ais states need to be migrated only when
> ais is supported.
> 
> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  hw/intc/s390_flic.c          | 20 ++++++++++++
>  hw/intc/s390_flic_kvm.c      | 75 ++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/s390x/s390_flic.h |  1 +
>  3 files changed, 96 insertions(+)
> 

> diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
> index d93503f..4cf73ee 100644
> --- a/hw/intc/s390_flic_kvm.c
> +++ b/hw/intc/s390_flic_kvm.c
> @@ -413,7 +413,78 @@ out:
>      return r;
>  }
>  
> +typedef struct KVMS390FLICStateMigTmp {
> +    KVMS390FLICState *parent;
> +    uint8_t simm;
> +    uint8_t nimm;
> +} KVMS390FLICStateMigTmp;
> +
> +static void kvm_flic_ais_pre_save(void *opaque)
> +{
> +    KVMS390FLICStateMigTmp *tmp = opaque;
> +    KVMS390FLICState *flic = tmp->parent;
> +    struct kvm_s390_ais_all ais;
> +    struct kvm_device_attr attr = {
> +        .group = KVM_DEV_FLIC_AISM_ALL,
> +        .addr = (uint64_t)&ais,
> +        .attr = sizeof(ais),
> +    };
> +
> +    if (ioctl(flic->fd, KVM_GET_DEVICE_ATTR, &attr)) {
> +        error_report("Failed to retrieve kvm flic ais states");

There's not much else we can do in that case, is there?

> +        return;
> +    }
> +
> +    tmp->simm = ais.simm;
> +    tmp->nimm = ais.nimm;
> +}
> +
> +static int kvm_flic_ais_post_load(void *opaque, int version_id)
> +{
> +    KVMS390FLICStateMigTmp *tmp = opaque;
> +    KVMS390FLICState *flic = tmp->parent;
> +    struct kvm_s390_ais_all ais = {
> +        .simm = tmp->simm,
> +        .nimm = tmp->nimm,
> +    };
> +    struct kvm_device_attr attr = {
> +        .group = KVM_DEV_FLIC_AISM_ALL,
> +        .addr = (uint64_t)&ais,
> +    };
> +
> +    if (!ais_needed(flic)) {
> +        return -ENOSYS;
> +    }

I do not understand this... does that mean that
- we should never get here or 
- we should not try to change the fields or
- I need coffee?

> +
> +    return ioctl(flic->fd, KVM_SET_DEVICE_ATTR, &attr) ? -errno : 0;
> +}
> +
> +static const VMStateDescription kvm_s390_flic_ais_tmp = {
> +    .name = "s390-flic-ais-tmp",
> +    .pre_save = kvm_flic_ais_pre_save,
> +    .post_load = kvm_flic_ais_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(simm, KVMS390FLICStateMigTmp),
> +        VMSTATE_UINT8(nimm, KVMS390FLICStateMigTmp),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static const VMStateDescription kvm_s390_flic_vmstate_ais = {
> +    .name = "s390-flic/ais",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = ais_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_WITH_TMP(KVMS390FLICState, KVMS390FLICStateMigTmp,
> +                         kvm_s390_flic_ais_tmp),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription kvm_s390_flic_vmstate = {
> +    /* should have been like kvm-s390-flic,
> +     * can't change without breaking compat */

:/

>      .name = "s390-flic",
>      .version_id = FLIC_SAVEVM_VERSION,
>      .minimum_version_id = FLIC_SAVEVM_VERSION,

Re: [Qemu-devel] [PATCH/s390-next 2/3] s390x/cpumodel: add zpci, aen and ais facilities

[Christian Borntraeger]

On 07/13/2017 02:11 PM, Cornelia Huck wrote:
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index 78ebe83..1901153 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -302,6 +302,9 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>>          }
>>      }
>>  
>> +    /* Try to enable AIS facility */
>> +    kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);
> 
> What happens if you fail to enable it? You probably don't want to allow
> the feature bit, then?

Then this bit is off. This call will enable it in the kernel, if that fails
the kernel will return this bit as disabled.
> 
>> +
>>      qemu_mutex_init(&qemu_sigp_mutex);
>>  
>>      return 0;
> 
> Let's summarize to make sure that I'm not confused:
> 
> - Starting with zEC12 GA1, we provide zpci, aen, ais in the full model
yes

> - Starting with zEC12 GA1, we provide zpci and aen in the default model
yes. ais has to be enabled manually for z12 and z13.
The alternative is to have ais as part of the default model. This has the big
disadvantage that -cpu zEC12 and -cpu z13 will stop working for all available
distro kernels. I believe that a working -cpu z13 is more important than the
need to manually enable ais.
For the future
 - We can make ais part of the default model for a future system when that happens since
   the features for a new system will require a new host kernel anyway
 - We can also make ais part of the default model for a future machine type (e.g. 2.13)
   when we believe that the world has moved on to a newer kernel


> - In the host model, we add zpci and aen; they might be switched off
>   after applying the found model

We also get ais from the kernel, so the host model will have zpci,ais and
aen

> - Compat for 2.9 and earlier switches off zpci, aen, ais

yes 

> - We unconditionally enable the kvm part of ais

We tell the KVM code in the kernel to enable the facility bit (before the cpu
model might take it way) and if QEMU really uses ais, that the kernel does
the right thing then
> 
> I'm still not sure what's supposed to happen with new qemu + old kernel
> (no ais) + full zEC12 GA1 or later model.

We enable aen and zpci, but disable ais for that guest. In theory a guest
can drive PCI devices without AIS. This is a valid configuration since zpci
does not require ais. 
The fact that Linux requires ais to use PCI is unfortunate but that could 
be "fixed" Linux if necessary.

Re: [Qemu-devel] [PATCH/s390-next 2/3] s390x/cpumodel: add zpci, aen and ais facilities

[Christian Borntraeger]

On 07/13/2017 02:11 PM, Cornelia Huck wrote:
> On Thu, 13 Jul 2017 12:40:28 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> From: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>>
>> zPCI instructions and facilities are available since IBM zEnterprise
>> EC12. To support z/PCI in QEMU we enable zpci, aen and ais facilities
>> starting with zEC12 GA1. And we always set zpci and aen bits in max cpu
>> model. Later they might be switched off due to applied real cpu model.
>> For ais bit, we only provide it in the full cpu model beginning with
>> zEC12 and defer its enablement in the default cpu model to a later point
>> in time. At the same time, disable them for 2.9 and older machines.
>>
>> Because of introducing AIS facility, we could check if it's enabled to
>> initialize flic->ais_supported with the real value.
>>
>> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  hw/intc/s390_flic.c             | 3 ++-
>>  hw/intc/s390_flic_kvm.c         | 3 ---
>>  hw/s390x/s390-virtio-ccw.c      | 3 +++
>>  target/s390x/cpu_features.c     | 3 +++
>>  target/s390x/cpu_features_def.h | 3 +++
>>  target/s390x/gen-features.c     | 5 +++++
>>  target/s390x/kvm.c              | 7 +++++++
>>  7 files changed, 23 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
>> index ff6e4ec..6e7c610 100644
>> --- a/hw/intc/s390_flic.c
>> +++ b/hw/intc/s390_flic.c
>> @@ -163,9 +163,10 @@ static void s390_flic_common_realize(DeviceState *dev, Error **errp)
>>      if (max_batch > ADAPTER_ROUTES_MAX_GSI) {
>>          error_setg(errp, "flic property adapter_routes_max_batch too big"
>>                     " (%d > %d)", max_batch, ADAPTER_ROUTES_MAX_GSI);
>> +        return;
> 
> Hunk should go into a previous patch?
> 
> (And does it really matter?)

I can
- move to the previous series patch 8 (s390x/flic: introduce modify_ais_mode callback)
- remove the return as it does not matter (this is realize, and realize failures here
are fatal as far as I can tell)
- keep it here since this version was tested and it does not hurt

Re: [Qemu-devel] [PATCH/s390-next 2/3] s390x/cpumodel: add zpci, aen and ais facilities

[Cornelia Huck]

On Thu, 13 Jul 2017 14:41:05 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 07/13/2017 02:11 PM, Cornelia Huck wrote:
> > On Thu, 13 Jul 2017 12:40:28 +0200
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >   
> >> From: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> >>
> >> zPCI instructions and facilities are available since IBM zEnterprise
> >> EC12. To support z/PCI in QEMU we enable zpci, aen and ais facilities
> >> starting with zEC12 GA1. And we always set zpci and aen bits in max cpu
> >> model. Later they might be switched off due to applied real cpu model.
> >> For ais bit, we only provide it in the full cpu model beginning with
> >> zEC12 and defer its enablement in the default cpu model to a later point
> >> in time. At the same time, disable them for 2.9 and older machines.
> >>
> >> Because of introducing AIS facility, we could check if it's enabled to
> >> initialize flic->ais_supported with the real value.
> >>
> >> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >> ---
> >>  hw/intc/s390_flic.c             | 3 ++-
> >>  hw/intc/s390_flic_kvm.c         | 3 ---
> >>  hw/s390x/s390-virtio-ccw.c      | 3 +++
> >>  target/s390x/cpu_features.c     | 3 +++
> >>  target/s390x/cpu_features_def.h | 3 +++
> >>  target/s390x/gen-features.c     | 5 +++++
> >>  target/s390x/kvm.c              | 7 +++++++
> >>  7 files changed, 23 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
> >> index ff6e4ec..6e7c610 100644
> >> --- a/hw/intc/s390_flic.c
> >> +++ b/hw/intc/s390_flic.c
> >> @@ -163,9 +163,10 @@ static void s390_flic_common_realize(DeviceState *dev, Error **errp)
> >>      if (max_batch > ADAPTER_ROUTES_MAX_GSI) {
> >>          error_setg(errp, "flic property adapter_routes_max_batch too big"
> >>                     " (%d > %d)", max_batch, ADAPTER_ROUTES_MAX_GSI);
> >> +        return;  
> > 
> > Hunk should go into a previous patch?
> > 
> > (And does it really matter?)  
> 
> I can
> - move to the previous series patch 8 (s390x/flic: introduce modify_ais_mode callback)
> - remove the return as it does not matter (this is realize, and realize failures here
> are fatal as far as I can tell)
> - keep it here since this version was tested and it does not hurt
> 

I'd say 2 or 3, no need to juggle existing patches for what is
basically a non-issue.

Re: [Qemu-devel] [PATCH/s390-next 3/3] s390x/flic: migrate ais states

[Christian Borntraeger]

On 07/13/2017 02:27 PM, Cornelia Huck wrote:
> On Thu, 13 Jul 2017 12:40:29 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> From: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>>
>> During migration we should transfer ais states to the target guest.
>> This patch introduces a subsection to kvm_s390_flic_vmstate and new
>> vmsd for qemu_flic. The ais states need to be migrated only when
>> ais is supported.
>>
>> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  hw/intc/s390_flic.c          | 20 ++++++++++++
>>  hw/intc/s390_flic_kvm.c      | 75 ++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/s390x/s390_flic.h |  1 +
>>  3 files changed, 96 insertions(+)
>>
> 
>> diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
>> index d93503f..4cf73ee 100644
>> --- a/hw/intc/s390_flic_kvm.c
>> +++ b/hw/intc/s390_flic_kvm.c
>> @@ -413,7 +413,78 @@ out:
>>      return r;
>>  }
>>  
>> +typedef struct KVMS390FLICStateMigTmp {
>> +    KVMS390FLICState *parent;
>> +    uint8_t simm;
>> +    uint8_t nimm;
>> +} KVMS390FLICStateMigTmp;
>> +
>> +static void kvm_flic_ais_pre_save(void *opaque)
>> +{
>> +    KVMS390FLICStateMigTmp *tmp = opaque;
>> +    KVMS390FLICState *flic = tmp->parent;
>> +    struct kvm_s390_ais_all ais;
>> +    struct kvm_device_attr attr = {
>> +        .group = KVM_DEV_FLIC_AISM_ALL,
>> +        .addr = (uint64_t)&ais,
>> +        .attr = sizeof(ais),
>> +    };
>> +
>> +    if (ioctl(flic->fd, KVM_GET_DEVICE_ATTR, &attr)) {
>> +        error_report("Failed to retrieve kvm flic ais states");
> 
> There's not much else we can do in that case, is there?
> 
>> +        return;
>> +    }
>> +
>> +    tmp->simm = ais.simm;
>> +    tmp->nimm = ais.nimm;
>> +}
>> +
>> +static int kvm_flic_ais_post_load(void *opaque, int version_id)
>> +{
>> +    KVMS390FLICStateMigTmp *tmp = opaque;
>> +    KVMS390FLICState *flic = tmp->parent;
>> +    struct kvm_s390_ais_all ais = {
>> +        .simm = tmp->simm,
>> +        .nimm = tmp->nimm,
>> +    };
>> +    struct kvm_device_attr attr = {
>> +        .group = KVM_DEV_FLIC_AISM_ALL,
>> +        .addr = (uint64_t)&ais,
>> +    };
>> +
>> +    if (!ais_needed(flic)) {
>> +        return -ENOSYS;
>> +    }
> 
> I do not understand this... does that mean that
> - we should never get here or 
> - we should not try to change the fields or
> - I need coffee?

My understanding is that this should not happen with a normal setup, 
but it can happen if the user does something "wrong". For example
use qemu without libvirt and with -cpu host (which is not migration safe)
and do a migration from a host that has AIS to a host that has no AIS.
In that case the target system will reject the migration (late, but hopefully
not too late).

Re: [Qemu-devel] [PATCH/s390-next 2/3] s390x/cpumodel: add zpci, aen and ais facilities

[Cornelia Huck]

On Thu, 13 Jul 2017 14:29:55 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 07/13/2017 02:11 PM, Cornelia Huck wrote:
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> >> index 78ebe83..1901153 100644
> >> --- a/target/s390x/kvm.c
> >> +++ b/target/s390x/kvm.c
> >> @@ -302,6 +302,9 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
> >>          }
> >>      }
> >>  
> >> +    /* Try to enable AIS facility */
> >> +    kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);  
> > 
> > What happens if you fail to enable it? You probably don't want to allow
> > the feature bit, then?  
> 
> Then this bit is off. This call will enable it in the kernel, if that fails
> the kernel will return this bit as disabled.

Looked at the kernel code again. I thought there was more to it, but I
misremembered. No further complaints here.

> >   
> >> +
> >>      qemu_mutex_init(&qemu_sigp_mutex);
> >>  
> >>      return 0;  
> > 
> > Let's summarize to make sure that I'm not confused:
> > 
> > - Starting with zEC12 GA1, we provide zpci, aen, ais in the full model  
> yes
> 
> > - Starting with zEC12 GA1, we provide zpci and aen in the default model  
> yes. ais has to be enabled manually for z12 and z13.
> The alternative is to have ais as part of the default model. This has the big
> disadvantage that -cpu zEC12 and -cpu z13 will stop working for all available
> distro kernels. I believe that a working -cpu z13 is more important than the
> need to manually enable ais.

Agreed.

> For the future
>  - We can make ais part of the default model for a future system when that happens since
>    the features for a new system will require a new host kernel anyway

Sounds fine.

>  - We can also make ais part of the default model for a future machine type (e.g. 2.13)
>    when we believe that the world has moved on to a newer kernel

I'd rather avoid relying on that.

> 
> 
> > - In the host model, we add zpci and aen; they might be switched off
> >   after applying the found model  
> 
> We also get ais from the kernel, so the host model will have zpci,ais and
> aen
> 
> > - Compat for 2.9 and earlier switches off zpci, aen, ais  
> 
> yes 
> 
> > - We unconditionally enable the kvm part of ais  
> 
> We tell the KVM code in the kernel to enable the facility bit (before the cpu
> model might take it way) and if QEMU really uses ais, that the kernel does
> the right thing then

Yeah, that's fine, see above.

> > 
> > I'm still not sure what's supposed to happen with new qemu + old kernel
> > (no ais) + full zEC12 GA1 or later model.  
> 
> We enable aen and zpci, but disable ais for that guest. In theory a guest
> can drive PCI devices without AIS. This is a valid configuration since zpci
> does not require ais. 

Yes, also fine. Looking at the kernel code cleared things up :)

> The fact that Linux requires ais to use PCI is unfortunate but that could 
> be "fixed" Linux if necessary.

I'm not sure there's a need for this. Once a change like this has
landed in distro kernels, the same distros will also have the ais
changes in their hypervisor kernels.

Thanks for spelling things out again, this stuff always gives me a
headache.

Re: [Qemu-devel] [PATCH/s390-next 2/3] s390x/cpumodel: add zpci, aen and ais facilities

[Halil Pasic]

On 07/13/2017 02:57 PM, Cornelia Huck wrote:
> On Thu, 13 Jul 2017 14:41:05 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 07/13/2017 02:11 PM, Cornelia Huck wrote:
>>> On Thu, 13 Jul 2017 12:40:28 +0200
>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>>   
>>>> From: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>>>>
>>>> zPCI instructions and facilities are available since IBM zEnterprise
>>>> EC12. To support z/PCI in QEMU we enable zpci, aen and ais facilities
>>>> starting with zEC12 GA1. And we always set zpci and aen bits in max cpu
>>>> model. Later they might be switched off due to applied real cpu model.
>>>> For ais bit, we only provide it in the full cpu model beginning with
>>>> zEC12 and defer its enablement in the default cpu model to a later point
>>>> in time. At the same time, disable them for 2.9 and older machines.
>>>>
>>>> Because of introducing AIS facility, we could check if it's enabled to
>>>> initialize flic->ais_supported with the real value.
>>>>
>>>> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> ---
>>>>  hw/intc/s390_flic.c             | 3 ++-
>>>>  hw/intc/s390_flic_kvm.c         | 3 ---
>>>>  hw/s390x/s390-virtio-ccw.c      | 3 +++
>>>>  target/s390x/cpu_features.c     | 3 +++
>>>>  target/s390x/cpu_features_def.h | 3 +++
>>>>  target/s390x/gen-features.c     | 5 +++++
>>>>  target/s390x/kvm.c              | 7 +++++++
>>>>  7 files changed, 23 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
>>>> index ff6e4ec..6e7c610 100644
>>>> --- a/hw/intc/s390_flic.c
>>>> +++ b/hw/intc/s390_flic.c
>>>> @@ -163,9 +163,10 @@ static void s390_flic_common_realize(DeviceState *dev, Error **errp)
>>>>      if (max_batch > ADAPTER_ROUTES_MAX_GSI) {
>>>>          error_setg(errp, "flic property adapter_routes_max_batch too big"
>>>>                     " (%d > %d)", max_batch, ADAPTER_ROUTES_MAX_GSI);
>>>> +        return;  
>>>
>>> Hunk should go into a previous patch?
>>>
>>> (And does it really matter?)  
>>
>> I can
>> - move to the previous series patch 8 (s390x/flic: introduce modify_ais_mode callback)
>> - remove the return as it does not matter (this is realize, and realize failures here
>> are fatal as far as I can tell)
>> - keep it here since this version was tested and it does not hurt
>>
> 
> I'd say 2 or 3, no need to juggle existing patches for what is
> basically a non-issue.
> 

Realize failures are supposed to be fatal and it's also working
like that for flic.

I'm in favor of 3 (keeping) as the resulting code is cleaner:
it does not make any sense to 'continue realizing', even if 
'continue realizing' and set a correct ais_supported just
to fail later does not hurt.

Re: [Qemu-devel] [PATCH/s390-next 3/3] s390x/flic: migrate ais states

[Cornelia Huck]

On Thu, 13 Jul 2017 15:02:08 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 07/13/2017 02:27 PM, Cornelia Huck wrote:
> > On Thu, 13 Jul 2017 12:40:29 +0200
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >   
> >> From: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> >>
> >> During migration we should transfer ais states to the target guest.
> >> This patch introduces a subsection to kvm_s390_flic_vmstate and new
> >> vmsd for qemu_flic. The ais states need to be migrated only when
> >> ais is supported.
> >>
> >> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >> ---
> >>  hw/intc/s390_flic.c          | 20 ++++++++++++
> >>  hw/intc/s390_flic_kvm.c      | 75 ++++++++++++++++++++++++++++++++++++++++++++
> >>  include/hw/s390x/s390_flic.h |  1 +
> >>  3 files changed, 96 insertions(+)

> >> +static int kvm_flic_ais_post_load(void *opaque, int version_id)
> >> +{
> >> +    KVMS390FLICStateMigTmp *tmp = opaque;
> >> +    KVMS390FLICState *flic = tmp->parent;
> >> +    struct kvm_s390_ais_all ais = {
> >> +        .simm = tmp->simm,
> >> +        .nimm = tmp->nimm,
> >> +    };
> >> +    struct kvm_device_attr attr = {
> >> +        .group = KVM_DEV_FLIC_AISM_ALL,
> >> +        .addr = (uint64_t)&ais,
> >> +    };
> >> +
> >> +    if (!ais_needed(flic)) {
> >> +        return -ENOSYS;
> >> +    }  
> > 
> > I do not understand this... does that mean that
> > - we should never get here or 
> > - we should not try to change the fields or
> > - I need coffee?  
> 
> My understanding is that this should not happen with a normal setup, 
> but it can happen if the user does something "wrong". For example
> use qemu without libvirt and with -cpu host (which is not migration safe)
> and do a migration from a host that has AIS to a host that has no AIS.
> In that case the target system will reject the migration (late, but hopefully
> not too late).

A comment would be helpful here.

Re: [Qemu-devel] [PATCH/s390-next 2/3] s390x/cpumodel: add zpci, aen and ais facilities

[Christian Borntraeger]

On 07/13/2017 03:06 PM, Cornelia Huck wrote:
> On Thu, 13 Jul 2017 14:29:55 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 07/13/2017 02:11 PM, Cornelia Huck wrote:
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>>>> index 78ebe83..1901153 100644
>>>> --- a/target/s390x/kvm.c
>>>> +++ b/target/s390x/kvm.c
>>>> @@ -302,6 +302,9 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>>>>          }
>>>>      }
>>>>  
>>>> +    /* Try to enable AIS facility */
>>>> +    kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);  
>>>
>>> What happens if you fail to enable it? You probably don't want to allow
>>> the feature bit, then?  
>>
>> Then this bit is off. This call will enable it in the kernel, if that fails
>> the kernel will return this bit as disabled.
> 
> Looked at the kernel code again. I thought there was more to it, but I
> misremembered. No further complaints here.
> 
>>>   
>>>> +
>>>>      qemu_mutex_init(&qemu_sigp_mutex);
>>>>  
>>>>      return 0;  
>>>
>>> Let's summarize to make sure that I'm not confused:
>>>
>>> - Starting with zEC12 GA1, we provide zpci, aen, ais in the full model  
>> yes
>>
>>> - Starting with zEC12 GA1, we provide zpci and aen in the default model  
>> yes. ais has to be enabled manually for z12 and z13.
>> The alternative is to have ais as part of the default model. This has the big
>> disadvantage that -cpu zEC12 and -cpu z13 will stop working for all available
>> distro kernels. I believe that a working -cpu z13 is more important than the
>> need to manually enable ais.
> 
> Agreed.
> 
>> For the future
>>  - We can make ais part of the default model for a future system when that happens since
>>    the features for a new system will require a new host kernel anyway
> 
> Sounds fine.
> 
>>  - We can also make ais part of the default model for a future machine type (e.g. 2.13)
>>    when we believe that the world has moved on to a newer kernel
> 
> I'd rather avoid relying on that.
> 
>>
>>
>>> - In the host model, we add zpci and aen; they might be switched off
>>>   after applying the found model  
>>
>> We also get ais from the kernel, so the host model will have zpci,ais and
>> aen
>>
>>> - Compat for 2.9 and earlier switches off zpci, aen, ais  
>>
>> yes 
>>
>>> - We unconditionally enable the kvm part of ais  
>>
>> We tell the KVM code in the kernel to enable the facility bit (before the cpu
>> model might take it way) and if QEMU really uses ais, that the kernel does
>> the right thing then
> 
> Yeah, that's fine, see above.
> 
>>>
>>> I'm still not sure what's supposed to happen with new qemu + old kernel
>>> (no ais) + full zEC12 GA1 or later model.  
>>
>> We enable aen and zpci, but disable ais for that guest. In theory a guest
>> can drive PCI devices without AIS. This is a valid configuration since zpci
>> does not require ais. 
> 
> Yes, also fine. Looking at the kernel code cleared things up :)
> 
>> The fact that Linux requires ais to use PCI is unfortunate but that could 
>> be "fixed" Linux if necessary.
> 
> I'm not sure there's a need for this. Once a change like this has
> landed in distro kernels, the same distros will also have the ais
> changes in their hypervisor kernels.
> 
> Thanks for spelling things out again, this stuff always gives me a
> headache.

Assuming that I will keep the return because I like Halils explanation of
"I'm in favor of 3 (keeping) as the resulting code is cleaner:
it does not make any sense to 'continue realizing', even if 
'continue realizing' and set a correct ais_supported just
to fail later does not hurt.
"

Is that an Acked-by or Reviewed-by ?

Re: [Qemu-devel] [PATCH/s390-next 2/3] s390x/cpumodel: add zpci, aen and ais facilities

[Cornelia Huck]

On Thu, 13 Jul 2017 15:11:15 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 07/13/2017 03:06 PM, Cornelia Huck wrote:
> > On Thu, 13 Jul 2017 14:29:55 +0200
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >   
> >> On 07/13/2017 02:11 PM, Cornelia Huck wrote:
> >> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c  
> >>>> index 78ebe83..1901153 100644
> >>>> --- a/target/s390x/kvm.c
> >>>> +++ b/target/s390x/kvm.c
> >>>> @@ -302,6 +302,9 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
> >>>>          }
> >>>>      }
> >>>>  
> >>>> +    /* Try to enable AIS facility */
> >>>> +    kvm_vm_enable_cap(s, KVM_CAP_S390_AIS, 0);    
> >>>
> >>> What happens if you fail to enable it? You probably don't want to allow
> >>> the feature bit, then?    
> >>
> >> Then this bit is off. This call will enable it in the kernel, if that fails
> >> the kernel will return this bit as disabled.  
> > 
> > Looked at the kernel code again. I thought there was more to it, but I
> > misremembered. No further complaints here.
> >   
> >>>     
> >>>> +
> >>>>      qemu_mutex_init(&qemu_sigp_mutex);
> >>>>  
> >>>>      return 0;    
> >>>
> >>> Let's summarize to make sure that I'm not confused:
> >>>
> >>> - Starting with zEC12 GA1, we provide zpci, aen, ais in the full model    
> >> yes
> >>  
> >>> - Starting with zEC12 GA1, we provide zpci and aen in the default model    
> >> yes. ais has to be enabled manually for z12 and z13.
> >> The alternative is to have ais as part of the default model. This has the big
> >> disadvantage that -cpu zEC12 and -cpu z13 will stop working for all available
> >> distro kernels. I believe that a working -cpu z13 is more important than the
> >> need to manually enable ais.  
> > 
> > Agreed.
> >   
> >> For the future
> >>  - We can make ais part of the default model for a future system when that happens since
> >>    the features for a new system will require a new host kernel anyway  
> > 
> > Sounds fine.
> >   
> >>  - We can also make ais part of the default model for a future machine type (e.g. 2.13)
> >>    when we believe that the world has moved on to a newer kernel  
> > 
> > I'd rather avoid relying on that.
> >   
> >>
> >>  
> >>> - In the host model, we add zpci and aen; they might be switched off
> >>>   after applying the found model    
> >>
> >> We also get ais from the kernel, so the host model will have zpci,ais and
> >> aen
> >>  
> >>> - Compat for 2.9 and earlier switches off zpci, aen, ais    
> >>
> >> yes 
> >>  
> >>> - We unconditionally enable the kvm part of ais    
> >>
> >> We tell the KVM code in the kernel to enable the facility bit (before the cpu
> >> model might take it way) and if QEMU really uses ais, that the kernel does
> >> the right thing then  
> > 
> > Yeah, that's fine, see above.
> >   
> >>>
> >>> I'm still not sure what's supposed to happen with new qemu + old kernel
> >>> (no ais) + full zEC12 GA1 or later model.    
> >>
> >> We enable aen and zpci, but disable ais for that guest. In theory a guest
> >> can drive PCI devices without AIS. This is a valid configuration since zpci
> >> does not require ais.   
> > 
> > Yes, also fine. Looking at the kernel code cleared things up :)
> >   
> >> The fact that Linux requires ais to use PCI is unfortunate but that could 
> >> be "fixed" Linux if necessary.  
> > 
> > I'm not sure there's a need for this. Once a change like this has
> > landed in distro kernels, the same distros will also have the ais
> > changes in their hypervisor kernels.
> > 
> > Thanks for spelling things out again, this stuff always gives me a
> > headache.  
> 
> Assuming that I will keep the return because I like Halils explanation of
> "I'm in favor of 3 (keeping) as the resulting code is cleaner:
> it does not make any sense to 'continue realizing', even if 
> 'continue realizing' and set a correct ais_supported just
> to fail later does not hurt.
> "
> 
> Is that an Acked-by or Reviewed-by ?
> 

_This_ is an R-b ;)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [Qemu-devel] [PATCH/s390-next 3/3] s390x/flic: migrate ais states

[Halil Pasic]

On 07/13/2017 02:27 PM, Cornelia Huck wrote:
>> +static void kvm_flic_ais_pre_save(void *opaque)
>> +{
>> +    KVMS390FLICStateMigTmp *tmp = opaque;
>> +    KVMS390FLICState *flic = tmp->parent;
>> +    struct kvm_s390_ais_all ais;
>> +    struct kvm_device_attr attr = {
>> +        .group = KVM_DEV_FLIC_AISM_ALL,
>> +        .addr = (uint64_t)&ais,
>> +        .attr = sizeof(ais),
>> +    };
>> +
>> +    if (ioctl(flic->fd, KVM_GET_DEVICE_ATTR, &attr)) {
>> +        error_report("Failed to retrieve kvm flic ais states");
> There's not much else we can do in that case, is there?
> 

I think this is a very good question! The ioctl should not fail
under any circumstances, but if it does we have a problem.

Carrying on happily (what we do now) means effectively discarding
ais state. In general just discarding state ain't a good idea.

In particular it might be OK, but the patch should explain that!

Regarding what could/should we do in such a case (instead
of discarding state and carrying on happily) I don't know, so
I tend to agree with you regarding 'not much else we can do'.

Adding Dave and Juan. Maybe they can tell.

Regards,
Halil

>> +        return;
>> +    }
>> +
>> +    tmp->simm = ais.simm;
>> +    tmp->nimm = ais.nimm;
>> +}
>> +

Re: [Qemu-devel] [PATCH/s390-next 3/3] s390x/flic: migrate ais states

[Halil Pasic]

On 07/13/2017 03:10 PM, Cornelia Huck wrote:
> On Thu, 13 Jul 2017 15:02:08 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 07/13/2017 02:27 PM, Cornelia Huck wrote:
>>> On Thu, 13 Jul 2017 12:40:29 +0200
>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>>   
>>>> From: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>>>>
>>>> During migration we should transfer ais states to the target guest.
>>>> This patch introduces a subsection to kvm_s390_flic_vmstate and new
>>>> vmsd for qemu_flic. The ais states need to be migrated only when
>>>> ais is supported.
>>>>
>>>> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> ---
>>>>  hw/intc/s390_flic.c          | 20 ++++++++++++
>>>>  hw/intc/s390_flic_kvm.c      | 75 ++++++++++++++++++++++++++++++++++++++++++++
>>>>  include/hw/s390x/s390_flic.h |  1 +
>>>>  3 files changed, 96 insertions(+)
> 
>>>> +static int kvm_flic_ais_post_load(void *opaque, int version_id)
>>>> +{
>>>> +    KVMS390FLICStateMigTmp *tmp = opaque;
>>>> +    KVMS390FLICState *flic = tmp->parent;
>>>> +    struct kvm_s390_ais_all ais = {
>>>> +        .simm = tmp->simm,
>>>> +        .nimm = tmp->nimm,
>>>> +    };
>>>> +    struct kvm_device_attr attr = {
>>>> +        .group = KVM_DEV_FLIC_AISM_ALL,
>>>> +        .addr = (uint64_t)&ais,
>>>> +    };
>>>> +
>>>> +    if (!ais_needed(flic)) {
>>>> +        return -ENOSYS;
>>>> +    }  
>>>
>>> I do not understand this... does that mean that
>>> - we should never get here or 
>>> - we should not try to change the fields or
>>> - I need coffee?  
>>
>> My understanding is that this should not happen with a normal setup, 
>> but it can happen if the user does something "wrong". For example
>> use qemu without libvirt and with -cpu host (which is not migration safe)
>> and do a migration from a host that has AIS to a host that has no AIS.
>> In that case the target system will reject the migration (late, but hopefully
>> not too late).
> 
> A comment would be helpful here.
> 

I was actually pushing for explaining the design decisions regarding these
corner cases (there is the inverse problem too) in the commit message
(during the internal review).

I'm not sure about this "wrong" though. Can one of you provide a
reference why is the scenario described "wrong". This question
decomposes into three: 
1) How do I, as a qemu developer, know what is the management
software supposed to do? (E.g. has to use a cpu model other than
host if it wants to migrate (probably).)
2) What can I assume about the stuff not properly covered in the
user manual when I reason "right" usage and "wrong" usage.
3) Is being 'fool-proof' a design goal, or are we satisfied with
providing mechanisms for using something safely and (like with
undefined behavior in C) don't really care about possible damage
if the user does not stick to these safe mechanisms (which are
documented as such)?

I've looked up the user manual regarding migration. It was not
very helpful regarding the matter of differentiating between
right and wrong usage.

Regards,
Halil

Re: [Qemu-devel] [PATCH/s390-next 3/3] s390x/flic: migrate ais states

[Christian Borntraeger]

On 07/13/2017 03:10 PM, Cornelia Huck wrote:
> On Thu, 13 Jul 2017 15:02:08 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 07/13/2017 02:27 PM, Cornelia Huck wrote:
>>> On Thu, 13 Jul 2017 12:40:29 +0200
>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>>   
>>>> From: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>>>>
>>>> During migration we should transfer ais states to the target guest.
>>>> This patch introduces a subsection to kvm_s390_flic_vmstate and new
>>>> vmsd for qemu_flic. The ais states need to be migrated only when
>>>> ais is supported.
>>>>
>>>> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> ---
>>>>  hw/intc/s390_flic.c          | 20 ++++++++++++
>>>>  hw/intc/s390_flic_kvm.c      | 75 ++++++++++++++++++++++++++++++++++++++++++++
>>>>  include/hw/s390x/s390_flic.h |  1 +
>>>>  3 files changed, 96 insertions(+)
> 
>>>> +static int kvm_flic_ais_post_load(void *opaque, int version_id)
>>>> +{
>>>> +    KVMS390FLICStateMigTmp *tmp = opaque;
>>>> +    KVMS390FLICState *flic = tmp->parent;
>>>> +    struct kvm_s390_ais_all ais = {
>>>> +        .simm = tmp->simm,
>>>> +        .nimm = tmp->nimm,
>>>> +    };
>>>> +    struct kvm_device_attr attr = {
>>>> +        .group = KVM_DEV_FLIC_AISM_ALL,
>>>> +        .addr = (uint64_t)&ais,
>>>> +    };
>>>> +
>>>> +    if (!ais_needed(flic)) {
>>>> +        return -ENOSYS;
>>>> +    }  
>>>
>>> I do not understand this... does that mean that
>>> - we should never get here or 
>>> - we should not try to change the fields or
>>> - I need coffee?  
>>
>> My understanding is that this should not happen with a normal setup, 
>> but it can happen if the user does something "wrong". For example
>> use qemu without libvirt and with -cpu host (which is not migration safe)
>> and do a migration from a host that has AIS to a host that has no AIS.
>> In that case the target system will reject the migration (late, but hopefully
>> not too late).
> 
> A comment would be helpful here.

Something like

diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
index 4cf73ee..9b8dbd1 100644
--- a/hw/intc/s390_flic_kvm.c
+++ b/hw/intc/s390_flic_kvm.c
@@ -452,6 +452,12 @@ static int kvm_flic_ais_post_load(void *opaque, int version_id)
         .addr = (uint64_t)&ais,
     };
 
+    /* This can happen when the user mis-configures its guests in an
+     * incompatible fashion or without a CPU model. For example using
+     * qemu with -cpu host (which is not migration safe) and do a
+     * migration from a host that has AIS to a host that has no AIS.
+     * In that case the target system will reject the migration here.
+     */
     if (!ais_needed(flic)) {
         return -ENOSYS;
     }

Re: [Qemu-devel] [PATCH/s390-next 3/3] s390x/flic: migrate ais states

[Cornelia Huck]

On Thu, 13 Jul 2017 15:58:30 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 07/13/2017 03:10 PM, Cornelia Huck wrote:
> > On Thu, 13 Jul 2017 15:02:08 +0200
> > Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >   
> >> On 07/13/2017 02:27 PM, Cornelia Huck wrote:  
> >>> On Thu, 13 Jul 2017 12:40:29 +0200
> >>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >>>     
> >>>> From: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> >>>>
> >>>> During migration we should transfer ais states to the target guest.
> >>>> This patch introduces a subsection to kvm_s390_flic_vmstate and new
> >>>> vmsd for qemu_flic. The ais states need to be migrated only when
> >>>> ais is supported.
> >>>>
> >>>> Signed-off-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> >>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >>>> ---
> >>>>  hw/intc/s390_flic.c          | 20 ++++++++++++
> >>>>  hw/intc/s390_flic_kvm.c      | 75 ++++++++++++++++++++++++++++++++++++++++++++
> >>>>  include/hw/s390x/s390_flic.h |  1 +
> >>>>  3 files changed, 96 insertions(+)  
> >   
> >>>> +static int kvm_flic_ais_post_load(void *opaque, int version_id)
> >>>> +{
> >>>> +    KVMS390FLICStateMigTmp *tmp = opaque;
> >>>> +    KVMS390FLICState *flic = tmp->parent;
> >>>> +    struct kvm_s390_ais_all ais = {
> >>>> +        .simm = tmp->simm,
> >>>> +        .nimm = tmp->nimm,
> >>>> +    };
> >>>> +    struct kvm_device_attr attr = {
> >>>> +        .group = KVM_DEV_FLIC_AISM_ALL,
> >>>> +        .addr = (uint64_t)&ais,
> >>>> +    };
> >>>> +
> >>>> +    if (!ais_needed(flic)) {
> >>>> +        return -ENOSYS;
> >>>> +    }    
> >>>
> >>> I do not understand this... does that mean that
> >>> - we should never get here or 
> >>> - we should not try to change the fields or
> >>> - I need coffee?    
> >>
> >> My understanding is that this should not happen with a normal setup, 
> >> but it can happen if the user does something "wrong". For example
> >> use qemu without libvirt and with -cpu host (which is not migration safe)
> >> and do a migration from a host that has AIS to a host that has no AIS.
> >> In that case the target system will reject the migration (late, but hopefully
> >> not too late).  
> > 
> > A comment would be helpful here.  
> 
> Something like
> 
> diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
> index 4cf73ee..9b8dbd1 100644
> --- a/hw/intc/s390_flic_kvm.c
> +++ b/hw/intc/s390_flic_kvm.c
> @@ -452,6 +452,12 @@ static int kvm_flic_ais_post_load(void *opaque, int version_id)
>          .addr = (uint64_t)&ais,
>      };
>  
> +    /* This can happen when the user mis-configures its guests in an
> +     * incompatible fashion or without a CPU model. For example using
> +     * qemu with -cpu host (which is not migration safe) and do a
> +     * migration from a host that has AIS to a host that has no AIS.
> +     * In that case the target system will reject the migration here.
> +     */
>      if (!ais_needed(flic)) {
>          return -ENOSYS;
>      }

With that added:

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [Qemu-devel] [PATCH/s390-next 3/3] s390x/flic: migrate ais states

[Dr. David Alan Gilbert]

* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> 
> 
> On 07/13/2017 02:27 PM, Cornelia Huck wrote:
> >> +static void kvm_flic_ais_pre_save(void *opaque)
> >> +{
> >> +    KVMS390FLICStateMigTmp *tmp = opaque;
> >> +    KVMS390FLICState *flic = tmp->parent;
> >> +    struct kvm_s390_ais_all ais;
> >> +    struct kvm_device_attr attr = {
> >> +        .group = KVM_DEV_FLIC_AISM_ALL,
> >> +        .addr = (uint64_t)&ais,
> >> +        .attr = sizeof(ais),
> >> +    };
> >> +
> >> +    if (ioctl(flic->fd, KVM_GET_DEVICE_ATTR, &attr)) {
> >> +        error_report("Failed to retrieve kvm flic ais states");
> > There's not much else we can do in that case, is there?
> > 
> 
> I think this is a very good question! The ioctl should not fail
> under any circumstances, but if it does we have a problem.
> 
> Carrying on happily (what we do now) means effectively discarding
> ais state. In general just discarding state ain't a good idea.
> 
> In particular it might be OK, but the patch should explain that!
> 
> Regarding what could/should we do in such a case (instead
> of discarding state and carrying on happily) I don't know, so
> I tend to agree with you regarding 'not much else we can do'.
> 
> Adding Dave and Juan. Maybe they can tell.

I keep meaning to make the pre_save give a return value for failure,
but it hasn't currently got one.

You could try something like:

  qemu_file_set_error(migrate_get_current()->to_dst_file, -EINVAL);

  I *think* the migration code should spot that before it finishes
but it might carry on for a little while before it does.

Dave

> Regards,
> Halil
> 
> >> +        return;
> >> +    }
> >> +
> >> +    tmp->simm = ais.simm;
> >> +    tmp->nimm = ais.nimm;
> >> +}
> >> +
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH/s390-next 3/3] s390x/flic: migrate ais states

[Christian Borntraeger]

On 07/13/2017 04:49 PM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>>
>>
>> On 07/13/2017 02:27 PM, Cornelia Huck wrote:
>>>> +static void kvm_flic_ais_pre_save(void *opaque)
>>>> +{
>>>> +    KVMS390FLICStateMigTmp *tmp = opaque;
>>>> +    KVMS390FLICState *flic = tmp->parent;
>>>> +    struct kvm_s390_ais_all ais;
>>>> +    struct kvm_device_attr attr = {
>>>> +        .group = KVM_DEV_FLIC_AISM_ALL,
>>>> +        .addr = (uint64_t)&ais,
>>>> +        .attr = sizeof(ais),
>>>> +    };
>>>> +
>>>> +    if (ioctl(flic->fd, KVM_GET_DEVICE_ATTR, &attr)) {
>>>> +        error_report("Failed to retrieve kvm flic ais states");
>>> There's not much else we can do in that case, is there?
>>>
>>
>> I think this is a very good question! The ioctl should not fail
>> under any circumstances, but if it does we have a problem.
>>
>> Carrying on happily (what we do now) means effectively discarding
>> ais state. In general just discarding state ain't a good idea.
>>
>> In particular it might be OK, but the patch should explain that!
>>
>> Regarding what could/should we do in such a case (instead
>> of discarding state and carrying on happily) I don't know, so
>> I tend to agree with you regarding 'not much else we can do'.
>>
>> Adding Dave and Juan. Maybe they can tell.
> 
> I keep meaning to make the pre_save give a return value for failure,
> but it hasn't currently got one.

Would you accept patches for that?

> 
> You could try something like:
> 
>   qemu_file_set_error(migrate_get_current()->to_dst_file, -EINVAL);
> 
>   I *think* the migration code should spot that before it finishes
> but it might carry on for a little while before it does.

I will keep this patch as is, since this is one of the "should not happen"
cases.

Re: [Qemu-devel] [PATCH/s390-next 3/3] s390x/flic: migrate ais states

[Dr. David Alan Gilbert]

* Christian Borntraeger (borntraeger@de.ibm.com) wrote:
> On 07/13/2017 04:49 PM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> >>
> >>
> >> On 07/13/2017 02:27 PM, Cornelia Huck wrote:
> >>>> +static void kvm_flic_ais_pre_save(void *opaque)
> >>>> +{
> >>>> +    KVMS390FLICStateMigTmp *tmp = opaque;
> >>>> +    KVMS390FLICState *flic = tmp->parent;
> >>>> +    struct kvm_s390_ais_all ais;
> >>>> +    struct kvm_device_attr attr = {
> >>>> +        .group = KVM_DEV_FLIC_AISM_ALL,
> >>>> +        .addr = (uint64_t)&ais,
> >>>> +        .attr = sizeof(ais),
> >>>> +    };
> >>>> +
> >>>> +    if (ioctl(flic->fd, KVM_GET_DEVICE_ATTR, &attr)) {
> >>>> +        error_report("Failed to retrieve kvm flic ais states");
> >>> There's not much else we can do in that case, is there?
> >>>
> >>
> >> I think this is a very good question! The ioctl should not fail
> >> under any circumstances, but if it does we have a problem.
> >>
> >> Carrying on happily (what we do now) means effectively discarding
> >> ais state. In general just discarding state ain't a good idea.
> >>
> >> In particular it might be OK, but the patch should explain that!
> >>
> >> Regarding what could/should we do in such a case (instead
> >> of discarding state and carrying on happily) I don't know, so
> >> I tend to agree with you regarding 'not much else we can do'.
> >>
> >> Adding Dave and Juan. Maybe they can tell.
> > 
> > I keep meaning to make the pre_save give a return value for failure,
> > but it hasn't currently got one.
> 
> Would you accept patches for that?

Sure.

> > 
> > You could try something like:
> > 
> >   qemu_file_set_error(migrate_get_current()->to_dst_file, -EINVAL);
> > 
> >   I *think* the migration code should spot that before it finishes
> > but it might carry on for a little while before it does.
> 
> I will keep this patch as is, since this is one of the "should not happen"
> cases.

And you've got a log message, so when the impossible happens at least
we should be able to debug it.

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH/s390-next 3/3] s390x/flic: migrate ais states

[Cornelia Huck]

On Thu, 13 Jul 2017 17:05:45 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 07/13/2017 04:49 PM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:  
> >>
> >>
> >> On 07/13/2017 02:27 PM, Cornelia Huck wrote:  
> >>>> +static void kvm_flic_ais_pre_save(void *opaque)
> >>>> +{
> >>>> +    KVMS390FLICStateMigTmp *tmp = opaque;
> >>>> +    KVMS390FLICState *flic = tmp->parent;
> >>>> +    struct kvm_s390_ais_all ais;
> >>>> +    struct kvm_device_attr attr = {
> >>>> +        .group = KVM_DEV_FLIC_AISM_ALL,
> >>>> +        .addr = (uint64_t)&ais,
> >>>> +        .attr = sizeof(ais),
> >>>> +    };
> >>>> +
> >>>> +    if (ioctl(flic->fd, KVM_GET_DEVICE_ATTR, &attr)) {
> >>>> +        error_report("Failed to retrieve kvm flic ais states");  
> >>> There's not much else we can do in that case, is there?
> >>>  
> >>
> >> I think this is a very good question! The ioctl should not fail
> >> under any circumstances, but if it does we have a problem.
> >>
> >> Carrying on happily (what we do now) means effectively discarding
> >> ais state. In general just discarding state ain't a good idea.
> >>
> >> In particular it might be OK, but the patch should explain that!
> >>
> >> Regarding what could/should we do in such a case (instead
> >> of discarding state and carrying on happily) I don't know, so
> >> I tend to agree with you regarding 'not much else we can do'.
> >>
> >> Adding Dave and Juan. Maybe they can tell.  
> > 
> > I keep meaning to make the pre_save give a return value for failure,
> > but it hasn't currently got one.  
> 
> Would you accept patches for that?
> 
> > 
> > You could try something like:
> > 
> >   qemu_file_set_error(migrate_get_current()->to_dst_file, -EINVAL);
> > 
> >   I *think* the migration code should spot that before it finishes
> > but it might carry on for a little while before it does.  
> 
> I will keep this patch as is, since this is one of the "should not happen"
> cases.
> 

Fine with me.

Re: [Qemu-devel] [PATCH/s390-next 3/3] s390x/flic: migrate ais states

[Halil Pasic]

On 07/13/2017 05:11 PM, Dr. David Alan Gilbert wrote:
> * Christian Borntraeger (borntraeger@de.ibm.com) wrote:
>> On 07/13/2017 04:49 PM, Dr. David Alan Gilbert wrote:
>>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>>>>
>>>>
>>>> On 07/13/2017 02:27 PM, Cornelia Huck wrote:
>>>>>> +static void kvm_flic_ais_pre_save(void *opaque)
>>>>>> +{
>>>>>> +    KVMS390FLICStateMigTmp *tmp = opaque;
>>>>>> +    KVMS390FLICState *flic = tmp->parent;
>>>>>> +    struct kvm_s390_ais_all ais;
>>>>>> +    struct kvm_device_attr attr = {
>>>>>> +        .group = KVM_DEV_FLIC_AISM_ALL,
>>>>>> +        .addr = (uint64_t)&ais,
>>>>>> +        .attr = sizeof(ais),
>>>>>> +    };
>>>>>> +
>>>>>> +    if (ioctl(flic->fd, KVM_GET_DEVICE_ATTR, &attr)) {
>>>>>> +        error_report("Failed to retrieve kvm flic ais states");
>>>>> There's not much else we can do in that case, is there?
>>>>>
>>>>
>>>> I think this is a very good question! The ioctl should not fail
>>>> under any circumstances, but if it does we have a problem.
>>>>
>>>> Carrying on happily (what we do now) means effectively discarding
>>>> ais state. In general just discarding state ain't a good idea.
>>>>
>>>> In particular it might be OK, but the patch should explain that!
>>>>
>>>> Regarding what could/should we do in such a case (instead
>>>> of discarding state and carrying on happily) I don't know, so
>>>> I tend to agree with you regarding 'not much else we can do'.
>>>>
>>>> Adding Dave and Juan. Maybe they can tell.
>>>
>>> I keep meaning to make the pre_save give a return value for failure,
>>> but it hasn't currently got one.
>>
>> Would you accept patches for that?
> 
> Sure.
> 
>>>
>>> You could try something like:
>>>
>>>   qemu_file_set_error(migrate_get_current()->to_dst_file, -EINVAL);

@Dave:
Thanks Dave! I was not aware of that! Had a quick look at the
code, I think qemu_file_set_error would indeed do the right thing.

I would prefer error handling being part of the pre_save interface,
because that would be easier to understand, and would provoke thinking
about these problems.

@Christian:
Would you like to implement 'return value for pre_save'
yourself? I mean, I the meanwhile I'm familiar with the code in question
and I enjoy working with Dave and Juan, so if you aren't interested in
doing it yourself but think it's important enough to get it done, I could
take it too?

@Dave:
There are a couple of questions I'm gonna have to ask/investigate should
it be me doing the 'return value for pre_save' (also notes to myself):

Would you see this error handling via pre_save as a parallel infrastructure
(keep the current qemu_file_set_error mechanism) or would you prefer
things converted? IMHO having a single method would be cleaner, but I
have not looked into this in great detail.

Also the question what is the semantic of qemu_file_set_error arises.
It ain't documented and I would intuitively suspect that it's rather
about the 'file' (that is transport) than the whole migration.



>>>
>>>   I *think* the migration code should spot that before it finishes
>>> but it might carry on for a little while before it does.
>>
>> I will keep this patch as is, since this is one of the "should not happen"
>> cases.

@Christian
I'm OK with it, because knowing the kernel code behind the ioctl
this is really unlikely and even if it should happen the risks involved
are rather limited. But I would be much happier if all such
cases would result in refusing migration.

Regards,
Halil

Re: [Qemu-devel] [PATCH/s390-next 3/3] s390x/flic: migrate ais states

[Dr. David Alan Gilbert]

* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> 
> 
> On 07/13/2017 05:11 PM, Dr. David Alan Gilbert wrote:
> > * Christian Borntraeger (borntraeger@de.ibm.com) wrote:
> >> On 07/13/2017 04:49 PM, Dr. David Alan Gilbert wrote:
> >>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> >>>>
> >>>>
> >>>> On 07/13/2017 02:27 PM, Cornelia Huck wrote:
> >>>>>> +static void kvm_flic_ais_pre_save(void *opaque)
> >>>>>> +{
> >>>>>> +    KVMS390FLICStateMigTmp *tmp = opaque;
> >>>>>> +    KVMS390FLICState *flic = tmp->parent;
> >>>>>> +    struct kvm_s390_ais_all ais;
> >>>>>> +    struct kvm_device_attr attr = {
> >>>>>> +        .group = KVM_DEV_FLIC_AISM_ALL,
> >>>>>> +        .addr = (uint64_t)&ais,
> >>>>>> +        .attr = sizeof(ais),
> >>>>>> +    };
> >>>>>> +
> >>>>>> +    if (ioctl(flic->fd, KVM_GET_DEVICE_ATTR, &attr)) {
> >>>>>> +        error_report("Failed to retrieve kvm flic ais states");
> >>>>> There's not much else we can do in that case, is there?
> >>>>>
> >>>>
> >>>> I think this is a very good question! The ioctl should not fail
> >>>> under any circumstances, but if it does we have a problem.
> >>>>
> >>>> Carrying on happily (what we do now) means effectively discarding
> >>>> ais state. In general just discarding state ain't a good idea.
> >>>>
> >>>> In particular it might be OK, but the patch should explain that!
> >>>>
> >>>> Regarding what could/should we do in such a case (instead
> >>>> of discarding state and carrying on happily) I don't know, so
> >>>> I tend to agree with you regarding 'not much else we can do'.
> >>>>
> >>>> Adding Dave and Juan. Maybe they can tell.
> >>>
> >>> I keep meaning to make the pre_save give a return value for failure,
> >>> but it hasn't currently got one.
> >>
> >> Would you accept patches for that?
> > 
> > Sure.
> > 
> >>>
> >>> You could try something like:
> >>>
> >>>   qemu_file_set_error(migrate_get_current()->to_dst_file, -EINVAL);
> 
> @Dave:
> Thanks Dave! I was not aware of that! Had a quick look at the
> code, I think qemu_file_set_error would indeed do the right thing.
> 
> I would prefer error handling being part of the pre_save interface,
> because that would be easier to understand, and would provoke thinking
> about these problems.
> 
> @Christian:
> Would you like to implement 'return value for pre_save'
> yourself? I mean, I the meanwhile I'm familiar with the code in question
> and I enjoy working with Dave and Juan, so if you aren't interested in
> doing it yourself but think it's important enough to get it done, I could
> take it too?
> 
> @Dave:
> There are a couple of questions I'm gonna have to ask/investigate should
> it be me doing the 'return value for pre_save' (also notes to myself):
> 
> Would you see this error handling via pre_save as a parallel infrastructure
> (keep the current qemu_file_set_error mechanism) or would you prefer
> things converted? IMHO having a single method would be cleaner, but I
> have not looked into this in great detail.

The only thing I'd like to change is make pre_save be:
  int (*pre_save)(void *opaque)

  rather than void.
If there are any current pre_save's that call set_error or assert or
anything like that then they could be converted, but I don't think there
are many.

> Also the question what is the semantic of qemu_file_set_error arises.
> It ain't documented and I would intuitively suspect that it's rather
> about the 'file' (that is transport) than the whole migration.

It's really an internal interface in migration (where it's not
very nice either); but really it shouldn't be used for anything else.

Dave
> 
> 
> >>>
> >>>   I *think* the migration code should spot that before it finishes
> >>> but it might carry on for a little while before it does.
> >>
> >> I will keep this patch as is, since this is one of the "should not happen"
> >> cases.
> 
> @Christian
> I'm OK with it, because knowing the kernel code behind the ioctl
> this is really unlikely and even if it should happen the risks involved
> are rather limited. But I would be much happier if all such
> cases would result in refusing migration.
> 
> Regards,
> Halil
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH/s390-next 3/3] s390x/flic: migrate ais states

[Juan Quintela]

Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> On 07/13/2017 05:11 PM, Dr. David Alan Gilbert wrote:
>> * Christian Borntraeger (borntraeger@de.ibm.com) wrote:
>>> On 07/13/2017 04:49 PM, Dr. David Alan Gilbert wrote:
>>>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:

> @Dave:
> There are a couple of questions I'm gonna have to ask/investigate should
> it be me doing the 'return value for pre_save' (also notes to myself):

As Dave said, just changing the prototype and fix callers will do a lot
of good.

> (keep the current qemu_file_set_error mechanism) or would you prefer
> things converted? IMHO having a single method would be cleaner, but I
> have not looked into this in great detail.

Error handling is a mess in qemu (in general), and worse in migration
(in particular).

History lesson (that is even older than my involvement with qemu)

It used to be that evrything was done with callbacks in the main loop.
So there were not an easy way to pass error messages.  Everything was
done through qemu_file_set_error() (well, actually it was done by hand).

Then the migration thread came, and we can do things asyncronously, so
we don't need to store the state in callbacks.  I did some changes long,
long ago about returning  error messages instead of:

if (error) {
   qemu_file_set_error(f, -error);
   return;
}

to

if (error) {
    return -error;
}

/* fix things to make errors negative, etc, etc, you get the idea */

But we need to fix some things to make this work.

Reception side is done through a coroutine, and then some of such things
still stand.  Moving to a thread would be a good idea at some point O:-)

> Also the question what is the semantic of qemu_file_set_error arises.
> It ain't documented and I would intuitively suspect that it's rather
> about the 'file' (that is transport) than the whole migration.

We test in the "interesting" parts if there is an error on the file,

from vmstate_load_state()

                } else {
                    ret = field->info->get(f, curr_elem, size, field);
                }
                if (ret >= 0) {
                    ret = qemu_file_get_error(f);
                }
                if (ret < 0) {
                    qemu_file_set_error(f, ret);
                    error_report("Failed to load %s:%s", vmsd->name,
                                 field->name);
                    trace_vmstate_load_field_error(field->name, ret);
                    return ret;
                }


You can see how many convolutions we do to maintain the error returned
and the one in QEMUFile in sync.  Removing the one in QEMUFile would be
a good idea, but requires lots of changes.

For starters, see that all qemu_get_* return void.

To add insult to injury:

int qemu_peek_byte(QEMUFile *f, int offset)
{
    int index = f->buf_index + offset;

    assert(!qemu_file_is_writable(f));
    assert(offset < IO_BUF_SIZE);

    if (index >= f->buf_size) {
        qemu_fill_buffer(f);
        index = f->buf_index + offset;
        if (index >= f->buf_size) {
            return 0;
        }
    }
    return f->buf[index];
}

I.e. it never returns errors, if there is nothing to read, it just
return 0.

/me stops

>
>
>
>>>>
>>>>   I *think* the migration code should spot that before it finishes
>>>> but it might carry on for a little while before it does.
>>>
>>> I will keep this patch as is, since this is one of the "should not happen"
>>> cases.

Later, Juan.

Re: [Qemu-devel] [PATCH/s390-next 3/3] s390x/flic: migrate ais states

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> > On 07/13/2017 05:11 PM, Dr. David Alan Gilbert wrote:
> >> * Christian Borntraeger (borntraeger@de.ibm.com) wrote:
> >>> On 07/13/2017 04:49 PM, Dr. David Alan Gilbert wrote:
> >>>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> 
> > @Dave:
> > There are a couple of questions I'm gonna have to ask/investigate should
> > it be me doing the 'return value for pre_save' (also notes to myself):
> 
> As Dave said, just changing the prototype and fix callers will do a lot
> of good.
> 
> > (keep the current qemu_file_set_error mechanism) or would you prefer
> > things converted? IMHO having a single method would be cleaner, but I
> > have not looked into this in great detail.
> 
> Error handling is a mess in qemu (in general), and worse in migration
> (in particular).
> 
> History lesson (that is even older than my involvement with qemu)
> 
> It used to be that evrything was done with callbacks in the main loop.
> So there were not an easy way to pass error messages.  Everything was
> done through qemu_file_set_error() (well, actually it was done by hand).
> 
> Then the migration thread came, and we can do things asyncronously, so
> we don't need to store the state in callbacks.  I did some changes long,
> long ago about returning  error messages instead of:
> 
> if (error) {
>    qemu_file_set_error(f, -error);
>    return;
> }
> 
> to
> 
> if (error) {
>     return -error;
> }
> 
> /* fix things to make errors negative, etc, etc, you get the idea */
> 
> But we need to fix some things to make this work.
> 
> Reception side is done through a coroutine, and then some of such things
> still stand.  Moving to a thread would be a good idea at some point O:-)
> 
> > Also the question what is the semantic of qemu_file_set_error arises.
> > It ain't documented and I would intuitively suspect that it's rather
> > about the 'file' (that is transport) than the whole migration.
> 
> We test in the "interesting" parts if there is an error on the file,
> 
> from vmstate_load_state()
> 
>                 } else {
>                     ret = field->info->get(f, curr_elem, size, field);
>                 }
>                 if (ret >= 0) {
>                     ret = qemu_file_get_error(f);
>                 }
>                 if (ret < 0) {
>                     qemu_file_set_error(f, ret);
>                     error_report("Failed to load %s:%s", vmsd->name,
>                                  field->name);
>                     trace_vmstate_load_field_error(field->name, ret);
>                     return ret;
>                 }
> 
> 
> You can see how many convolutions we do to maintain the error returned
> and the one in QEMUFile in sync.  Removing the one in QEMUFile would be
> a good idea, but requires lots of changes.

Yes, I'd like to get to having the ones in QEMUFile just representing
actual file IO errors rather than other types of problem.

Dave

> For starters, see that all qemu_get_* return void.
> 
> To add insult to injury:
> 
> int qemu_peek_byte(QEMUFile *f, int offset)
> {
>     int index = f->buf_index + offset;
> 
>     assert(!qemu_file_is_writable(f));
>     assert(offset < IO_BUF_SIZE);
> 
>     if (index >= f->buf_size) {
>         qemu_fill_buffer(f);
>         index = f->buf_index + offset;
>         if (index >= f->buf_size) {
>             return 0;
>         }
>     }
>     return f->buf[index];
> }
> 
> I.e. it never returns errors, if there is nothing to read, it just
> return 0.
> 
> /me stops
> 
> >
> >
> >
> >>>>
> >>>>   I *think* the migration code should spot that before it finishes
> >>>> but it might carry on for a little while before it does.
> >>>
> >>> I will keep this patch as is, since this is one of the "should not happen"
> >>> cases.
> 
> Later, Juan.
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH/s390-next 3/3] s390x/flic: migrate ais states

[Halil Pasic]

On 07/18/2017 12:31 PM, Juan Quintela wrote:
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>> On 07/13/2017 05:11 PM, Dr. David Alan Gilbert wrote:
>>> * Christian Borntraeger (borntraeger@de.ibm.com) wrote:
>>>> On 07/13/2017 04:49 PM, Dr. David Alan Gilbert wrote:
>>>>> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> 
>> @Dave:
>> There are a couple of questions I'm gonna have to ask/investigate should
>> it be me doing the 'return value for pre_save' (also notes to myself):
> 
> As Dave said, just changing the prototype and fix callers will do a lot
> of good.
> 
>> (keep the current qemu_file_set_error mechanism) or would you prefer
>> things converted? IMHO having a single method would be cleaner, but I
>> have not looked into this in great detail.
> 
> Error handling is a mess in qemu (in general), and worse in migration
> (in particular).
> 
> History lesson (that is even older than my involvement with qemu)
> 
> It used to be that evrything was done with callbacks in the main loop.
> So there were not an easy way to pass error messages.  Everything was
> done through qemu_file_set_error() (well, actually it was done by hand).
> 
> Then the migration thread came, and we can do things asyncronously, so
> we don't need to store the state in callbacks.  I did some changes long,
> long ago about returning  error messages instead of:
> 
> if (error) {
>    qemu_file_set_error(f, -error);
>    return;
> }
> 
> to
> 
> if (error) {
>     return -error;
> }
> 
> /* fix things to make errors negative, etc, etc, you get the idea */
> 
> But we need to fix some things to make this work.
> 
> Reception side is done through a coroutine, and then some of such things
> still stand.  Moving to a thread would be a good idea at some point O:-)
> 
>> Also the question what is the semantic of qemu_file_set_error arises.
>> It ain't documented and I would intuitively suspect that it's rather
>> about the 'file' (that is transport) than the whole migration.
> 
> We test in the "interesting" parts if there is an error on the file,
> 
> from vmstate_load_state()
> 
>                 } else {
>                     ret = field->info->get(f, curr_elem, size, field);
>                 }
>                 if (ret >= 0) {
>                     ret = qemu_file_get_error(f);
>                 }
>                 if (ret < 0) {
>                     qemu_file_set_error(f, ret);
>                     error_report("Failed to load %s:%s", vmsd->name,
>                                  field->name);
>                     trace_vmstate_load_field_error(field->name, ret);
>                     return ret;
>                 }
> 
> 
> You can see how many convolutions we do to maintain the error returned
> and the one in QEMUFile in sync.  Removing the one in QEMUFile would be
> a good idea, but requires lots of changes.
> 
> For starters, see that all qemu_get_* return void.
> 
> To add insult to injury:
> 
> int qemu_peek_byte(QEMUFile *f, int offset)
> {
>     int index = f->buf_index + offset;
> 
>     assert(!qemu_file_is_writable(f));
>     assert(offset < IO_BUF_SIZE);
> 
>     if (index >= f->buf_size) {
>         qemu_fill_buffer(f);
>         index = f->buf_index + offset;
>         if (index >= f->buf_size) {
>             return 0;
>         }
>     }
>     return f->buf[index];
> }
> 
> I.e. it never returns errors, if there is nothing to read, it just
> return 0.
> 
> /me stops
> 

Hi Juan,

Many thanks for providing me with your perspective and a little history
lesson too. Knowing the history things do make much more sense. For me
this is quite often the case (history helps understanding) but at the
same time extracting the historical perspective form git is in my experience
quite time consuming in average case, so I don't try too often (without
having a very good reason).

I will keep the things you explained in mind and try to push the
migration subsystem into the right direction as far my time allows
(or at least avoid pushing in the wrong direction).

Regards,
Halil

>>
>>
>>
>>>>>
>>>>>   I *think* the migration code should spot that before it finishes
>>>>> but it might carry on for a little while before it does.
>>>>
>>>> I will keep this patch as is, since this is one of the "should not happen"
>>>> cases.
> 
> Later, Juan.
>