[for-2.5,7/8] s390x: Migrate guest storage keys (initial memory only)

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(+)

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

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

Indeed, although that should never fail, we never know.
We don't want to overengineer the protocol but still abort migration at least
on the loading side in that (theoretical) case.

> 
> > +        }
> > +
> > +        /* 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.

My shot would be, to send invalid storage keys if getting the keys from the
kernel fails. So we can detect it on the loading side and abort migration
gracefully.

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

Thanks Thomas!


David

On 07/21/2015 06:37 AM, David Hildenbrand wrote:
>>
>> 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 ...
>
> Indeed, although that should never fail, we never know.
> We don't want to overengineer the protocol but still abort migration at least
> on the loading side in that (theoretical) case.
>

I don't have a strong opinion on this either way. I think it is fine 
just the way
it is (for the reasons David described above). However, if people are 
worried I
can see about writing some code that sends fake keys to the destination as
described below. Thoughts?

>>
>>> +        }
>>> +
>>> +        /* 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.
>
> My shot would be, to send invalid storage keys if getting the keys from the
> kernel fails. So we can detect it on the loading side and abort migration
> gracefully.
>
>>
>>> +                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
>
> Thanks Thomas!
>
>
> David
>

On 30/07/15 17:00, Jason J. Herne wrote:
> On 07/21/2015 06:37 AM, David Hildenbrand wrote:
>>>
>>> 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 ...
>>
>> Indeed, although that should never fail, we never know.
>> We don't want to overengineer the protocol but still abort migration
>> at least
>> on the loading side in that (theoretical) case.
>>
> 
> I don't have a strong opinion on this either way. I think it is fine
> just the way
> it is (for the reasons David described above). However, if people are
> worried I
> can see about writing some code that sends fake keys to the destination as
> described below. Thoughts?

If David is right and the skeyclass->get_skeys() really never fails (I
did not check), then simply do an "assert (ret == 0)" afterwards - that
way you can be sure that it really never fails. And if it ever fails,
you notice it immediately - and that's certainly way much better than
debugging the currently-wrong error handling code.

 Thomas

On 07/21/2015 06:37 AM, David Hildenbrand wrote:
>>
>> 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 ...
>
> Indeed, although that should never fail, we never know.
> We don't want to overengineer the protocol but still abort migration at least
> on the loading side in that (theoretical) case.
>
>>
>>> +        }
>>> +
>>> +        /* 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.
>
> My shot would be, to send invalid storage keys if getting the keys from the
> kernel fails. So we can detect it on the loading side and abort migration
> gracefully.
>

What storage key value would you consider invalid? All combinations of 
the upper four bits are valid. And of the lower four, we have the FP, 
reference and change bits with the final bit marked as reserved. The 
only possible answer would be to abuse the reserved bit and set it to 1 
when there is an error. The major problem with that: This bit could be 
used for something someday which would require us to stop using it for 
an error indicator. Another problem is that we would then have to check 
every single storage key for this error bit on the destination side.

This ioctl should not fail if we've made it this far. If it does we are 
still covered because the sudden hole in the data will throw off 
everything else. It could (in VERY rare cases, if at all) cause error 
messages to surface that are unrelated to the problem but the correct 
"S390_GET_KEYS error %d" message will still be displayed first. 
Certainly it is not 100% perfect but since the sending side is not 
allowed to fail there seems to be simple option here. We could 
re-engineer the protocol to send  packets of [Length][KeyData] and we 
could decide on an  error value for length (0xFFFFFFFF) that would 
indicate an error, perhaps with 0x0 indicating end of data. I'm happy to 
do the work if requested but is it really worth it?

>>
>>> +                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
>
> Thanks Thomas!
>
>
> David
>

Am 21.07.2015 um 12:37 schrieb David Hildenbrand:
>>
>> 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 ...
> 
> Indeed, although that should never fail, we never know.

I think -ENOMEM would be a possible return code.

> We don't want to overengineer the protocol but still abort migration at least
> on the loading side in that (theoretical) case.
[..]

>>> +end_stream:
>>> +    qemu_put_be64(f, S390_SKEYS_SAVE_FLAG_EOS);

what about  sending zeroes for failed keys
defining an error code

 #define S390_SKEYS_SAVE_FLAG_EOS 0x01
 #define S390_SKEYS_SAVE_FLAG_SKEYS 0x02
+#define S390_SKEYS_SAVE_FLAG_FAILED 0x03

and then do 
	if (ok) {
		 qemu_put_be64(f, S390_SKEYS_SAVE_FLAG_EOS);
	} else {
		 qemu_put_be64(f, S390_SKEYS_SAVE_FLAG_FAILED);
	}

and the let target system fail migration (which should let the guest run at the source system?)