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

[Dr. David Alan Gilbert]

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

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

You do need to know which field you're working on to be able to safely
use container_of, so I'm not sure how it would work for you in this
case.

The other thought I'd had was that perhaps we could change the temporary
structure in VMSTATE_WITH_TMP to:

  struct foo {
     struct whatever **parent;

so now you could write to *parent in cases like these.

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

Yes; I'm not too worried about the actual name as long as it's short
and obvious.

I'd thought of 'allocator' since in most cases it's used where the
load-time code allocates memory for the object being loaded.
A constructor is normally something I think of as happening after
allocation; and a factory, hmm maybe.  However, if it does the right
thing I wouldn't object to any of those names.

Dave
P.S. I'm out for about a week.

> Regards,
> Halil
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK