Re: [Qemu-devel] [PATCH for-2.5 7/8] s390x: Migrate guest storage keys (initial memory only)

[Thomas Huth <thuth@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5346 bytes --]

On 20/07/15 15:49, Cornelia Huck wrote:
> From: "Jason J. Herne" <jjherne@linux.vnet.ibm.com>
> 
> Routines to save/load guest storage keys are provided. register_savevm is
> called to register them as migration handlers.
> 
> We prepare the protocol to support more complex parameters. So we will
> later be able to support standby memory (having empty holes), compression
> and "state live migration" like done for ram.
> 
> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
>  hw/s390x/s390-skeys.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 113 insertions(+)
> 
> diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
> index d355c8f..a927c98 100644
> --- a/hw/s390x/s390-skeys.c
> +++ b/hw/s390x/s390-skeys.c
> @@ -11,10 +11,13 @@
>  
>  #include "hw/boards.h"
>  #include "qmp-commands.h"
> +#include "migration/qemu-file.h"
>  #include "hw/s390x/storage-keys.h"
>  #include "qemu/error-report.h"
>  
>  #define S390_SKEYS_BUFFER_SIZE 131072  /* Room for 128k storage keys */
> +#define S390_SKEYS_SAVE_FLAG_EOS 0x01
> +#define S390_SKEYS_SAVE_FLAG_SKEYS 0x02
>  
>  S390SKeysState *s390_get_skeys_device(void)
>  {
> @@ -241,6 +244,115 @@ static const TypeInfo qemu_s390_skeys_info = {
>      .instance_size = sizeof(S390SKeysClass),
>  };
>  
> +static void s390_storage_keys_save(QEMUFile *f, void *opaque)
> +{
> +    S390SKeysState *ss = S390_SKEYS(opaque);
> +    S390SKeysClass *skeyclass = S390_SKEYS_GET_CLASS(ss);
> +    const uint64_t total_count = ram_size / TARGET_PAGE_SIZE;
> +    uint64_t cur_count, handled_count = 0;
> +    vaddr cur_gfn = 0;
> +    uint8_t *buf;
> +    int ret;
> +
> +    if (!skeyclass->skeys_enabled(ss)) {
> +        goto end_stream;
> +    }
> +
> +    buf = g_try_malloc(S390_SKEYS_BUFFER_SIZE);
> +    if (!buf) {
> +        error_report("storage key save could not allocate memory\n");
> +        goto end_stream;
> +    }
> +
> +    /* We only support initital memory. Standby memory is not handled yet. */
> +    qemu_put_be64(f, (cur_gfn * TARGET_PAGE_SIZE) |
> +                     S390_SKEYS_SAVE_FLAG_SKEYS);
> +    qemu_put_be64(f, total_count);

So if I've got this code right, you send here a "header" that announces
a packet with all pages ...

> +    while (handled_count < total_count) {
> +        cur_count = MIN(total_count - handled_count, S390_SKEYS_BUFFER_SIZE);
> +
> +        ret = skeyclass->get_skeys(ss, cur_gfn, cur_count, buf);
> +        if (ret < 0) {
> +            error_report("S390_GET_KEYS error %d\n", ret);
> +            break;

... but when an error occurs here, you suddenly stop in the middle of
that "packet" with all pages ...

> +        }
> +
> +        /* write keys to stream */
> +        qemu_put_buffer(f, buf, cur_count);
> +
> +        cur_gfn += cur_count;
> +        handled_count += cur_count;
> +    }
> +
> +    g_free(buf);
> +end_stream:
> +    qemu_put_be64(f, S390_SKEYS_SAVE_FLAG_EOS);

... and send an EOS marker here instead ...

> +}
> +
> +static int s390_storage_keys_load(QEMUFile *f, void *opaque, int version_id)
> +{
> +    S390SKeysState *ss = S390_SKEYS(opaque);
> +    S390SKeysClass *skeyclass = S390_SKEYS_GET_CLASS(ss);
> +    int ret = 0;
> +
> +    while (!ret) {
> +        ram_addr_t addr;
> +        int flags;
> +
> +        addr = qemu_get_be64(f);
> +        flags = addr & ~TARGET_PAGE_MASK;
> +        addr &= TARGET_PAGE_MASK;
> +
> +        switch (flags) {
> +        case S390_SKEYS_SAVE_FLAG_SKEYS: {
> +            const uint64_t total_count = qemu_get_be64(f);
> +            uint64_t handled_count = 0, cur_count;
> +            uint64_t cur_gfn = addr / TARGET_PAGE_SIZE;
> +            uint8_t *buf = g_try_malloc(S390_SKEYS_BUFFER_SIZE);
> +
> +            if (!buf) {
> +                error_report("storage key load could not allocate memory\n");
> +                ret = -ENOMEM;
> +                break;
> +            }
> +
> +            while (handled_count < total_count) {
> +                cur_count = MIN(total_count - handled_count,
> +                                S390_SKEYS_BUFFER_SIZE);
> +                qemu_get_buffer(f, buf, cur_count);

... while the receiver can not handle the EOS marker here.

This looks fishy to me (or I might have just missed something), but
anyway please double check whether your error handling in the sender
really makes sense.

> +                ret = skeyclass->set_skeys(ss, cur_gfn, cur_count, buf);
> +                if (ret < 0) {
> +                    error_report("S390_SET_KEYS error %d\n", ret);
> +                    break;
> +                }
> +                handled_count += cur_count;
> +                cur_gfn += cur_count;
> +            }
> +            g_free(buf);
> +            break;
> +        }
> +        case S390_SKEYS_SAVE_FLAG_EOS:
> +            /* normal exit */
> +            return 0;
> +        default:
> +            error_report("Unexpected storage key flag data: %#x", flags);
> +            ret = -EINVAL;
> +        }
> +    }
> +
> +    return ret;
> +}

 Thomas




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]