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

[Jason J. Herne]

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 (address@hidden)