[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css |
Date: |
Fri, 19 May 2017 18:43:17 +0100 |
User-agent: |
Mutt/1.8.2 (2017-04-18) |
* Halil Pasic (address@hidden) wrote:
>
>
> On 05/19/2017 04:55 PM, Dr. David Alan Gilbert wrote:
> > * Halil Pasic (address@hidden) wrote:
> >>
> >>
> >> On 05/15/2017 08:01 PM, Dr. David Alan Gilbert wrote:
> >>> * Halil Pasic (address@hidden) wrote:
> >>>>
> >>>>
> >>>> On 05/08/2017 06:45 PM, Dr. David Alan Gilbert wrote:
> >>>>> * Halil Pasic (address@hidden) wrote:
> >>>>>> As a preparation for switching to a vmstate based migration let us
> >>>>>> introduce vmstate entities (e.g. VMStateDescription) for the css
> >>>>>> entities
> >>>>>> to be migrated. Alongside some comments explaining or indicating the
> >>>>>> not
> >>>>>> migration of certain members are introduced too.
> >>>>>>
> >>>>>> No changes in behavior, we just added some dead code -- which should
> >>>>>> rise to life soon.
> >>>>>>
> >>>>>> Signed-off-by: Halil Pasic <address@hidden>
> >>>>>> ---
> >>>>>> hw/s390x/css.c | 276
> >>>>>> +++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>> include/hw/s390x/css.h | 10 +-
> >>>>>> 2 files changed, 285 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> >>>>>> index c03bb20..2bda7d0 100644
> >>>>>> --- a/hw/s390x/css.c
> >>>>>> +++ b/hw/s390x/css.c
> >>>>>> @@ -20,29 +20,231 @@
> >>>>>> #include "hw/s390x/css.h"
> >>>>>> #include "trace.h"
> >>>>>> #include "hw/s390x/s390_flic.h"
> >>>>>> +#include "hw/s390x/s390-virtio-ccw.h"
> >>>>>>
> >>>>
> >>>> [..]
> >>>>
> >>>>>> +static int css_get_ind_addr(QEMUFile *f, void *pv, size_t size,
> >>>>>> + VMStateField *field)
> >>>>>> +{
> >>>>>> + int32_t len;
> >>>>>> + IndAddr **ind_addr = pv;
> >>>>>> +
> >>>>>> + len = qemu_get_be32(f);
> >>>>>> + if (len != 0) {
> >>>>>> + *ind_addr = get_indicator(qemu_get_be64(f), len);
> >>>>>> + } else {
> >>>>>> + qemu_get_be64(f);
> >>>>>> + *ind_addr = NULL;
> >>>>>> + }
> >>>>>> + return 0;
> >>>>>> +}
> >>>>>> +
> >>>>>> +static int css_put_ind_addr(QEMUFile *f, void *pv, size_t size,
> >>>>>> + VMStateField *field, QJSON *vmdesc)
> >>>>>> +{
> >>>>>> + IndAddr *ind_addr = *(IndAddr **) pv;
> >>>>>> +
> >>>>>> + if (ind_addr != NULL) {
> >>>>>> + qemu_put_be32(f, ind_addr->len);
> >>>>>> + qemu_put_be64(f, ind_addr->addr);
> >>>>>> + } else {
> >>>>>> + qemu_put_be32(f, 0);
> >>>>>> + qemu_put_be64(f, 0UL);
> >>>>>> + }
> >>>>>> + return 0;
> >>>>>> +}
> >>>>>> +
> >>>>>> +const VMStateInfo vmstate_info_ind_addr = {
> >>>>>> + .name = "s390_ind_addr",
> >>>>>> + .get = css_get_ind_addr,
> >>>>>> + .put = css_put_ind_addr
> >>>>>> +};
> >>>>>
> >>>>> You should be able to avoid this .get/.put by using VMSTATE_WITH_TMP,
> >>>>> declare a temporary struct something like:
> >>>>> struct tmp_ind_addr {
> >>>>> IndAddr *parent;
> >>>>> uint32_t len;
> >>>>> uint64_t addr;
> >>>>> }
> >>>>>
> >>>>> and then your .get/.put routines turn into pre_save/post_load
> >>>>> routines to just setup the len/addr.
> >>>>>
> >>>>
> >>>> I don't think this is going to work -- unfortunately! You can see below,
> >>>> how this IndAddr* migration stuff is supposed to be used:
> >>>> the client code just uses the VMSTATE_PTR_TO_IND_ADDR macro as a
> >>>> field when describing state which needs and IndAddr* migrated.
> >>>>
> >>>> The problem is, we do not know in what state will this field
> >>>> be embedded, the pre_save/post_load called by put_tmp/get_tmp
> >>>> is however copying the pointer to this state into the parent.
> >>>> So instead of having a pointer to IndAddr* in those functions
> >>>> and updating it accordingly, I would have to find the IndAddr*
> >>>> in some arbitrary state (in our case VirtioCcwDevice) first,
> >>>> and I lack information for that.
> >>>>
> >>>> If it's hard to follow I can give you the patch I was debugging
> >>>> to come to this conclusion. (By the way I ended up with 10
> >>>> lines of code more than in this version, and although I think
> >>>> it looks nicer, it's simpler only if one knows how WITH_TMP
> >>>> works. My plan was to ask you which version do you like more
> >>>> and go with that before I realized it ain't gonna work.)
> >>>>
> >>>
> >>> Yes, I see - I've got some similar other cases; the challenge
> >>> is it's a custom allocator - 'get_indicator' - and it's used
> >>> as fields in a few places. Hmm.
> >>>
> >>>
> >>
> >> The problem can be worked around by wrapping the WITH_TMP into a another
> >> vmsd and using VMSTATE_STRUCT for describing the field in question. It's
> >> quite some boilerplate (+16 lines). Should I post the patch here?
> >
> > Yes please.
> >
> --------------------------------8<------------------------------------------
>
> From a0b6c725b114745c8434f683133995c4e33d936e Mon Sep 17 00:00:00 2001
> From: Halil Pasic <address@hidden>
> Date: Tue, 9 May 2017 12:06:42 +0200
> Subject: [PATCH 1/1] s390x/css: replace info with VMSTATE_WITH_TMP
>
> Convert s VMSatateInfo based solution manipulating the migration stream
> directly to VMSTATE_WITH_TMP witch keeps IO and transformation logic
> separate.
>
> Signed-off-by: Halil Pasic <address@hidden>
> ---
> hw/s390x/css.c | 56
> ++++++++++++++++++++++++++++++++------------------
> include/hw/s390x/css.h | 4 ++--
> 2 files changed, 38 insertions(+), 22 deletions(-)
>
> diff --git a/hw/s390x/css.c b/hw/s390x/css.c
> index 694365b395..71d1b95e3f 100644
> --- a/hw/s390x/css.c
> +++ b/hw/s390x/css.c
> @@ -224,41 +224,57 @@ const VMStateDescription vmstate_subch_dev = {
> }
> };
>
> -static int css_get_ind_addr(QEMUFile *f, void *pv, size_t size,
> - VMStateField *field)
> -{
> +typedef struct IndAddrPtrTmp {
> + IndAddr **parent;
> + uint64_t addr;
> int32_t len;
> - IndAddr **ind_addr = pv;
> +} IndAddrPtrTmp;
>
> - len = qemu_get_be32(f);
> - if (len != 0) {
> - *ind_addr = get_indicator(qemu_get_be64(f), len);
> +static int post_load_ind_addr(void *opaque, int version_id)
> +{
> + IndAddrPtrTmp *ptmp = opaque;
> + IndAddr **ind_addr = ptmp->parent;
> +
> + if (ptmp->len != 0) {
> + *ind_addr = get_indicator(ptmp->addr, ptmp->len);
> } else {
> - qemu_get_be64(f);
> *ind_addr = NULL;
> }
> return 0;
> }
>
> -static int css_put_ind_addr(QEMUFile *f, void *pv, size_t size,
> - VMStateField *field, QJSON *vmdesc)
> +static void pre_save_ind_addr(void *opaque)
> {
> - IndAddr *ind_addr = *(IndAddr **) pv;
> + IndAddrPtrTmp *ptmp = opaque;
> + IndAddr *ind_addr = *(ptmp->parent);
>
> if (ind_addr != NULL) {
> - qemu_put_be32(f, ind_addr->len);
> - qemu_put_be64(f, ind_addr->addr);
> + ptmp->len = ind_addr->len;
> + ptmp->addr = ind_addr->addr;
> } else {
> - qemu_put_be32(f, 0);
> - qemu_put_be64(f, 0UL);
> + ptmp->len = 0;
> + ptmp->addr = 0L;
> }
> - return 0;
> }
>
> -const VMStateInfo vmstate_info_ind_addr = {
> - .name = "s390_ind_addr",
> - .get = css_get_ind_addr,
> - .put = css_put_ind_addr
> +const VMStateDescription vmstate_ind_addr_tmp = {
> + .name = "s390_ind_addr_tmp",
> + .pre_save = pre_save_ind_addr,
> + .post_load = post_load_ind_addr,
> +
> + .fields = (VMStateField[]) {
> + VMSTATE_INT32(len, IndAddrPtrTmp),
> + VMSTATE_UINT64(addr, IndAddrPtrTmp),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +const VMStateDescription vmstate_ind_addr = {
> + .name = "s390_ind_addr_tmp",
> + .fields = (VMStateField[]) {
> + VMSTATE_WITH_TMP(IndAddr*, IndAddrPtrTmp, vmstate_ind_addr_tmp),
> + VMSTATE_END_OF_LIST()
> + }
> };
>
> typedef struct CssImage {
> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
> index be5eb81ece..efe7cafef0 100644
> --- a/include/hw/s390x/css.h
> +++ b/include/hw/s390x/css.h
> @@ -106,10 +106,10 @@ typedef struct IndAddr {
> QTAILQ_ENTRY(IndAddr) sibling;
> } IndAddr;
>
> -extern const VMStateInfo vmstate_info_ind_addr;
> +extern const VMStateDescription vmstate_ind_addr;
>
> #define VMSTATE_PTR_TO_IND_ADDR(_f, _s) \
> - VMSTATE_SINGLE(_f, _s, 1 , vmstate_info_ind_addr, IndAddr*)
> + VMSTATE_STRUCT(_f, _s, 1, vmstate_ind_addr, IndAddr*)
>
> IndAddr *get_indicator(hwaddr ind_addr, int len);
> void release_indicator(AdapterInfo *adapter, IndAddr *indicator);
Oh if that works that's great; I'd thought we'd need to change
something in VMSTATE_WITH_TMP to be able to get it to be an IndAddr **parent;
there is a bit much boiler-plate but I've not got any easy
ideas for removing that for now.
Dave
> --
> 2.11.2
>
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK
- [Qemu-devel] [PATCH 04/10] s390x/css: add vmstate macro for CcwDevice, (continued)
- [Qemu-devel] [PATCH 04/10] s390x/css: add vmstate macro for CcwDevice, Halil Pasic, 2017/05/05
- [Qemu-devel] [PATCH 01/10] s390x: add helper get_machine_class, Halil Pasic, 2017/05/05
- [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css, Halil Pasic, 2017/05/05
- Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css, Dr. David Alan Gilbert, 2017/05/08
- Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css, Halil Pasic, 2017/05/09
- Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css, Dr. David Alan Gilbert, 2017/05/15
- Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css, Halil Pasic, 2017/05/18
- Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css, Dr. David Alan Gilbert, 2017/05/19
- Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css, Halil Pasic, 2017/05/19
- Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css, Halil Pasic, 2017/05/19
- Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css,
Dr. David Alan Gilbert <=
- Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css, Halil Pasic, 2017/05/19
- Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css, Dr. David Alan Gilbert, 2017/05/19
- Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css, Halil Pasic, 2017/05/19
- Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css, Halil Pasic, 2017/05/09
[Qemu-devel] [PATCH 05/10] virtio-ccw: add vmstate entities for VirtioCcwDevice, Halil Pasic, 2017/05/05
[Qemu-devel] [PATCH 06/10] virtio-ccw: use vmstate way for config migration, Halil Pasic, 2017/05/05