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

["Jason J. Herne" <jjherne@linux.vnet.ibm.com>]

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
>

-- 
-- Jason J. Herne (jjherne@linux.vnet.ibm.com)