Re: [Qemu-devel] [PATCH 03/10] s390x/css: add vmstate entities for css

[Halil Pasic]

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.)


>> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
>> index f1f0d7f..6a451b2 100644
>> --- a/include/hw/s390x/css.h

[..]

>>  
>> +extern const VMStateInfo vmstate_info_ind_addr;
>> +
>> +#define VMSTATE_PTR_TO_IND_ADDR(_f, _s)                                   \
>> +    VMSTATE_SINGLE(_f, _s, 1 , vmstate_info_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);
>> -- 
>> 2.10.2
>>
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK
> 


Cheers,
Halil