[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/1] s390x: vmstatify config migration for virti
From: |
Halil Pasic |
Subject: |
Re: [Qemu-devel] [PATCH 1/1] s390x: vmstatify config migration for virtio-ccw |
Date: |
Thu, 1 Jun 2017 13:28:41 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 |
On 06/01/2017 01:19 PM, Christian Borntraeger wrote:
> On 06/01/2017 01:02 PM, Halil Pasic wrote:
>>
>>
>> On 06/01/2017 12:52 PM, Cornelia Huck wrote:
>>> On Mon, 29 May 2017 15:17:16 +0200
>>> Halil Pasic <address@hidden> wrote:
>>>
>>>> Let's vmstatify virtio_ccw_save_config and virtio_ccw_load_config for
>>>> flexibility (extending using subsections) and for fun.
>>>
>>> You have interesting ideas of fun :)
>>>
>>>>
>>>> To achieve this we need to hack the config_vector, which is VirtIODevice
>>>> (that is common virtio) state, in the middle of the VirtioCcwDevice state
>>>> representation. This somewhat ugly, but we have no choice because the
>>>> stream format needs to be preserved.
>>>>
>>>> Almost no changes in behavior. Exception is everything that comes with
>>>> vmstate like extra bookkeeping about what's in the stream, and maybe some
>>>> extra checks and better error reporting.
>>>>
>>>> Signed-off-by: Halil Pasic <address@hidden>
>>>> ---
>> [..]
>>>> --- a/hw/s390x/css.c
>>>> +++ b/hw/s390x/css.c
>> [..]
>>>> +static const VMStateDescription vmstate_sense_id = {
>>>> + .name = "s390_sense_id",
>>>> + .version_id = 1,
>>>> + .minimum_version_id = 1,
>>>> + .fields = (VMStateField[]) {
>>>> + VMSTATE_UINT8(reserved, SenseId),
>>>> + VMSTATE_UINT16(cu_type, SenseId),
>>>> + VMSTATE_UINT8(cu_model, SenseId),
>>>> + VMSTATE_UINT16(dev_type, SenseId),
>>>> + VMSTATE_UINT8(dev_model, SenseId),
>>>> + VMSTATE_UINT8(unused, SenseId),
>>>
>>> This is strictly due to stream format preservation, right?
>>>
>>
>> Yes!
>>
>> [..]
>>>> --- a/hw/s390x/virtio-ccw.c
>>>> +++ b/hw/s390x/virtio-ccw.c
>>>> @@ -34,9 +34,87 @@
>>>> #include "virtio-ccw.h"
>>>> #include "trace.h"
>>>> #include "hw/s390x/css-bridge.h"
>>>> +#include "hw/s390x/s390-virtio-ccw.h"
>>>>
>>>> #define NR_CLASSIC_INDICATOR_BITS 64
>>>>
>>>> +static int virtio_ccw_dev_post_load(void *opaque, int version_id)
>>>> +{
>>>> + VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(opaque);
>>>> + CcwDevice *ccw_dev = CCW_DEVICE(dev);
>>>> + CCWDeviceClass *ck = CCW_DEVICE_GET_CLASS(ccw_dev);
>>>> +
>>>> + ccw_dev->sch->driver_data = dev;
>>>> + if (CCW_DEVICE(dev)->sch->thinint_active) {
>>>
>>> Please use ccw_dev instead of CCW_DEVICE(dev).
>>>
>>
>> Sure.
>>
>>>> + dev->routes.adapter.adapter_id = css_get_adapter_id(
>>>> + CSS_IO_ADAPTER_VIRTIO,
>>>> + dev->thinint_isc);
>>>> + }
>>>> + /* Re-fill subch_id after loading the subchannel states.*/
>>>> + if (ck->refill_ids) {
>>>> + ck->refill_ids(ccw_dev);
>>>> + }
>>>> + return 0;
>>>> +}
>>>> +
>>>> +typedef struct VirtioCcwDeviceTmp {
>>>> + VirtioCcwDevice *parent;
>>>> + uint16_t config_vector;
>>>> +} VirtioCcwDeviceTmp;
>>>> +
>>>> +static void virtio_ccw_dev_tmp_pre_save(void *opaque)
>>>> +{
>>>> + VirtioCcwDeviceTmp *tmp = opaque;
>>>> + VirtioCcwDevice *dev = tmp->parent;
>>>> + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
>>>> +
>>>> + tmp->config_vector = vdev->config_vector;
>>>> +}
>>>> +
>>>> +static int virtio_ccw_dev_tmp_post_load(void *opaque, int version_id)
>>>> +{
>>>> + VirtioCcwDeviceTmp *tmp = opaque;
>>>> + VirtioCcwDevice *dev = tmp->parent;
>>>> + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus);
>>>> +
>>>> + vdev->config_vector = tmp->config_vector;
>>>> + return 0;
>>>> +}
>>>> +
>>>> +const VMStateDescription vmstate_virtio_ccw_dev_tmp = {
>>>> + .name = "s390_virtio_ccw_dev_tmp",
>>>> + .pre_save = virtio_ccw_dev_tmp_pre_save,
>>>> + .post_load = virtio_ccw_dev_tmp_post_load,
>>>> + .fields = (VMStateField[]) {
>>>> + VMSTATE_UINT16(config_vector, VirtioCcwDeviceTmp),
>>>> + VMSTATE_END_OF_LIST()
>>>> + }
>>>> +};
>>>> +
>>>> +const VMStateDescription vmstate_virtio_ccw_dev = {
>>>> + .name = "s390_virtio_ccw_dev",
>>>> + .version_id = 1,
>>>> + .minimum_version_id = 1,
>>>> + .post_load = virtio_ccw_dev_post_load,
>>>> + .fields = (VMStateField[]) {
>>>> + VMSTATE_CCW_DEVICE(parent_obj, VirtioCcwDevice),
>>>> + VMSTATE_PTR_TO_IND_ADDR(indicators, VirtioCcwDevice),
>>>> + VMSTATE_PTR_TO_IND_ADDR(indicators2, VirtioCcwDevice),
>>>> + VMSTATE_PTR_TO_IND_ADDR(summary_indicator, VirtioCcwDevice),
>>>> + /*
>>>> + * Ugly hack because VirtIODevice does not migrate itself.
>>>> + * This also makes legacy via vmstate_save_state possible.
>>>> + */
>>>> + VMSTATE_WITH_TMP(VirtioCcwDevice, VirtioCcwDeviceTmp,
>>>> + vmstate_virtio_ccw_dev_tmp),
>>>> + VMSTATE_STRUCT(routes, VirtioCcwDevice, 1,
>>>> vmstate_adapter_routes, \
>>>> + AdapterRoutes),
>>>> + VMSTATE_UINT8(thinint_isc, VirtioCcwDevice),
>>>> + VMSTATE_INT32(revision, VirtioCcwDevice),
>>>> + VMSTATE_END_OF_LIST()
>>>> + }
>>>> +};
>>>> +
>>>> static void virtio_ccw_bus_new(VirtioBusState *bus, size_t bus_size,
>>>> VirtioCcwDevice *dev);
>>>>
>>>> @@ -1234,85 +1312,13 @@ static int virtio_ccw_load_queue(DeviceState *d,
>>>> int n, QEMUFile *f)
>>>> static void virtio_ccw_save_config(DeviceState *d, QEMUFile *f)
>>>> {
>>>> VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
>>>> - CcwDevice *ccw_dev = CCW_DEVICE(d);
>>>> - SubchDev *s = ccw_dev->sch;
>>>> - VirtIODevice *vdev = virtio_ccw_get_vdev(s);
>>>> -
>>>> - subch_device_save(s, f);
>>>> - if (dev->indicators != NULL) {
>>>> - qemu_put_be32(f, dev->indicators->len);
>>>> - qemu_put_be64(f, dev->indicators->addr);
>>>> - } else {
>>>> - qemu_put_be32(f, 0);
>>>> - qemu_put_be64(f, 0UL);
>>>> - }
>>>> - if (dev->indicators2 != NULL) {
>>>> - qemu_put_be32(f, dev->indicators2->len);
>>>> - qemu_put_be64(f, dev->indicators2->addr);
>>>> - } else {
>>>> - qemu_put_be32(f, 0);
>>>> - qemu_put_be64(f, 0UL);
>>>> - }
>>>> - if (dev->summary_indicator != NULL) {
>>>> - qemu_put_be32(f, dev->summary_indicator->len);
>>>> - qemu_put_be64(f, dev->summary_indicator->addr);
>>>> - } else {
>>>> - qemu_put_be32(f, 0);
>>>> - qemu_put_be64(f, 0UL);
>>>> - }
>>>> - qemu_put_be16(f, vdev->config_vector);
>>>> - qemu_put_be64(f, dev->routes.adapter.ind_offset);
>>>> - qemu_put_byte(f, dev->thinint_isc);
>>>> - qemu_put_be32(f, dev->revision);
>>>> + vmstate_save_state(f, &vmstate_virtio_ccw_dev, dev, NULL);
>>>> }
>>>>
>>>> static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f)
>>>> {
>>>> VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
>>>> - CcwDevice *ccw_dev = CCW_DEVICE(d);
>>>> - CCWDeviceClass *ck = CCW_DEVICE_GET_CLASS(ccw_dev);
>>>> - SubchDev *s = ccw_dev->sch;
>>>> - VirtIODevice *vdev = virtio_ccw_get_vdev(s);
>>>> - int len;
>>>> -
>>>> - s->driver_data = dev;
>>>> - subch_device_load(s, f);
>>>> - /* Re-fill subch_id after loading the subchannel states.*/
>>>> - if (ck->refill_ids) {
>>>> - ck->refill_ids(ccw_dev);
>>>> - }
>>>> - len = qemu_get_be32(f);
>>>> - if (len != 0) {
>>>> - dev->indicators = get_indicator(qemu_get_be64(f), len);
>>>> - } else {
>>>> - qemu_get_be64(f);
>>>> - dev->indicators = NULL;
>>>> - }
>>>> - len = qemu_get_be32(f);
>>>> - if (len != 0) {
>>>> - dev->indicators2 = get_indicator(qemu_get_be64(f), len);
>>>> - } else {
>>>> - qemu_get_be64(f);
>>>> - dev->indicators2 = NULL;
>>>> - }
>>>> - len = qemu_get_be32(f);
>>>> - if (len != 0) {
>>>> - dev->summary_indicator = get_indicator(qemu_get_be64(f), len);
>>>> - } else {
>>>> - qemu_get_be64(f);
>>>> - dev->summary_indicator = NULL;
>>>> - }
>>>> - qemu_get_be16s(f, &vdev->config_vector);
>>>> - dev->routes.adapter.ind_offset = qemu_get_be64(f);
>>>> - dev->thinint_isc = qemu_get_byte(f);
>>>> - dev->revision = qemu_get_be32(f);
>>>> - if (s->thinint_active) {
>>>> - dev->routes.adapter.adapter_id = css_get_adapter_id(
>>>> - CSS_IO_ADAPTER_VIRTIO,
>>>> - dev->thinint_isc);
>>>> - }
>>>> -
>>>> - return 0;
>>>> + return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1);
>>>> }
>>>>
>>>> static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp)
>>>> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
>>>> index e61fa74d9b..d74e44d312 100644
>>>> --- a/include/hw/s390x/css.h
>>>> +++ b/include/hw/s390x/css.h
>>>> @@ -88,6 +88,7 @@ struct SubchDev {
>>>> bool ccw_fmt_1;
>>>> bool thinint_active;
>>>> uint8_t ccw_no_data_cnt;
>>>> + uint16_t migrated_schid; /* used for missmatch detection */
>>>> /* transport-provided data: */
>>>> int (*ccw_cb) (SubchDev *, CCW1);
>>>> void (*disable_cb)(SubchDev *);
>>>> @@ -95,22 +96,27 @@ struct SubchDev {
>>>> void *driver_data;
>>>> };
>>>>
>>>> +extern const VMStateDescription vmstate_subch_dev;
>>>> +
>>>> typedef struct IndAddr {
>>>> hwaddr addr;
>>>> uint64_t map;
>>>> unsigned long refcnt;
>>>> - int len;
>>>> + int32_t len;
>>>> QTAILQ_ENTRY(IndAddr) sibling;
>>>> } IndAddr;
>>>>
>>>> +extern const VMStateDescription vmstate_ind_addr;
>>>> +
>>>> +#define VMSTATE_PTR_TO_IND_ADDR(_f, _s)
>>>> \
>>>> + VMSTATE_STRUCT(_f, _s, 1, vmstate_ind_addr, IndAddr*)
>>>> +
>>>> IndAddr *get_indicator(hwaddr ind_addr, int len);
>>>> void release_indicator(AdapterInfo *adapter, IndAddr *indicator);
>>>> int map_indicator(AdapterInfo *adapter, IndAddr *indicator);
>>>>
>>>> typedef SubchDev *(*css_subch_cb_func)(uint8_t m, uint8_t cssid, uint8_t
>>>> ssid,
>>>> uint16_t schid);
>>>> -void subch_device_save(SubchDev *s, QEMUFile *f);
>>>> -int subch_device_load(SubchDev *s, QEMUFile *f);
>>>> int css_create_css_image(uint8_t cssid, bool default_image);
>>>> bool css_devno_used(uint8_t cssid, uint8_t ssid, uint16_t devno);
>>>> void css_subch_assign(uint8_t cssid, uint8_t ssid, uint16_t schid,
>>>> diff --git a/include/hw/s390x/s390_flic.h b/include/hw/s390x/s390_flic.h
>>>> index f9e6890c90..caa6fc608d 100644
>>>> --- a/include/hw/s390x/s390_flic.h
>>>> +++ b/include/hw/s390x/s390_flic.h
>>>> @@ -31,6 +31,11 @@ typedef struct AdapterRoutes {
>>>> int gsi[ADAPTER_ROUTES_MAX_GSI];
>>>> } AdapterRoutes;
>>>>
>>>> +extern const VMStateDescription vmstate_adapter_routes;
>>>> +
>>>> +#define VMSTATE_ADAPTER_ROUTES(_f, _s) \
>>>> + VMSTATE_STRUCT(_f, _s, 1, vmstate_adapter_routes, AdapterRoutes)
>>>> +
>>>> #define TYPE_S390_FLIC_COMMON "s390-flic"
>>>> #define S390_FLIC_COMMON(obj) \
>>>> OBJECT_CHECK(S390FLICState, (obj), TYPE_S390_FLIC_COMMON)
>>>
>>> Other than the nit above, looks good to me. Especially the migration of
>>> the pmcw & co. is now more readable. So, with the nit fixed,
>>>
>>> Reviewed-by: Cornelia Huck <address@hidden>
>>
>> Thanks!
>>
>>>
>>> Christian, can you please take this?
>>>
>>
>> Should I do a re-spin with the nit's picked and the r-b's added?
>
> Yes please. Can you then give me a confirmation that you have tested a
> migration between 2.9 and this patch set?
>
>
I have tested between 2.5 and this (2.5 binary and compat both). I think
that should be equivalent with testing with 2.9, but I can give it a
couple of spins with 2.9.
Though my tests are semi automated at best. Given the nature of changes I
do not expect problems, but from the methodology perspective it ain't
really satisfactory. It could might make sense to throw some well
designed test-suite at it.
Cheers,
Halil