[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: |
Halil Pasic |
Subject: |
Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css |
Date: |
Thu, 18 May 2017 16:15:04 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 |
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?
We could also consider making WITH_TMP act as a normal field.
Working on the whole state looks like a bit like a corner case:
we have some stuff adjacent in the migration stream, and we have
to map it on multiple fields (and vice-versa). Getting the whole
state with a pointer to a certain field could work via container_of.
Btw, I would rather call it get_indicator a factory method or even a
constructor than an allocator, but I think we understand each-other
anyway.
Regards,
Halil
- [Qemu-devel] [PATCH 00/10] migration: s390x css migration, Halil Pasic, 2017/05/05
- [Qemu-devel] [PATCH 02/10] s390x: add css_migration_enabled to machine class, Halil Pasic, 2017/05/05
- [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 <=
- 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, 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, 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