Re: [Qemu-devel] [PATCH v2 4/7] s390x/css: add missing css state conditionally

[Juan Quintela]

Halil Pasic <address@hidden> wrote:
> Although we have recently vmstatified the migration of some css
> infrastructure,  for some css entities there is still state to be
> migrated left, because the focus was keeping migration stream
> compatibility (that is basically everything as-is).
>
> Let us add vmstate helpers and extend existing vmstate descriptions so
> that we have everything we need. Let us guard the added state with via
> css_migration_enabled, so we keep the compatible behavior if css
> migration is disabled.
>
> Let's also annotate the bits which do not need to be migrated for better
> readability.
>
> Signed-off-by: Halil Pasic <address@hidden>
> ---
>  hw/intc/s390_flic.c | 20 +++++++++++++++
>  hw/s390x/css.c      | 74 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 94 insertions(+)

Reviewed-by: Juan Quintela <address@hidden>


For the vmstate bits.  I have exactly zero clue about s390

> +static const VMStateDescription vmstate_chp_info = {
> +    .name = "s390_chp_info",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(in_use, ChpInfo),
> +        VMSTATE_UINT8(type, ChpInfo),
> +        VMSTATE_UINT8(is_virtual, ChpInfo),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  typedef struct SubchSet {
>      SubchDev *sch[MAX_SCHID + 1];
>      unsigned long schids_used[BITS_TO_LONGS(MAX_SCHID + 1)];
> @@ -215,6 +248,19 @@ typedef struct CssImage {
>      ChpInfo chpids[MAX_CHPID + 1];
>  } CssImage;
>  
> +static const VMStateDescription vmstate_css_img = {
> +    .name = "s390_css_img",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        /* Subchannel sets have no relevant state. */
> +        VMSTATE_STRUCT_ARRAY(chpids, CssImage, MAX_CHPID + 1, 0,
> +                             vmstate_chp_info, ChpInfo),
> +        VMSTATE_END_OF_LIST()
> +    }
> +
> +};

> +static const VMStateDescription vmstate_css = {
> +    .name = "s390_css",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_QTAILQ_V(pending_crws, ChannelSubSys, 1, 
> vmstate_crw_container,
> +                         CrwContainer, sibling),
> +        VMSTATE_BOOL(sei_pending, ChannelSubSys),
> +        VMSTATE_BOOL(do_crw_mchk, ChannelSubSys),
> +        VMSTATE_BOOL(crws_lost, ChannelSubSys),
> +        /* These were kind of migrated by virtio */
> +        VMSTATE_UINT8(max_cssid, ChannelSubSys),
> +        VMSTATE_UINT8(max_ssid, ChannelSubSys),
> +        VMSTATE_BOOL(chnmon_active, ChannelSubSys),
> +        VMSTATE_UINT64(chnmon_area, ChannelSubSys),
> +        VMSTATE_ARRAY_OF_POINTER_TO_STRUCT(css, ChannelSubSys, MAX_CSSID + 1,
> +                0, vmstate_css_img, CssImage),

I was about to suggest to move css from pointer to an embedded struct,
and then noticed that MAX_CSSID is .... 65535.  I guess that this is
going to be sparse, very sparse.  Perhaps there is an easier way to
transmit it?

You are transmiting:

65000 structs each of size MAX_CHPID (255) and each element is byte +
byte +byte = 65000 * 255 * 3 = 47 MB.

If it is really sparse, I think that sending something like a list
of <index in array> <contents> could make sense, no?

Or I am missunderstanding something?

Later, Juan.