Re: [PATCH v2 4/5] common/domain: add a domain context record for shared_info...

[Jan Beulich <jbeulich@suse.com>]

On 07.04.2020 19:38, Paul Durrant wrote:
> --- a/tools/misc/xen-domctx.c
> +++ b/tools/misc/xen-domctx.c
> @@ -59,6 +59,16 @@ static void dump_header(struct domain_save_descriptor *desc)
>      off += desc->length;
>  }
>  
> +static void dump_shared_info(struct domain_save_descriptor *desc)
> +{
> +    DOMAIN_SAVE_TYPE(SHARED_INFO) s;
> +    READ(s);
> +    printf("    SHARED_INFO: field_width %u buffer size: %lu\n",
> +           s.field_width, desc->length - sizeof(s));
> +
> +    off += desc->length;
> +}

And nothing about the actual contents of the shared info struct?

> @@ -1646,6 +1647,86 @@ int continue_hypercall_on_cpu(
>      return 0;
>  }
>  
> +static int save_shared_info(const struct vcpu *v, struct domain_context *c,
> +                            bool dry_run)
> +{
> +    struct domain *d = v->domain;

const?

> +    struct domain_shared_info_context ctxt = {};
> +    size_t hdr_size = offsetof(typeof(ctxt), buffer);
> +    size_t size = hdr_size + PAGE_SIZE;
> +    int rc;
> +
> +    rc = DOMAIN_SAVE_BEGIN(SHARED_INFO, c, v, size);
> +    if ( rc )
> +        return rc;
> +
> +    if ( !dry_run )
> +        ctxt.field_width =
> +#ifdef CONFIG_COMPAT
> +            has_32bit_shinfo(d) ? 4 :
> +#endif
> +            8;

What are 4 and 8 to represent here? Taking Arm32 into account, the
only things I could think of are sizeof(xen_ulong_t) or
sizeof(guest_handle). I'd prefer if literal numbers could be avoided
here, such that it would become clear what property is really meant.

> +    rc = domain_save_data(c, &ctxt, hdr_size);
> +    if ( rc )
> +        return rc;
> +
> +    rc = domain_save_data(c, d->shared_info, PAGE_SIZE);
> +    if ( rc )
> +        return rc;
> +
> +    return domain_save_end(c);
> +}
> +
> +static int load_shared_info(struct vcpu *v, struct domain_context *c)
> +{
> +    struct domain *d = v->domain;
> +    struct domain_shared_info_context ctxt = {};
> +    size_t hdr_size = offsetof(typeof(ctxt), buffer);
> +    size_t size = hdr_size + PAGE_SIZE;
> +    unsigned int i;
> +    int rc;
> +
> +    rc = DOMAIN_LOAD_BEGIN(SHARED_INFO, c, v, size, true);
> +    if ( rc )
> +        return rc;
> +
> +    rc = domain_load_data(c, &ctxt, hdr_size);
> +    if ( rc )
> +        return rc;
> +
> +    for ( i = 0; i < ARRAY_SIZE(ctxt.pad); i++ )
> +        if ( ctxt.pad[i] )
> +            return -EINVAL;
> +
> +    switch ( ctxt.field_width )
> +    {
> +#ifdef CONFIG_COMPAT
> +    case 4:
> +        d->arch.has_32bit_shinfo = 1;

true and ...

> +        break;
> +#endif
> +    case 8:
> +#ifdef CONFIG_COMPAT
> +        d->arch.has_32bit_shinfo = 0;

... false respectively, please.

> +#endif
> +        break;
> +
> +    default:
> +        rc = -EINVAL;
> +        break;
> +    }
> +
> +    rc = domain_load_data(c, d->shared_info, PAGE_SIZE);
> +    if ( rc )
> +        return rc;
> +
> +    return domain_load_end(c);
> +}

While you check the padding fields of the header above, what about
currently unused fields of the shared_info struct itself?

There's a connection between shared_info and vcpu_info - this way
you may or may not restore vcpu_info - depending on guest behavior.
There not being a patch in the series to deal with vcpu_info, the
description here should imo at least outline the intended
interaction (including e.g. ordering between the [supposed] records).

Jan